Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
Hi Rafael, On Tue, 24 Feb 2015 02:02:59 +0100 "Rafael J. Wysocki" wrote: > On Friday, February 20, 2015 10:31:44 AM Mark Rutland wrote: > > [...] > > [cut] > > > Given all of the above I'll go back to the IRQF_SHARED_TIMER_OK approach > > you proposed, along with documentation updates and comments at usage > > sites to make it clear when it is valid to use. > > > > Thank you for bearing with me so far. > > No prob. :-) > > Actually, there's one more approach possible. Thomas might not like it, but > here it goes anyway for consideration. > > What about making it possible to provide a special irqaction to be called > while suspended for the shared IRQF_NO_SUSPEND interrupts. > > Say we have a "suspended_action" pointer in struct irq_desc in addition to > "action" and we swap them in suspend_device_irq() if desc->no_suspend_depth is > nonzero and "suspended_action" is not NULL (and then back in resume_irq()). > Then, the platform might supply "suspended_action" that will do the right > thing for the IRQ. > > So the requirement would be to have "suspended_action" set to start with > and then the WARN_ON() in irq_pm_install_action() may check if that is present > and only trigger the warning if it isn't. > > Thoughs? That should work for my case. Note that I also proposed a version that does not touch the irq_desc struct, but I had to add an irq_is_wakeup_armed helper function to test in which state the irq handler is called. Your solution would make this easier. Could you remind me what was Thomas concern regarding this suspended action list approach ? Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Friday, February 20, 2015 10:31:44 AM Mark Rutland wrote: > [...] [cut] > Given all of the above I'll go back to the IRQF_SHARED_TIMER_OK approach > you proposed, along with documentation updates and comments at usage > sites to make it clear when it is valid to use. > > Thank you for bearing with me so far. No prob. :-) Actually, there's one more approach possible. Thomas might not like it, but here it goes anyway for consideration. What about making it possible to provide a special irqaction to be called while suspended for the shared IRQF_NO_SUSPEND interrupts. Say we have a "suspended_action" pointer in struct irq_desc in addition to "action" and we swap them in suspend_device_irq() if desc->no_suspend_depth is nonzero and "suspended_action" is not NULL (and then back in resume_irq()). Then, the platform might supply "suspended_action" that will do the right thing for the IRQ. So the requirement would be to have "suspended_action" set to start with and then the WARN_ON() in irq_pm_install_action() may check if that is present and only trigger the warning if it isn't. Thoughs? Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Mon, 23 Feb 2015 18:14:48 + Mark Rutland wrote: [...] > > This is because irq_may_run [1], which is called to decide whether we > > should handle this irq or just wake the system up [2], will always > > return true if at least one of the shared action has tagged the irq > > line as a wakeup source. > > I assume you mean we return false in this case (having triggered the > wakeup within irq_pm_check_wakeup, which returned true), but otherwise > agreed. Yep, I meant 'return false'. > > I can envisage problems if the irq handler of a wakeup device can't be > run safely until resume time, though I'm not sure if that happens in > practice given the device is necessarily going to be active. Isn't that the purpose of the IRQF_NO_SUSPEND_SAFE/IRQF_SHARED_TIMER_OK/IRQF_SHARED_WAKEUP_SIBLING_OK flag ? > > > Sorry for summarizing things you most likely already know, but I want > > to be sure I'm actually understanding it correctly. > > > > Now, let's look at how this could be solved. > > Here is a proposal [3] that does the following: > > This would be a lot easier to follow/review as an RFC post to the > mailing list. Yep, that was the plan, just wanted to make sure I had correctly understood the problem before posting an RFC. > Otherwise I have some high-level comments on the stuff > below, which I think matches the shape of what we discussed on IRC. > > > 1/ prevent a system wakeup when at least one of the action handler > > has set the IRQF_NO_SUSPEND flag > > We might need to add some logic to enable_irq_wake and > irq_pm_install_action to prevent some of the horrible mismatch cases we > can get here (e.g. if we have a wakeup handler, a IRQF_NO_SUSPEND > handler, and another handler which is neither). We may need to > reconsider temporarily stashing the other potential interrupts. Actually if we force users to pass the IRQF_XXX_SAFE (I'm tired writing all the potential names :-)), when mixing IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND handlers, we shouldn't bother deactivating normal handlers (those without IRQF_NO_SUSPEND), 'cause they claimed they could safely be called in suspended state. > > Do we perhaps need an IRQF_SHARED_WAKEUP_SIBLING_OK for timer drivers to > assert their handlers are safe for the whole suspend period rather than > just the period they expect to be enabled for? Or do those always > happen to be safe in practice? I thought they were always safe... > > > 2/ Add a few helpers to deal with system wakeup from drivers code > > The irq_pm_force_wakeup part looks like what I had in mind. > > > 3/ Rework the at91 RTC driver to show how such weird cases could be > > handled > > It might be simpler to do this with a PM notifier within the driver > rather than having to traverse all the irq_descs, though perhaps not. I'm not sure to understand that one. Where am I traversing irq_descs (irq_to_desc, which is called when testing wakeup_armed status, is a direct table indexing operation) ? Moreover, I'm not sure when the PM_POST_SUSPEND event is sent, and testing the WAKEUP_ARMED flag should be safe in all cases, right ? > > > Of course, I'll need the IRQF_SHARED_TIMER_OK patch to prevent the > > WARN_ON backtrace. > > That should be fine; it's backed up in the list archive ;) > > > Please, let me know if I missed anything important, share your opinion > > on this proposal, and feel free to propose any other solution. > > Hopefully the above covers that! Yes it does. Thanks for the review. Best Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Mon, Feb 23, 2015 at 05:00:57PM +, Boris Brezillon wrote: > Hi Mark, > > Thanks for the clarification, and sorry if I've been a bit harsh to you > in my previous answer, but this whole shared irq thing is starting to > drive me crazy. No worries. Having lost a few days exploring the core and call sites, and having seen how widesrpead IRQF_NO_SUSPEND misuse is, it's beginning to grate on me too. [...] > On at91 platforms, irq line 1 is shared by a timer (PIT) and some > devices that can register themselves as wakeup sources (especially the > RTC IP). > I doubt at91 users will agree on dropping either of these features (the > timer or the wakeup on RTC/UART/...), so, can we try to find a solution > that would both make irq, DT and at91 guys happy (that doesn't sound > like an easy task :-)) ? >From the DT side, I think all the necessary information is there. We know how the interrupts are wired, so nothing needs to change w.r.t. the topology description. I hope that we have sufficient information to when a device may operate and raise interrupts during suspend. So the fun part is how we solve this within Linux. > The whole problem here is that we can't have both IRQF_NO_SUSPEND flag > set on one of the shared action and others that are configuring the irq > as a wakeup source, because it would always lead to the system being > woken up (even when the only thing we were supposed to do is handle the > timer event). > > This is because irq_may_run [1], which is called to decide whether we > should handle this irq or just wake the system up [2], will always > return true if at least one of the shared action has tagged the irq > line as a wakeup source. I assume you mean we return false in this case (having triggered the wakeup within irq_pm_check_wakeup, which returned true), but otherwise agreed. I can envisage problems if the irq handler of a wakeup device can't be run safely until resume time, though I'm not sure if that happens in practice given the device is necessarily going to be active. > Sorry for summarizing things you most likely already know, but I want > to be sure I'm actually understanding it correctly. > > Now, let's look at how this could be solved. > Here is a proposal [3] that does the following: This would be a lot easier to follow/review as an RFC post to the mailing list. Otherwise I have some high-level comments on the stuff below, which I think matches the shape of what we discussed on IRC. > 1/ prevent a system wakeup when at least one of the action handler > has set the IRQF_NO_SUSPEND flag We might need to add some logic to enable_irq_wake and irq_pm_install_action to prevent some of the horrible mismatch cases we can get here (e.g. if we have a wakeup handler, a IRQF_NO_SUSPEND handler, and another handler which is neither). We may need to reconsider temporarily stashing the other potential interrupts. Do we perhaps need an IRQF_SHARED_WAKEUP_SIBLING_OK for timer drivers to assert their handlers are safe for the whole suspend period rather than just the period they expect to be enabled for? Or do those always happen to be safe in practice? > 2/ Add a few helpers to deal with system wakeup from drivers code The irq_pm_force_wakeup part looks like what I had in mind. > 3/ Rework the at91 RTC driver to show how such weird cases could be > handled It might be simpler to do this with a PM notifier within the driver rather than having to traverse all the irq_descs, though perhaps not. > Of course, I'll need the IRQF_SHARED_TIMER_OK patch to prevent the > WARN_ON backtrace. That should be fine; it's backed up in the list archive ;) > Please, let me know if I missed anything important, share your opinion > on this proposal, and feel free to propose any other solution. Hopefully the above covers that! Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
Hi Mark, Thanks for the clarification, and sorry if I've been a bit harsh to you in my previous answer, but this whole shared irq thing is starting to drive me crazy. On Fri, 20 Feb 2015 15:16:56 + Mark Rutland wrote: [...] > > An IRQ cannot be shared between a device with IRQF_NO_SUSPEND and a > device that wishes to operate as a wakeup device, because the semantics > don't align. One wishes to handle IRQs continuously and one wants to > abort suspend at the first IRQ (waiting until part-way through the > resume before handling it). > > So those devices above which wish to operate as wakeup devices are > either irrelevant or unsalvageable with the current approaches. > > The flag or demux chip only happens to mask the problem in the absence > of strict sanity checking. > > If you want to be able to share the line then you will need to > fundamentally rework the way wakeup interrupts work in order to be able > to decide when you've encountered a real wakeup event. Okay, I'll try to summarize the discussion we had on IRC regarding this aspect. On at91 platforms, irq line 1 is shared by a timer (PIT) and some devices that can register themselves as wakeup sources (especially the RTC IP). I doubt at91 users will agree on dropping either of these features (the timer or the wakeup on RTC/UART/...), so, can we try to find a solution that would both make irq, DT and at91 guys happy (that doesn't sound like an easy task :-)) ? The whole problem here is that we can't have both IRQF_NO_SUSPEND flag set on one of the shared action and others that are configuring the irq as a wakeup source, because it would always lead to the system being woken up (even when the only thing we were supposed to do is handle the timer event). This is because irq_may_run [1], which is called to decide whether we should handle this irq or just wake the system up [2], will always return true if at least one of the shared action has tagged the irq line as a wakeup source. Sorry for summarizing things you most likely already know, but I want to be sure I'm actually understanding it correctly. Now, let's look at how this could be solved. Here is a proposal [3] that does the following: 1/ prevent a system wakeup when at least one of the action handler has set the IRQF_NO_SUSPEND flag 2/ Add a few helpers to deal with system wakeup from drivers code 3/ Rework the at91 RTC driver to show how such weird cases could be handled Of course, I'll need the IRQF_SHARED_TIMER_OK patch to prevent the WARN_ON backtrace. Please, let me know if I missed anything important, share your opinion on this proposal, and feel free to propose any other solution. [1]http://lxr.free-electrons.com/source/kernel/irq/chip.c#L348 [2]http://lxr.free-electrons.com/source/kernel/irq/chip.c#L386 [3]http://code.bulix.org/gboee1-87936 -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
> > * The pmc looks like it could be a valid use of the new flag. It also > > seems to function as an irqchip. > > > > Do any of its child IRQs need to be handled during the suspend-resume > > cycle? If so using IRQF_NO_SUSPEND would seem to be valid. > > No they don't, they are used for clock activation only, and thus should > be disabled on suspend. Ok. So the IRQF_SHARED_TIMER_OK flag would make sense here. > > * atmel_serial seems to be intended to be used as a wakeup device (given > > it calls device_set_wakeup_enable). Therefore it needs to call > > enable_irq_wake, and when it does so it can share an IRQ with other > > interrupts, just not IRQF_NO_SUSPEND interrupts. > > I'll have a look, but AFAIR serial core already taking care of that. > > > > > None of the approaches thus far can fix the fundamental mismatch > > between wakeup interrupts and IRQF_NO_SUSPEND interrupts. > > > > * Similarly, rtc-at91rm9200 and rtc-at91sam9 are intended to be used as > > wakeup devices. They call enable_irq_wake (though don't bother > > checking the return value). They can share an IRQ with other > > interrupts, just not IRQF_NO_SUSPEND interrupts. > > Yep. > > > > > * at91sam9_wdt seems to be fundamentally incompatible with suspend. If > > the watchdog cannot be disabled, and you need to handle it during > > suspend, then it needs to be a wakeup interrupt, not an > > IRQF_NO_SUSPEND interrupt. If they're not shared with an IRQF_NO_SUSPEND IRQ, then everything is already OK. If they are shared with an IRQF_NO_SUSPEND IRQ, then the fundamental problem is not solved by any approach so far. > You forgot the PIT timer, which is the one in cause here (no other > drivers are specifying this IRQF_NO_SUSPEND flag), and, as you already > know, a timer sets the IRQF_NO_SUSPEND flag. > > > As far as I can see, the flag or virtual irqchip approaches only help > > the PMC case, and even then might not be necessary. All the others seem > > to be relying on guarantees the genirq layer don't provide, and fixing > > that would mean moving them further from IRQF_NO_SUSPEND. > > I don't know what you're trying to prove here, but I never said my goal > was to set IRQF_NO_SUSPEND flags on existing at91 drivers ? > The problem here, is that all those IPs are sharing the irq line with a > timer which sets IRQF_NO_SUSPEND. As Rafael and I described, sharing an IRQ between a wakeup device (the serial, rtc, and wdt) and an IRQF_NO_SUSPEND device is broken. You have the choice between losing wakeup events or masking timer events. So those device above which operate as wakeup devices cannot share a line with a timer. > I just want to be able to share an irq line with a timer and other > devices that do not set IRQF_NO_SUSPEND. > Could we focus on that ? I'm trying to focus on the cases we can actually salvage. An IRQ cannot be shared between a device with IRQF_NO_SUSPEND and a device that wishes to operate as a wakeup device, because the semantics don't align. One wishes to handle IRQs continuously and one wants to abort suspend at the first IRQ (waiting until part-way through the resume before handling it). So those devices above which wish to operate as wakeup devices are either irrelevant or unsalvageable with the current approaches. The flag or demux chip only happens to mask the problem in the absence of strict sanity checking. If you want to be able to share the line then you will need to fundamentally rework the way wakeup interrupts work in order to be able to decide when you've encountered a real wakeup event. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
Hi Mark, On Fri, 20 Feb 2015 14:22:08 + Mark Rutland wrote: > Hi Boris, > > On Wed, Feb 11, 2015 at 04:38:23PM +, Boris Brezillon wrote: > > [...] > > > For the list of impacted drivers, you can have a look at this series [1] > > (patches 2 to 5), and I'll take care of the testing part once every one > > has agreed on the solution ;-). > > > > [1]https://lkml.org/lkml/2014/12/15/552 > > Looking at those: > > * The pmc looks like it could be a valid use of the new flag. It also > seems to function as an irqchip. > > Do any of its child IRQs need to be handled during the suspend-resume > cycle? If so using IRQF_NO_SUSPEND would seem to be valid. No they don't, they are used for clock activation only, and thus should be disabled on suspend. > > * atmel_serial seems to be intended to be used as a wakeup device (given > it calls device_set_wakeup_enable). Therefore it needs to call > enable_irq_wake, and when it does so it can share an IRQ with other > interrupts, just not IRQF_NO_SUSPEND interrupts. I'll have a look, but AFAIR serial core already taking care of that. > > None of the approaches thus far can fix the fundamental mismatch > between wakeup interrupts and IRQF_NO_SUSPEND interrupts. > > * Similarly, rtc-at91rm9200 and rtc-at91sam9 are intended to be used as > wakeup devices. They call enable_irq_wake (though don't bother > checking the return value). They can share an IRQ with other > interrupts, just not IRQF_NO_SUSPEND interrupts. Yep. > > * at91sam9_wdt seems to be fundamentally incompatible with suspend. If > the watchdog cannot be disabled, and you need to handle it during > suspend, then it needs to be a wakeup interrupt, not an > IRQF_NO_SUSPEND interrupt. You forgot the PIT timer, which is the one in cause here (no other drivers are specifying this IRQF_NO_SUSPEND flag), and, as you already know, a timer sets the IRQF_NO_SUSPEND flag. > > As far as I can see, the flag or virtual irqchip approaches only help > the PMC case, and even then might not be necessary. All the others seem > to be relying on guarantees the genirq layer don't provide, and fixing > that would mean moving them further from IRQF_NO_SUSPEND. I don't know what you're trying to prove here, but I never said my goal was to set IRQF_NO_SUSPEND flags on existing at91 drivers ? The problem here, is that all those IPs are sharing the irq line with a timer which sets IRQF_NO_SUSPEND. I just want to be able to share an irq line with a timer and other devices that do not set IRQF_NO_SUSPEND. Could we focus on that ? Best Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
Hi Boris, On Wed, Feb 11, 2015 at 04:38:23PM +, Boris Brezillon wrote: [...] > For the list of impacted drivers, you can have a look at this series [1] > (patches 2 to 5), and I'll take care of the testing part once every one > has agreed on the solution ;-). > > [1]https://lkml.org/lkml/2014/12/15/552 Looking at those: * The pmc looks like it could be a valid use of the new flag. It also seems to function as an irqchip. Do any of its child IRQs need to be handled during the suspend-resume cycle? If so using IRQF_NO_SUSPEND would seem to be valid. * atmel_serial seems to be intended to be used as a wakeup device (given it calls device_set_wakeup_enable). Therefore it needs to call enable_irq_wake, and when it does so it can share an IRQ with other interrupts, just not IRQF_NO_SUSPEND interrupts. None of the approaches thus far can fix the fundamental mismatch between wakeup interrupts and IRQF_NO_SUSPEND interrupts. * Similarly, rtc-at91rm9200 and rtc-at91sam9 are intended to be used as wakeup devices. They call enable_irq_wake (though don't bother checking the return value). They can share an IRQ with other interrupts, just not IRQF_NO_SUSPEND interrupts. * at91sam9_wdt seems to be fundamentally incompatible with suspend. If the watchdog cannot be disabled, and you need to handle it during suspend, then it needs to be a wakeup interrupt, not an IRQF_NO_SUSPEND interrupt. As far as I can see, the flag or virtual irqchip approaches only help the PMC case, and even then might not be necessary. All the others seem to be relying on guarantees the genirq layer don't provide, and fixing that would mean moving them further from IRQF_NO_SUSPEND. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
[...] > > > IRQF_NO_SUSPEND and wakeup fundamentally don't match due to the way > > > wakeup is implemented in the IRQ core now. > > > > > > Unless drivers with IRQF_NO_SUSPEND do the wakeup behind the core's back > > > which is just disgusting and should never happen. > > > > I completely agree that using IRQF_NO_SUSPEND in that manner is a > > disgusting abuse. It seems like some drivers are abusing it for wakeup, > > and those need to be corrected. > > > > If those are corrected, the same issue with mismatch will happen with > > those wakeup interrupts, and we need to fix that somehow given people > > seem to already be relying on being able to share a line with a wakeup > > device. > > The way we handle wakeup interrupts in the core prevents that, because > wakeup is handled at the IRQ level rather than at a handler (irqaction) > level. Interrupt handlers from irqactions are not run for wakeup > interrupts at all after suspend_device_irqs(), so if you have anyone > sharing an IRQ with a wakeup source, their interrupt handler won't be > run anyway then. I had missed that fact; thank you for correcting me! > > I propose we add a new IRQF_BIKESHED, meaning that this interrupt line > > may be shared with an IRQF_NO_SUSPEND or wakeup interrupt handler. > > As I said, sharing an IRQ with a wakeup source is OK (worst case you'll > cause spurious wakeup interrupts to occur if your device is not suspended > properly). The problem is *entirely* about IRQF_NO_SUSPEND. I see that now. > So I claim that the only things having legitimate reasons to ever use > IRQF_NO_SUSPEND are (a) timers and (b) IPIs. There really are no other > cases to worry about in my view, but I may be overlooking something. Assuming you also include any chained irqchip handlers necessary for those timers or IPIs, that makes sense to me. > Anyway, I wouldn't like to add flags allowing driver writers to do fishy > things in a hush-hush manner. The warning trigger is there for a good > reason and if we allow it to be avoided, that has to be for a good reason > too. In other words, it shouldn't be too easy to do that. Sure. Given all of the above I'll go back to the IRQF_SHARED_TIMER_OK approach you proposed, along with documentation updates and comments at usage sites to make it clear when it is valid to use. Thank you for bearing with me so far. Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Thursday, February 19, 2015 11:23:46 AM Mark Rutland wrote:
> On Thu, Feb 19, 2015 at 01:16:50AM +, Rafael J. Wysocki wrote:
> > On Monday, February 16, 2015 12:23:43 PM Mark Rutland wrote:
> > > [...]
> > >
> > > > > The "suspend" part is kind of a distraction to me here, because that
> > > > > really
> > > > > only is about sharing an IRQ with a timer and the "your interrupt
> > > > > handler
> > > > > may be called when the device is suspended" part is just a
> > > > > consequence of that.
> > > > >
> > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging
> > > > > people to
> > > > > abuse this for other purposes not related to timers.
> > > >
> > > > Sorry to be late to the bike-shed party, but what about:
> > >
> > > [...]
> > >
> > > > arch/arm/mach-omap2/mux.c: omap_hwmod_mux_handle_irq,
> > > > IRQF_SHARED | IRQF_NO_SUSPEND,
> > > > arch/arm/mach-omap2/pm34xx.c: _prcm_int_handle_io,
> > > > IRQF_SHARED | IRQF_NO_SUSPEND, "pm_io",
> > > > drivers/pinctrl/pinctrl-single.c:
> > > > IRQF_SHARED | IRQF_NO_SUSPEND,
> > >
> > > These are chained IRQ handlers. If any of these have a chained timer irq
> > > then the IRQF_NO_SUSPEND may be legitimate. I can't imagine why these
> > > would be shared, however.
> > >
> > > It also looks like these abuse IRQF_NO_SUSPEND for wakeup interrupts.
> > >
> > > > drivers/rtc/rtc-pl031.c:.irqflags = IRQF_SHARED |
> > > > IRQF_NO_SUSPEND,
> > >
> > > This looks to be an abuse and should use {enable,disable}_irq_wake.
> > >
> > > However, we'd then need to handle mismatch with wakeup interrupts (which
> > > is effectively the same problem as sharing with a timer).
> >
> > IRQF_NO_SUSPEND and wakeup fundamentally don't match due to the way
> > wakeup is implemented in the IRQ core now.
> >
> > Unless drivers with IRQF_NO_SUSPEND do the wakeup behind the core's back
> > which is just disgusting and should never happen.
>
> I completely agree that using IRQF_NO_SUSPEND in that manner is a
> disgusting abuse. It seems like some drivers are abusing it for wakeup,
> and those need to be corrected.
>
> If those are corrected, the same issue with mismatch will happen with
> those wakeup interrupts, and we need to fix that somehow given people
> seem to already be relying on being able to share a line with a wakeup
> device.
The way we handle wakeup interrupts in the core prevents that, because
wakeup is handled at the IRQ level rather than at a handler (irqaction)
level. Interrupt handlers from irqactions are not run for wakeup
interrupts at all after suspend_device_irqs(), so if you have anyone
sharing an IRQ with a wakeup source, their interrupt handler won't be
run anyway then.
> I propose we add a new IRQF_BIKESHED, meaning that this interrupt line
> may be shared with an IRQF_NO_SUSPEND or wakeup interrupt handler.
As I said, sharing an IRQ with a wakeup source is OK (worst case you'll
cause spurious wakeup interrupts to occur if your device is not suspended
properly). The problem is *entirely* about IRQF_NO_SUSPEND.
Moreover, I don't think any watchdogs have a legitimate reason to use
IRQF_NO_SUSPEND, because quite frankly if you don't want your watchdog
to abort system suspends in progress, then you have no reason to run that
watchdog during system suspend at all. Conversely, if you want the watchdog
to abort system suspends in progress, its wakeup interrupt should be a wakeup
one.
So I claim that the only things having legitimate reasons to ever use
IRQF_NO_SUSPEND are (a) timers and (b) IPIs. There really are no other
cases to worry about in my view, but I may be overlooking something.
Anyway, I wouldn't like to add flags allowing driver writers to do fishy
things in a hush-hush manner. The warning trigger is there for a good
reason and if we allow it to be avoided, that has to be for a good reason
too. In other words, it shouldn't be too easy to do that.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Thu, Feb 19, 2015 at 01:16:50AM +, Rafael J. Wysocki wrote:
> On Monday, February 16, 2015 12:23:43 PM Mark Rutland wrote:
> > [...]
> >
> > > > The "suspend" part is kind of a distraction to me here, because that
> > > > really
> > > > only is about sharing an IRQ with a timer and the "your interrupt
> > > > handler
> > > > may be called when the device is suspended" part is just a consequence
> > > > of that.
> > > >
> > > > So IMO it's better to have "TIMER" in the names to avoid encouraging
> > > > people to
> > > > abuse this for other purposes not related to timers.
> > >
> > > Sorry to be late to the bike-shed party, but what about:
> >
> > [...]
> >
> > > arch/arm/mach-omap2/mux.c:omap_hwmod_mux_handle_irq,
> > > IRQF_SHARED | IRQF_NO_SUSPEND,
> > > arch/arm/mach-omap2/pm34xx.c: _prcm_int_handle_io,
> > > IRQF_SHARED | IRQF_NO_SUSPEND, "pm_io",
> > > drivers/pinctrl/pinctrl-single.c: IRQF_SHARED |
> > > IRQF_NO_SUSPEND,
> >
> > These are chained IRQ handlers. If any of these have a chained timer irq
> > then the IRQF_NO_SUSPEND may be legitimate. I can't imagine why these
> > would be shared, however.
> >
> > It also looks like these abuse IRQF_NO_SUSPEND for wakeup interrupts.
> >
> > > drivers/rtc/rtc-pl031.c: .irqflags = IRQF_SHARED | IRQF_NO_SUSPEND,
> >
> > This looks to be an abuse and should use {enable,disable}_irq_wake.
> >
> > However, we'd then need to handle mismatch with wakeup interrupts (which
> > is effectively the same problem as sharing with a timer).
>
> IRQF_NO_SUSPEND and wakeup fundamentally don't match due to the way
> wakeup is implemented in the IRQ core now.
>
> Unless drivers with IRQF_NO_SUSPEND do the wakeup behind the core's back
> which is just disgusting and should never happen.
I completely agree that using IRQF_NO_SUSPEND in that manner is a
disgusting abuse. It seems like some drivers are abusing it for wakeup,
and those need to be corrected.
If those are corrected, the same issue with mismatch will happen with
those wakeup interrupts, and we need to fix that somehow given people
seem to already be relying on being able to share a line with a wakeup
device.
I propose we add a new IRQF_BIKESHED, meaning that this interrupt line
may be shared with an IRQF_NO_SUSPEND or wakeup interrupt handler.
Specifically:
* This driver ensures that its device will be quiesced during suspend,
and will not assert interrupts.
* This handler can be called spuriously during suspend (or we somehow
mask out IRQF_BIKSHED irq actions in the core).
* It will be documented and enforced that each use of IRQF_BIKESHED must
have an associated comment explaining why the driver has to use it,
and how it meets the requirements.
With that in place we can audit+fix the drivers sharing the line to use
IRQF_BIKESHED, which solves the warning Boris is seeing. In parallel
with that we can audit+fix the IRQF_NO_SUSPEND abuses to use the correct
infrastructure.
Does that make any sense? I'll have a go at patches on the assumption
that it's not the absolute worst idea, unless/until corrected otherwise.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Monday, February 16, 2015 12:23:43 PM Mark Rutland wrote:
> [...]
>
> > > The "suspend" part is kind of a distraction to me here, because that
> > > really
> > > only is about sharing an IRQ with a timer and the "your interrupt handler
> > > may be called when the device is suspended" part is just a consequence of
> > > that.
> > >
> > > So IMO it's better to have "TIMER" in the names to avoid encouraging
> > > people to
> > > abuse this for other purposes not related to timers.
> >
> > Sorry to be late to the bike-shed party, but what about:
>
> [...]
>
> > arch/arm/mach-omap2/mux.c: omap_hwmod_mux_handle_irq, IRQF_SHARED
> > | IRQF_NO_SUSPEND,
> > arch/arm/mach-omap2/pm34xx.c: _prcm_int_handle_io,
> > IRQF_SHARED | IRQF_NO_SUSPEND, "pm_io",
> > drivers/pinctrl/pinctrl-single.c: IRQF_SHARED |
> > IRQF_NO_SUSPEND,
>
> These are chained IRQ handlers. If any of these have a chained timer irq
> then the IRQF_NO_SUSPEND may be legitimate. I can't imagine why these
> would be shared, however.
>
> It also looks like these abuse IRQF_NO_SUSPEND for wakeup interrupts.
>
> > drivers/rtc/rtc-pl031.c:.irqflags = IRQF_SHARED | IRQF_NO_SUSPEND,
>
> This looks to be an abuse and should use {enable,disable}_irq_wake.
>
> However, we'd then need to handle mismatch with wakeup interrupts (which
> is effectively the same problem as sharing with a timer).
IRQF_NO_SUSPEND and wakeup fundamentally don't match due to the way
wakeup is implemented in the IRQ core now.
Unless drivers with IRQF_NO_SUSPEND do the wakeup behind the core's back
which is just disgusting and should never happen.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
[...]
> > The "suspend" part is kind of a distraction to me here, because that really
> > only is about sharing an IRQ with a timer and the "your interrupt handler
> > may be called when the device is suspended" part is just a consequence of
> > that.
> >
> > So IMO it's better to have "TIMER" in the names to avoid encouraging people
> > to
> > abuse this for other purposes not related to timers.
>
> Sorry to be late to the bike-shed party, but what about:
[...]
> arch/arm/mach-omap2/mux.c:omap_hwmod_mux_handle_irq, IRQF_SHARED
> | IRQF_NO_SUSPEND,
> arch/arm/mach-omap2/pm34xx.c: _prcm_int_handle_io, IRQF_SHARED |
> IRQF_NO_SUSPEND, "pm_io",
> drivers/pinctrl/pinctrl-single.c: IRQF_SHARED |
> IRQF_NO_SUSPEND,
These are chained IRQ handlers. If any of these have a chained timer irq
then the IRQF_NO_SUSPEND may be legitimate. I can't imagine why these
would be shared, however.
It also looks like these abuse IRQF_NO_SUSPEND for wakeup interrupts.
> drivers/rtc/rtc-pl031.c: .irqflags = IRQF_SHARED | IRQF_NO_SUSPEND,
This looks to be an abuse and should use {enable,disable}_irq_wake.
However, we'd then need to handle mismatch with wakeup interrupts (which
is effectively the same problem as sharing with a timer).
> drivers/mfd/ab8500-debugfs.c:IRQF_SHARED |
> IRQF_NO_SUSPEND,
> drivers/mfd/ab8500-gpadc.c: IRQF_NO_SUSPEND | IRQF_SHARED,
> "ab8500-gpadc-sw",
> drivers/mfd/ab8500-gpadc.c: IRQF_NO_SUSPEND | IRQF_SHARED,
> "ab8500-gpadc-hw",
> drivers/power/ab8500_btemp.c: IRQF_SHARED | IRQF_NO_SUSPEND,
> drivers/power/ab8500_charger.c: IRQF_SHARED |
> IRQF_NO_SUSPEND,
> drivers/power/ab8500_fg.c:IRQF_SHARED | IRQF_NO_SUSPEND,
> drivers/usb/phy/phy-ab8500-usb.c: IRQF_NO_SUSPEND
> | IRQF_SHARED,
> drivers/usb/phy/phy-ab8500-usb.c: IRQF_NO_SUSPEND
> | IRQF_SHARED,
> drivers/usb/phy/phy-ab8500-usb.c: IRQF_NO_SUSPEND
> | IRQF_SHARED,
All the *ab8500* look cargo-culted. There's other nonsense in these
(e.g. mutex_lock in irq handlers...). I suspect these are not
legitimate.
> drivers/watchdog/intel-mid_wdt.c:IRQF_SHARED |
> IRQF_NO_SUSPEND, "watchdog",
Watchdogs could be a legitimate case, but this driver relies on another
timer and the timeout irq handler simply calls panic(), which seems a
little extreme.
> Is there a single legitimate user in that list? If so, the TIMER name
> might be misleading.
The watchdog case could be legitimate, and with drivers corrected to use
{enable,disable}_irq_wake we'll need to handle mismatch for wakeup
interrupts too.
Having separate flags for sharing with timers and sharing with wakeup
sources seems redundant, and IRQF_SHARED_TIMER_OK would be misleading.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
Please change the Subject to start with [PATCH] again when including patches, otherwise its too easy for them to get lost. Esp. with excessive quoting on top. I nearly missed the patch here, seeing nothing in the first page of text. On Wed, Feb 11, 2015 at 05:13:13PM +, Mark Rutland wrote: > --- > include/linux/interrupt.h | 5 + > kernel/irq/pm.c | 44 ++-- > 2 files changed, 47 insertions(+), 2 deletions(-) > > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h > index d9b05b5..2b8ff50 100644 > --- a/include/linux/interrupt.h > +++ b/include/linux/interrupt.h > @@ -57,6 +57,9 @@ > * IRQF_NO_THREAD - Interrupt cannot be threaded > * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device > *resume time. > + * IRQF_SHARED_TIMER_OK - Interrupt is safe to be shared with a timer. The > + *handler may be called spuriously during suspend > + *without issue. I feel we should do better documenting this; at the very least refer to Documentation/power/suspend-and-interrupts.txt and ideally put a scary note in telling people that if they use this as a bandaid to make the warn go away, they will end up with a broken system. Now ideally every driver that employs this should also have a comment next to it how it does indeed behave nicely, such that we can 'quickly' see people have indeed thought about things and not just slapped it on to make the WARN go away. > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c > index 3ca5325..e4ec91a 100644 > --- a/kernel/irq/pm.c > +++ b/kernel/irq/pm.c > @@ -28,6 +28,47 @@ bool irq_pm_check_wakeup(struct irq_desc *desc) > + for (action = desc->action; action; action = action->next) > + if (!(action->flags & safe_flags)) > + return false; In general I prefer braces around the for loop even though C does not strictly require it. Its just too easy to confuse multi-line statements with multiple statements. Extra braces comfort the brain in this case. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
Guys, trim your emails, please! On Wed, Feb 11, 2015 at 04:51:36PM +0100, Rafael J. Wysocki wrote: > On Wednesday, February 11, 2015 03:12:38 PM Mark Rutland wrote: > > I guess that would have to imply IRQF_SHARED, so we'd have something > > like: > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during > > suspend in the case the line is shared. The > > handler will not access unavailable hardware > > or kernel infrastructure during this period. > > > > #define __IRQF_SUSPEND_SPURIOUS 0x0004 > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | > > __IRQF_SUSPEND_SPURIOUS) > > What about > > #define __IRQF_TIMER_SIBLING_OK 0x0004 > #define IRQF_SHARED_TIMER_OK(IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > The "suspend" part is kind of a distraction to me here, because that really > only is about sharing an IRQ with a timer and the "your interrupt handler > may be called when the device is suspended" part is just a consequence of > that. > > So IMO it's better to have "TIMER" in the names to avoid encouraging people to > abuse this for other purposes not related to timers. Sorry to be late to the bike-shed party, but what about: Documentation/power/suspend-and-interrupts.txt:the IRQ's users. For this reason, using IRQF_NO_SUSPEND and IRQF_SHARED at the arch/arm/mach-omap2/mux.c: omap_hwmod_mux_handle_irq, IRQF_SHARED | IRQF_NO_SUSPEND, arch/arm/mach-omap2/pm34xx.c: _prcm_int_handle_io, IRQF_SHARED | IRQF_NO_SUSPEND, "pm_io", drivers/mfd/ab8500-debugfs.c: IRQF_SHARED | IRQF_NO_SUSPEND, drivers/mfd/ab8500-gpadc.c: IRQF_NO_SUSPEND | IRQF_SHARED, "ab8500-gpadc-sw", drivers/mfd/ab8500-gpadc.c: IRQF_NO_SUSPEND | IRQF_SHARED, "ab8500-gpadc-hw", drivers/pinctrl/pinctrl-single.c: IRQF_SHARED | IRQF_NO_SUSPEND, drivers/power/ab8500_btemp.c: IRQF_SHARED | IRQF_NO_SUSPEND, drivers/power/ab8500_charger.c: IRQF_SHARED | IRQF_NO_SUSPEND, drivers/power/ab8500_fg.c: IRQF_SHARED | IRQF_NO_SUSPEND, drivers/rtc/rtc-pl031.c:.irqflags = IRQF_SHARED | IRQF_NO_SUSPEND, drivers/usb/phy/phy-ab8500-usb.c: IRQF_NO_SUSPEND | IRQF_SHARED, drivers/usb/phy/phy-ab8500-usb.c: IRQF_NO_SUSPEND | IRQF_SHARED, drivers/usb/phy/phy-ab8500-usb.c: IRQF_NO_SUSPEND | IRQF_SHARED, drivers/watchdog/intel-mid_wdt.c: IRQF_SHARED | IRQF_NO_SUSPEND, "watchdog", Is there a single legitimate user in that list? If so, the TIMER name might be misleading. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Thu, Feb 12, 2015 at 11:09:17AM +, Boris Brezillon wrote:
> On Thu, 12 Feb 2015 10:52:15 +
> Mark Rutland wrote:
>
> > [...]
> >
> > > > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> > > > index d9b05b5..2b8ff50 100644
> > > > --- a/include/linux/interrupt.h
> > > > +++ b/include/linux/interrupt.h
> > > > @@ -57,6 +57,9 @@
> > > > * IRQF_NO_THREAD - Interrupt cannot be threaded
> > > > * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at
> > > > device
> > > > *resume time.
> > > > + * IRQF_SHARED_TIMER_OK - Interrupt is safe to be shared with a timer.
> > > > The
> > > > + *handler may be called spuriously during
> > > > suspend
> > > > + *without issue.
> > > > */
> > > > #define IRQF_DISABLED 0x0020
> > > > #define IRQF_SHARED0x0080
> > > > @@ -70,8 +73,10 @@
> > > > #define IRQF_FORCE_RESUME 0x8000
> > > > #define IRQF_NO_THREAD 0x0001
> > > > #define IRQF_EARLY_RESUME 0x0002
> > > > +#define __IRQF_TIMER_SIBLING_OK0x0004
> > > >
> > > > #define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND |
> > > > IRQF_NO_THREAD)
> > > > +#define IRQF_SHARED_TIMER_OK (IRQF_SHARED | __IRQF_TIMER_SIBLING_OK)
> > > >
> > > > /*
> > > > * These values can be returned by request_any_context_irq() and
> > > > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > > > index 3ca5325..e4ec91a 100644
> > > > --- a/kernel/irq/pm.c
> > > > +++ b/kernel/irq/pm.c
> > > > @@ -28,6 +28,47 @@ bool irq_pm_check_wakeup(struct irq_desc *desc)
> > > > }
> > > >
> > > > /*
> > > > + * Check whether an interrupt is safe to occur during suspend.
> > > > + *
> > > > + * Physical IRQ lines may be shared between devices which may be
> > > > expected to
> > > > + * raise interrupts during suspend (e.g. timers) and those which may
> > > > not (e.g.
> > > > + * anything we cut the power to). Not all handlers will be safe to
> > > > call during
> > > > + * suspend, so we need to scream if there's the possibility an unsafe
> > > > handler
> > > > + * will be called.
> > > > + *
> > > > + * A small number of handlers are safe to be shared with timer
> > > > interrupts, and
> > > > + * we don't want to warn erroneously for these. Such handlers will not
> > > > poke
> > > > + * hardware that's not powered or call into kernel infrastructure not
> > > > available
> > > > + * during suspend. These are marked with __IRQF_TIMER_SIBLING_OK.
> > > > + */
> > > > +bool irq_safe_during_suspend(struct irq_desc * desc, struct irqaction
> > > > *action)
> > > > +{
> > > > + const unsigned int safe_flags =
> > > > + __IRQF_TIMER_SIBLING_OK | IRQF_NO_SUSPEND;
> > > > +
> > > > + /*
> > > > +* If no-one wants to be called during suspend, or if everyone
> > > > does,
> > > > +* then there's no potential conflict.
> > > > +*/
> > > > + if (!desc->no_suspend_depth)
> > > > + return true;
> > > > + if (desc->no_suspend_depth == desc->nr_actions)
> > > > + return true;
>
> Just another nit, can't we also return early when
> desc->nr_actions == 1 (I mean, the handler cannot conflict with anything
> since it is the only one registered) ?
I guess we can, but that case is already covered by the above tests.
If the single action was not IRQF_NO_SUSPEND, then
desc->no_suspend_depth == 0, and we return early.
If the single action was IRQF_NO_SUSPEND, then
desc->no_suspend_depth == desc->nr_actions, and we return early.
We could change the second test to:
if (desc->nr_actions == 1 || desc->nr_actions == desc->no_suspend_depth)
...but I don't see that we gain much by doing so.
> > > > +
> > > > + /*
> > > > +* If any action hasn't asked to be called during suspend or is
> > > > not
> > > > +* happy to be called during suspend, we have a potential
> > > > problem.
> > > > +*/
> > > > + if (!(action->flags & safe_flags))
> > > > + return false;
> > > else if (!(action->flags & IRQF_NO_SUSPEND) ||
> > >desc->no_suspend_depth > 1)
> > > return true;
> > >
> > > Am I missing something or is the following loop only required if
> > > we're adding an action with the IRQF_NO_SUSPEND flag set for the
> > > first time ?
> >
> > With the check above we could return true incorrectly after the first
> > time we return true. Consider adding the following in order to an empty
> > desc:
> >
> > flags = IRQF_SHARED // safe, returns true
> > flags = IRQF_NO_SUSPEND // unsafe, returns false
> > flags = IRQF_NO_SUSPEND // unsafe, but returns true
>
> Yep, you're right.
>
> >
> > Currently it shouldn't matter as the only caller is a WARN_ON_ONCE(),
> > but it seems unfortunate to allow this.
>
> Absolutely, forget about that, I guess we don't have to optim
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Thu, 12 Feb 2015 10:52:15 +
Mark Rutland wrote:
> [...]
>
> > > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> > > index d9b05b5..2b8ff50 100644
> > > --- a/include/linux/interrupt.h
> > > +++ b/include/linux/interrupt.h
> > > @@ -57,6 +57,9 @@
> > > * IRQF_NO_THREAD - Interrupt cannot be threaded
> > > * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at
> > > device
> > > *resume time.
> > > + * IRQF_SHARED_TIMER_OK - Interrupt is safe to be shared with a timer.
> > > The
> > > + *handler may be called spuriously during suspend
> > > + *without issue.
> > > */
> > > #define IRQF_DISABLED0x0020
> > > #define IRQF_SHARED 0x0080
> > > @@ -70,8 +73,10 @@
> > > #define IRQF_FORCE_RESUME0x8000
> > > #define IRQF_NO_THREAD 0x0001
> > > #define IRQF_EARLY_RESUME0x0002
> > > +#define __IRQF_TIMER_SIBLING_OK 0x0004
> > >
> > > #define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND |
> > > IRQF_NO_THREAD)
> > > +#define IRQF_SHARED_TIMER_OK (IRQF_SHARED | __IRQF_TIMER_SIBLING_OK)
> > >
> > > /*
> > > * These values can be returned by request_any_context_irq() and
> > > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > > index 3ca5325..e4ec91a 100644
> > > --- a/kernel/irq/pm.c
> > > +++ b/kernel/irq/pm.c
> > > @@ -28,6 +28,47 @@ bool irq_pm_check_wakeup(struct irq_desc *desc)
> > > }
> > >
> > > /*
> > > + * Check whether an interrupt is safe to occur during suspend.
> > > + *
> > > + * Physical IRQ lines may be shared between devices which may be
> > > expected to
> > > + * raise interrupts during suspend (e.g. timers) and those which may not
> > > (e.g.
> > > + * anything we cut the power to). Not all handlers will be safe to call
> > > during
> > > + * suspend, so we need to scream if there's the possibility an unsafe
> > > handler
> > > + * will be called.
> > > + *
> > > + * A small number of handlers are safe to be shared with timer
> > > interrupts, and
> > > + * we don't want to warn erroneously for these. Such handlers will not
> > > poke
> > > + * hardware that's not powered or call into kernel infrastructure not
> > > available
> > > + * during suspend. These are marked with __IRQF_TIMER_SIBLING_OK.
> > > + */
> > > +bool irq_safe_during_suspend(struct irq_desc * desc, struct irqaction
> > > *action)
> > > +{
> > > + const unsigned int safe_flags =
> > > + __IRQF_TIMER_SIBLING_OK | IRQF_NO_SUSPEND;
> > > +
> > > + /*
> > > + * If no-one wants to be called during suspend, or if everyone does,
> > > + * then there's no potential conflict.
> > > + */
> > > + if (!desc->no_suspend_depth)
> > > + return true;
> > > + if (desc->no_suspend_depth == desc->nr_actions)
> > > + return true;
Just another nit, can't we also return early when
desc->nr_actions == 1 (I mean, the handler cannot conflict with anything
since it is the only one registered) ?
> > > +
> > > + /*
> > > + * If any action hasn't asked to be called during suspend or is not
> > > + * happy to be called during suspend, we have a potential problem.
> > > + */
> > > + if (!(action->flags & safe_flags))
> > > + return false;
> > else if (!(action->flags & IRQF_NO_SUSPEND) ||
> > desc->no_suspend_depth > 1)
> > return true;
> >
> > Am I missing something or is the following loop only required if
> > we're adding an action with the IRQF_NO_SUSPEND flag set for the
> > first time ?
>
> With the check above we could return true incorrectly after the first
> time we return true. Consider adding the following in order to an empty
> desc:
>
> flags = IRQF_SHARED // safe, returns true
> flags = IRQF_NO_SUSPEND // unsafe, returns false
> flags = IRQF_NO_SUSPEND // unsafe, but returns true
Yep, you're right.
>
> Currently it shouldn't matter as the only caller is a WARN_ON_ONCE(),
> but it seems unfortunate to allow this.
Absolutely, forget about that, I guess we don't have to optimize that
test anyway.
>
> We'd also run the loop until we had at least two IRQF_NO_SUSPEND
> irqactions:
>
> flags = IRQF_SHARED_TIMER_OK// early return
> flags = IRQF_NO_SUSPEND // run loop
> flags = IRQF_SHARED_TIMER_OK// run loop
Hm, no, this one would return directly (it's an '||' operator not an
'&&' one), because we're not adding an IRQF_NO_SUSPEND handler here, and
adding IRQF_SHARED_TIMER_OK is always safe, isn't it ?
> flags = IRQF_SHARED_TIMER_OK// run loop
> flags = IRQF_SHARED_TIMER_OK// run loop
> flags = IRQF_NO_SUSPEND // don't run loop.
> flags = IRQF_SHARED_TIMER_OK// don't run loop
>
> I assume that we only have one IRQF_NO_SUSPEND action sharing the line
> anyway in your case?
Yep.
>
> Given that we'll only bother to run the
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
[...]
> > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> > index d9b05b5..2b8ff50 100644
> > --- a/include/linux/interrupt.h
> > +++ b/include/linux/interrupt.h
> > @@ -57,6 +57,9 @@
> > * IRQF_NO_THREAD - Interrupt cannot be threaded
> > * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device
> > *resume time.
> > + * IRQF_SHARED_TIMER_OK - Interrupt is safe to be shared with a timer. The
> > + *handler may be called spuriously during suspend
> > + *without issue.
> > */
> > #define IRQF_DISABLED 0x0020
> > #define IRQF_SHARED0x0080
> > @@ -70,8 +73,10 @@
> > #define IRQF_FORCE_RESUME 0x8000
> > #define IRQF_NO_THREAD 0x0001
> > #define IRQF_EARLY_RESUME 0x0002
> > +#define __IRQF_TIMER_SIBLING_OK0x0004
> >
> > #define IRQF_TIMER (__IRQF_TIMER | IRQF_NO_SUSPEND |
> > IRQF_NO_THREAD)
> > +#define IRQF_SHARED_TIMER_OK (IRQF_SHARED | __IRQF_TIMER_SIBLING_OK)
> >
> > /*
> > * These values can be returned by request_any_context_irq() and
> > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > index 3ca5325..e4ec91a 100644
> > --- a/kernel/irq/pm.c
> > +++ b/kernel/irq/pm.c
> > @@ -28,6 +28,47 @@ bool irq_pm_check_wakeup(struct irq_desc *desc)
> > }
> >
> > /*
> > + * Check whether an interrupt is safe to occur during suspend.
> > + *
> > + * Physical IRQ lines may be shared between devices which may be expected
> > to
> > + * raise interrupts during suspend (e.g. timers) and those which may not
> > (e.g.
> > + * anything we cut the power to). Not all handlers will be safe to call
> > during
> > + * suspend, so we need to scream if there's the possibility an unsafe
> > handler
> > + * will be called.
> > + *
> > + * A small number of handlers are safe to be shared with timer interrupts,
> > and
> > + * we don't want to warn erroneously for these. Such handlers will not poke
> > + * hardware that's not powered or call into kernel infrastructure not
> > available
> > + * during suspend. These are marked with __IRQF_TIMER_SIBLING_OK.
> > + */
> > +bool irq_safe_during_suspend(struct irq_desc * desc, struct irqaction
> > *action)
> > +{
> > + const unsigned int safe_flags =
> > + __IRQF_TIMER_SIBLING_OK | IRQF_NO_SUSPEND;
> > +
> > + /*
> > +* If no-one wants to be called during suspend, or if everyone does,
> > +* then there's no potential conflict.
> > +*/
> > + if (!desc->no_suspend_depth)
> > + return true;
> > + if (desc->no_suspend_depth == desc->nr_actions)
> > + return true;
> > +
> > + /*
> > +* If any action hasn't asked to be called during suspend or is not
> > +* happy to be called during suspend, we have a potential problem.
> > +*/
> > + if (!(action->flags & safe_flags))
> > + return false;
> else if (!(action->flags & IRQF_NO_SUSPEND) ||
>desc->no_suspend_depth > 1)
> return true;
>
> Am I missing something or is the following loop only required if
> we're adding an action with the IRQF_NO_SUSPEND flag set for the
> first time ?
With the check above we could return true incorrectly after the first
time we return true. Consider adding the following in order to an empty
desc:
flags = IRQF_SHARED // safe, returns true
flags = IRQF_NO_SUSPEND // unsafe, returns false
flags = IRQF_NO_SUSPEND // unsafe, but returns true
Currently it shouldn't matter as the only caller is a WARN_ON_ONCE(),
but it seems unfortunate to allow this.
We'd also run the loop until we had at least two IRQF_NO_SUSPEND
irqactions:
flags = IRQF_SHARED_TIMER_OK// early return
flags = IRQF_NO_SUSPEND // run loop
flags = IRQF_SHARED_TIMER_OK// run loop
flags = IRQF_SHARED_TIMER_OK// run loop
flags = IRQF_SHARED_TIMER_OK// run loop
flags = IRQF_NO_SUSPEND // don't run loop.
flags = IRQF_SHARED_TIMER_OK// don't run loop
I assume that we only have one IRQF_NO_SUSPEND action sharing the line
anyway in your case?
Given that we'll only bother to run the test if there's a mismatch
between desc->no_suspend_depth and desc->nr_actions, I don't think we
win much. These cases should be rare in practice, the tests only
performed when we request the irq, and there shouldn't be that many
actions to loop over.
Thanks,
Mark.
>
> > + for (action = desc->action; action; action = action->next)
> > + if (!(action->flags & safe_flags))
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +/*
> > * Called from __setup_irq() with desc->lock held after @action has
> > * been installed in the action chain.
> > */
> > @@ -44,8 +85,7 @@ void irq_pm_install_action(struct irq_desc *desc, struct
> > irqaction *action)
> > if (action->f
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, 11 Feb 2015 17:13:13 + Mark Rutland wrote: > On Wed, Feb 11, 2015 at 04:42:22PM +, Rafael J. Wysocki wrote: > > On Wednesday, February 11, 2015 05:15:15 PM Boris Brezillon wrote: > > > On Wed, 11 Feb 2015 15:57:20 + > > > Mark Rutland wrote: > > > > > > > [...] > > > > > > > > > > > > So for the flag at request time approach to work, all the > > > > > > > > drivers using > > > > > > > > the interrupt would have to flag they're safe in that context. > > > > > > > > > > > > > > Something like IRQF_"I can share the line with a timer" I guess? > > > > > > > That wouldn't > > > > > > > hurt and can be checked at request time even. > > > > > > > > > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > > > > > like: > > > > > > > > > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously > > > > > > during > > > > > > suspend in the case the line is shared. The > > > > > > handler will not access unavailable hardware > > > > > > or kernel infrastructure during this period. > > > > > > > > > > > > #define __IRQF_SUSPEND_SPURIOUS 0x0004 > > > > > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | > > > > > > __IRQF_SUSPEND_SPURIOUS) > > > > > > > > > > What about > > > > > > > > > > #define __IRQF_TIMER_SIBLING_OK 0x0004 > > > > > #define IRQF_SHARED_TIMER_OK(IRQF_SHARED | > > > > > __IRQF_TIMER_SIBLING_OK) > > > > > > > > > > The "suspend" part is kind of a distraction to me here, because that > > > > > really > > > > > only is about sharing an IRQ with a timer and the "your interrupt > > > > > handler > > > > > may be called when the device is suspended" part is just a > > > > > consequence of that. > > > > > > > > My rationale was that you didn't really care who else was using the IRQ > > > > (e.g. the timer); you're just stating that you can survive being called > > > > during suspend (which is what the driver may need to check for in the > > > > handler if the device happens to be powered down or whatever). > > > > > > > > So I guess I see it the other way around. This is essentially claiming > > > > we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > > > > > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging > > > > > people to > > > > > abuse this for other purposes not related to timers. > > > > > > > > In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is > > > > better I shan't complain. > > > > > > > > The fundamental issue I'm concerned with is addressed by this approach. > > > > > > Okay then, is anyone taking care of submitting such a patch (Mark ?) ? > > > > Well, I guess I should take the responsibility for that. :-) > > > > I'll try to cut one later today or tomorrow unless someone else beats me to > > that. > > I had a go at the core part below. Does it look like what you had in > mind? > > I've given it a go on a hacked-up platform, but I don't have any at91 > stuff to test with, so I haven't bothered with the driver portions just > yet. > > Thanks, > Mark. > > >8 > From 2d9013517637bb567fbcde3e20797cb2fab1c4c5 Mon Sep 17 00:00:00 2001 > From: Mark Rutland > Date: Wed, 11 Feb 2015 16:44:06 + > Subject: [PATCH] genirq: allow safe sharing of irqs during suspend > > In some cases a physical IRQ line may be shared between devices from > which we expect interrupts during suspend (e.g. timers) and those we do > not (e.g. anything we cut the power to). Where a driver did not request > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > being called during suspend, and where the IRQ PM code detects a > mismatch it produces a loud warning (via WARN_ON_ONCE). > > In a small set of cases the handlers for the devices other than timers > can tolerate being called during suspend time. In these cases the > warning is spurious, and masks other potentially unsafe mismatches as it > is only printed for the first mismatch detected. As the behaviour of the > handlers is an implementation detail, we cannot rely on external data to > decide when it is safe for a given interrupt line to be shared in this > manner. > > This patch adds a new flag, IRQF_SHARED_TIMER_OK, which drivers can use > when requesting an IRQ to state that they can cope if the interrupt is > shared with a timer driver (and hence may be raised during suspend). The > PM code is updated to only warn when a mismatch occurs and at least one > irqaction has neither asked to be called during suspend or has stated it > is safe to be called during suspend. > > This reduces the set of warnings to those cases where there is a real > problem. While it is possible that this flag may be abused, any such > abuses will be explicit in the kernel source and can be detected. > > Cc: Boris Brezillon > Cc: Jason Cooper > Cc: Nicolas Ferre > Cc: Peter Zijlstra > Cc: Raf
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, Feb 11, 2015 at 04:38:23PM +, Boris Brezillon wrote: > On Wed, 11 Feb 2015 16:32:31 + > Mark Rutland wrote: > > > On Wed, Feb 11, 2015 at 04:15:15PM +, Boris Brezillon wrote: > > > On Wed, 11 Feb 2015 15:57:20 + > > > Mark Rutland wrote: > > > > > > > [...] > > > > > > > > > > > > So for the flag at request time approach to work, all the > > > > > > > > drivers using > > > > > > > > the interrupt would have to flag they're safe in that context. > > > > > > > > > > > > > > Something like IRQF_"I can share the line with a timer" I guess? > > > > > > > That wouldn't > > > > > > > hurt and can be checked at request time even. > > > > > > > > > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > > > > > like: > > > > > > > > > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously > > > > > > during > > > > > > suspend in the case the line is shared. The > > > > > > handler will not access unavailable hardware > > > > > > or kernel infrastructure during this period. > > > > > > > > > > > > #define __IRQF_SUSPEND_SPURIOUS 0x0004 > > > > > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | > > > > > > __IRQF_SUSPEND_SPURIOUS) > > > > > > > > > > What about > > > > > > > > > > #define __IRQF_TIMER_SIBLING_OK 0x0004 > > > > > #define IRQF_SHARED_TIMER_OK(IRQF_SHARED | > > > > > __IRQF_TIMER_SIBLING_OK) > > > > > > > > > > The "suspend" part is kind of a distraction to me here, because that > > > > > really > > > > > only is about sharing an IRQ with a timer and the "your interrupt > > > > > handler > > > > > may be called when the device is suspended" part is just a > > > > > consequence of that. > > > > > > > > My rationale was that you didn't really care who else was using the IRQ > > > > (e.g. the timer); you're just stating that you can survive being called > > > > during suspend (which is what the driver may need to check for in the > > > > handler if the device happens to be powered down or whatever). > > > > > > > > So I guess I see it the other way around. This is essentially claiming > > > > we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > > > > > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging > > > > > people to > > > > > abuse this for other purposes not related to timers. > > > > > > > > In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is > > > > better I shan't complain. > > > > > > > > The fundamental issue I'm concerned with is addressed by this approach. > > > > > > Okay then, is anyone taking care of submitting such a patch (Mark ?) ? > > > > I'll have the core patch shortly. > > > > I'll need to ask for your help tagging the relevant drivers and testing. > > For the list of impacted drivers, you can have a look at this series [1] > (patches 2 to 5), and I'll take care of the testing part once every one > has agreed on the solution ;-). > > [1]https://lkml.org/lkml/2014/12/15/552 Thanks for the link. I'll take a look at this once Rafael's given the core patch a once-over. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, Feb 11, 2015 at 04:42:22PM +, Rafael J. Wysocki wrote: > On Wednesday, February 11, 2015 05:15:15 PM Boris Brezillon wrote: > > On Wed, 11 Feb 2015 15:57:20 + > > Mark Rutland wrote: > > > > > [...] > > > > > > > > > > So for the flag at request time approach to work, all the drivers > > > > > > > using > > > > > > > the interrupt would have to flag they're safe in that context. > > > > > > > > > > > > Something like IRQF_"I can share the line with a timer" I guess? > > > > > > That wouldn't > > > > > > hurt and can be checked at request time even. > > > > > > > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > > > > like: > > > > > > > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously > > > > > during > > > > >suspend in the case the line is shared. The > > > > >handler will not access unavailable hardware > > > > >or kernel infrastructure during this period. > > > > > > > > > > #define __IRQF_SUSPEND_SPURIOUS 0x0004 > > > > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | > > > > > __IRQF_SUSPEND_SPURIOUS) > > > > > > > > What about > > > > > > > > #define __IRQF_TIMER_SIBLING_OK 0x0004 > > > > #define IRQF_SHARED_TIMER_OK(IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > > > > > > > The "suspend" part is kind of a distraction to me here, because that > > > > really > > > > only is about sharing an IRQ with a timer and the "your interrupt > > > > handler > > > > may be called when the device is suspended" part is just a consequence > > > > of that. > > > > > > My rationale was that you didn't really care who else was using the IRQ > > > (e.g. the timer); you're just stating that you can survive being called > > > during suspend (which is what the driver may need to check for in the > > > handler if the device happens to be powered down or whatever). > > > > > > So I guess I see it the other way around. This is essentially claiming > > > we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > > > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging > > > > people to > > > > abuse this for other purposes not related to timers. > > > > > > In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is > > > better I shan't complain. > > > > > > The fundamental issue I'm concerned with is addressed by this approach. > > > > Okay then, is anyone taking care of submitting such a patch (Mark ?) ? > > Well, I guess I should take the responsibility for that. :-) > > I'll try to cut one later today or tomorrow unless someone else beats me to > that. I had a go at the core part below. Does it look like what you had in mind? I've given it a go on a hacked-up platform, but I don't have any at91 stuff to test with, so I haven't bothered with the driver portions just yet. Thanks, Mark. >8 >From 2d9013517637bb567fbcde3e20797cb2fab1c4c5 Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Wed, 11 Feb 2015 16:44:06 + Subject: [PATCH] genirq: allow safe sharing of irqs during suspend In some cases a physical IRQ line may be shared between devices from which we expect interrupts during suspend (e.g. timers) and those we do not (e.g. anything we cut the power to). Where a driver did not request the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle being called during suspend, and where the IRQ PM code detects a mismatch it produces a loud warning (via WARN_ON_ONCE). In a small set of cases the handlers for the devices other than timers can tolerate being called during suspend time. In these cases the warning is spurious, and masks other potentially unsafe mismatches as it is only printed for the first mismatch detected. As the behaviour of the handlers is an implementation detail, we cannot rely on external data to decide when it is safe for a given interrupt line to be shared in this manner. This patch adds a new flag, IRQF_SHARED_TIMER_OK, which drivers can use when requesting an IRQ to state that they can cope if the interrupt is shared with a timer driver (and hence may be raised during suspend). The PM code is updated to only warn when a mismatch occurs and at least one irqaction has neither asked to be called during suspend or has stated it is safe to be called during suspend. This reduces the set of warnings to those cases where there is a real problem. While it is possible that this flag may be abused, any such abuses will be explicit in the kernel source and can be detected. Cc: Boris Brezillon Cc: Jason Cooper Cc: Nicolas Ferre Cc: Peter Zijlstra Cc: Rafael J. Wysocki Cc: Thomas Gleixner Signed-off-by: Mark Rutland --- include/linux/interrupt.h | 5 + kernel/irq/pm.c | 44 ++-- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/include/linux/interrupt.h b/include/linux
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, 11 Feb 2015 16:32:31 + Mark Rutland wrote: > On Wed, Feb 11, 2015 at 04:15:15PM +, Boris Brezillon wrote: > > On Wed, 11 Feb 2015 15:57:20 + > > Mark Rutland wrote: > > > > > [...] > > > > > > > > > > So for the flag at request time approach to work, all the drivers > > > > > > > using > > > > > > > the interrupt would have to flag they're safe in that context. > > > > > > > > > > > > Something like IRQF_"I can share the line with a timer" I guess? > > > > > > That wouldn't > > > > > > hurt and can be checked at request time even. > > > > > > > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > > > > like: > > > > > > > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously > > > > > during > > > > >suspend in the case the line is shared. The > > > > >handler will not access unavailable hardware > > > > >or kernel infrastructure during this period. > > > > > > > > > > #define __IRQF_SUSPEND_SPURIOUS 0x0004 > > > > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | > > > > > __IRQF_SUSPEND_SPURIOUS) > > > > > > > > What about > > > > > > > > #define __IRQF_TIMER_SIBLING_OK 0x0004 > > > > #define IRQF_SHARED_TIMER_OK(IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > > > > > > > The "suspend" part is kind of a distraction to me here, because that > > > > really > > > > only is about sharing an IRQ with a timer and the "your interrupt > > > > handler > > > > may be called when the device is suspended" part is just a consequence > > > > of that. > > > > > > My rationale was that you didn't really care who else was using the IRQ > > > (e.g. the timer); you're just stating that you can survive being called > > > during suspend (which is what the driver may need to check for in the > > > handler if the device happens to be powered down or whatever). > > > > > > So I guess I see it the other way around. This is essentially claiming > > > we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > > > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging > > > > people to > > > > abuse this for other purposes not related to timers. > > > > > > In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is > > > better I shan't complain. > > > > > > The fundamental issue I'm concerned with is addressed by this approach. > > > > Okay then, is anyone taking care of submitting such a patch (Mark ?) ? > > I'll have the core patch shortly. > > I'll need to ask for your help tagging the relevant drivers and testing. For the list of impacted drivers, you can have a look at this series [1] (patches 2 to 5), and I'll take care of the testing part once every one has agreed on the solution ;-). [1]https://lkml.org/lkml/2014/12/15/552 -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, Feb 11, 2015 at 04:15:15PM +, Boris Brezillon wrote: > On Wed, 11 Feb 2015 15:57:20 + > Mark Rutland wrote: > > > [...] > > > > > > > > So for the flag at request time approach to work, all the drivers > > > > > > using > > > > > > the interrupt would have to flag they're safe in that context. > > > > > > > > > > Something like IRQF_"I can share the line with a timer" I guess? > > > > > That wouldn't > > > > > hurt and can be checked at request time even. > > > > > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > > > like: > > > > > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during > > > > suspend in the case the line is shared. The > > > > handler will not access unavailable hardware > > > > or kernel infrastructure during this period. > > > > > > > > #define __IRQF_SUSPEND_SPURIOUS 0x0004 > > > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | > > > > __IRQF_SUSPEND_SPURIOUS) > > > > > > What about > > > > > > #define __IRQF_TIMER_SIBLING_OK 0x0004 > > > #define IRQF_SHARED_TIMER_OK(IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > > > > > The "suspend" part is kind of a distraction to me here, because that > > > really > > > only is about sharing an IRQ with a timer and the "your interrupt handler > > > may be called when the device is suspended" part is just a consequence of > > > that. > > > > My rationale was that you didn't really care who else was using the IRQ > > (e.g. the timer); you're just stating that you can survive being called > > during suspend (which is what the driver may need to check for in the > > handler if the device happens to be powered down or whatever). > > > > So I guess I see it the other way around. This is essentially claiming > > we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging > > > people to > > > abuse this for other purposes not related to timers. > > > > In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is > > better I shan't complain. > > > > The fundamental issue I'm concerned with is addressed by this approach. > > Okay then, is anyone taking care of submitting such a patch (Mark ?) ? I'll have the core patch shortly. I'll need to ask for your help tagging the relevant drivers and testing. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, 11 Feb 2015 17:42:22 +0100 "Rafael J. Wysocki" wrote: > On Wednesday, February 11, 2015 05:15:15 PM Boris Brezillon wrote: > > On Wed, 11 Feb 2015 15:57:20 + > > Mark Rutland wrote: > > > > > [...] > > > > > > > > > > So for the flag at request time approach to work, all the drivers > > > > > > > using > > > > > > > the interrupt would have to flag they're safe in that context. > > > > > > > > > > > > Something like IRQF_"I can share the line with a timer" I guess? > > > > > > That wouldn't > > > > > > hurt and can be checked at request time even. > > > > > > > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > > > > like: > > > > > > > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously > > > > > during > > > > >suspend in the case the line is shared. The > > > > >handler will not access unavailable hardware > > > > >or kernel infrastructure during this period. > > > > > > > > > > #define __IRQF_SUSPEND_SPURIOUS 0x0004 > > > > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | > > > > > __IRQF_SUSPEND_SPURIOUS) > > > > > > > > What about > > > > > > > > #define __IRQF_TIMER_SIBLING_OK 0x0004 > > > > #define IRQF_SHARED_TIMER_OK(IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > > > > > > > The "suspend" part is kind of a distraction to me here, because that > > > > really > > > > only is about sharing an IRQ with a timer and the "your interrupt > > > > handler > > > > may be called when the device is suspended" part is just a consequence > > > > of that. > > > > > > My rationale was that you didn't really care who else was using the IRQ > > > (e.g. the timer); you're just stating that you can survive being called > > > during suspend (which is what the driver may need to check for in the > > > handler if the device happens to be powered down or whatever). > > > > > > So I guess I see it the other way around. This is essentially claiming > > > we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > > > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging > > > > people to > > > > abuse this for other purposes not related to timers. > > > > > > In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is > > > better I shan't complain. > > > > > > The fundamental issue I'm concerned with is addressed by this approach. > > > > Okay then, is anyone taking care of submitting such a patch (Mark ?) ? > > Well, I guess I should take the responsibility for that. :-) > > I'll try to cut one later today or tomorrow unless someone else beats me to > that. I won't (I'm done with these irq stuff for now ;-)). Peter, if this patch is accepted, I guess you'll have to drop (or revert my patches). Thanks, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wednesday, February 11, 2015 05:15:15 PM Boris Brezillon wrote: > On Wed, 11 Feb 2015 15:57:20 + > Mark Rutland wrote: > > > [...] > > > > > > > > So for the flag at request time approach to work, all the drivers > > > > > > using > > > > > > the interrupt would have to flag they're safe in that context. > > > > > > > > > > Something like IRQF_"I can share the line with a timer" I guess? > > > > > That wouldn't > > > > > hurt and can be checked at request time even. > > > > > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > > > like: > > > > > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during > > > > suspend in the case the line is shared. The > > > > handler will not access unavailable hardware > > > > or kernel infrastructure during this period. > > > > > > > > #define __IRQF_SUSPEND_SPURIOUS 0x0004 > > > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | > > > > __IRQF_SUSPEND_SPURIOUS) > > > > > > What about > > > > > > #define __IRQF_TIMER_SIBLING_OK 0x0004 > > > #define IRQF_SHARED_TIMER_OK(IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > > > > > The "suspend" part is kind of a distraction to me here, because that > > > really > > > only is about sharing an IRQ with a timer and the "your interrupt handler > > > may be called when the device is suspended" part is just a consequence of > > > that. > > > > My rationale was that you didn't really care who else was using the IRQ > > (e.g. the timer); you're just stating that you can survive being called > > during suspend (which is what the driver may need to check for in the > > handler if the device happens to be powered down or whatever). > > > > So I guess I see it the other way around. This is essentially claiming > > we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > > > > > So IMO it's better to have "TIMER" in the names to avoid encouraging > > > people to > > > abuse this for other purposes not related to timers. > > > > In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is > > better I shan't complain. > > > > The fundamental issue I'm concerned with is addressed by this approach. > > Okay then, is anyone taking care of submitting such a patch (Mark ?) ? Well, I guess I should take the responsibility for that. :-) I'll try to cut one later today or tomorrow unless someone else beats me to that. Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, 11 Feb 2015 15:57:20 + Mark Rutland wrote: > [...] > > > > > > So for the flag at request time approach to work, all the drivers > > > > > using > > > > > the interrupt would have to flag they're safe in that context. > > > > > > > > Something like IRQF_"I can share the line with a timer" I guess? That > > > > wouldn't > > > > hurt and can be checked at request time even. > > > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > > like: > > > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during > > >suspend in the case the line is shared. The > > >handler will not access unavailable hardware > > >or kernel infrastructure during this period. > > > > > > #define __IRQF_SUSPEND_SPURIOUS 0x0004 > > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | > > > __IRQF_SUSPEND_SPURIOUS) > > > > What about > > > > #define __IRQF_TIMER_SIBLING_OK 0x0004 > > #define IRQF_SHARED_TIMER_OK(IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > > > The "suspend" part is kind of a distraction to me here, because that really > > only is about sharing an IRQ with a timer and the "your interrupt handler > > may be called when the device is suspended" part is just a consequence of > > that. > > My rationale was that you didn't really care who else was using the IRQ > (e.g. the timer); you're just stating that you can survive being called > during suspend (which is what the driver may need to check for in the > handler if the device happens to be powered down or whatever). > > So I guess I see it the other way around. This is essentially claiming > we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > > > So IMO it's better to have "TIMER" in the names to avoid encouraging people > > to > > abuse this for other purposes not related to timers. > > In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is > better I shan't complain. > > The fundamental issue I'm concerned with is addressed by this approach. Okay then, is anyone taking care of submitting such a patch (Mark ?) ? -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
[...] > > > > So for the flag at request time approach to work, all the drivers using > > > > the interrupt would have to flag they're safe in that context. > > > > > > Something like IRQF_"I can share the line with a timer" I guess? That > > > wouldn't > > > hurt and can be checked at request time even. > > > > I guess that would have to imply IRQF_SHARED, so we'd have something > > like: > > > > IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during > > suspend in the case the line is shared. The > > handler will not access unavailable hardware > > or kernel infrastructure during this period. > > > > #define __IRQF_SUSPEND_SPURIOUS 0x0004 > > #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | > > __IRQF_SUSPEND_SPURIOUS) > > What about > > #define __IRQF_TIMER_SIBLING_OK 0x0004 > #define IRQF_SHARED_TIMER_OK(IRQF_SHARED | __IRQF_TIMER_SIBLING_OK) > > The "suspend" part is kind of a distraction to me here, because that really > only is about sharing an IRQ with a timer and the "your interrupt handler > may be called when the device is suspended" part is just a consequence of > that. My rationale was that you didn't really care who else was using the IRQ (e.g. the timer); you're just stating that you can survive being called during suspend (which is what the driver may need to check for in the handler if the device happens to be powered down or whatever). So I guess I see it the other way around. This is essentially claiming we can handle sharing with IRQF_NO_SUSPEND rather than IRQF_TIMER. > So IMO it's better to have "TIMER" in the names to avoid encouraging people to > abuse this for other purposes not related to timers. In the end a name is a name, and if you think IRQF_SHARED_TIMER_OK is better I shan't complain. The fundamental issue I'm concerned with is addressed by this approach. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wednesday, February 11, 2015 03:12:38 PM Mark Rutland wrote:
> On Wed, Feb 11, 2015 at 03:17:20PM +, Rafael J. Wysocki wrote:
> > On Wednesday, February 11, 2015 02:43:45 PM Mark Rutland wrote:
> > > [...]
> > >
> > > > > > > > +static irqreturn_t __handle_irq_event_percpu(unsigned int irq,
> > > > > > > > struct irqaction *action)
> > > > > > > > +{
> > > > > > > > + /*
> > > > > > > > +* During suspend we must not call potentially unsafe
> > > > > > > > irq handlers.
> > > > > > > > +* See suspend_suspendable_actions.
> > > > > > > > +*/
> > > > > > > > + if (unlikely(action->flags & IRQF_NO_ACTION))
> > > > > > > > + return IRQ_NONE;
> > > > > > >
> > > > > > > Thomas was trying to avoid any new conditional code in the
> > > > > > > interrupt
> > > > > > > handling path, that's why I added a suspended_action list in my
> > > > > > > proposal.
> > > > > > > Even if your 'unlikely' statement make things better I'm pretty
> > > > > > > sure it
> > > > > > > adds some latency.
> > > > > >
> > > > > > I can see that we don't want to add more code here to keep things
> > > > > > clean/pure, but I find it hard to believe that a single bit test and
> > > > > > branch (for data that should be hot in the cache) are going to add
> > > > > > measurable latency to a path that does pointer chasing to get to the
> > > > > > irqaction in the first place. I could be wrong though, and I'm
> > > > > > happy to
> > > > > > benchmark.
> > > > >
> > > > > Again, I don't have enough experience to say this is (or isn't)
> > > > > impacting irq handling latency, I'm just reporting what Thomas told
> > > > > me.
> > > > >
> > > > > >
> > > > > > It would be possible to go for your list shuffling approach here
> > > > > > while
> > > > > > still keeping the flag internal and all the logic hidden away in
> > > > > > kernel/irq/pm.c. I wasn't sure how actions could be manipulated
> > > > > > during
> > > > > > suspend, which made me wary of moving them to a separate list.
> > > > >
> > > > > Moving them to a temporary list on suspend and restoring them on
> > > > > resume should not be a problem.
> > > > > The only drawback I see is that actions might be reordered after the
> > > > > first resume (anyway, relying on shared irq action order is dangerous
> > > > > IMHO).
> > > >
> > > > We considered doing that too and saw some drawbacks (in addition to the
> > > > reordering of actions you've mentioned). It added just too much
> > > > complexity
> > > > to the IRQ suspend-resume code.
> > > >
> > > > I, personally, would be fine with adding an IRQ flag to silence the
> > > > warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that
> > > > would
> > > > be set automatically if someone requested IRQF_TIMER | IRQF_SHARED.
> > > >
> > > > Thoughts?
> > >
> > > Even if the timer driver does that, we still require the other handlers
> > > sharing the line to do the right thing across suspend, no? So either
> > > their actions need to be masked at suspend time, or the handlers need to
> > > detect when they're called during suspend and return early.
> >
> > Well, the issue at hand is about things that share an IRQ with a timer
> > AFAICS.
> >
> > That is odd enough already and I'd say everyone in that situation needs to
> > be prepared to take the pain (including having to check if the device is not
> > suspended in their interrupt handlers).
>
> IMO if the line is shared it would be ideal for the core to mask the
> action (as that's essentially the behaviour when the line isn't shared
> with an IRQF_NO_SUSPEND action), but that's not esseential if a flag is
> OK for now.
>
> > And quite frankly they need to do that already, because we've never
> > suspended
> > timer IRQs.
>
> This is a very good point.
>
> > > So for the flag at request time approach to work, all the drivers using
> > > the interrupt would have to flag they're safe in that context.
> >
> > Something like IRQF_"I can share the line with a timer" I guess? That
> > wouldn't
> > hurt and can be checked at request time even.
>
> I guess that would have to imply IRQF_SHARED, so we'd have something
> like:
>
> IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during
>suspend in the case the line is shared. The
>handler will not access unavailable hardware
>or kernel infrastructure during this period.
>
> #define __IRQF_SUSPEND_SPURIOUS 0x0004
> #define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED |
> __IRQF_SUSPEND_SPURIOUS)
What about
#define __IRQF_TIMER_SIBLING_OK 0x0004
#define IRQF_SHARED_TIMER_OK(IRQF_SHARED | __IRQF_TIMER_SIBLING_OK)
The "suspend" part is kind of a distraction to me here, because that really
only is about sharing an IRQ with a timer and the "your interrupt handler
may be called when the device is suspended" part is just a conseq
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, Feb 11, 2015 at 03:39:48PM +, Rafael J. Wysocki wrote:
> On Wednesday, February 11, 2015 04:03:17 PM Boris Brezillon wrote:
> > On Wed, 11 Feb 2015 16:17:20 +0100
> > "Rafael J. Wysocki" wrote:
> >
> > > On Wednesday, February 11, 2015 02:43:45 PM Mark Rutland wrote:
> > > > [...]
> > > >
> > > > > > > > > +static irqreturn_t __handle_irq_event_percpu(unsigned int
> > > > > > > > > irq, struct irqaction *action)
> > > > > > > > > +{
> > > > > > > > > + /*
> > > > > > > > > + * During suspend we must not call potentially unsafe
> > > > > > > > > irq handlers.
> > > > > > > > > + * See suspend_suspendable_actions.
> > > > > > > > > + */
> > > > > > > > > + if (unlikely(action->flags & IRQF_NO_ACTION))
> > > > > > > > > + return IRQ_NONE;
> > > > > > > >
> > > > > > > > Thomas was trying to avoid any new conditional code in the
> > > > > > > > interrupt
> > > > > > > > handling path, that's why I added a suspended_action list in my
> > > > > > > > proposal.
> > > > > > > > Even if your 'unlikely' statement make things better I'm pretty
> > > > > > > > sure it
> > > > > > > > adds some latency.
> > > > > > >
> > > > > > > I can see that we don't want to add more code here to keep things
> > > > > > > clean/pure, but I find it hard to believe that a single bit test
> > > > > > > and
> > > > > > > branch (for data that should be hot in the cache) are going to add
> > > > > > > measurable latency to a path that does pointer chasing to get to
> > > > > > > the
> > > > > > > irqaction in the first place. I could be wrong though, and I'm
> > > > > > > happy to
> > > > > > > benchmark.
> > > > > >
> > > > > > Again, I don't have enough experience to say this is (or isn't)
> > > > > > impacting irq handling latency, I'm just reporting what Thomas told
> > > > > > me.
> > > > > >
> > > > > > >
> > > > > > > It would be possible to go for your list shuffling approach here
> > > > > > > while
> > > > > > > still keeping the flag internal and all the logic hidden away in
> > > > > > > kernel/irq/pm.c. I wasn't sure how actions could be manipulated
> > > > > > > during
> > > > > > > suspend, which made me wary of moving them to a separate list.
> > > > > >
> > > > > > Moving them to a temporary list on suspend and restoring them on
> > > > > > resume should not be a problem.
> > > > > > The only drawback I see is that actions might be reordered after the
> > > > > > first resume (anyway, relying on shared irq action order is
> > > > > > dangerous
> > > > > > IMHO).
> > > > >
> > > > > We considered doing that too and saw some drawbacks (in addition to
> > > > > the
> > > > > reordering of actions you've mentioned). It added just too much
> > > > > complexity
> > > > > to the IRQ suspend-resume code.
> > > > >
> > > > > I, personally, would be fine with adding an IRQ flag to silence the
> > > > > warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED
> > > > > that would
> > > > > be set automatically if someone requested IRQF_TIMER | IRQF_SHARED.
> > > > >
> > > > > Thoughts?
> > > >
> > > > Even if the timer driver does that, we still require the other handlers
> > > > sharing the line to do the right thing across suspend, no? So either
> > > > their actions need to be masked at suspend time, or the handlers need to
> > > > detect when they're called during suspend and return early.
> > >
> > > Well, the issue at hand is about things that share an IRQ with a timer
> > > AFAICS.
> > >
> > > That is odd enough already and I'd say everyone in that situation needs to
> > > be prepared to take the pain (including having to check if the device is
> > > not
> > > suspended in their interrupt handlers).
> > >
> > > And quite frankly they need to do that already, because we've never
> > > suspended
> > > timer IRQs.
> > >
> > > > So for the flag at request time approach to work, all the drivers using
> > > > the interrupt would have to flag they're safe in that context.
> > >
> > > Something like IRQF_"I can share the line with a timer" I guess? That
> > > wouldn't
> > > hurt and can be checked at request time even.
> > >
> > > > I'm not averse to having that (only a few drivers shuold be affected and
> > > > we can sanity check them now).
> > >
> > > Right.
> >
> > Okay, if everyone agrees on this solution, then I'm fine with that too
> > (even if I'm a bit disappointed to have spent so much time on this
> > problem to eventually end-up with a simple IRQF_SHARED_TIMER flag :-().
>
> IRQD_SHARED_TIMER (that needs to be an IRQ flag, not an irqaction one).
Surely for the drivers to be able to announce that they can handle
suspend safely this has to be an irqaction flag?
For IRQD_SHARED_TIMER to work the irqchip would need to set this, and it
doesn't know anything about the drivers (or potentially what any of the
interrupts are if the block is reused).
> And spending time on things that never go in happens a lot in the core
> develo
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wednesday, February 11, 2015 04:03:17 PM Boris Brezillon wrote:
> On Wed, 11 Feb 2015 16:17:20 +0100
> "Rafael J. Wysocki" wrote:
>
> > On Wednesday, February 11, 2015 02:43:45 PM Mark Rutland wrote:
> > > [...]
> > >
> > > > > > > > +static irqreturn_t __handle_irq_event_percpu(unsigned int irq,
> > > > > > > > struct irqaction *action)
> > > > > > > > +{
> > > > > > > > + /*
> > > > > > > > +* During suspend we must not call potentially unsafe
> > > > > > > > irq handlers.
> > > > > > > > +* See suspend_suspendable_actions.
> > > > > > > > +*/
> > > > > > > > + if (unlikely(action->flags & IRQF_NO_ACTION))
> > > > > > > > + return IRQ_NONE;
> > > > > > >
> > > > > > > Thomas was trying to avoid any new conditional code in the
> > > > > > > interrupt
> > > > > > > handling path, that's why I added a suspended_action list in my
> > > > > > > proposal.
> > > > > > > Even if your 'unlikely' statement make things better I'm pretty
> > > > > > > sure it
> > > > > > > adds some latency.
> > > > > >
> > > > > > I can see that we don't want to add more code here to keep things
> > > > > > clean/pure, but I find it hard to believe that a single bit test and
> > > > > > branch (for data that should be hot in the cache) are going to add
> > > > > > measurable latency to a path that does pointer chasing to get to the
> > > > > > irqaction in the first place. I could be wrong though, and I'm
> > > > > > happy to
> > > > > > benchmark.
> > > > >
> > > > > Again, I don't have enough experience to say this is (or isn't)
> > > > > impacting irq handling latency, I'm just reporting what Thomas told
> > > > > me.
> > > > >
> > > > > >
> > > > > > It would be possible to go for your list shuffling approach here
> > > > > > while
> > > > > > still keeping the flag internal and all the logic hidden away in
> > > > > > kernel/irq/pm.c. I wasn't sure how actions could be manipulated
> > > > > > during
> > > > > > suspend, which made me wary of moving them to a separate list.
> > > > >
> > > > > Moving them to a temporary list on suspend and restoring them on
> > > > > resume should not be a problem.
> > > > > The only drawback I see is that actions might be reordered after the
> > > > > first resume (anyway, relying on shared irq action order is dangerous
> > > > > IMHO).
> > > >
> > > > We considered doing that too and saw some drawbacks (in addition to the
> > > > reordering of actions you've mentioned). It added just too much
> > > > complexity
> > > > to the IRQ suspend-resume code.
> > > >
> > > > I, personally, would be fine with adding an IRQ flag to silence the
> > > > warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that
> > > > would
> > > > be set automatically if someone requested IRQF_TIMER | IRQF_SHARED.
> > > >
> > > > Thoughts?
> > >
> > > Even if the timer driver does that, we still require the other handlers
> > > sharing the line to do the right thing across suspend, no? So either
> > > their actions need to be masked at suspend time, or the handlers need to
> > > detect when they're called during suspend and return early.
> >
> > Well, the issue at hand is about things that share an IRQ with a timer
> > AFAICS.
> >
> > That is odd enough already and I'd say everyone in that situation needs to
> > be prepared to take the pain (including having to check if the device is not
> > suspended in their interrupt handlers).
> >
> > And quite frankly they need to do that already, because we've never
> > suspended
> > timer IRQs.
> >
> > > So for the flag at request time approach to work, all the drivers using
> > > the interrupt would have to flag they're safe in that context.
> >
> > Something like IRQF_"I can share the line with a timer" I guess? That
> > wouldn't
> > hurt and can be checked at request time even.
> >
> > > I'm not averse to having that (only a few drivers shuold be affected and
> > > we can sanity check them now).
> >
> > Right.
>
> Okay, if everyone agrees on this solution, then I'm fine with that too
> (even if I'm a bit disappointed to have spent so much time on this
> problem to eventually end-up with a simple IRQF_SHARED_TIMER flag :-().
IRQD_SHARED_TIMER (that needs to be an IRQ flag, not an irqaction one).
And spending time on things that never go in happens a lot in the core
development. I've sent several versions of the wakeup interrupts handling
rework and then Thomas wrote the final one himself. :-)
The goal is to find a way that will address everyone's needs and that's
not always starightforward.
Rafael
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, Feb 11, 2015 at 03:17:20PM +, Rafael J. Wysocki wrote:
> On Wednesday, February 11, 2015 02:43:45 PM Mark Rutland wrote:
> > [...]
> >
> > > > > > > +static irqreturn_t __handle_irq_event_percpu(unsigned int irq,
> > > > > > > struct irqaction *action)
> > > > > > > +{
> > > > > > > + /*
> > > > > > > + * During suspend we must not call potentially unsafe irq
> > > > > > > handlers.
> > > > > > > + * See suspend_suspendable_actions.
> > > > > > > + */
> > > > > > > + if (unlikely(action->flags & IRQF_NO_ACTION))
> > > > > > > + return IRQ_NONE;
> > > > > >
> > > > > > Thomas was trying to avoid any new conditional code in the interrupt
> > > > > > handling path, that's why I added a suspended_action list in my
> > > > > > proposal.
> > > > > > Even if your 'unlikely' statement make things better I'm pretty
> > > > > > sure it
> > > > > > adds some latency.
> > > > >
> > > > > I can see that we don't want to add more code here to keep things
> > > > > clean/pure, but I find it hard to believe that a single bit test and
> > > > > branch (for data that should be hot in the cache) are going to add
> > > > > measurable latency to a path that does pointer chasing to get to the
> > > > > irqaction in the first place. I could be wrong though, and I'm happy
> > > > > to
> > > > > benchmark.
> > > >
> > > > Again, I don't have enough experience to say this is (or isn't)
> > > > impacting irq handling latency, I'm just reporting what Thomas told me.
> > > >
> > > > >
> > > > > It would be possible to go for your list shuffling approach here while
> > > > > still keeping the flag internal and all the logic hidden away in
> > > > > kernel/irq/pm.c. I wasn't sure how actions could be manipulated during
> > > > > suspend, which made me wary of moving them to a separate list.
> > > >
> > > > Moving them to a temporary list on suspend and restoring them on
> > > > resume should not be a problem.
> > > > The only drawback I see is that actions might be reordered after the
> > > > first resume (anyway, relying on shared irq action order is dangerous
> > > > IMHO).
> > >
> > > We considered doing that too and saw some drawbacks (in addition to the
> > > reordering of actions you've mentioned). It added just too much
> > > complexity
> > > to the IRQ suspend-resume code.
> > >
> > > I, personally, would be fine with adding an IRQ flag to silence the
> > > warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that
> > > would
> > > be set automatically if someone requested IRQF_TIMER | IRQF_SHARED.
> > >
> > > Thoughts?
> >
> > Even if the timer driver does that, we still require the other handlers
> > sharing the line to do the right thing across suspend, no? So either
> > their actions need to be masked at suspend time, or the handlers need to
> > detect when they're called during suspend and return early.
>
> Well, the issue at hand is about things that share an IRQ with a timer AFAICS.
>
> That is odd enough already and I'd say everyone in that situation needs to
> be prepared to take the pain (including having to check if the device is not
> suspended in their interrupt handlers).
IMO if the line is shared it would be ideal for the core to mask the
action (as that's essentially the behaviour when the line isn't shared
with an IRQF_NO_SUSPEND action), but that's not esseential if a flag is
OK for now.
> And quite frankly they need to do that already, because we've never suspended
> timer IRQs.
This is a very good point.
> > So for the flag at request time approach to work, all the drivers using
> > the interrupt would have to flag they're safe in that context.
>
> Something like IRQF_"I can share the line with a timer" I guess? That
> wouldn't
> hurt and can be checked at request time even.
I guess that would have to imply IRQF_SHARED, so we'd have something
like:
IRQF_SHARED_SUSPEND_OK - This handler is safe to call spuriously during
suspend in the case the line is shared. The
handler will not access unavailable hardware
or kernel infrastructure during this period.
#define __IRQF_SUSPEND_SPURIOUS 0x0004
#define IRQF_SHARED_SUSPEND_OK (IRQF_SHARED | __IRQF_SUSPEND_SPURIOUS)
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, Feb 11, 2015 at 03:07:48PM +, Rafael J. Wysocki wrote: > On Wednesday, February 11, 2015 02:14:37 PM Mark Rutland wrote: > > On Wed, Feb 11, 2015 at 02:31:18PM +, Rafael J. Wysocki wrote: > > > On Wednesday, February 11, 2015 11:15:17 AM Mark Rutland wrote: > > > > On Wed, Feb 11, 2015 at 09:11:59AM +, Peter Zijlstra wrote: > > > > > On Tue, Feb 10, 2015 at 08:48:36PM +, Mark Rutland wrote: > > > > > > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 > > > > > > 2001 > > > > > > From: Mark Rutland > > > > > > Date: Tue, 10 Feb 2015 20:14:33 + > > > > > > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > > > > > > > > > > > In some cases a physical IRQ line may be shared between devices from > > > > > > which we expect interrupts during suspend (e.g. timers) and those > > > > > > we do > > > > > > not (e.g. anything we cut the power to). Where a driver did not > > > > > > request > > > > > > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > > > > > > being called during suspend, and it may bring down the system. > > > > > > > > > > > > This patch adds logic to automatically mark the irqactions for these > > > > > > potentially unsafe handlers as disabled during suspend, leaving > > > > > > actions > > > > > > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared > > > > > > line > > > > > > during suspend, only the handlers requested with IRQF_NO_SUSPEND > > > > > > will be > > > > > > called. The handlers requested without IRQF_NO_SUSPEND will be > > > > > > skipped > > > > > > as if they had immediately returned IRQF_NONE. > > > > > > > > > > > > Cc: Boris Brezillon > > > > > > Cc: Jason Cooper > > > > > > Cc: Nicolas Ferre > > > > > > Cc: Peter Zijlstra > > > > > > Cc: Rafael J. Wysocki > > > > > > Cc: Thomas Gleixner > > > > > > Signed-off-by: Mark Rutland > > > > > > > > > > Aw gawd.. not that again. > > > > > > > > I agree this isn't pretty, but at least it doesn't require the HW > > > > description to know about Linux internals, and it can work for !DT > > > > systems. > > > > > > > > I'm really not happy with placing Linux implementation details into > > > > DTBs. > > > > > > > > > So Rafael and tglx went over this a few months ago I think: > > > > > > > > > > lkml.kernel.org/r/26580319.ozp7jvj...@vostro.rjw.lan > > > > > > > > > > is the last series I could find. Maybe Rafael can summarize? > > > > > > > > I can't get at any commentary from that link, unfortunately. > > > > > > > > Rafael? > > > > > > Well, the commentary is not there, because both I and Thomas implicitly > > > agreed > > > on one thing: We cannot add any suspend-related checks to the interrupt > > > handling > > > hot path, because that will affect everyone including people who don't use > > > suspend at all and who *really* care about interrupt handling performance. > > > > That's fair enough, and I'm happy to avoid that by other means. > > > > My fundamental objection(s) to the current approach is that we create a > > binding for a non-existent device that people will abuse without > > considering the consequences. All we will end up with is more DTBs > > containing the mux regardless of wether the drivers (or hardware) are > > actually safe with a shared line. > > That is a valid concern in my view. > > > So with the changes moves out of the hot-path (e.g. with shuffling > > to/from a suspended_actions list in the pm code), is there some issue > > that I have not considered? > > When we were reworking the handling of wakeup interrupts during the 3.18 > cycle, one of my proposals was to move the "suspended" irqactions to an > "inactive" list during suspend_device_irqs() and back during > resume_device_irqs(), but Thomas didn't like that approach. His main > argument was that it made the code in question overly complex which > was fair enough to me. I've just looked into that, and have a trivial implementation that's contained within kernel/irq/pm.c, but it assumes that during suspend nothing needs to modify actions. > What about adding a new flag like I said? That works for me. I'll respond in the other mail. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, 11 Feb 2015 16:17:20 +0100
"Rafael J. Wysocki" wrote:
> On Wednesday, February 11, 2015 02:43:45 PM Mark Rutland wrote:
> > [...]
> >
> > > > > > > +static irqreturn_t __handle_irq_event_percpu(unsigned int irq,
> > > > > > > struct irqaction *action)
> > > > > > > +{
> > > > > > > + /*
> > > > > > > + * During suspend we must not call potentially unsafe irq
> > > > > > > handlers.
> > > > > > > + * See suspend_suspendable_actions.
> > > > > > > + */
> > > > > > > + if (unlikely(action->flags & IRQF_NO_ACTION))
> > > > > > > + return IRQ_NONE;
> > > > > >
> > > > > > Thomas was trying to avoid any new conditional code in the interrupt
> > > > > > handling path, that's why I added a suspended_action list in my
> > > > > > proposal.
> > > > > > Even if your 'unlikely' statement make things better I'm pretty
> > > > > > sure it
> > > > > > adds some latency.
> > > > >
> > > > > I can see that we don't want to add more code here to keep things
> > > > > clean/pure, but I find it hard to believe that a single bit test and
> > > > > branch (for data that should be hot in the cache) are going to add
> > > > > measurable latency to a path that does pointer chasing to get to the
> > > > > irqaction in the first place. I could be wrong though, and I'm happy
> > > > > to
> > > > > benchmark.
> > > >
> > > > Again, I don't have enough experience to say this is (or isn't)
> > > > impacting irq handling latency, I'm just reporting what Thomas told me.
> > > >
> > > > >
> > > > > It would be possible to go for your list shuffling approach here while
> > > > > still keeping the flag internal and all the logic hidden away in
> > > > > kernel/irq/pm.c. I wasn't sure how actions could be manipulated during
> > > > > suspend, which made me wary of moving them to a separate list.
> > > >
> > > > Moving them to a temporary list on suspend and restoring them on
> > > > resume should not be a problem.
> > > > The only drawback I see is that actions might be reordered after the
> > > > first resume (anyway, relying on shared irq action order is dangerous
> > > > IMHO).
> > >
> > > We considered doing that too and saw some drawbacks (in addition to the
> > > reordering of actions you've mentioned). It added just too much
> > > complexity
> > > to the IRQ suspend-resume code.
> > >
> > > I, personally, would be fine with adding an IRQ flag to silence the
> > > warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that
> > > would
> > > be set automatically if someone requested IRQF_TIMER | IRQF_SHARED.
> > >
> > > Thoughts?
> >
> > Even if the timer driver does that, we still require the other handlers
> > sharing the line to do the right thing across suspend, no? So either
> > their actions need to be masked at suspend time, or the handlers need to
> > detect when they're called during suspend and return early.
>
> Well, the issue at hand is about things that share an IRQ with a timer AFAICS.
>
> That is odd enough already and I'd say everyone in that situation needs to
> be prepared to take the pain (including having to check if the device is not
> suspended in their interrupt handlers).
>
> And quite frankly they need to do that already, because we've never suspended
> timer IRQs.
>
> > So for the flag at request time approach to work, all the drivers using
> > the interrupt would have to flag they're safe in that context.
>
> Something like IRQF_"I can share the line with a timer" I guess? That
> wouldn't
> hurt and can be checked at request time even.
>
> > I'm not averse to having that (only a few drivers shuold be affected and
> > we can sanity check them now).
>
> Right.
Okay, if everyone agrees on this solution, then I'm fine with that too
(even if I'm a bit disappointed to have spent so much time on this
problem to eventually end-up with a simple IRQF_SHARED_TIMER flag :-().
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wednesday, February 11, 2015 02:43:45 PM Mark Rutland wrote:
> [...]
>
> > > > > > +static irqreturn_t __handle_irq_event_percpu(unsigned int irq,
> > > > > > struct irqaction *action)
> > > > > > +{
> > > > > > + /*
> > > > > > +* During suspend we must not call potentially unsafe irq
> > > > > > handlers.
> > > > > > +* See suspend_suspendable_actions.
> > > > > > +*/
> > > > > > + if (unlikely(action->flags & IRQF_NO_ACTION))
> > > > > > + return IRQ_NONE;
> > > > >
> > > > > Thomas was trying to avoid any new conditional code in the interrupt
> > > > > handling path, that's why I added a suspended_action list in my
> > > > > proposal.
> > > > > Even if your 'unlikely' statement make things better I'm pretty sure
> > > > > it
> > > > > adds some latency.
> > > >
> > > > I can see that we don't want to add more code here to keep things
> > > > clean/pure, but I find it hard to believe that a single bit test and
> > > > branch (for data that should be hot in the cache) are going to add
> > > > measurable latency to a path that does pointer chasing to get to the
> > > > irqaction in the first place. I could be wrong though, and I'm happy to
> > > > benchmark.
> > >
> > > Again, I don't have enough experience to say this is (or isn't)
> > > impacting irq handling latency, I'm just reporting what Thomas told me.
> > >
> > > >
> > > > It would be possible to go for your list shuffling approach here while
> > > > still keeping the flag internal and all the logic hidden away in
> > > > kernel/irq/pm.c. I wasn't sure how actions could be manipulated during
> > > > suspend, which made me wary of moving them to a separate list.
> > >
> > > Moving them to a temporary list on suspend and restoring them on
> > > resume should not be a problem.
> > > The only drawback I see is that actions might be reordered after the
> > > first resume (anyway, relying on shared irq action order is dangerous
> > > IMHO).
> >
> > We considered doing that too and saw some drawbacks (in addition to the
> > reordering of actions you've mentioned). It added just too much complexity
> > to the IRQ suspend-resume code.
> >
> > I, personally, would be fine with adding an IRQ flag to silence the
> > warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that would
> > be set automatically if someone requested IRQF_TIMER | IRQF_SHARED.
> >
> > Thoughts?
>
> Even if the timer driver does that, we still require the other handlers
> sharing the line to do the right thing across suspend, no? So either
> their actions need to be masked at suspend time, or the handlers need to
> detect when they're called during suspend and return early.
Well, the issue at hand is about things that share an IRQ with a timer AFAICS.
That is odd enough already and I'd say everyone in that situation needs to
be prepared to take the pain (including having to check if the device is not
suspended in their interrupt handlers).
And quite frankly they need to do that already, because we've never suspended
timer IRQs.
> So for the flag at request time approach to work, all the drivers using
> the interrupt would have to flag they're safe in that context.
Something like IRQF_"I can share the line with a timer" I guess? That wouldn't
hurt and can be checked at request time even.
> I'm not averse to having that (only a few drivers shuold be affected and
> we can sanity check them now).
Right.
Rafael
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wednesday, February 11, 2015 02:14:37 PM Mark Rutland wrote: > On Wed, Feb 11, 2015 at 02:31:18PM +, Rafael J. Wysocki wrote: > > On Wednesday, February 11, 2015 11:15:17 AM Mark Rutland wrote: > > > On Wed, Feb 11, 2015 at 09:11:59AM +, Peter Zijlstra wrote: > > > > On Tue, Feb 10, 2015 at 08:48:36PM +, Mark Rutland wrote: > > > > > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 > > > > > From: Mark Rutland > > > > > Date: Tue, 10 Feb 2015 20:14:33 + > > > > > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > > > > > > > > > In some cases a physical IRQ line may be shared between devices from > > > > > which we expect interrupts during suspend (e.g. timers) and those we > > > > > do > > > > > not (e.g. anything we cut the power to). Where a driver did not > > > > > request > > > > > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > > > > > being called during suspend, and it may bring down the system. > > > > > > > > > > This patch adds logic to automatically mark the irqactions for these > > > > > potentially unsafe handlers as disabled during suspend, leaving > > > > > actions > > > > > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared > > > > > line > > > > > during suspend, only the handlers requested with IRQF_NO_SUSPEND will > > > > > be > > > > > called. The handlers requested without IRQF_NO_SUSPEND will be skipped > > > > > as if they had immediately returned IRQF_NONE. > > > > > > > > > > Cc: Boris Brezillon > > > > > Cc: Jason Cooper > > > > > Cc: Nicolas Ferre > > > > > Cc: Peter Zijlstra > > > > > Cc: Rafael J. Wysocki > > > > > Cc: Thomas Gleixner > > > > > Signed-off-by: Mark Rutland > > > > > > > > Aw gawd.. not that again. > > > > > > I agree this isn't pretty, but at least it doesn't require the HW > > > description to know about Linux internals, and it can work for !DT > > > systems. > > > > > > I'm really not happy with placing Linux implementation details into > > > DTBs. > > > > > > > So Rafael and tglx went over this a few months ago I think: > > > > > > > > lkml.kernel.org/r/26580319.ozp7jvj...@vostro.rjw.lan > > > > > > > > is the last series I could find. Maybe Rafael can summarize? > > > > > > I can't get at any commentary from that link, unfortunately. > > > > > > Rafael? > > > > Well, the commentary is not there, because both I and Thomas implicitly > > agreed > > on one thing: We cannot add any suspend-related checks to the interrupt > > handling > > hot path, because that will affect everyone including people who don't use > > suspend at all and who *really* care about interrupt handling performance. > > That's fair enough, and I'm happy to avoid that by other means. > > My fundamental objection(s) to the current approach is that we create a > binding for a non-existent device that people will abuse without > considering the consequences. All we will end up with is more DTBs > containing the mux regardless of wether the drivers (or hardware) are > actually safe with a shared line. That is a valid concern in my view. > So with the changes moves out of the hot-path (e.g. with shuffling > to/from a suspended_actions list in the pm code), is there some issue > that I have not considered? When we were reworking the handling of wakeup interrupts during the 3.18 cycle, one of my proposals was to move the "suspended" irqactions to an "inactive" list during suspend_device_irqs() and back during resume_device_irqs(), but Thomas didn't like that approach. His main argument was that it made the code in question overly complex which was fair enough to me. What about adding a new flag like I said? Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, 11 Feb 2015 15:55:47 +0100 "Rafael J. Wysocki" wrote: > On Wednesday, February 11, 2015 01:24:37 PM Boris Brezillon wrote: > > Hi Mark, > > > > On Wed, 11 Feb 2015 11:11:06 + > > Mark Rutland wrote: > > > > > On Wed, Feb 11, 2015 at 08:53:39AM +, Boris Brezillon wrote: > > > > Hi Mark, > > > > > > > > On Tue, 10 Feb 2015 20:48:36 + > > > > Mark Rutland wrote: > > > > > > > > > On Tue, Feb 10, 2015 at 03:52:01PM +, Boris Brezillon wrote: > > > > > > Hi Mark, > > > > > > > > > > > > On Tue, 10 Feb 2015 15:36:28 + > > > > > > Mark Rutland wrote: > > > > > > > > > > > > > Hi Boris, > > > > > > > > > > > > > > On Thu, Jan 29, 2015 at 10:33:38AM +, Boris Brezillon wrote: > > > > > > > > Add documentation for the virtual irq demuxer. > > > > > > > > > > > > > > > > Signed-off-by: Boris Brezillon > > > > > > > > > > > > > > > > Acked-by: Nicolas Ferre > > > > > > > > --- > > > > > > > > .../bindings/interrupt-controller/dumb-demux.txt | 41 > > > > > > > > ++ > > > > > > > > 1 file changed, 41 insertions(+) > > > > > > > > create mode 100644 > > > > > > > > Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > > > > > > > > > > > > > diff --git > > > > > > > > a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > > > > > > > > > > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > > > > > new file mode 100644 > > > > > > > > index 000..b9a7830 > > > > > > > > --- /dev/null > > > > > > > > +++ > > > > > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > > > > > @@ -0,0 +1,41 @@ > > > > > > > > +* Virtual Interrupt Demultiplexer > > > > > > > > + > > > > > > > > +This virtual demultiplexer simply forward all incoming > > > > > > > > interrupts to its > > > > > > > > +enabled/unmasked children. > > > > > > > > +It is only intended to be used by hardware that do not provide > > > > > > > > a proper way > > > > > > > > +to demultiplex a source interrupt, and thus have to wake all > > > > > > > > their children > > > > > > > > +up so that they can possibly handle the interrupt (if needed). > > > > > > > > +This can be seen as an alternative to shared interrupts when > > > > > > > > at least one > > > > > > > > +of the interrupt children is a timer (and require the irq to > > > > > > > > stay enabled > > > > > > > > +on suspend) while others are not. This will prevent calling > > > > > > > > irq handlers of > > > > > > > > +non timer devices while they are suspended. > > > > > > > > > > > > > > This sounds like a DT-workaround for a Linux implementation > > > > > > > problem, and > > > > > > > I don't think this the right way to solve your problem. > > > > > > > > > > > > I understand your concern, but why are you answering while I asked > > > > > > for > > > > > > DT maintainers reviews for several days (if not several weeks). > > > > > > > > > > > > > > > > > > > > Why does this have to be in DT at all? Why can we not fix the > > > > > > > core to > > > > > > > handle these details? > > > > > > > > > > > > We already discussed that with Rob and Thomas, and hiding such a > > > > > > demuxer chip is not an easy task. > > > > > > I'm open to any suggestion to do that, though I'd like you (I mean > > > > > > DT > > > > > > guys) to provide a working implementation (or at least a viable > > > > > > concept) > > > > > > that would silently demultiplex an irq. > > > > > > > > > > > > > > > > > > > > I am very much not keen on this binding. > > > > > > > > > > > > Yes, but do you have anything else to propose. > > > > > > We're experiencing this warning for 2 releases now, and this is > > > > > > time to > > > > > > find a solution (even if it's not a perfect one). > > > > > > > > > > Thoughts on the patch below? > > > > > > > > That's pretty much what I proposed in my first attempt to solve this > > > > problem [1] (except for a few things commented below). > > > > Anyway, Thomas suggested to go for the "dumb/virt irq demultiplexer" > > > > approach instead. > > > > > > There is one fundamental difference in that this patch does not require > > > drivers to be updated (the new flag is only used internally). Which > > > avoids having to patch every single driver that could possibly be used > > > in combination with one wanting interrupts during suspend. > > > > Actually, that was one of the requirements expressed by Thomas (Thomas, > > correct me if I'm wrong). > > The point was to force shared irq users to explicitly specify that they > > are mixing !IRQF_NO_SUSPEND and IRQF_NO_SUSPEND because they have no > > other choice. > > > > With your patch, there's no way to inform users that they are > > erroneously setting the IRQF_NO_SUSPEND flag on one of their shared > > interrupt. > > > > > > > > Any used which did not explicitly request with IRQF_NO_SUSPEND will not > > > receive interrupts during susp
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
[...]
> > > > > +static irqreturn_t __handle_irq_event_percpu(unsigned int irq,
> > > > > struct irqaction *action)
> > > > > +{
> > > > > + /*
> > > > > + * During suspend we must not call potentially unsafe irq
> > > > > handlers.
> > > > > + * See suspend_suspendable_actions.
> > > > > + */
> > > > > + if (unlikely(action->flags & IRQF_NO_ACTION))
> > > > > + return IRQ_NONE;
> > > >
> > > > Thomas was trying to avoid any new conditional code in the interrupt
> > > > handling path, that's why I added a suspended_action list in my
> > > > proposal.
> > > > Even if your 'unlikely' statement make things better I'm pretty sure it
> > > > adds some latency.
> > >
> > > I can see that we don't want to add more code here to keep things
> > > clean/pure, but I find it hard to believe that a single bit test and
> > > branch (for data that should be hot in the cache) are going to add
> > > measurable latency to a path that does pointer chasing to get to the
> > > irqaction in the first place. I could be wrong though, and I'm happy to
> > > benchmark.
> >
> > Again, I don't have enough experience to say this is (or isn't)
> > impacting irq handling latency, I'm just reporting what Thomas told me.
> >
> > >
> > > It would be possible to go for your list shuffling approach here while
> > > still keeping the flag internal and all the logic hidden away in
> > > kernel/irq/pm.c. I wasn't sure how actions could be manipulated during
> > > suspend, which made me wary of moving them to a separate list.
> >
> > Moving them to a temporary list on suspend and restoring them on
> > resume should not be a problem.
> > The only drawback I see is that actions might be reordered after the
> > first resume (anyway, relying on shared irq action order is dangerous
> > IMHO).
>
> We considered doing that too and saw some drawbacks (in addition to the
> reordering of actions you've mentioned). It added just too much complexity
> to the IRQ suspend-resume code.
>
> I, personally, would be fine with adding an IRQ flag to silence the
> warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that would
> be set automatically if someone requested IRQF_TIMER | IRQF_SHARED.
>
> Thoughts?
Even if the timer driver does that, we still require the other handlers
sharing the line to do the right thing across suspend, no? So either
their actions need to be masked at suspend time, or the handlers need to
detect when they're called during suspend and return early.
So for the flag at request time approach to work, all the drivers using
the interrupt would have to flag they're safe in that context.
I'm not averse to having that (only a few drivers shuold be affected and
we can sanity check them now).
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wednesday, February 11, 2015 01:24:37 PM Boris Brezillon wrote:
> Hi Mark,
>
> On Wed, 11 Feb 2015 11:11:06 +
> Mark Rutland wrote:
>
> > On Wed, Feb 11, 2015 at 08:53:39AM +, Boris Brezillon wrote:
> > > Hi Mark,
> > >
> > > On Tue, 10 Feb 2015 20:48:36 +
> > > Mark Rutland wrote:
> > >
> > > > On Tue, Feb 10, 2015 at 03:52:01PM +, Boris Brezillon wrote:
> > > > > Hi Mark,
> > > > >
> > > > > On Tue, 10 Feb 2015 15:36:28 +
> > > > > Mark Rutland wrote:
> > > > >
> > > > > > Hi Boris,
> > > > > >
> > > > > > On Thu, Jan 29, 2015 at 10:33:38AM +, Boris Brezillon wrote:
> > > > > > > Add documentation for the virtual irq demuxer.
> > > > > > >
> > > > > > > Signed-off-by: Boris Brezillon
> > > > > > >
> > > > > > > Acked-by: Nicolas Ferre
> > > > > > > ---
> > > > > > > .../bindings/interrupt-controller/dumb-demux.txt | 41
> > > > > > > ++
> > > > > > > 1 file changed, 41 insertions(+)
> > > > > > > create mode 100644
> > > > > > > Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > > >
> > > > > > > diff --git
> > > > > > > a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > > >
> > > > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > > > new file mode 100644
> > > > > > > index 000..b9a7830
> > > > > > > --- /dev/null
> > > > > > > +++
> > > > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > > > @@ -0,0 +1,41 @@
> > > > > > > +* Virtual Interrupt Demultiplexer
> > > > > > > +
> > > > > > > +This virtual demultiplexer simply forward all incoming
> > > > > > > interrupts to its
> > > > > > > +enabled/unmasked children.
> > > > > > > +It is only intended to be used by hardware that do not provide a
> > > > > > > proper way
> > > > > > > +to demultiplex a source interrupt, and thus have to wake all
> > > > > > > their children
> > > > > > > +up so that they can possibly handle the interrupt (if needed).
> > > > > > > +This can be seen as an alternative to shared interrupts when at
> > > > > > > least one
> > > > > > > +of the interrupt children is a timer (and require the irq to
> > > > > > > stay enabled
> > > > > > > +on suspend) while others are not. This will prevent calling irq
> > > > > > > handlers of
> > > > > > > +non timer devices while they are suspended.
> > > > > >
> > > > > > This sounds like a DT-workaround for a Linux implementation
> > > > > > problem, and
> > > > > > I don't think this the right way to solve your problem.
> > > > >
> > > > > I understand your concern, but why are you answering while I asked for
> > > > > DT maintainers reviews for several days (if not several weeks).
> > > > >
> > > > > >
> > > > > > Why does this have to be in DT at all? Why can we not fix the core
> > > > > > to
> > > > > > handle these details?
> > > > >
> > > > > We already discussed that with Rob and Thomas, and hiding such a
> > > > > demuxer chip is not an easy task.
> > > > > I'm open to any suggestion to do that, though I'd like you (I mean DT
> > > > > guys) to provide a working implementation (or at least a viable
> > > > > concept)
> > > > > that would silently demultiplex an irq.
> > > > >
> > > > > >
> > > > > > I am very much not keen on this binding.
> > > > >
> > > > > Yes, but do you have anything else to propose.
> > > > > We're experiencing this warning for 2 releases now, and this is time
> > > > > to
> > > > > find a solution (even if it's not a perfect one).
> > > >
> > > > Thoughts on the patch below?
> > >
> > > That's pretty much what I proposed in my first attempt to solve this
> > > problem [1] (except for a few things commented below).
> > > Anyway, Thomas suggested to go for the "dumb/virt irq demultiplexer"
> > > approach instead.
> >
> > There is one fundamental difference in that this patch does not require
> > drivers to be updated (the new flag is only used internally). Which
> > avoids having to patch every single driver that could possibly be used
> > in combination with one wanting interrupts during suspend.
>
> Actually, that was one of the requirements expressed by Thomas (Thomas,
> correct me if I'm wrong).
> The point was to force shared irq users to explicitly specify that they
> are mixing !IRQF_NO_SUSPEND and IRQF_NO_SUSPEND because they have no
> other choice.
>
> With your patch, there's no way to inform users that they are
> erroneously setting the IRQF_NO_SUSPEND flag on one of their shared
> interrupt.
>
> >
> > Any used which did not explicitly request with IRQF_NO_SUSPEND will not
> > receive interrupts during suspend.
> >
> > [...]
> >
> > > > +static irqreturn_t __handle_irq_event_percpu(unsigned int irq, struct
> > > > irqaction *action)
> > > > +{
> > > > + /*
> > > > +* During suspend we must not call potentially unsafe irq
> > > > handlers.
> > > > +* See suspend_suspendable_actions.
> > > > +
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wednesday, February 11, 2015 11:11:06 AM Mark Rutland wrote:
> On Wed, Feb 11, 2015 at 08:53:39AM +, Boris Brezillon wrote:
> > Hi Mark,
> >
> > On Tue, 10 Feb 2015 20:48:36 +
> > Mark Rutland wrote:
> >
> > > On Tue, Feb 10, 2015 at 03:52:01PM +, Boris Brezillon wrote:
> > > > Hi Mark,
> > > >
> > > > On Tue, 10 Feb 2015 15:36:28 +
> > > > Mark Rutland wrote:
> > > >
> > > > > Hi Boris,
> > > > >
> > > > > On Thu, Jan 29, 2015 at 10:33:38AM +, Boris Brezillon wrote:
> > > > > > Add documentation for the virtual irq demuxer.
> > > > > >
> > > > > > Signed-off-by: Boris Brezillon
> > > > > > Acked-by: Nicolas Ferre
> > > > > > ---
> > > > > > .../bindings/interrupt-controller/dumb-demux.txt | 41
> > > > > > ++
> > > > > > 1 file changed, 41 insertions(+)
> > > > > > create mode 100644
> > > > > > Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > >
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > >
> > > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > > new file mode 100644
> > > > > > index 000..b9a7830
> > > > > > --- /dev/null
> > > > > > +++
> > > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > > @@ -0,0 +1,41 @@
> > > > > > +* Virtual Interrupt Demultiplexer
> > > > > > +
> > > > > > +This virtual demultiplexer simply forward all incoming interrupts
> > > > > > to its
> > > > > > +enabled/unmasked children.
> > > > > > +It is only intended to be used by hardware that do not provide a
> > > > > > proper way
> > > > > > +to demultiplex a source interrupt, and thus have to wake all their
> > > > > > children
> > > > > > +up so that they can possibly handle the interrupt (if needed).
> > > > > > +This can be seen as an alternative to shared interrupts when at
> > > > > > least one
> > > > > > +of the interrupt children is a timer (and require the irq to stay
> > > > > > enabled
> > > > > > +on suspend) while others are not. This will prevent calling irq
> > > > > > handlers of
> > > > > > +non timer devices while they are suspended.
> > > > >
> > > > > This sounds like a DT-workaround for a Linux implementation problem,
> > > > > and
> > > > > I don't think this the right way to solve your problem.
> > > >
> > > > I understand your concern, but why are you answering while I asked for
> > > > DT maintainers reviews for several days (if not several weeks).
> > > >
> > > > >
> > > > > Why does this have to be in DT at all? Why can we not fix the core to
> > > > > handle these details?
> > > >
> > > > We already discussed that with Rob and Thomas, and hiding such a
> > > > demuxer chip is not an easy task.
> > > > I'm open to any suggestion to do that, though I'd like you (I mean DT
> > > > guys) to provide a working implementation (or at least a viable concept)
> > > > that would silently demultiplex an irq.
> > > >
> > > > >
> > > > > I am very much not keen on this binding.
> > > >
> > > > Yes, but do you have anything else to propose.
> > > > We're experiencing this warning for 2 releases now, and this is time to
> > > > find a solution (even if it's not a perfect one).
> > >
> > > Thoughts on the patch below?
> >
> > That's pretty much what I proposed in my first attempt to solve this
> > problem [1] (except for a few things commented below).
> > Anyway, Thomas suggested to go for the "dumb/virt irq demultiplexer"
> > approach instead.
>
> There is one fundamental difference in that this patch does not require
> drivers to be updated (the new flag is only used internally). Which
> avoids having to patch every single driver that could possibly be used
> in combination with one wanting interrupts during suspend.
>
> Any used which did not explicitly request with IRQF_NO_SUSPEND will not
> receive interrupts during suspend.
>
> [...]
>
> > > +static irqreturn_t __handle_irq_event_percpu(unsigned int irq, struct
> > > irqaction *action)
> > > +{
> > > + /*
> > > + * During suspend we must not call potentially unsafe irq handlers.
> > > + * See suspend_suspendable_actions.
> > > + */
> > > + if (unlikely(action->flags & IRQF_NO_ACTION))
> > > + return IRQ_NONE;
> >
> > Thomas was trying to avoid any new conditional code in the interrupt
> > handling path, that's why I added a suspended_action list in my
> > proposal.
> > Even if your 'unlikely' statement make things better I'm pretty sure it
> > adds some latency.
>
> I can see that we don't want to add more code here to keep things
> clean/pure, but I find it hard to believe that a single bit test and
> branch (for data that should be hot in the cache) are going to add
> measurable latency to a path that does pointer chasing to get to the
> irqaction in the first place. I could be wrong though, and I'm happy to
> benchmark.
You don't have to. There are people who care about every s
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, Feb 11, 2015 at 02:31:18PM +, Rafael J. Wysocki wrote: > On Wednesday, February 11, 2015 11:15:17 AM Mark Rutland wrote: > > On Wed, Feb 11, 2015 at 09:11:59AM +, Peter Zijlstra wrote: > > > On Tue, Feb 10, 2015 at 08:48:36PM +, Mark Rutland wrote: > > > > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 > > > > From: Mark Rutland > > > > Date: Tue, 10 Feb 2015 20:14:33 + > > > > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > > > > > > > In some cases a physical IRQ line may be shared between devices from > > > > which we expect interrupts during suspend (e.g. timers) and those we do > > > > not (e.g. anything we cut the power to). Where a driver did not request > > > > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > > > > being called during suspend, and it may bring down the system. > > > > > > > > This patch adds logic to automatically mark the irqactions for these > > > > potentially unsafe handlers as disabled during suspend, leaving actions > > > > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared line > > > > during suspend, only the handlers requested with IRQF_NO_SUSPEND will be > > > > called. The handlers requested without IRQF_NO_SUSPEND will be skipped > > > > as if they had immediately returned IRQF_NONE. > > > > > > > > Cc: Boris Brezillon > > > > Cc: Jason Cooper > > > > Cc: Nicolas Ferre > > > > Cc: Peter Zijlstra > > > > Cc: Rafael J. Wysocki > > > > Cc: Thomas Gleixner > > > > Signed-off-by: Mark Rutland > > > > > > Aw gawd.. not that again. > > > > I agree this isn't pretty, but at least it doesn't require the HW > > description to know about Linux internals, and it can work for !DT > > systems. > > > > I'm really not happy with placing Linux implementation details into > > DTBs. > > > > > So Rafael and tglx went over this a few months ago I think: > > > > > > lkml.kernel.org/r/26580319.ozp7jvj...@vostro.rjw.lan > > > > > > is the last series I could find. Maybe Rafael can summarize? > > > > I can't get at any commentary from that link, unfortunately. > > > > Rafael? > > Well, the commentary is not there, because both I and Thomas implicitly agreed > on one thing: We cannot add any suspend-related checks to the interrupt > handling > hot path, because that will affect everyone including people who don't use > suspend at all and who *really* care about interrupt handling performance. That's fair enough, and I'm happy to avoid that by other means. My fundamental objection(s) to the current approach is that we create a binding for a non-existent device that people will abuse without considering the consequences. All we will end up with is more DTBs containing the mux regardless of wether the drivers (or hardware) are actually safe with a shared line. So with the changes moves out of the hot-path (e.g. with shuffling to/from a suspended_actions list in the pm code), is there some issue that I have not considered? Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Tuesday, February 10, 2015 08:48:36 PM Mark Rutland wrote: > On Tue, Feb 10, 2015 at 03:52:01PM +, Boris Brezillon wrote: > > Hi Mark, > > > > On Tue, 10 Feb 2015 15:36:28 + > > Mark Rutland wrote: > > > > > Hi Boris, > > > > > > On Thu, Jan 29, 2015 at 10:33:38AM +, Boris Brezillon wrote: > > > > Add documentation for the virtual irq demuxer. > > > > > > > > Signed-off-by: Boris Brezillon > > > > Acked-by: Nicolas Ferre > > > > --- > > > > .../bindings/interrupt-controller/dumb-demux.txt | 41 > > > > ++ > > > > 1 file changed, 41 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > new file mode 100644 > > > > index 000..b9a7830 > > > > --- /dev/null > > > > +++ > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > @@ -0,0 +1,41 @@ > > > > +* Virtual Interrupt Demultiplexer > > > > + > > > > +This virtual demultiplexer simply forward all incoming interrupts to > > > > its > > > > +enabled/unmasked children. > > > > +It is only intended to be used by hardware that do not provide a > > > > proper way > > > > +to demultiplex a source interrupt, and thus have to wake all their > > > > children > > > > +up so that they can possibly handle the interrupt (if needed). > > > > +This can be seen as an alternative to shared interrupts when at least > > > > one > > > > +of the interrupt children is a timer (and require the irq to stay > > > > enabled > > > > +on suspend) while others are not. This will prevent calling irq > > > > handlers of > > > > +non timer devices while they are suspended. > > > > > > This sounds like a DT-workaround for a Linux implementation problem, and > > > I don't think this the right way to solve your problem. > > > > I understand your concern, but why are you answering while I asked for > > DT maintainers reviews for several days (if not several weeks). > > > > > > > > Why does this have to be in DT at all? Why can we not fix the core to > > > handle these details? > > > > We already discussed that with Rob and Thomas, and hiding such a > > demuxer chip is not an easy task. > > I'm open to any suggestion to do that, though I'd like you (I mean DT > > guys) to provide a working implementation (or at least a viable concept) > > that would silently demultiplex an irq. > > > > > > > > I am very much not keen on this binding. > > > > Yes, but do you have anything else to propose. > > We're experiencing this warning for 2 releases now, and this is time to > > find a solution (even if it's not a perfect one). > > Thoughts on the patch below? > > Rather than handling this at the desc level it adds an extra flag to the > irqaction which can be set/unset during suspend for those irqs we don't > want to handle. That way we don't need to tell the core about the > mismatch explicitly in DT (or ACPI/board files/whatever). > > If we can request/free interrupts during suspend then there's some logic > missing, but it shows the basic idea. > > I didn't have a system to hand with shared mismatched IRQF_NO_SUSPEND > interrupts, so I had to fake that up in code for testing. > > Thanks, > Mark. > > >8 > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 > From: Mark Rutland > Date: Tue, 10 Feb 2015 20:14:33 + > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > In some cases a physical IRQ line may be shared between devices from > which we expect interrupts during suspend (e.g. timers) and those we do > not (e.g. anything we cut the power to). Where a driver did not request > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > being called during suspend, and it may bring down the system. > > This patch adds logic to automatically mark the irqactions for these > potentially unsafe handlers as disabled during suspend, leaving actions > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared line > during suspend, only the handlers requested with IRQF_NO_SUSPEND will be > called. The handlers requested without IRQF_NO_SUSPEND will be skipped > as if they had immediately returned IRQF_NONE. > > Cc: Boris Brezillon > Cc: Jason Cooper > Cc: Nicolas Ferre > Cc: Peter Zijlstra > Cc: Rafael J. Wysocki > Cc: Thomas Gleixner > Signed-off-by: Mark Rutland > --- > include/linux/interrupt.h | 4 > kernel/irq/handle.c | 13 +++- > kernel/irq/pm.c | 50 > +++ > 3 files changed, 62 insertions(+), 5 deletions(-) > > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h > index d9b05b5..49dcb35 100644 > --- a/include/linux/interrupt.h > +++ b/include/linux/interrupt.h > @@ -57,6 +57,9 @
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wednesday, February 11, 2015 11:15:17 AM Mark Rutland wrote: > On Wed, Feb 11, 2015 at 09:11:59AM +, Peter Zijlstra wrote: > > On Tue, Feb 10, 2015 at 08:48:36PM +, Mark Rutland wrote: > > > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 > > > From: Mark Rutland > > > Date: Tue, 10 Feb 2015 20:14:33 + > > > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > > > > > In some cases a physical IRQ line may be shared between devices from > > > which we expect interrupts during suspend (e.g. timers) and those we do > > > not (e.g. anything we cut the power to). Where a driver did not request > > > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > > > being called during suspend, and it may bring down the system. > > > > > > This patch adds logic to automatically mark the irqactions for these > > > potentially unsafe handlers as disabled during suspend, leaving actions > > > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared line > > > during suspend, only the handlers requested with IRQF_NO_SUSPEND will be > > > called. The handlers requested without IRQF_NO_SUSPEND will be skipped > > > as if they had immediately returned IRQF_NONE. > > > > > > Cc: Boris Brezillon > > > Cc: Jason Cooper > > > Cc: Nicolas Ferre > > > Cc: Peter Zijlstra > > > Cc: Rafael J. Wysocki > > > Cc: Thomas Gleixner > > > Signed-off-by: Mark Rutland > > > > Aw gawd.. not that again. > > I agree this isn't pretty, but at least it doesn't require the HW > description to know about Linux internals, and it can work for !DT > systems. > > I'm really not happy with placing Linux implementation details into > DTBs. > > > So Rafael and tglx went over this a few months ago I think: > > > > lkml.kernel.org/r/26580319.ozp7jvj...@vostro.rjw.lan > > > > is the last series I could find. Maybe Rafael can summarize? > > I can't get at any commentary from that link, unfortunately. > > Rafael? Well, the commentary is not there, because both I and Thomas implicitly agreed on one thing: We cannot add any suspend-related checks to the interrupt handling hot path, because that will affect everyone including people who don't use suspend at all and who *really* care about interrupt handling performance. Rafael -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, Feb 11, 2015 at 01:38:59PM +, Alexandre Belloni wrote: > On 11/02/2015 at 12:36:56 +, Mark Rutland wrote : > > > Actually, that was one of the requirements expressed by Thomas (Thomas, > > > correct me if I'm wrong). > > > The point was to force shared irq users to explicitly specify that they > > > are mixing !IRQF_NO_SUSPEND and IRQF_NO_SUSPEND because they have no > > > other choice. > > > > > > With your patch, there's no way to inform users that they are > > > erroneously setting the IRQF_NO_SUSPEND flag on one of their shared > > > interrupt. > > > > Sure, but even with the demux that's still the case (because it pretends > > that this mismatch is a HW property rather than a property of the set of > > drivers sharing the interrupt). > > > > Whether there's a demux node in the DTB is entirely separate from > > whether the drivers can actually handle the situation. > > > > So if we need a warning in the presence of mismatch and action masking, > > we need the exact same warning with the demux. > > > > Actually, we only care about removing the warning. It is effectively the > HW that forces us to do so. So we would be completely happy with a new > flag to silence the warning as we know what we are doing (I think that > has already been suggested). Sure. Given that the warning is there to tell you that the _drivers_ aren't using the interrupt in a consistent manner, I don't see how introducing a new fake device in the DT solves that. It simply masks some irqactions at a different level of the SW stack from my patch, completely independently of the drivers. So whether or not the warning is necessary is orthogonal to whether or not this patch is reasonable. If it's not necessary, neither patch needs it. If it is necessary, it's also necessary for the demux. > > The presence of a demux implies the DTB author believes they have solved > > the problem with the demux, not necessarily that they have considered > > the situation and updated drivers appropriately. Relying on the demux to > > imply that everything is fine only gives us the illusion that everything > > is fine. > > > > Whatever the solution, it could be used as a workaround the warning as > this is exactly what we need for our platform. Let's hope we can get this patch into shape quickly then :) Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On 11/02/2015 at 12:36:56 +, Mark Rutland wrote : > > Actually, that was one of the requirements expressed by Thomas (Thomas, > > correct me if I'm wrong). > > The point was to force shared irq users to explicitly specify that they > > are mixing !IRQF_NO_SUSPEND and IRQF_NO_SUSPEND because they have no > > other choice. > > > > With your patch, there's no way to inform users that they are > > erroneously setting the IRQF_NO_SUSPEND flag on one of their shared > > interrupt. > > Sure, but even with the demux that's still the case (because it pretends > that this mismatch is a HW property rather than a property of the set of > drivers sharing the interrupt). > > Whether there's a demux node in the DTB is entirely separate from > whether the drivers can actually handle the situation. > > So if we need a warning in the presence of mismatch and action masking, > we need the exact same warning with the demux. > Actually, we only care about removing the warning. It is effectively the HW that forces us to do so. So we would be completely happy with a new flag to silence the warning as we know what we are doing (I think that has already been suggested). > The presence of a demux implies the DTB author believes they have solved > the problem with the demux, not necessarily that they have considered > the situation and updated drivers appropriately. Relying on the demux to > imply that everything is fine only gives us the illusion that everything > is fine. > Whatever the solution, it could be used as a workaround the warning as this is exactly what we need for our platform. -- Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, Feb 11, 2015 at 12:24:37PM +, Boris Brezillon wrote: > Hi Mark, > > On Wed, 11 Feb 2015 11:11:06 + > Mark Rutland wrote: > > > On Wed, Feb 11, 2015 at 08:53:39AM +, Boris Brezillon wrote: > > > Hi Mark, > > > > > > On Tue, 10 Feb 2015 20:48:36 + > > > Mark Rutland wrote: > > > > > > > On Tue, Feb 10, 2015 at 03:52:01PM +, Boris Brezillon wrote: > > > > > Hi Mark, > > > > > > > > > > On Tue, 10 Feb 2015 15:36:28 + > > > > > Mark Rutland wrote: > > > > > > > > > > > Hi Boris, > > > > > > > > > > > > On Thu, Jan 29, 2015 at 10:33:38AM +, Boris Brezillon wrote: > > > > > > > Add documentation for the virtual irq demuxer. > > > > > > > > > > > > > > Signed-off-by: Boris Brezillon > > > > > > > > > > > > > > Acked-by: Nicolas Ferre > > > > > > > --- > > > > > > > .../bindings/interrupt-controller/dumb-demux.txt | 41 > > > > > > > ++ > > > > > > > 1 file changed, 41 insertions(+) > > > > > > > create mode 100644 > > > > > > > Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > > > > > > > > > > > diff --git > > > > > > > a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > > > > > > > > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > > > > new file mode 100644 > > > > > > > index 000..b9a7830 > > > > > > > --- /dev/null > > > > > > > +++ > > > > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > > > > @@ -0,0 +1,41 @@ > > > > > > > +* Virtual Interrupt Demultiplexer > > > > > > > + > > > > > > > +This virtual demultiplexer simply forward all incoming > > > > > > > interrupts to its > > > > > > > +enabled/unmasked children. > > > > > > > +It is only intended to be used by hardware that do not provide a > > > > > > > proper way > > > > > > > +to demultiplex a source interrupt, and thus have to wake all > > > > > > > their children > > > > > > > +up so that they can possibly handle the interrupt (if needed). > > > > > > > +This can be seen as an alternative to shared interrupts when at > > > > > > > least one > > > > > > > +of the interrupt children is a timer (and require the irq to > > > > > > > stay enabled > > > > > > > +on suspend) while others are not. This will prevent calling irq > > > > > > > handlers of > > > > > > > +non timer devices while they are suspended. > > > > > > > > > > > > This sounds like a DT-workaround for a Linux implementation > > > > > > problem, and > > > > > > I don't think this the right way to solve your problem. > > > > > > > > > > I understand your concern, but why are you answering while I asked for > > > > > DT maintainers reviews for several days (if not several weeks). > > > > > > > > > > > > > > > > > Why does this have to be in DT at all? Why can we not fix the core > > > > > > to > > > > > > handle these details? > > > > > > > > > > We already discussed that with Rob and Thomas, and hiding such a > > > > > demuxer chip is not an easy task. > > > > > I'm open to any suggestion to do that, though I'd like you (I mean DT > > > > > guys) to provide a working implementation (or at least a viable > > > > > concept) > > > > > that would silently demultiplex an irq. > > > > > > > > > > > > > > > > > I am very much not keen on this binding. > > > > > > > > > > Yes, but do you have anything else to propose. > > > > > We're experiencing this warning for 2 releases now, and this is time > > > > > to > > > > > find a solution (even if it's not a perfect one). > > > > > > > > Thoughts on the patch below? > > > > > > That's pretty much what I proposed in my first attempt to solve this > > > problem [1] (except for a few things commented below). > > > Anyway, Thomas suggested to go for the "dumb/virt irq demultiplexer" > > > approach instead. > > > > There is one fundamental difference in that this patch does not require > > drivers to be updated (the new flag is only used internally). Which > > avoids having to patch every single driver that could possibly be used > > in combination with one wanting interrupts during suspend. > > Actually, that was one of the requirements expressed by Thomas (Thomas, > correct me if I'm wrong). > The point was to force shared irq users to explicitly specify that they > are mixing !IRQF_NO_SUSPEND and IRQF_NO_SUSPEND because they have no > other choice. > > With your patch, there's no way to inform users that they are > erroneously setting the IRQF_NO_SUSPEND flag on one of their shared > interrupt. Sure, but even with the demux that's still the case (because it pretends that this mismatch is a HW property rather than a property of the set of drivers sharing the interrupt). Whether there's a demux node in the DTB is entirely separate from whether the drivers can actually handle the situation. So if we need a warning in the presence of mismatch and action masking, we need the exact same warning with the demux. > > Any used w
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
Hi Mark,
On Wed, 11 Feb 2015 11:11:06 +
Mark Rutland wrote:
> On Wed, Feb 11, 2015 at 08:53:39AM +, Boris Brezillon wrote:
> > Hi Mark,
> >
> > On Tue, 10 Feb 2015 20:48:36 +
> > Mark Rutland wrote:
> >
> > > On Tue, Feb 10, 2015 at 03:52:01PM +, Boris Brezillon wrote:
> > > > Hi Mark,
> > > >
> > > > On Tue, 10 Feb 2015 15:36:28 +
> > > > Mark Rutland wrote:
> > > >
> > > > > Hi Boris,
> > > > >
> > > > > On Thu, Jan 29, 2015 at 10:33:38AM +, Boris Brezillon wrote:
> > > > > > Add documentation for the virtual irq demuxer.
> > > > > >
> > > > > > Signed-off-by: Boris Brezillon
> > > > > > Acked-by: Nicolas Ferre
> > > > > > ---
> > > > > > .../bindings/interrupt-controller/dumb-demux.txt | 41
> > > > > > ++
> > > > > > 1 file changed, 41 insertions(+)
> > > > > > create mode 100644
> > > > > > Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > >
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > >
> > > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > > new file mode 100644
> > > > > > index 000..b9a7830
> > > > > > --- /dev/null
> > > > > > +++
> > > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > > @@ -0,0 +1,41 @@
> > > > > > +* Virtual Interrupt Demultiplexer
> > > > > > +
> > > > > > +This virtual demultiplexer simply forward all incoming interrupts
> > > > > > to its
> > > > > > +enabled/unmasked children.
> > > > > > +It is only intended to be used by hardware that do not provide a
> > > > > > proper way
> > > > > > +to demultiplex a source interrupt, and thus have to wake all their
> > > > > > children
> > > > > > +up so that they can possibly handle the interrupt (if needed).
> > > > > > +This can be seen as an alternative to shared interrupts when at
> > > > > > least one
> > > > > > +of the interrupt children is a timer (and require the irq to stay
> > > > > > enabled
> > > > > > +on suspend) while others are not. This will prevent calling irq
> > > > > > handlers of
> > > > > > +non timer devices while they are suspended.
> > > > >
> > > > > This sounds like a DT-workaround for a Linux implementation problem,
> > > > > and
> > > > > I don't think this the right way to solve your problem.
> > > >
> > > > I understand your concern, but why are you answering while I asked for
> > > > DT maintainers reviews for several days (if not several weeks).
> > > >
> > > > >
> > > > > Why does this have to be in DT at all? Why can we not fix the core to
> > > > > handle these details?
> > > >
> > > > We already discussed that with Rob and Thomas, and hiding such a
> > > > demuxer chip is not an easy task.
> > > > I'm open to any suggestion to do that, though I'd like you (I mean DT
> > > > guys) to provide a working implementation (or at least a viable concept)
> > > > that would silently demultiplex an irq.
> > > >
> > > > >
> > > > > I am very much not keen on this binding.
> > > >
> > > > Yes, but do you have anything else to propose.
> > > > We're experiencing this warning for 2 releases now, and this is time to
> > > > find a solution (even if it's not a perfect one).
> > >
> > > Thoughts on the patch below?
> >
> > That's pretty much what I proposed in my first attempt to solve this
> > problem [1] (except for a few things commented below).
> > Anyway, Thomas suggested to go for the "dumb/virt irq demultiplexer"
> > approach instead.
>
> There is one fundamental difference in that this patch does not require
> drivers to be updated (the new flag is only used internally). Which
> avoids having to patch every single driver that could possibly be used
> in combination with one wanting interrupts during suspend.
Actually, that was one of the requirements expressed by Thomas (Thomas,
correct me if I'm wrong).
The point was to force shared irq users to explicitly specify that they
are mixing !IRQF_NO_SUSPEND and IRQF_NO_SUSPEND because they have no
other choice.
With your patch, there's no way to inform users that they are
erroneously setting the IRQF_NO_SUSPEND flag on one of their shared
interrupt.
>
> Any used which did not explicitly request with IRQF_NO_SUSPEND will not
> receive interrupts during suspend.
>
> [...]
>
> > > +static irqreturn_t __handle_irq_event_percpu(unsigned int irq, struct
> > > irqaction *action)
> > > +{
> > > + /*
> > > + * During suspend we must not call potentially unsafe irq handlers.
> > > + * See suspend_suspendable_actions.
> > > + */
> > > + if (unlikely(action->flags & IRQF_NO_ACTION))
> > > + return IRQ_NONE;
> >
> > Thomas was trying to avoid any new conditional code in the interrupt
> > handling path, that's why I added a suspended_action list in my
> > proposal.
> > Even if your 'unlikely' statement make things better I'm pretty sure it
> > adds some latency.
>
> I can see that we don't
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, Feb 11, 2015 at 09:11:59AM +, Peter Zijlstra wrote: > On Tue, Feb 10, 2015 at 08:48:36PM +, Mark Rutland wrote: > > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 > > From: Mark Rutland > > Date: Tue, 10 Feb 2015 20:14:33 + > > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > > > In some cases a physical IRQ line may be shared between devices from > > which we expect interrupts during suspend (e.g. timers) and those we do > > not (e.g. anything we cut the power to). Where a driver did not request > > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > > being called during suspend, and it may bring down the system. > > > > This patch adds logic to automatically mark the irqactions for these > > potentially unsafe handlers as disabled during suspend, leaving actions > > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared line > > during suspend, only the handlers requested with IRQF_NO_SUSPEND will be > > called. The handlers requested without IRQF_NO_SUSPEND will be skipped > > as if they had immediately returned IRQF_NONE. > > > > Cc: Boris Brezillon > > Cc: Jason Cooper > > Cc: Nicolas Ferre > > Cc: Peter Zijlstra > > Cc: Rafael J. Wysocki > > Cc: Thomas Gleixner > > Signed-off-by: Mark Rutland > > Aw gawd.. not that again. I agree this isn't pretty, but at least it doesn't require the HW description to know about Linux internals, and it can work for !DT systems. I'm really not happy with placing Linux implementation details into DTBs. > So Rafael and tglx went over this a few months ago I think: > > lkml.kernel.org/r/26580319.ozp7jvj...@vostro.rjw.lan > > is the last series I could find. Maybe Rafael can summarize? I can't get at any commentary from that link, unfortunately. Rafael? Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Wed, Feb 11, 2015 at 08:53:39AM +, Boris Brezillon wrote:
> Hi Mark,
>
> On Tue, 10 Feb 2015 20:48:36 +
> Mark Rutland wrote:
>
> > On Tue, Feb 10, 2015 at 03:52:01PM +, Boris Brezillon wrote:
> > > Hi Mark,
> > >
> > > On Tue, 10 Feb 2015 15:36:28 +
> > > Mark Rutland wrote:
> > >
> > > > Hi Boris,
> > > >
> > > > On Thu, Jan 29, 2015 at 10:33:38AM +, Boris Brezillon wrote:
> > > > > Add documentation for the virtual irq demuxer.
> > > > >
> > > > > Signed-off-by: Boris Brezillon
> > > > > Acked-by: Nicolas Ferre
> > > > > ---
> > > > > .../bindings/interrupt-controller/dumb-demux.txt | 41
> > > > > ++
> > > > > 1 file changed, 41 insertions(+)
> > > > > create mode 100644
> > > > > Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > >
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > >
> > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > new file mode 100644
> > > > > index 000..b9a7830
> > > > > --- /dev/null
> > > > > +++
> > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
> > > > > @@ -0,0 +1,41 @@
> > > > > +* Virtual Interrupt Demultiplexer
> > > > > +
> > > > > +This virtual demultiplexer simply forward all incoming interrupts to
> > > > > its
> > > > > +enabled/unmasked children.
> > > > > +It is only intended to be used by hardware that do not provide a
> > > > > proper way
> > > > > +to demultiplex a source interrupt, and thus have to wake all their
> > > > > children
> > > > > +up so that they can possibly handle the interrupt (if needed).
> > > > > +This can be seen as an alternative to shared interrupts when at
> > > > > least one
> > > > > +of the interrupt children is a timer (and require the irq to stay
> > > > > enabled
> > > > > +on suspend) while others are not. This will prevent calling irq
> > > > > handlers of
> > > > > +non timer devices while they are suspended.
> > > >
> > > > This sounds like a DT-workaround for a Linux implementation problem, and
> > > > I don't think this the right way to solve your problem.
> > >
> > > I understand your concern, but why are you answering while I asked for
> > > DT maintainers reviews for several days (if not several weeks).
> > >
> > > >
> > > > Why does this have to be in DT at all? Why can we not fix the core to
> > > > handle these details?
> > >
> > > We already discussed that with Rob and Thomas, and hiding such a
> > > demuxer chip is not an easy task.
> > > I'm open to any suggestion to do that, though I'd like you (I mean DT
> > > guys) to provide a working implementation (or at least a viable concept)
> > > that would silently demultiplex an irq.
> > >
> > > >
> > > > I am very much not keen on this binding.
> > >
> > > Yes, but do you have anything else to propose.
> > > We're experiencing this warning for 2 releases now, and this is time to
> > > find a solution (even if it's not a perfect one).
> >
> > Thoughts on the patch below?
>
> That's pretty much what I proposed in my first attempt to solve this
> problem [1] (except for a few things commented below).
> Anyway, Thomas suggested to go for the "dumb/virt irq demultiplexer"
> approach instead.
There is one fundamental difference in that this patch does not require
drivers to be updated (the new flag is only used internally). Which
avoids having to patch every single driver that could possibly be used
in combination with one wanting interrupts during suspend.
Any used which did not explicitly request with IRQF_NO_SUSPEND will not
receive interrupts during suspend.
[...]
> > +static irqreturn_t __handle_irq_event_percpu(unsigned int irq, struct
> > irqaction *action)
> > +{
> > + /*
> > +* During suspend we must not call potentially unsafe irq handlers.
> > +* See suspend_suspendable_actions.
> > +*/
> > + if (unlikely(action->flags & IRQF_NO_ACTION))
> > + return IRQ_NONE;
>
> Thomas was trying to avoid any new conditional code in the interrupt
> handling path, that's why I added a suspended_action list in my
> proposal.
> Even if your 'unlikely' statement make things better I'm pretty sure it
> adds some latency.
I can see that we don't want to add more code here to keep things
clean/pure, but I find it hard to believe that a single bit test and
branch (for data that should be hot in the cache) are going to add
measurable latency to a path that does pointer chasing to get to the
irqaction in the first place. I could be wrong though, and I'm happy to
benchmark.
It would be possible to go for your list shuffling approach here while
still keeping the flag internal and all the logic hidden away in
kernel/irq/pm.c. I wasn't sure how actions could be manipulated during
suspend, which made me wary of moving them to a separate list.
> > + return action->handler(irq, action->dev_id);
> > +}
> > +
> > irqreturn_t
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Tue, Feb 10, 2015 at 08:48:36PM +, Mark Rutland wrote: > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 > From: Mark Rutland > Date: Tue, 10 Feb 2015 20:14:33 + > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > In some cases a physical IRQ line may be shared between devices from > which we expect interrupts during suspend (e.g. timers) and those we do > not (e.g. anything we cut the power to). Where a driver did not request > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > being called during suspend, and it may bring down the system. > > This patch adds logic to automatically mark the irqactions for these > potentially unsafe handlers as disabled during suspend, leaving actions > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared line > during suspend, only the handlers requested with IRQF_NO_SUSPEND will be > called. The handlers requested without IRQF_NO_SUSPEND will be skipped > as if they had immediately returned IRQF_NONE. > > Cc: Boris Brezillon > Cc: Jason Cooper > Cc: Nicolas Ferre > Cc: Peter Zijlstra > Cc: Rafael J. Wysocki > Cc: Thomas Gleixner > Signed-off-by: Mark Rutland Aw gawd.. not that again. So Rafael and tglx went over this a few months ago I think: lkml.kernel.org/r/26580319.ozp7jvj...@vostro.rjw.lan is the last series I could find. Maybe Rafael can summarize? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
Hi Mark, On Tue, 10 Feb 2015 20:48:36 + Mark Rutland wrote: > On Tue, Feb 10, 2015 at 03:52:01PM +, Boris Brezillon wrote: > > Hi Mark, > > > > On Tue, 10 Feb 2015 15:36:28 + > > Mark Rutland wrote: > > > > > Hi Boris, > > > > > > On Thu, Jan 29, 2015 at 10:33:38AM +, Boris Brezillon wrote: > > > > Add documentation for the virtual irq demuxer. > > > > > > > > Signed-off-by: Boris Brezillon > > > > Acked-by: Nicolas Ferre > > > > --- > > > > .../bindings/interrupt-controller/dumb-demux.txt | 41 > > > > ++ > > > > 1 file changed, 41 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > new file mode 100644 > > > > index 000..b9a7830 > > > > --- /dev/null > > > > +++ > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > @@ -0,0 +1,41 @@ > > > > +* Virtual Interrupt Demultiplexer > > > > + > > > > +This virtual demultiplexer simply forward all incoming interrupts to > > > > its > > > > +enabled/unmasked children. > > > > +It is only intended to be used by hardware that do not provide a > > > > proper way > > > > +to demultiplex a source interrupt, and thus have to wake all their > > > > children > > > > +up so that they can possibly handle the interrupt (if needed). > > > > +This can be seen as an alternative to shared interrupts when at least > > > > one > > > > +of the interrupt children is a timer (and require the irq to stay > > > > enabled > > > > +on suspend) while others are not. This will prevent calling irq > > > > handlers of > > > > +non timer devices while they are suspended. > > > > > > This sounds like a DT-workaround for a Linux implementation problem, and > > > I don't think this the right way to solve your problem. > > > > I understand your concern, but why are you answering while I asked for > > DT maintainers reviews for several days (if not several weeks). > > > > > > > > Why does this have to be in DT at all? Why can we not fix the core to > > > handle these details? > > > > We already discussed that with Rob and Thomas, and hiding such a > > demuxer chip is not an easy task. > > I'm open to any suggestion to do that, though I'd like you (I mean DT > > guys) to provide a working implementation (or at least a viable concept) > > that would silently demultiplex an irq. > > > > > > > > I am very much not keen on this binding. > > > > Yes, but do you have anything else to propose. > > We're experiencing this warning for 2 releases now, and this is time to > > find a solution (even if it's not a perfect one). > > Thoughts on the patch below? That's pretty much what I proposed in my first attempt to solve this problem [1] (except for a few things commented below). Anyway, Thomas suggested to go for the "dumb/virt irq demultiplexer" approach instead. > > Rather than handling this at the desc level it adds an extra flag to the > irqaction which can be set/unset during suspend for those irqs we don't > want to handle. That way we don't need to tell the core about the > mismatch explicitly in DT (or ACPI/board files/whatever). > > If we can request/free interrupts during suspend then there's some logic > missing, but it shows the basic idea. > > I didn't have a system to hand with shared mismatched IRQF_NO_SUSPEND > interrupts, so I had to fake that up in code for testing. > > Thanks, > Mark. > > >8 > From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 > From: Mark Rutland > Date: Tue, 10 Feb 2015 20:14:33 + > Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests > > In some cases a physical IRQ line may be shared between devices from > which we expect interrupts during suspend (e.g. timers) and those we do > not (e.g. anything we cut the power to). Where a driver did not request > the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle > being called during suspend, and it may bring down the system. > > This patch adds logic to automatically mark the irqactions for these > potentially unsafe handlers as disabled during suspend, leaving actions > with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared line > during suspend, only the handlers requested with IRQF_NO_SUSPEND will be > called. The handlers requested without IRQF_NO_SUSPEND will be skipped > as if they had immediately returned IRQF_NONE. > > Cc: Boris Brezillon > Cc: Jason Cooper > Cc: Nicolas Ferre > Cc: Peter Zijlstra > Cc: Rafael J. Wysocki > Cc: Thomas Gleixner > Signed-off-by: Mark Rutland > --- > include/linux/interrupt.h | 4 > kernel/irq/handle.c | 13 +++- > kernel/irq/pm.c | 50 > +++ > 3 files changed, 62 in
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Tue, Feb 10, 2015 at 03:52:01PM +, Boris Brezillon wrote: > Hi Mark, > > On Tue, 10 Feb 2015 15:36:28 + > Mark Rutland wrote: > > > Hi Boris, > > > > On Thu, Jan 29, 2015 at 10:33:38AM +, Boris Brezillon wrote: > > > Add documentation for the virtual irq demuxer. > > > > > > Signed-off-by: Boris Brezillon > > > Acked-by: Nicolas Ferre > > > --- > > > .../bindings/interrupt-controller/dumb-demux.txt | 41 > > > ++ > > > 1 file changed, 41 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > > > diff --git > > > a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > new file mode 100644 > > > index 000..b9a7830 > > > --- /dev/null > > > +++ > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > @@ -0,0 +1,41 @@ > > > +* Virtual Interrupt Demultiplexer > > > + > > > +This virtual demultiplexer simply forward all incoming interrupts to its > > > +enabled/unmasked children. > > > +It is only intended to be used by hardware that do not provide a proper > > > way > > > +to demultiplex a source interrupt, and thus have to wake all their > > > children > > > +up so that they can possibly handle the interrupt (if needed). > > > +This can be seen as an alternative to shared interrupts when at least one > > > +of the interrupt children is a timer (and require the irq to stay enabled > > > +on suspend) while others are not. This will prevent calling irq handlers > > > of > > > +non timer devices while they are suspended. > > > > This sounds like a DT-workaround for a Linux implementation problem, and > > I don't think this the right way to solve your problem. > > I understand your concern, but why are you answering while I asked for > DT maintainers reviews for several days (if not several weeks). > > > > > Why does this have to be in DT at all? Why can we not fix the core to > > handle these details? > > We already discussed that with Rob and Thomas, and hiding such a > demuxer chip is not an easy task. > I'm open to any suggestion to do that, though I'd like you (I mean DT > guys) to provide a working implementation (or at least a viable concept) > that would silently demultiplex an irq. > > > > > I am very much not keen on this binding. > > Yes, but do you have anything else to propose. > We're experiencing this warning for 2 releases now, and this is time to > find a solution (even if it's not a perfect one). Thoughts on the patch below? Rather than handling this at the desc level it adds an extra flag to the irqaction which can be set/unset during suspend for those irqs we don't want to handle. That way we don't need to tell the core about the mismatch explicitly in DT (or ACPI/board files/whatever). If we can request/free interrupts during suspend then there's some logic missing, but it shows the basic idea. I didn't have a system to hand with shared mismatched IRQF_NO_SUSPEND interrupts, so I had to fake that up in code for testing. Thanks, Mark. >8 >From f390ccbb31f06efee49b4469943c8d85d963bfb5 Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Tue, 10 Feb 2015 20:14:33 + Subject: [PATCH] genirq: allow mixed IRQF_NO_SUSPEND requests In some cases a physical IRQ line may be shared between devices from which we expect interrupts during suspend (e.g. timers) and those we do not (e.g. anything we cut the power to). Where a driver did not request the interrupt with IRQF_NO_SUSPEND, it's unlikely that it can handle being called during suspend, and it may bring down the system. This patch adds logic to automatically mark the irqactions for these potentially unsafe handlers as disabled during suspend, leaving actions with IRQF_NO_SUSPEND enabled. If an interrupt is raised on a shared line during suspend, only the handlers requested with IRQF_NO_SUSPEND will be called. The handlers requested without IRQF_NO_SUSPEND will be skipped as if they had immediately returned IRQF_NONE. Cc: Boris Brezillon Cc: Jason Cooper Cc: Nicolas Ferre Cc: Peter Zijlstra Cc: Rafael J. Wysocki Cc: Thomas Gleixner Signed-off-by: Mark Rutland --- include/linux/interrupt.h | 4 kernel/irq/handle.c | 13 +++- kernel/irq/pm.c | 50 +++ 3 files changed, 62 insertions(+), 5 deletions(-) diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index d9b05b5..49dcb35 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -57,6 +57,9 @@ * IRQF_NO_THREAD - Interrupt cannot be threaded * IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device *resume time. + * IRQF_NO_ACTION - This irqaction should not be triggered. + * Used during suspend for !IRQF_NO_SUSPEND irqactions which + * share lin
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Tue, 10 Feb 2015 16:16:00 + Mark Rutland wrote: > On Tue, Feb 10, 2015 at 03:52:01PM +, Boris Brezillon wrote: > > Hi Mark, > > > > On Tue, 10 Feb 2015 15:36:28 + > > Mark Rutland wrote: > > > > > Hi Boris, > > > > > > On Thu, Jan 29, 2015 at 10:33:38AM +, Boris Brezillon wrote: > > > > Add documentation for the virtual irq demuxer. > > > > > > > > Signed-off-by: Boris Brezillon > > > > Acked-by: Nicolas Ferre > > > > --- > > > > .../bindings/interrupt-controller/dumb-demux.txt | 41 > > > > ++ > > > > 1 file changed, 41 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > new file mode 100644 > > > > index 000..b9a7830 > > > > --- /dev/null > > > > +++ > > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > @@ -0,0 +1,41 @@ > > > > +* Virtual Interrupt Demultiplexer > > > > + > > > > +This virtual demultiplexer simply forward all incoming interrupts to > > > > its > > > > +enabled/unmasked children. > > > > +It is only intended to be used by hardware that do not provide a > > > > proper way > > > > +to demultiplex a source interrupt, and thus have to wake all their > > > > children > > > > +up so that they can possibly handle the interrupt (if needed). > > > > +This can be seen as an alternative to shared interrupts when at least > > > > one > > > > +of the interrupt children is a timer (and require the irq to stay > > > > enabled > > > > +on suspend) while others are not. This will prevent calling irq > > > > handlers of > > > > +non timer devices while they are suspended. > > > > > > This sounds like a DT-workaround for a Linux implementation problem, and > > > I don't think this the right way to solve your problem. > > > > I understand your concern, but why are you answering while I asked for > > DT maintainers reviews for several days (if not several weeks). > > I am sorry that I did not spot those, and I am very sorry that this > means I am only now able to air my concerns. > > > > Why does this have to be in DT at all? Why can we not fix the core to > > > handle these details? > > > > We already discussed that with Rob and Thomas, and hiding such a > > demuxer chip is not an easy task. > > I'm open to any suggestion to do that, though I'd like you (I mean DT > > guys) to provide a working implementation (or at least a viable concept) > > that would silently demultiplex an irq. > > Is it truly necessary to drop a emux in the middle? > > As far as I can see, all that we're attempting to do here is hide the > IRQF_NO_SUSPEND mismatch from the core IRQ code, though I've only just > started digging and haven't yet figured out where/why the core code > cares. Any hints? You should have a look at this thread [1] ;-) [1]https://lkml.org/lkml/2014/12/15/552 -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
On Tue, Feb 10, 2015 at 03:52:01PM +, Boris Brezillon wrote: > Hi Mark, > > On Tue, 10 Feb 2015 15:36:28 + > Mark Rutland wrote: > > > Hi Boris, > > > > On Thu, Jan 29, 2015 at 10:33:38AM +, Boris Brezillon wrote: > > > Add documentation for the virtual irq demuxer. > > > > > > Signed-off-by: Boris Brezillon > > > Acked-by: Nicolas Ferre > > > --- > > > .../bindings/interrupt-controller/dumb-demux.txt | 41 > > > ++ > > > 1 file changed, 41 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > > > diff --git > > > a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > new file mode 100644 > > > index 000..b9a7830 > > > --- /dev/null > > > +++ > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > @@ -0,0 +1,41 @@ > > > +* Virtual Interrupt Demultiplexer > > > + > > > +This virtual demultiplexer simply forward all incoming interrupts to its > > > +enabled/unmasked children. > > > +It is only intended to be used by hardware that do not provide a proper > > > way > > > +to demultiplex a source interrupt, and thus have to wake all their > > > children > > > +up so that they can possibly handle the interrupt (if needed). > > > +This can be seen as an alternative to shared interrupts when at least one > > > +of the interrupt children is a timer (and require the irq to stay enabled > > > +on suspend) while others are not. This will prevent calling irq handlers > > > of > > > +non timer devices while they are suspended. > > > > This sounds like a DT-workaround for a Linux implementation problem, and > > I don't think this the right way to solve your problem. > > I understand your concern, but why are you answering while I asked for > DT maintainers reviews for several days (if not several weeks). I am sorry that I did not spot those, and I am very sorry that this means I am only now able to air my concerns. > > Why does this have to be in DT at all? Why can we not fix the core to > > handle these details? > > We already discussed that with Rob and Thomas, and hiding such a > demuxer chip is not an easy task. > I'm open to any suggestion to do that, though I'd like you (I mean DT > guys) to provide a working implementation (or at least a viable concept) > that would silently demultiplex an irq. Is it truly necessary to drop a emux in the middle? As far as I can see, all that we're attempting to do here is hide the IRQF_NO_SUSPEND mismatch from the core IRQ code, though I've only just started digging and haven't yet figured out where/why the core code cares. Any hints? > > I am very much not keen on this binding. > > Yes, but do you have anything else to propose. > We're experiencing this warning for 2 releases now, and this is time to > find a solution (even if it's not a perfect one). I appreciate this, and I am really sorry that I have come to this so late. > > > +Required properties: > > > +- compatible: Should be "virtual,irq-demux". > > > +- interrupt-controller: Identifies the node as an interrupt controller. > > > +- interrupts-extended or interrupt-parent and interrupts: Reference the > > > source > > > + interrupt connected to this dumb demuxer. > > > +- #interrupt-cells: The number of cells to define the interrupts (should > > > be 1). > > > + The only cell is the IRQ number. > > > +- irqs: u32 bitfield specifying the interrupts provided by the demuxer. > > > > Arbitrary limitation? > > I first proposed to make this field unlimited, but Thomas suggested to > keep it limited to 32 bits (and I didn't complain since my HW needs > far less than 32 interrupts). Ok. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
I'm fixing my own answer :-) On Tue, 10 Feb 2015 16:52:01 +0100 Boris Brezillon wrote: > Hi Mark, > > On Tue, 10 Feb 2015 15:36:28 + > Mark Rutland wrote: > > > Hi Boris, > > > > On Thu, Jan 29, 2015 at 10:33:38AM +, Boris Brezillon wrote: > > > Add documentation for the virtual irq demuxer. > > > > > > Signed-off-by: Boris Brezillon > > > Acked-by: Nicolas Ferre > > > --- > > > .../bindings/interrupt-controller/dumb-demux.txt | 41 > > > ++ > > > 1 file changed, 41 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > > > diff --git > > > a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > new file mode 100644 > > > index 000..b9a7830 > > > --- /dev/null > > > +++ > > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > @@ -0,0 +1,41 @@ > > > +* Virtual Interrupt Demultiplexer > > > + > > > +This virtual demultiplexer simply forward all incoming interrupts to its > > > +enabled/unmasked children. > > > +It is only intended to be used by hardware that do not provide a proper > > > way > > > +to demultiplex a source interrupt, and thus have to wake all their > > > children > > > +up so that they can possibly handle the interrupt (if needed). > > > +This can be seen as an alternative to shared interrupts when at least one > > > +of the interrupt children is a timer (and require the irq to stay enabled > > > +on suspend) while others are not. This will prevent calling irq handlers > > > of > > > +non timer devices while they are suspended. > > > > This sounds like a DT-workaround for a Linux implementation problem, and > > I don't think this the right way to solve your problem. > > I understand your concern, but why are you answering while I asked for but why are you answering now, while > DT maintainers reviews for several days (if not several weeks). > > > > > Why does this have to be in DT at all? Why can we not fix the core to > > handle these details? > > We already discussed that with Rob and Thomas, and hiding such a > demuxer chip is not an easy task. > I'm open to any suggestion to do that, though I'd like you (I mean DT > guys) to provide a working implementation (or at least a viable concept) > that would silently demultiplex an irq. > > > > > I am very much not keen on this binding. > > Yes, but do you have anything else to propose. > We're experiencing this warning for 2 releases now, and this is time to > find a solution (even if it's not a perfect one). > > > > > > + > > > +Required properties: > > > +- compatible: Should be "virtual,irq-demux". > > > +- interrupt-controller: Identifies the node as an interrupt controller. > > > +- interrupts-extended or interrupt-parent and interrupts: Reference the > > > source > > > + interrupt connected to this dumb demuxer. > > > +- #interrupt-cells: The number of cells to define the interrupts (should > > > be 1). > > > + The only cell is the IRQ number. > > > +- irqs: u32 bitfield specifying the interrupts provided by the demuxer. > > > > Arbitrary limitation? > > I first proposed to make this field unlimited, but Thomas suggested to > keep it limited to 32 bits (and I didn't complain since my HW needs > far less than 32 interrupts). Here is the first implementation I proposed [1], where the 'irqs' property was an array of u32, each entry containing an irq id. [1]https://lkml.org/lkml/2015/1/8/233 -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
Hi Mark, On Tue, 10 Feb 2015 15:36:28 + Mark Rutland wrote: > Hi Boris, > > On Thu, Jan 29, 2015 at 10:33:38AM +, Boris Brezillon wrote: > > Add documentation for the virtual irq demuxer. > > > > Signed-off-by: Boris Brezillon > > Acked-by: Nicolas Ferre > > --- > > .../bindings/interrupt-controller/dumb-demux.txt | 41 > > ++ > > 1 file changed, 41 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > > > diff --git > > a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > new file mode 100644 > > index 000..b9a7830 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > @@ -0,0 +1,41 @@ > > +* Virtual Interrupt Demultiplexer > > + > > +This virtual demultiplexer simply forward all incoming interrupts to its > > +enabled/unmasked children. > > +It is only intended to be used by hardware that do not provide a proper way > > +to demultiplex a source interrupt, and thus have to wake all their children > > +up so that they can possibly handle the interrupt (if needed). > > +This can be seen as an alternative to shared interrupts when at least one > > +of the interrupt children is a timer (and require the irq to stay enabled > > +on suspend) while others are not. This will prevent calling irq handlers of > > +non timer devices while they are suspended. > > This sounds like a DT-workaround for a Linux implementation problem, and > I don't think this the right way to solve your problem. I understand your concern, but why are you answering while I asked for DT maintainers reviews for several days (if not several weeks). > > Why does this have to be in DT at all? Why can we not fix the core to > handle these details? We already discussed that with Rob and Thomas, and hiding such a demuxer chip is not an easy task. I'm open to any suggestion to do that, though I'd like you (I mean DT guys) to provide a working implementation (or at least a viable concept) that would silently demultiplex an irq. > > I am very much not keen on this binding. Yes, but do you have anything else to propose. We're experiencing this warning for 2 releases now, and this is time to find a solution (even if it's not a perfect one). > > > + > > +Required properties: > > +- compatible: Should be "virtual,irq-demux". > > +- interrupt-controller: Identifies the node as an interrupt controller. > > +- interrupts-extended or interrupt-parent and interrupts: Reference the > > source > > + interrupt connected to this dumb demuxer. > > +- #interrupt-cells: The number of cells to define the interrupts (should > > be 1). > > + The only cell is the IRQ number. > > +- irqs: u32 bitfield specifying the interrupts provided by the demuxer. > > Arbitrary limitation? I first proposed to make this field unlimited, but Thomas suggested to keep it limited to 32 bits (and I didn't complain since my HW needs far less than 32 interrupts). Best Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
Hi Boris, On Thu, Jan 29, 2015 at 10:33:38AM +, Boris Brezillon wrote: > Add documentation for the virtual irq demuxer. > > Signed-off-by: Boris Brezillon > Acked-by: Nicolas Ferre > --- > .../bindings/interrupt-controller/dumb-demux.txt | 41 > ++ > 1 file changed, 41 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > > diff --git > a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > new file mode 100644 > index 000..b9a7830 > --- /dev/null > +++ b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt > @@ -0,0 +1,41 @@ > +* Virtual Interrupt Demultiplexer > + > +This virtual demultiplexer simply forward all incoming interrupts to its > +enabled/unmasked children. > +It is only intended to be used by hardware that do not provide a proper way > +to demultiplex a source interrupt, and thus have to wake all their children > +up so that they can possibly handle the interrupt (if needed). > +This can be seen as an alternative to shared interrupts when at least one > +of the interrupt children is a timer (and require the irq to stay enabled > +on suspend) while others are not. This will prevent calling irq handlers of > +non timer devices while they are suspended. This sounds like a DT-workaround for a Linux implementation problem, and I don't think this the right way to solve your problem. Why does this have to be in DT at all? Why can we not fix the core to handle these details? I am very much not keen on this binding. > + > +Required properties: > +- compatible: Should be "virtual,irq-demux". > +- interrupt-controller: Identifies the node as an interrupt controller. > +- interrupts-extended or interrupt-parent and interrupts: Reference the > source > + interrupt connected to this dumb demuxer. > +- #interrupt-cells: The number of cells to define the interrupts (should be > 1). > + The only cell is the IRQ number. > +- irqs: u32 bitfield specifying the interrupts provided by the demuxer. Arbitrary limitation? Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/5] irqchip: Add DT binding doc for the virtual irq demuxer chip
Add documentation for the virtual irq demuxer.
Signed-off-by: Boris Brezillon
Acked-by: Nicolas Ferre
---
.../bindings/interrupt-controller/dumb-demux.txt | 41 ++
1 file changed, 41 insertions(+)
create mode 100644
Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
diff --git
a/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
new file mode 100644
index 000..b9a7830
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/dumb-demux.txt
@@ -0,0 +1,41 @@
+* Virtual Interrupt Demultiplexer
+
+This virtual demultiplexer simply forward all incoming interrupts to its
+enabled/unmasked children.
+It is only intended to be used by hardware that do not provide a proper way
+to demultiplex a source interrupt, and thus have to wake all their children
+up so that they can possibly handle the interrupt (if needed).
+This can be seen as an alternative to shared interrupts when at least one
+of the interrupt children is a timer (and require the irq to stay enabled
+on suspend) while others are not. This will prevent calling irq handlers of
+non timer devices while they are suspended.
+
+Required properties:
+- compatible: Should be "virtual,irq-demux".
+- interrupt-controller: Identifies the node as an interrupt controller.
+- interrupts-extended or interrupt-parent and interrupts: Reference the source
+ interrupt connected to this dumb demuxer.
+- #interrupt-cells: The number of cells to define the interrupts (should be 1).
+ The only cell is the IRQ number.
+- irqs: u32 bitfield specifying the interrupts provided by the demuxer.
+
+Examples:
+ /*
+* virtual demuxer controller
+*/
+ virt_irq1_demux: virt-irq-demux@1 {
+ compatible = "virtual,irq-demux";
+ interrupt-controller;
+ #interrupt-cells = <1>;
+ interrupts-extended = <&aic 1 IRQ_TYPE_LEVEL_HIGH 7>;
+ irqs = <0x3f>;
+ };
+
+ /*
+* Device connected on this dumb demuxer
+*/
+ dma: dma-controller@ec00 {
+ compatible = "atmel,at91sam9g45-dma";
+ reg = <0xec00 0x200>;
+ interrupts-extended = <&virt_irq1_demux 0>;
+ };
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
