| Message ID | 20141113174030.GM26481@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Thomas Gleixner <tglx@linutronix.de> [141113 14:27]: > 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 :) No problem, just that we need to bring in few other includes and devres.c is currently free of any core irq stuff :) I can switch to internals.h no problem if you prefer that. > > + * 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. OK :) > 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. OK I'll take a look if we can just set some pm_runtime flag and use the pm_runtime counters for that. > 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 :) Heh !!true. > > + 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? Yes the drivers may need to restore hardware state in the pm_runtime calls and who knows what else drivers will be doing. So that too might be a good reason to just set a flag in pm_runtime land. Anyways, thanks for your comments. I'll post a complete series after looking into the wake-up counters a bit. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Nov 13, 2014 at 09:40:31AM -0800, Tony Lindgren wrote: [snip] > From: Tony Lindgren <tony@atomide.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@atomide.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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
* Felipe Balbi <balbi@ti.com> [141114 08:20]: > On Thu, Nov 13, 2014 at 09:40:31AM -0800, Tony Lindgren wrote: > > +/** > > + * 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. Yeah I'll take a look. > > + 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(). It currently assumes the consumer driver takes care of it. But I get your point, we should be able to automate this further. And right now there's also a dependency on dev->power.irq_safe so RPM_ASYNC is not set. And this all should ideally work even with runtime PM not set as it's also needed for resume from suspend. > 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 :-) Good point :) Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 14, 2014 at 09:08:17AM -0800, Tony Lindgren wrote: > * Felipe Balbi <balbi@ti.com> [141114 08:20]: > > On Thu, Nov 13, 2014 at 09:40:31AM -0800, Tony Lindgren wrote: > > > +/** > > > + * 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. > > Yeah I'll take a look. note that at the end of the day, the outcome will be pretty similar, but with the added benefit that current users of pm_runtime_irq_safe() can be updated as time allows, rather than in one go. > > > + 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(). > > It currently assumes the consumer driver takes care of it. But I get > your point, we should be able to automate this further. right, consumer calls disable_irq() and that's fine, should be there anyway, but currently you still have a window where wakeirq will be unmasked, if you look at irq_finalize_oneshot(), it's easy to see that it will unmask wakeirq after ->thread_fn() runs: 686 static void irq_finalize_oneshot(struct irq_desc *desc, 687 struct irqaction *action) 688 { [...] 726 if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) && 727 irqd_irq_masked(&desc->irq_data)) 728 unmask_threaded_irq(desc); 729 730 out_unlock: 731 raw_spin_unlock_irq(&desc->lock); 732 chip_bus_sync_unlock(desc); 733 } [...] 800 static irqreturn_t irq_thread_fn(struct irq_desc *desc, 801 struct irqaction *action) 802 { 803 irqreturn_t ret; 804 805 ret = action->thread_fn(action->irq, action->dev_id); 806 irq_finalize_oneshot(desc, action); 807 return ret; 808 } so, ->thread_fn() returns and wakeirq is unmasked. You don't know when your ->runtime_resume() will be scheduled, which means that wakeirq could be unmasked for quite a while and it could refire depending on PCB layout. The problem should be minimal, but it's there anyway. Also, you know that once the runtime is resumed, you don't want wakeirq to be unmasked, so why not just mask it from handle_wake_irq() ? Another thing, this assumes that drivers are using pm_runtime and, furthermore, it assumes that drivers' ->runtime_resume() will properly handle pending IRQs. This is definitely not the case for most drivers. Note that quite a few of them aren't either using pm_runtime or have blank/NULL runtime callbacks. Due to these, I think Thomas' suggestion of setting device IRQ pending is the best solution. That takes care of all cases. If drivers are using pm_runtime, then they are required to check if device is still pm_runtime_suspended from IRQ handler, for those who aren't, they can assume device is ready to handle IRQs once the IRQ handler is called. > And right now there's also a dependency on dev->power.irq_safe so > RPM_ASYNC is not set. And this all should ideally work even with runtime > PM not set as it's also needed for resume from suspend. exactly.
--- 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); + ret = IRQ_HANDLED; + } + + 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; + } + + 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);