Message-ID: <20141114161901.GG11538@saruman>
Date:	Fri, 14 Nov 2014 10:19:10 -0600
From:	Felipe Balbi <balbi@...com>
To:	Tony Lindgren <tony@...mide.com>
CC:	Thomas Gleixner <tglx@...utronix.de>, 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

Hi,

On Thu, Nov 13, 2014 at 09:40:31AM -0800, Tony Lindgren wrote:

[snip]

> From: Tony Lindgren <tony@...mide.com>
> Date: Tue, 11 Nov 2014 07:53:55 -0800
> 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 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.
> 
> Let's also add a minimal manage.h to allow us keeping the separation
> of devm functions and without having to include internals.h in devres.c.
> 
> 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/devres.c
> +++ b/kernel/irq/devres.c
> @@ -3,6 +3,8 @@
>  #include <linux/device.h>
>  #include <linux/gfp.h>
>  
> +#include "manage.h"
> +
>  /*
>   * Device resource management aware IRQ request/free implementation.
>   */
> @@ -118,6 +120,30 @@ int devm_request_any_context_irq(struct device *dev, unsigned int irq,
>  EXPORT_SYMBOL(devm_request_any_context_irq);
>  
>  /**
> + *	devm_request_wake_irq - request a wake-up interrupt for a device
> + *	@dev: device to wake on the wake-up interrupt
> + *	@wakeirq: wake-up interrupt for the device
> + *	@wakeirq: wake-up interrupt flags
> + *
> + *	The wake-up interrupt starts disabled and is typically enabled
> + *	when needed by the device driver runtime PM calls.
> + */
> +int devm_request_wake_irq(struct device *dev, unsigned int wakeirq,
> +			  unsigned long wakeflags)
> +{
> +	int ret;
> +
> +	ret = init_disabled_wakeirq(dev, wakeirq, wakeflags);
> +	if (ret)
> +		return ret;
> +
> +	return devm_request_threaded_irq(dev, wakeirq, NULL,
> +					 handle_wakeirq_thread,
> +					 wakeflags, dev_name(dev), dev);
> +}
> +EXPORT_SYMBOL_GPL(devm_request_wake_irq);
> +
> +/**
>   *	devm_free_irq - free an interrupt
>   *	@dev: device to free interrupt for
>   *	@irq: Interrupt line to free
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -14,12 +14,14 @@
>  #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>
>  #include <linux/task_work.h>
>  
>  #include "internals.h"
> +#include "manage.h"
>  
>  #ifdef CONFIG_IRQ_FORCED_THREADING
>  __read_mostly bool force_irqthreads;
> @@ -1564,6 +1566,112 @@ 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
> + */
> +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);

this assumes that every driver's ->resume() callback has a:

	if (pending)
		handle_pending_irqs();

which might not be very nice. I'd rather follow what Thomas suggested
and always pass device irq so this can mark it pending. Keep in mind
that we *don't* need a pm_runtime_get_sync() in every IRQ handler
because of that. Adding it is but the easiest way to get things working
and, quite frankly, very silly.

what we want is rather:

	irqreturn_t my_handler(int irq, void *dev_id)
	{
		struct device *dev = dev_id;

		if (pm_runtime_suspended(dev)) {
			pending_irqs_to_be_handled_from_runtime_resume = true;
			pm_runtime_get(dev);
			clear_irq_source(dev);
			return IRQ_HANDLED;
		}
	}

or something similar.

> +		ret = IRQ_HANDLED;
> +	}

you're not masking the wake irq here which means that when this handler
returns, wake irq will be unmasked by core IRQ subsystem leaving it
unmasked after ->resume().

> +	return ret;
> +}
> +
> +/**
> + *	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.
> + */
> +int init_disabled_wakeirq(struct device *dev, unsigned int wakeirq,
> +			  unsigned long wakeflags)
> +{
> +	if (!(dev && wakeirq)) {
> +		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;
> +	}

you *know* you'll pass a NULL top half handler, why don't you just force
IRQF_ONESHOT instead of erroring out ? Just add:

	wakeflags |= IRQF_ONESHOT;

and get it over with :-)

> +	if (wakeflags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))
> +		pr_warn("Not replaying device IRQs for %s on wakeirq%d\n",
> +			dev_name(dev), wakeirq);
> +
> +	irq_set_status_flags(wakeirq, _IRQ_NOAUTOEN);
> +
> +	return 0;
> +}
> +
> +/**
> + *	request_wake_irq - request a wake-up interrupt for a device
> + *	@dev: device to wake on the wake-up interrupt
> + *	@wakeirq: wake-up interrupt for the device
> + *	@wakeirq: wake-up interrupt flags
> + *
> + *	The wake-up interrupt starts disabled and is typically enabled
> + *	when needed by the device driver runtime PM calls.
> + */
> +int request_wake_irq(struct device *dev, unsigned int wakeirq,
> +		     unsigned long wakeflags)
> +{
> +	int ret;
> +
> +	ret = init_disabled_wakeirq(dev, wakeirq, wakeflags);
> +	if (ret)
> +		return ret;
> +
> +	return request_threaded_irq(wakeirq, NULL,
> +				    handle_wakeirq_thread,
> +				    wakeflags, dev_name(dev), dev);
> +}
> +EXPORT_SYMBOL_GPL(request_wake_irq);
> +
>  void enable_percpu_irq(unsigned int irq, unsigned int type)
>  {
>  	unsigned int cpu = smp_processor_id();
> --- /dev/null
> +++ b/kernel/irq/manage.h
> @@ -0,0 +1,11 @@
> +/*
> + * IRQ subsystem internal management functions and variables:
> + *
> + * Do not ever include this file from anything else than
> + * kernel/irq/. Do not even think about using any information outside
> + * of this file for your non core code.
> + */
> +
> +irqreturn_t handle_wakeirq_thread(int wakeirq, void *dev_id);
> +int init_disabled_wakeirq(struct device *dev, unsigned int wakeirq,
> +			  unsigned long wakeflags);
> --
> 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/

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists