Message-ID: <alpine.DEB.2.11.1411132251170.3935@nanos>
Date:	Thu, 13 Nov 2014 23:25:28 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Tony Lindgren <tony@...mide.com>
cc:	Nishanth Menon <nm@...com>, lee.jones@...aro.org,
	LKML <linux-kernel@...r.kernel.org>, devicetree@...r.kernel.org,
	Keerthy <j-keerthy@...com>, Mark Brown <broonie@...aro.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	linux-omap@...r.kernel.org,
	LAK <linux-arm-kernel@...ts.infradead.org>,
	Kevin Hilman <khilman@...aro.org>
Subject: Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup

On Thu, 13 Nov 2014, Tony Lindgren wrote:
> Oops thanks for catching that. As the devres stuff is separate, I've
> updated the patch to keep it that way by adding a minimal manage.h.
> This avoids including internals.h in devres.c. Does that seem usable
> for you?

What's wrong with internals.h? devres.c is core code, so it is not
affected of the ban to include internals.h :)
 
> +/**
> + *	init_disabled_wakeirq - initialize a wake-up interrupt for a device
> + *	@dev: device to wake up on the wake-up interrupt
> + *	@wakeirq: wake-up interrupt for the device
> + *	@wakeflags: wake-up interrupt flags
> + *
> + *	Note that the wake-up interrupt starts disabled. The wake-up interrupt
> + *	is typically enabled from the device pm_runtime_suspend() and disabled
> + *	again in the device pm_runtime_resume(). For runtime PM, the wake-up
> + *	interrupt should be always enabled, and for device suspend and resume,
> + *	the wake-up interrupt should be enabled depending on the device specific
> + *	configuration for device_can_wakeup().
> + *
> + *	Note also that we are not resending the lost device interrupts.
> + *	We assume that the wake-up interrupt just needs to wake-up the device,
> + *	and then device pm_runtime_resume() can deal with the situation.
> + *
> + *	There are at least the following reasons to not resend the lost device
> + *	interrupts automatically based on the wake-up interrupt:
> + *
> + *	1. There can be interrupt reentry issues calling the device interrupt
> + *	   based on the wake-up interrupt if done in the device driver. It
> + *	   could be done with check_irq_resend() after checking the device
> + *	   interrupt mask if we really wanted to though.
> + *
> + *	2. The device interrupt handler would need to be set up properly with
> + *	   pm_runtime_irq_safe(). Ideally you don't want to call pm_runtime
> + *	   calls from the device interrupt handler at all.
> + *
> + *	3. The IRQ subsystem may not know if it's safe to call the device
> + *	   interrupt unless the driver updates the interrupt status with
> + *	   disable_irq() and enable_irq() in addition to just disabling the
> + *	   interrupt at the hardware level in the device registers.
> + *
> + *	So if replaying the lost device interrupts is absolutely needed from the
> + *	hardware point of view, it's probably best to set up a completely
> + *	separate wake-up interrupt handler for the wake-up interrupt in the
> + *	device driver because of the reasons above.

Can we please kill this last paragraph? I'm already seeing the
gazillion of "I think it is required to do so for my soooo special
chip" implementations in random drivers which all get it wrong again.

So I'd rather provide a mechanism upfront which lets the driver know
that the wakeup interrupt originated from that device, i.e. let the
wake up handler call

     pm_wakeup_irq(dev);

which calls:

      pm_runtime_mark_last_busy(dev);
      pm_request_resume(dev);

and aside of that tells the device via a flag or preferrably a
sequence counter that the wakeup irq has been triggered. So affected
devices can handle it based on that information w/o implementing the
next broken variant of wakeup irq handlers.

That also allows to remove the wakeflags check for level/edge.

> + */
> +int init_disabled_wakeirq(struct device *dev, unsigned int wakeirq,
> +			  unsigned long wakeflags)
> +{
> +	if (!(dev && wakeirq)) {

This is the second time I stumbled over this. While it is correct it
would be simpler to parse 

      if (!dev || !wakeirq) {

At least for my review damaged brain :)

> +		pr_err("Missing device or wakeirq for %s irq %d\n",
> +		       dev_name(dev), wakeirq);
> +		return -EINVAL;
> +	}
> +
> +	if (!(wakeflags & IRQF_ONESHOT)) {
> +		pr_err("Invalid wakeirq for %s irq %d, must be oneshot\n",
> +		       dev_name(dev), wakeirq);
> +		return -EINVAL;
> +	}

Is there a reason why we force the wakeirq into a threaded handler?

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists