Message-ID: <20141106204629.GF31454@atomide.com>
Date:	Thu, 6 Nov 2014 12:46:30 -0800
From:	Tony Lindgren <tony@...mide.com>
To:	Thomas Gleixner <tglx@...utronix.de>
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

Thomas,

Any comments on the patch below? Let me know if you want to keep the
devm stuff out of kernel/irq/manage.c.

* Tony Lindgren <tony@...mide.com> [141001 20:45]:
> Hi Thomas,
> 
> * Thomas Gleixner <tglx@...utronix.de> [140919 12:47]:
> > 
> > The wakeup handler is supposed to bring the thing out of deep sleep
> > and nothing else. All you want it to do is to mask itself and save the
> > information that the real device irq is pending.
> > 
> > A stub handler for the wakeup irq is enough. We can have that in the
> > irq/pm core and all it would do is simply:
> 
> Here's a patch along the lines of what you described, hopefully that's
> fairly close to what you had in mind.
> 
> I also did play with the replaying of the interrupts but I don't think
> that's needed. Well at least not for the omap case. I added some
> comments about that to the code.
> 
> So far I've tested with the omap-serial and omap_hsmmc drivers. The
> serial driver does not have any status as the device is powered off.
> So replaying of the interrupt does not help there, we need to wait for
> the next event anyways.
> 
> Then with omap_hsmmc the SDIO interrupt on dat1 line is level
> sensitive and is noticed after the MMC controller is powered on
> again. So no replaying of the device interrupt needed here either.
> 
> I still have not tested the MMC remux lines to GPIO for wake-up
> events that's also needed for some omaps.
> 
> Regards,
> 
> Tony
> 
> 8<-----------
> From: Tony Lindgren <tony@...mide.com>
> Date: Wed, 1 Oct 2014 14:56:35 -0700
> Subject: [PATCH] genirq: Add support for wake-up interrupts to fix irq reentry issues in drivers
> 
> As pointed out by Thomas Gleixner, at least omap wake-up interrupts
> have an issue with re-entrant interrupts because the wake-up interrupts
> are now handled as a secondary interrupt controller. Further, the
> wake-up interrupt just needs wake the system at least for omaps. So we
> should just make the wake-up interrupt handling generic.
> 
> Note that at least initially we are keeping things simple by assuming the
> wake-up interrupt is level sensitive, and the device pm_runtime_resume()
> can deal with the situation, and no replaying of the lost device interrupts
> is needed.
> 
> After tinkering with replaying of the lost device interrupts, my opinion is
> that it should be avoided because of the issues listed in the comments of
> this patch.
> 
> Signed-off-by: Tony Lindgren <tony@...mide.com>
> 
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -139,11 +139,15 @@ extern int __must_check
>  request_percpu_irq(unsigned int irq, irq_handler_t handler,
>  		   const char *devname, void __percpu *percpu_dev_id);
>  
> +struct device;
> +
> +extern int __must_check
> +request_wake_irq(struct device *dev, unsigned int wakeirq,
> +		 unsigned long irqflags);
> +
>  extern void free_irq(unsigned int, void *);
>  extern void free_percpu_irq(unsigned int, void __percpu *);
>  
> -struct device;
> -
>  extern int __must_check
>  devm_request_threaded_irq(struct device *dev, unsigned int irq,
>  			  irq_handler_t handler, irq_handler_t thread_fn,
> @@ -163,6 +167,10 @@ devm_request_any_context_irq(struct device *dev, unsigned int irq,
>  		 irq_handler_t handler, unsigned long irqflags,
>  		 const char *devname, void *dev_id);
>  
> +extern int __must_check
> +devm_request_wake_irq(struct device *dev, unsigned int wakeirq,
> +		      unsigned long irqflags);
> +
>  extern void devm_free_irq(struct device *dev, unsigned int irq, void *dev_id);
>  
>  /*
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -14,6 +14,7 @@
>  #include <linux/module.h>
>  #include <linux/random.h>
>  #include <linux/interrupt.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  #include <linux/sched.h>
>  #include <linux/sched/rt.h>
> @@ -1578,6 +1579,111 @@ int request_any_context_irq(unsigned int irq, irq_handler_t handler,
>  }
>  EXPORT_SYMBOL_GPL(request_any_context_irq);
>  
> +/**
> + *	handle_wakeirq_thread - call device runtime pm calls on wake-up interrupt
> + *	@wakeirq: device specific wake-up interrupt
> + *	@dev_id: struct device entry
> + */
> +static irqreturn_t handle_wakeirq_thread(int wakeirq, void *dev_id)
> +{
> +	struct device *dev = dev_id;
> +	irqreturn_t ret = IRQ_NONE;
> +
> +	if (pm_runtime_suspended(dev)) {
> +		pm_runtime_mark_last_busy(dev);
> +		pm_request_resume(dev);
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + *	setup_wakeirq - allocate a wake-up interrupt for a device
> + *	@dev: device to wake up
> + *	@wakeirq: interrupt that wakes up the device
> + *	@wakeflags: flags to pass to the interrupt handler
> + *	@devm: use devm
> + *
> + *	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.
> + */
> +static int setup_wakeirq(struct device *dev, unsigned int wakeirq,
> +			 unsigned long wakeflags, bool devm)
> +{
> +	int ret;
> +
> +	if (!(dev && wakeirq)) {
> +		pr_err("Missing device or wakeirq for %s irq %d\n",
> +		       dev_name(dev), wakeirq);
> +		return -EINVAL;
> +	}
> +
> +	if (!(wakeflags &
> +	      (IRQF_TRIGGER_LOW | IRQF_TRIGGER_HIGH | IRQF_ONESHOT))) {
> +		pr_err("Invalid wakeirq for %s irq %d, must be level oneshot\n",
> +		       dev_name(dev), wakeirq);
> +		return -EINVAL;
> +	}
> +
> +	irq_set_status_flags(wakeirq, _IRQ_NOAUTOEN);
> +
> +	if (devm)
> +		ret = devm_request_threaded_irq(dev, wakeirq, NULL,
> +						handle_wakeirq_thread,
> +						wakeflags, dev_name(dev), dev);
> +	else
> +		ret = request_threaded_irq(wakeirq, NULL,
> +					   handle_wakeirq_thread,
> +					   wakeflags, dev_name(dev), dev);
> +
> +	return ret;
> +}
> +
> +int request_wake_irq(struct device *dev, unsigned int wakeirq,
> +		     unsigned long wakeflags)
> +{
> +	return setup_wakeirq(dev, wakeirq, wakeflags, false);
> +}
> +EXPORT_SYMBOL(request_wake_irq);
> +
> +int devm_request_wake_irq(struct device *dev, unsigned int wakeirq,
> +			  unsigned long wakeflags)
> +{
> +	return setup_wakeirq(dev, wakeirq, wakeflags, false);
> +}
> +EXPORT_SYMBOL(devm_request_wake_irq);
> +
>  void enable_percpu_irq(unsigned int irq, unsigned int type)
>  {
>  	unsigned int cpu = smp_processor_id();
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
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