Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Thu, 15 Jan 2015, Rob Herring wrote: > On Thu, Jan 15, 2015 at 3:11 AM, Thomas Gleixner wrote: > > On Wed, 14 Jan 2015, Rob Herring wrote: > > > We do not change shared interrupts in any way. We provide an > > alternative mechanism for braindead hardware. And if the at91 folks > > are fine with the DT change, then it's their decision. Nothing forces > > this on everyone. > > We are changing how shared interrupts are described in DT. We don't > need 2 ways to describe them. We could say this is only for AT91 and > continue to describe shared interrupts as has been done forever. Then > the next platform that hits this problem will have to go thru the same > ABI breakage. Or we change DT practices to describe all shared > interrupts with a demux node. Given the way DTs are incrementally > created, it is not something we can check with review or tools, so we > will still have the same ABI breakage problem. This is not describing the proper shared interrupts. This is a special case for a special case of braindamaged hardware. Whats wrong with doing that? We dont have to change that for all shared interrupts because 99% of them have a proper hardware implementation and are not affected by this. What's wrong with serving the AT91 with a proper solution, which does NOT inflict horrible hacks into the core code and does NOT weaken sanity checks and does NOT require irq chip specific knowledge in device drivers? > >> There are probably ways to do this demux irqchip without a DT change. So far you have not provided any useful hint how to do so. > > What's the problem with a DT change for a single platform, if the > > maintainers are willing to take it and deal with the fallout? > > What's the solution for a platform that an ABI break is not okay and > can't deal with the fallout? There is no other platform affected. This is a break for a specific set of devices and the 'fallout' is confined, well known and accepted. So what's your problem, really? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Thu, 15 Jan 2015, Rob Herring wrote: On Thu, Jan 15, 2015 at 3:11 AM, Thomas Gleixner t...@linutronix.de wrote: On Wed, 14 Jan 2015, Rob Herring wrote: We do not change shared interrupts in any way. We provide an alternative mechanism for braindead hardware. And if the at91 folks are fine with the DT change, then it's their decision. Nothing forces this on everyone. We are changing how shared interrupts are described in DT. We don't need 2 ways to describe them. We could say this is only for AT91 and continue to describe shared interrupts as has been done forever. Then the next platform that hits this problem will have to go thru the same ABI breakage. Or we change DT practices to describe all shared interrupts with a demux node. Given the way DTs are incrementally created, it is not something we can check with review or tools, so we will still have the same ABI breakage problem. This is not describing the proper shared interrupts. This is a special case for a special case of braindamaged hardware. Whats wrong with doing that? We dont have to change that for all shared interrupts because 99% of them have a proper hardware implementation and are not affected by this. What's wrong with serving the AT91 with a proper solution, which does NOT inflict horrible hacks into the core code and does NOT weaken sanity checks and does NOT require irq chip specific knowledge in device drivers? There are probably ways to do this demux irqchip without a DT change. So far you have not provided any useful hint how to do so. What's the problem with a DT change for a single platform, if the maintainers are willing to take it and deal with the fallout? What's the solution for a platform that an ABI break is not okay and can't deal with the fallout? There is no other platform affected. This is a break for a specific set of devices and the 'fallout' is confined, well known and accepted. So what's your problem, really? Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Thu, Jan 15, 2015 at 3:11 AM, Thomas Gleixner wrote: > On Wed, 14 Jan 2015, Rob Herring wrote: >> On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner wrote: >> > All attempts to work around that have resulted in horrible bandaids so >> > far. That's why I guided Boris to implement this dummy demultiplexing >> > mechanism. It solves the problem at hand nicely without adding nasty >> > hackarounds into the suspend/resume code and inflicting platform >> > knowledge on multi-platform device drivers. >> >> This change will break on old kernels with a new dtb. Would you be >> happy if a BIOS update required a new kernel? Fixing this for any >> platform requires a dtb update which may not be possible on some >> platforms. I don't have a problem with this breakage for 1 platform >> and the at91 guys may not care, but we'd ultimately be changing how >> all shared irqs are specified for all DT. Maybe we decide that this is >> how we want to describe things, but that needs much wider discussion >> and agreement. > > We do not change shared interrupts in any way. We provide an > alternative mechanism for braindead hardware. And if the at91 folks > are fine with the DT change, then it's their decision. Nothing forces > this on everyone. We are changing how shared interrupts are described in DT. We don't need 2 ways to describe them. We could say this is only for AT91 and continue to describe shared interrupts as has been done forever. Then the next platform that hits this problem will have to go thru the same ABI breakage. Or we change DT practices to describe all shared interrupts with a demux node. Given the way DTs are incrementally created, it is not something we can check with review or tools, so we will still have the same ABI breakage problem. >> > If you have a proper solution for the problem at hand which >> > >> >- avoids the demux dummy >> > >> >- works straight forward with suspend/resume/wakeup >> > >> >- does not add horrible new APIs >> > >> >- does not add conditionals to the interrupt hotpath >> > >> >- does not inflict platform knowledge about interrupt chip details >> > on drivers >> > >> > then I'm happy to take it. >> > >> > But as long as you can't come up with anything sane, the demux dummy >> > is the best solution for this problem we've seen so far. >> >> What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a >> suspended action list? This would leave only the actions with >> IRQF_NO_SUSPEND set in the active action list. The cost would be a >> pointer in irq_desc and moving the actions during suspend and resume. > > That's exactly what we want NOT. Because it prevents us to do proper > sanity checks for IRQF_NO_SUSPEND. We've been there and I rejected it > for that very reason. > >> There are probably ways to do this demux irqchip without a DT change. > > What's the problem with a DT change for a single platform, if the > maintainers are willing to take it and deal with the fallout? What's the solution for a platform that an ABI break is not okay and can't deal with the fallout? > And in case of AT91 the new kernel will simply work with the old DT > and just emit the same warning vs. the IRQF_NO_SUSPEND mismatch. Older > kernels won't work with a new DT, but that's about it. > > Aside of that, I'm quite amused about your DT update worries. DTs > break with every kernel version on very popular platforms in very > weird and subtle ways. Someone has to. It is a problem that we need to get better at preventing. It's an ABI. I don't think I need to explain what that means. Certainly there are folks who would just prefer DT to be a kernel data structure so we don't have to worry about this ABI nonsense. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Le 14/01/2015 23:55, Boris Brezillon a écrit : > Hi Rob, > > On Wed, 14 Jan 2015 16:24:32 -0600 > Rob Herring wrote: > >> On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner wrote: >>> On Tue, 13 Jan 2015, Rob Herring wrote: On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon wrote: > Some interrupt controllers are multiplexing several peripheral IRQs on > a single interrupt line. > While this is not a problem for most IRQs (as long as all peripherals > request the interrupt with IRQF_SHARED flag set), multiplexing timers and > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND > and !IRQF_NO_SUSPEND is prohibited). > > Create a dumb irq demultiplexer which simply forwards interrupts to all > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND > and !IRQF_NO_SUSPEND mix on a given interrupt. This really seems like a work-around for how IRQF_SHARED works. It >>> >>> It's a workaround for a short coming of IRQF_SHARED. >>> >>> IRQF_SHARED has a massive short coming versus suspend and wakeup >>> interrupts. If one of the demultiplexed interrupts is a valid wakeup >>> source then we have no sane way to express this with IRQF_SHARED >>> simply because the drivers need to be aware whether they run on stupid >>> or well designed hardware. >> >> Unfortunately, the drivers will still have to know this. They cannot >> assume that they can call disable_irq and their device irq state does >> not matter. >> >> Perhaps we need a debug feature such that disable_irq/enable_irq are >> nops with IRQF_SHARED? >> seems like what is really desired is just per handler disabling. It is >>> >>> So you want a magic API like disable/enable_irq_action()? >>> >>> Certainly not. >> >> Agreed. >> >>> You'd open just another can of worms which will bring us abuse and >>> hard to debug problems because driver writers think it's a good idea >>> to use it for random purposes. >>> >>> Aside of that it would add another conditional into the interrupt >>> delivery hotpath which is not desired either. >>> fragile in that devices can deadlock the system if the drivers don't disable the interrupt source before calling disable_irq. But unlike >>> >>> Any misdesigned driver can do that for you. >>> IRQF_SHARED, there is nothing explicit in the driver indicating it is designed to work properly with a shared interrupt line. >>> >>> IRQF_SHARED is a pretty bad indicator. Look at all the drivers which >>> slap this flag onto request_irq() and have no single line of code >>> which makes sure that the driver would ever work on a shared line. >>> >>> If it's just for annotational purposes, we can add a new IRQF flag, >>> which is required to request such a interrupt line. >>> I see no reason to accept this into DT either. We already can support shared lines and modeling an OR gate as an interrupt controller is pointless. >>> >>> It's absolutely not pointless. >>> >>> All attempts to work around that have resulted in horrible bandaids so >>> far. That's why I guided Boris to implement this dummy demultiplexing >>> mechanism. It solves the problem at hand nicely without adding nasty >>> hackarounds into the suspend/resume code and inflicting platform >>> knowledge on multi-platform device drivers. >> >> This change will break on old kernels with a new dtb. Would you be >> happy if a BIOS update required a new kernel? Fixing this for any >> platform requires a dtb update which may not be possible on some >> platforms. I don't have a problem with this breakage for 1 platform >> and the at91 guys may not care, but we'd ultimately be changing how >> all shared irqs are specified for all DT. Maybe we decide that this is >> how we want to describe things, but that needs much wider discussion >> and agreement. > > I tried really hard on finding a DT representation that would not break > the DT ABI, but didn't find any easy solution. > How about keeping all platforms with the shared irq pattern, except for > those that really have an irq line shared by a timer and several other > devices (not sure yet, but at91 seems to be the only impacted platform > so far). > >> >>> If you have a proper solution for the problem at hand which >>> >>>- avoids the demux dummy >>> >>>- works straight forward with suspend/resume/wakeup >>> >>>- does not add horrible new APIs >>> >>>- does not add conditionals to the interrupt hotpath >>> >>>- does not inflict platform knowledge about interrupt chip details >>> on drivers >>> >>> then I'm happy to take it. >>> >>> But as long as you can't come up with anything sane, the demux dummy >>> is the best solution for this problem we've seen so far. >> >> What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a >> suspended action list? This would leave only the actions with >>
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Le 15/01/2015 10:11, Thomas Gleixner a écrit : > On Wed, 14 Jan 2015, Rob Herring wrote: >> On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner wrote: >>> All attempts to work around that have resulted in horrible bandaids so >>> far. That's why I guided Boris to implement this dummy demultiplexing >>> mechanism. It solves the problem at hand nicely without adding nasty >>> hackarounds into the suspend/resume code and inflicting platform >>> knowledge on multi-platform device drivers. >> >> This change will break on old kernels with a new dtb. Would you be >> happy if a BIOS update required a new kernel? Fixing this for any >> platform requires a dtb update which may not be possible on some >> platforms. I don't have a problem with this breakage for 1 platform >> and the at91 guys may not care, but we'd ultimately be changing how >> all shared irqs are specified for all DT. Maybe we decide that this is >> how we want to describe things, but that needs much wider discussion >> and agreement. > > We do not change shared interrupts in any way. We provide an > alternative mechanism for braindead hardware. And if the at91 folks Let me add subtle details: "Old braindead hardware, with possibility to use alternative set of timers which doesn't have shared interrupt lines" ;-) > are fine with the DT change, then it's their decision. Nothing forces > this on everyone. Yes I do agree to change DT. >>> If you have a proper solution for the problem at hand which >>> >>>- avoids the demux dummy >>> >>>- works straight forward with suspend/resume/wakeup >>> >>>- does not add horrible new APIs >>> >>>- does not add conditionals to the interrupt hotpath >>> >>>- does not inflict platform knowledge about interrupt chip details >>> on drivers >>> >>> then I'm happy to take it. >>> >>> But as long as you can't come up with anything sane, the demux dummy >>> is the best solution for this problem we've seen so far. >> >> What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a >> suspended action list? This would leave only the actions with >> IRQF_NO_SUSPEND set in the active action list. The cost would be a >> pointer in irq_desc and moving the actions during suspend and resume. > > That's exactly what we want NOT. Because it prevents us to do proper > sanity checks for IRQF_NO_SUSPEND. We've been there and I rejected it > for that very reason. > >> There are probably ways to do this demux irqchip without a DT change. > > What's the problem with a DT change for a single platform, if the > maintainers are willing to take it and deal with the fallout? > > And in case of AT91 the new kernel will simply work with the old DT > and just emit the same warning vs. the IRQF_NO_SUSPEND mismatch. Older > kernels won't work with a new DT, but that's about it. > > Aside of that, I'm quite amused about your DT update worries. DTs > break with every kernel version on very popular platforms in very > weird and subtle ways. DT on AT91 is a Work In Progress (for 3.5 years) and the facts have told us that DT updates were absolutely necessary. So, for now, I agree to change DT as far as AT91 is concerned. Bye, -- Nicolas Ferre -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Wed, 14 Jan 2015, Rob Herring wrote: > On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner wrote: > > All attempts to work around that have resulted in horrible bandaids so > > far. That's why I guided Boris to implement this dummy demultiplexing > > mechanism. It solves the problem at hand nicely without adding nasty > > hackarounds into the suspend/resume code and inflicting platform > > knowledge on multi-platform device drivers. > > This change will break on old kernels with a new dtb. Would you be > happy if a BIOS update required a new kernel? Fixing this for any > platform requires a dtb update which may not be possible on some > platforms. I don't have a problem with this breakage for 1 platform > and the at91 guys may not care, but we'd ultimately be changing how > all shared irqs are specified for all DT. Maybe we decide that this is > how we want to describe things, but that needs much wider discussion > and agreement. We do not change shared interrupts in any way. We provide an alternative mechanism for braindead hardware. And if the at91 folks are fine with the DT change, then it's their decision. Nothing forces this on everyone. > > If you have a proper solution for the problem at hand which > > > >- avoids the demux dummy > > > >- works straight forward with suspend/resume/wakeup > > > >- does not add horrible new APIs > > > >- does not add conditionals to the interrupt hotpath > > > >- does not inflict platform knowledge about interrupt chip details > > on drivers > > > > then I'm happy to take it. > > > > But as long as you can't come up with anything sane, the demux dummy > > is the best solution for this problem we've seen so far. > > What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a > suspended action list? This would leave only the actions with > IRQF_NO_SUSPEND set in the active action list. The cost would be a > pointer in irq_desc and moving the actions during suspend and resume. That's exactly what we want NOT. Because it prevents us to do proper sanity checks for IRQF_NO_SUSPEND. We've been there and I rejected it for that very reason. > There are probably ways to do this demux irqchip without a DT change. What's the problem with a DT change for a single platform, if the maintainers are willing to take it and deal with the fallout? And in case of AT91 the new kernel will simply work with the old DT and just emit the same warning vs. the IRQF_NO_SUSPEND mismatch. Older kernels won't work with a new DT, but that's about it. Aside of that, I'm quite amused about your DT update worries. DTs break with every kernel version on very popular platforms in very weird and subtle ways. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Thu, Jan 15, 2015 at 3:11 AM, Thomas Gleixner t...@linutronix.de wrote: On Wed, 14 Jan 2015, Rob Herring wrote: On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner t...@linutronix.de wrote: All attempts to work around that have resulted in horrible bandaids so far. That's why I guided Boris to implement this dummy demultiplexing mechanism. It solves the problem at hand nicely without adding nasty hackarounds into the suspend/resume code and inflicting platform knowledge on multi-platform device drivers. This change will break on old kernels with a new dtb. Would you be happy if a BIOS update required a new kernel? Fixing this for any platform requires a dtb update which may not be possible on some platforms. I don't have a problem with this breakage for 1 platform and the at91 guys may not care, but we'd ultimately be changing how all shared irqs are specified for all DT. Maybe we decide that this is how we want to describe things, but that needs much wider discussion and agreement. We do not change shared interrupts in any way. We provide an alternative mechanism for braindead hardware. And if the at91 folks are fine with the DT change, then it's their decision. Nothing forces this on everyone. We are changing how shared interrupts are described in DT. We don't need 2 ways to describe them. We could say this is only for AT91 and continue to describe shared interrupts as has been done forever. Then the next platform that hits this problem will have to go thru the same ABI breakage. Or we change DT practices to describe all shared interrupts with a demux node. Given the way DTs are incrementally created, it is not something we can check with review or tools, so we will still have the same ABI breakage problem. If you have a proper solution for the problem at hand which - avoids the demux dummy - works straight forward with suspend/resume/wakeup - does not add horrible new APIs - does not add conditionals to the interrupt hotpath - does not inflict platform knowledge about interrupt chip details on drivers then I'm happy to take it. But as long as you can't come up with anything sane, the demux dummy is the best solution for this problem we've seen so far. What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a suspended action list? This would leave only the actions with IRQF_NO_SUSPEND set in the active action list. The cost would be a pointer in irq_desc and moving the actions during suspend and resume. That's exactly what we want NOT. Because it prevents us to do proper sanity checks for IRQF_NO_SUSPEND. We've been there and I rejected it for that very reason. There are probably ways to do this demux irqchip without a DT change. What's the problem with a DT change for a single platform, if the maintainers are willing to take it and deal with the fallout? What's the solution for a platform that an ABI break is not okay and can't deal with the fallout? And in case of AT91 the new kernel will simply work with the old DT and just emit the same warning vs. the IRQF_NO_SUSPEND mismatch. Older kernels won't work with a new DT, but that's about it. Aside of that, I'm quite amused about your DT update worries. DTs break with every kernel version on very popular platforms in very weird and subtle ways. Someone has to. It is a problem that we need to get better at preventing. It's an ABI. I don't think I need to explain what that means. Certainly there are folks who would just prefer DT to be a kernel data structure so we don't have to worry about this ABI nonsense. Rob -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Wed, 14 Jan 2015, Rob Herring wrote: On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner t...@linutronix.de wrote: All attempts to work around that have resulted in horrible bandaids so far. That's why I guided Boris to implement this dummy demultiplexing mechanism. It solves the problem at hand nicely without adding nasty hackarounds into the suspend/resume code and inflicting platform knowledge on multi-platform device drivers. This change will break on old kernels with a new dtb. Would you be happy if a BIOS update required a new kernel? Fixing this for any platform requires a dtb update which may not be possible on some platforms. I don't have a problem with this breakage for 1 platform and the at91 guys may not care, but we'd ultimately be changing how all shared irqs are specified for all DT. Maybe we decide that this is how we want to describe things, but that needs much wider discussion and agreement. We do not change shared interrupts in any way. We provide an alternative mechanism for braindead hardware. And if the at91 folks are fine with the DT change, then it's their decision. Nothing forces this on everyone. If you have a proper solution for the problem at hand which - avoids the demux dummy - works straight forward with suspend/resume/wakeup - does not add horrible new APIs - does not add conditionals to the interrupt hotpath - does not inflict platform knowledge about interrupt chip details on drivers then I'm happy to take it. But as long as you can't come up with anything sane, the demux dummy is the best solution for this problem we've seen so far. What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a suspended action list? This would leave only the actions with IRQF_NO_SUSPEND set in the active action list. The cost would be a pointer in irq_desc and moving the actions during suspend and resume. That's exactly what we want NOT. Because it prevents us to do proper sanity checks for IRQF_NO_SUSPEND. We've been there and I rejected it for that very reason. There are probably ways to do this demux irqchip without a DT change. What's the problem with a DT change for a single platform, if the maintainers are willing to take it and deal with the fallout? And in case of AT91 the new kernel will simply work with the old DT and just emit the same warning vs. the IRQF_NO_SUSPEND mismatch. Older kernels won't work with a new DT, but that's about it. Aside of that, I'm quite amused about your DT update worries. DTs break with every kernel version on very popular platforms in very weird and subtle ways. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Le 14/01/2015 23:55, Boris Brezillon a écrit : Hi Rob, On Wed, 14 Jan 2015 16:24:32 -0600 Rob Herring robherri...@gmail.com wrote: On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner t...@linutronix.de wrote: On Tue, 13 Jan 2015, Rob Herring wrote: On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon boris.brezil...@free-electrons.com wrote: Some interrupt controllers are multiplexing several peripheral IRQs on a single interrupt line. While this is not a problem for most IRQs (as long as all peripherals request the interrupt with IRQF_SHARED flag set), multiplexing timers and other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND is prohibited). Create a dumb irq demultiplexer which simply forwards interrupts to all peripherals (exactly what's happening with IRQ_SHARED) but keep a unique irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND mix on a given interrupt. This really seems like a work-around for how IRQF_SHARED works. It It's a workaround for a short coming of IRQF_SHARED. IRQF_SHARED has a massive short coming versus suspend and wakeup interrupts. If one of the demultiplexed interrupts is a valid wakeup source then we have no sane way to express this with IRQF_SHARED simply because the drivers need to be aware whether they run on stupid or well designed hardware. Unfortunately, the drivers will still have to know this. They cannot assume that they can call disable_irq and their device irq state does not matter. Perhaps we need a debug feature such that disable_irq/enable_irq are nops with IRQF_SHARED? seems like what is really desired is just per handler disabling. It is So you want a magic API like disable/enable_irq_action()? Certainly not. Agreed. You'd open just another can of worms which will bring us abuse and hard to debug problems because driver writers think it's a good idea to use it for random purposes. Aside of that it would add another conditional into the interrupt delivery hotpath which is not desired either. fragile in that devices can deadlock the system if the drivers don't disable the interrupt source before calling disable_irq. But unlike Any misdesigned driver can do that for you. IRQF_SHARED, there is nothing explicit in the driver indicating it is designed to work properly with a shared interrupt line. IRQF_SHARED is a pretty bad indicator. Look at all the drivers which slap this flag onto request_irq() and have no single line of code which makes sure that the driver would ever work on a shared line. If it's just for annotational purposes, we can add a new IRQF flag, which is required to request such a interrupt line. I see no reason to accept this into DT either. We already can support shared lines and modeling an OR gate as an interrupt controller is pointless. It's absolutely not pointless. All attempts to work around that have resulted in horrible bandaids so far. That's why I guided Boris to implement this dummy demultiplexing mechanism. It solves the problem at hand nicely without adding nasty hackarounds into the suspend/resume code and inflicting platform knowledge on multi-platform device drivers. This change will break on old kernels with a new dtb. Would you be happy if a BIOS update required a new kernel? Fixing this for any platform requires a dtb update which may not be possible on some platforms. I don't have a problem with this breakage for 1 platform and the at91 guys may not care, but we'd ultimately be changing how all shared irqs are specified for all DT. Maybe we decide that this is how we want to describe things, but that needs much wider discussion and agreement. I tried really hard on finding a DT representation that would not break the DT ABI, but didn't find any easy solution. How about keeping all platforms with the shared irq pattern, except for those that really have an irq line shared by a timer and several other devices (not sure yet, but at91 seems to be the only impacted platform so far). If you have a proper solution for the problem at hand which - avoids the demux dummy - works straight forward with suspend/resume/wakeup - does not add horrible new APIs - does not add conditionals to the interrupt hotpath - does not inflict platform knowledge about interrupt chip details on drivers then I'm happy to take it. But as long as you can't come up with anything sane, the demux dummy is the best solution for this problem we've seen so far. What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a suspended action list? This would leave only the actions with IRQF_NO_SUSPEND set in the active action list. The cost would be a pointer in irq_desc and moving the actions during suspend and resume. That really looks like what I suggested here [1]. There are probably ways to do this demux irqchip without a DT change. Since
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Le 15/01/2015 10:11, Thomas Gleixner a écrit : On Wed, 14 Jan 2015, Rob Herring wrote: On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner t...@linutronix.de wrote: All attempts to work around that have resulted in horrible bandaids so far. That's why I guided Boris to implement this dummy demultiplexing mechanism. It solves the problem at hand nicely without adding nasty hackarounds into the suspend/resume code and inflicting platform knowledge on multi-platform device drivers. This change will break on old kernels with a new dtb. Would you be happy if a BIOS update required a new kernel? Fixing this for any platform requires a dtb update which may not be possible on some platforms. I don't have a problem with this breakage for 1 platform and the at91 guys may not care, but we'd ultimately be changing how all shared irqs are specified for all DT. Maybe we decide that this is how we want to describe things, but that needs much wider discussion and agreement. We do not change shared interrupts in any way. We provide an alternative mechanism for braindead hardware. And if the at91 folks Let me add subtle details: Old braindead hardware, with possibility to use alternative set of timers which doesn't have shared interrupt lines ;-) are fine with the DT change, then it's their decision. Nothing forces this on everyone. Yes I do agree to change DT. If you have a proper solution for the problem at hand which - avoids the demux dummy - works straight forward with suspend/resume/wakeup - does not add horrible new APIs - does not add conditionals to the interrupt hotpath - does not inflict platform knowledge about interrupt chip details on drivers then I'm happy to take it. But as long as you can't come up with anything sane, the demux dummy is the best solution for this problem we've seen so far. What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a suspended action list? This would leave only the actions with IRQF_NO_SUSPEND set in the active action list. The cost would be a pointer in irq_desc and moving the actions during suspend and resume. That's exactly what we want NOT. Because it prevents us to do proper sanity checks for IRQF_NO_SUSPEND. We've been there and I rejected it for that very reason. There are probably ways to do this demux irqchip without a DT change. What's the problem with a DT change for a single platform, if the maintainers are willing to take it and deal with the fallout? And in case of AT91 the new kernel will simply work with the old DT and just emit the same warning vs. the IRQF_NO_SUSPEND mismatch. Older kernels won't work with a new DT, but that's about it. Aside of that, I'm quite amused about your DT update worries. DTs break with every kernel version on very popular platforms in very weird and subtle ways. DT on AT91 is a Work In Progress (for 3.5 years) and the facts have told us that DT updates were absolutely necessary. So, for now, I agree to change DT as far as AT91 is concerned. Bye, -- Nicolas Ferre -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Hi Rob, On Wed, 14 Jan 2015 16:24:32 -0600 Rob Herring wrote: > On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner wrote: > > On Tue, 13 Jan 2015, Rob Herring wrote: > >> On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon > >> wrote: > >> > Some interrupt controllers are multiplexing several peripheral IRQs on > >> > a single interrupt line. > >> > While this is not a problem for most IRQs (as long as all peripherals > >> > request the interrupt with IRQF_SHARED flag set), multiplexing timers and > >> > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND > >> > and !IRQF_NO_SUSPEND is prohibited). > >> > > >> > Create a dumb irq demultiplexer which simply forwards interrupts to all > >> > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique > >> > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND > >> > and !IRQF_NO_SUSPEND mix on a given interrupt. > >> > >> This really seems like a work-around for how IRQF_SHARED works. It > > > > It's a workaround for a short coming of IRQF_SHARED. > > > > IRQF_SHARED has a massive short coming versus suspend and wakeup > > interrupts. If one of the demultiplexed interrupts is a valid wakeup > > source then we have no sane way to express this with IRQF_SHARED > > simply because the drivers need to be aware whether they run on stupid > > or well designed hardware. > > Unfortunately, the drivers will still have to know this. They cannot > assume that they can call disable_irq and their device irq state does > not matter. > > Perhaps we need a debug feature such that disable_irq/enable_irq are > nops with IRQF_SHARED? > > >> seems like what is really desired is just per handler disabling. It is > > > > So you want a magic API like disable/enable_irq_action()? > > > > Certainly not. > > Agreed. > > > You'd open just another can of worms which will bring us abuse and > > hard to debug problems because driver writers think it's a good idea > > to use it for random purposes. > > > > Aside of that it would add another conditional into the interrupt > > delivery hotpath which is not desired either. > > > >> fragile in that devices can deadlock the system if the drivers don't > >> disable the interrupt source before calling disable_irq. But unlike > > > > Any misdesigned driver can do that for you. > > > >> IRQF_SHARED, there is nothing explicit in the driver indicating it is > >> designed to work properly with a shared interrupt line. > > > > IRQF_SHARED is a pretty bad indicator. Look at all the drivers which > > slap this flag onto request_irq() and have no single line of code > > which makes sure that the driver would ever work on a shared line. > > > > If it's just for annotational purposes, we can add a new IRQF flag, > > which is required to request such a interrupt line. > > > >> I see no reason to accept this into DT either. We already can support > >> shared lines and modeling an OR gate as an interrupt controller is > >> pointless. > > > > It's absolutely not pointless. > > > > All attempts to work around that have resulted in horrible bandaids so > > far. That's why I guided Boris to implement this dummy demultiplexing > > mechanism. It solves the problem at hand nicely without adding nasty > > hackarounds into the suspend/resume code and inflicting platform > > knowledge on multi-platform device drivers. > > This change will break on old kernels with a new dtb. Would you be > happy if a BIOS update required a new kernel? Fixing this for any > platform requires a dtb update which may not be possible on some > platforms. I don't have a problem with this breakage for 1 platform > and the at91 guys may not care, but we'd ultimately be changing how > all shared irqs are specified for all DT. Maybe we decide that this is > how we want to describe things, but that needs much wider discussion > and agreement. I tried really hard on finding a DT representation that would not break the DT ABI, but didn't find any easy solution. How about keeping all platforms with the shared irq pattern, except for those that really have an irq line shared by a timer and several other devices (not sure yet, but at91 seems to be the only impacted platform so far). > > > If you have a proper solution for the problem at hand which > > > >- avoids the demux dummy > > > >- works straight forward with suspend/resume/wakeup > > > >- does not add horrible new APIs > > > >- does not add conditionals to the interrupt hotpath > > > >- does not inflict platform knowledge about interrupt chip details > > on drivers > > > > then I'm happy to take it. > > > > But as long as you can't come up with anything sane, the demux dummy > > is the best solution for this problem we've seen so far. > > What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a > suspended action list? This would leave only the actions with > IRQF_NO_SUSPEND set in the active action list. The cost would be a > pointer in irq_desc
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner wrote: > On Tue, 13 Jan 2015, Rob Herring wrote: >> On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon >> wrote: >> > Some interrupt controllers are multiplexing several peripheral IRQs on >> > a single interrupt line. >> > While this is not a problem for most IRQs (as long as all peripherals >> > request the interrupt with IRQF_SHARED flag set), multiplexing timers and >> > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND >> > and !IRQF_NO_SUSPEND is prohibited). >> > >> > Create a dumb irq demultiplexer which simply forwards interrupts to all >> > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique >> > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND >> > and !IRQF_NO_SUSPEND mix on a given interrupt. >> >> This really seems like a work-around for how IRQF_SHARED works. It > > It's a workaround for a short coming of IRQF_SHARED. > > IRQF_SHARED has a massive short coming versus suspend and wakeup > interrupts. If one of the demultiplexed interrupts is a valid wakeup > source then we have no sane way to express this with IRQF_SHARED > simply because the drivers need to be aware whether they run on stupid > or well designed hardware. Unfortunately, the drivers will still have to know this. They cannot assume that they can call disable_irq and their device irq state does not matter. Perhaps we need a debug feature such that disable_irq/enable_irq are nops with IRQF_SHARED? >> seems like what is really desired is just per handler disabling. It is > > So you want a magic API like disable/enable_irq_action()? > > Certainly not. Agreed. > You'd open just another can of worms which will bring us abuse and > hard to debug problems because driver writers think it's a good idea > to use it for random purposes. > > Aside of that it would add another conditional into the interrupt > delivery hotpath which is not desired either. > >> fragile in that devices can deadlock the system if the drivers don't >> disable the interrupt source before calling disable_irq. But unlike > > Any misdesigned driver can do that for you. > >> IRQF_SHARED, there is nothing explicit in the driver indicating it is >> designed to work properly with a shared interrupt line. > > IRQF_SHARED is a pretty bad indicator. Look at all the drivers which > slap this flag onto request_irq() and have no single line of code > which makes sure that the driver would ever work on a shared line. > > If it's just for annotational purposes, we can add a new IRQF flag, > which is required to request such a interrupt line. > >> I see no reason to accept this into DT either. We already can support >> shared lines and modeling an OR gate as an interrupt controller is >> pointless. > > It's absolutely not pointless. > > All attempts to work around that have resulted in horrible bandaids so > far. That's why I guided Boris to implement this dummy demultiplexing > mechanism. It solves the problem at hand nicely without adding nasty > hackarounds into the suspend/resume code and inflicting platform > knowledge on multi-platform device drivers. This change will break on old kernels with a new dtb. Would you be happy if a BIOS update required a new kernel? Fixing this for any platform requires a dtb update which may not be possible on some platforms. I don't have a problem with this breakage for 1 platform and the at91 guys may not care, but we'd ultimately be changing how all shared irqs are specified for all DT. Maybe we decide that this is how we want to describe things, but that needs much wider discussion and agreement. > If you have a proper solution for the problem at hand which > >- avoids the demux dummy > >- works straight forward with suspend/resume/wakeup > >- does not add horrible new APIs > >- does not add conditionals to the interrupt hotpath > >- does not inflict platform knowledge about interrupt chip details > on drivers > > then I'm happy to take it. > > But as long as you can't come up with anything sane, the demux dummy > is the best solution for this problem we've seen so far. What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a suspended action list? This would leave only the actions with IRQF_NO_SUSPEND set in the active action list. The cost would be a pointer in irq_desc and moving the actions during suspend and resume. There are probably ways to do this demux irqchip without a DT change. Since we can't just move Linux irq numbers to different irq_chips during request_irq, we would have to parse the DT up front to find all shared interrupts and create a demux irqchip for them. That wouldn't be very efficient, but is straight-forward. Then we'd have to handle the translation into Linux irq numbers correctly which is probably the more difficult part. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Le 14/01/2015 15:03, Boris Brezillon a écrit :
> On Wed, 14 Jan 2015 14:36:42 +0100
> Nicolas Ferre wrote:
>
>> Le 13/01/2015 19:46, Boris Brezillon a écrit :
>>> Some interrupt controllers are multiplexing several peripheral IRQs on
>>> a single interrupt line.
>>> While this is not a problem for most IRQs (as long as all peripherals
>>> request the interrupt with IRQF_SHARED flag set), multiplexing timers and
>>> other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
>>> and !IRQF_NO_SUSPEND is prohibited).
>>>
>>> Create a dumb irq demultiplexer which simply forwards interrupts to all
>>> peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
>>> irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
>>> and !IRQF_NO_SUSPEND mix on a given interrupt.
>>>
>>> Signed-off-by: Boris Brezillon
>>> ---
>>> drivers/irqchip/Kconfig | 4 ++
>>> drivers/irqchip/Makefile | 1 +
>>> drivers/irqchip/irq-dumb-demux.c | 70
>>> include/linux/irq.h | 49 ++
>>> include/linux/irqdomain.h| 1 +
>>> kernel/irq/Kconfig | 5 ++
>>> kernel/irq/Makefile | 1 +
>>> kernel/irq/chip.c| 41
>>> kernel/irq/dumb-demux-chip.c | 140
>>> +++
>>> kernel/irq/handle.c | 31 -
>>> kernel/irq/internals.h | 3 +
>>> 11 files changed, 344 insertions(+), 2 deletions(-)
>>> create mode 100644 drivers/irqchip/irq-dumb-demux.c
>>> create mode 100644 kernel/irq/dumb-demux-chip.c
[..]
>>> +static void irq_dumb_demux_mask(struct irq_data *d)
>>> +{
>>> + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
>>> +
>>> + clear_bit(d->hwirq, >unmasked);
>>> +
>>> + if (!demux->unmasked)
>>> + disable_irq_nosync(demux->src_irq);
>>> +}
>>> +
>>> +static void irq_dumb_demux_unmask(struct irq_data *d)
>>> +{
>>> + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
>>> + bool enable_src_irq = !demux->unmasked;
>>
>> Why this additional "bool" unlike the other function above?
>
> Because set_bit will modify the unmasked status and we must check if it
> is equal to 0 (in other terms, all irqs are masked) before modifying it
> in order to know whether we should enable the src irq or not.
pf! ok, sorry for the noise then ;-)
>>> +
>>> + set_bit(d->hwirq, >unmasked);
>>> +
>>> + if (enable_src_irq)
>>> + enable_irq(demux->src_irq);
>>> +}
>>> +
[...]
Bye,
--
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Wed, 14 Jan 2015 14:36:42 +0100
Nicolas Ferre wrote:
> Le 13/01/2015 19:46, Boris Brezillon a écrit :
> > Some interrupt controllers are multiplexing several peripheral IRQs on
> > a single interrupt line.
> > While this is not a problem for most IRQs (as long as all peripherals
> > request the interrupt with IRQF_SHARED flag set), multiplexing timers and
> > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
> > and !IRQF_NO_SUSPEND is prohibited).
> >
> > Create a dumb irq demultiplexer which simply forwards interrupts to all
> > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
> > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
> > and !IRQF_NO_SUSPEND mix on a given interrupt.
> >
> > Signed-off-by: Boris Brezillon
> > ---
> > drivers/irqchip/Kconfig | 4 ++
> > drivers/irqchip/Makefile | 1 +
> > drivers/irqchip/irq-dumb-demux.c | 70
> > include/linux/irq.h | 49 ++
> > include/linux/irqdomain.h| 1 +
> > kernel/irq/Kconfig | 5 ++
> > kernel/irq/Makefile | 1 +
> > kernel/irq/chip.c| 41
> > kernel/irq/dumb-demux-chip.c | 140
> > +++
> > kernel/irq/handle.c | 31 -
> > kernel/irq/internals.h | 3 +
> > 11 files changed, 344 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/irqchip/irq-dumb-demux.c
> > create mode 100644 kernel/irq/dumb-demux-chip.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index cc79d2a..8a9df88 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ
> > select GENERIC_IRQ_CHIP
> > select IRQ_DOMAIN
> >
> > +config DUMB_DEMUX_IRQ
> > + bool
> > + select DUMB_IRQ_DEMUX_CHIP
> > +
> > config DW_APB_ICTL
> > bool
> > select GENERIC_IRQ_CHIP
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 9516a32..77f3c51 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU)+= irq-armada-370-xp.o
> > obj-$(CONFIG_ARCH_MXS) += irq-mxs.o
> > obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o
> > obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o
> > +obj-$(CONFIG_DUMB_DEMUX_IRQ) += irq-dumb-demux.o
> > obj-$(CONFIG_METAG)+= irq-metag-ext.o
> > obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o
> > obj-$(CONFIG_ARCH_MOXART) += irq-moxart.o
> > diff --git a/drivers/irqchip/irq-dumb-demux.c
> > b/drivers/irqchip/irq-dumb-demux.c
> > new file mode 100644
> > index 000..dfa05ce
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-dumb-demux.c
> > @@ -0,0 +1,70 @@
>
> Maybe add a little file header here. It's always better.
Sure, I just forgot it.
> > +#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP
> > +/**
> > + * handle_dumb_demux_irq - Dumb demuxer irq handle function.
> > + * @irq: the interrupt number
> > + * @desc: the interrupt description structure for this irq
> > + *
> > + * Dumb demux interrupts are sent from a demultiplexing interrupt handler
> > + * which is not able to decide which child interrupt interrupt handler
>
> typo: "interrupt interrupt"
I'll fix that.
>
> > + * should be called.
> > + *
> > + * Note: The caller is expected to handle the ack, clear, mask and
> > + * unmask issues if necessary.
> > + */
[...]
> > +
> > /*
> > * Called unconditionally from handle_level_irq() and only for oneshot
> > * interrupts from handle_fasteoi_irq()
> > diff --git a/kernel/irq/dumb-demux-chip.c b/kernel/irq/dumb-demux-chip.c
> > new file mode 100644
> > index 000..8e2de1d
> > --- /dev/null
> > +++ b/kernel/irq/dumb-demux-chip.c
> > @@ -0,0 +1,140 @@
> > +/*
> > + * Library implementing common dumb irq demux chip functions
> > + *
> > + * Copyright (C) 2015, Boris Brezillon
>
> License here, please.
Yep, I'll add it.
>
> > + */
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "internals.h"
> > +
> > +static void irq_dumb_demux_mask(struct irq_data *d)
> > +{
> > + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
> > +
> > + clear_bit(d->hwirq, >unmasked);
> > +
> > + if (!demux->unmasked)
> > + disable_irq_nosync(demux->src_irq);
> > +}
> > +
> > +static void irq_dumb_demux_unmask(struct irq_data *d)
> > +{
> > + struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
> > + bool enable_src_irq = !demux->unmasked;
>
> Why this additional "bool" unlike the other function above?
Because set_bit will modify the unmasked status and we must check if it
is equal to 0 (in other terms, all irqs are
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Le 13/01/2015 19:46, Boris Brezillon a écrit :
> Some interrupt controllers are multiplexing several peripheral IRQs on
> a single interrupt line.
> While this is not a problem for most IRQs (as long as all peripherals
> request the interrupt with IRQF_SHARED flag set), multiplexing timers and
> other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
> and !IRQF_NO_SUSPEND is prohibited).
>
> Create a dumb irq demultiplexer which simply forwards interrupts to all
> peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
> irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
> and !IRQF_NO_SUSPEND mix on a given interrupt.
>
> Signed-off-by: Boris Brezillon
> ---
> drivers/irqchip/Kconfig | 4 ++
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-dumb-demux.c | 70
> include/linux/irq.h | 49 ++
> include/linux/irqdomain.h| 1 +
> kernel/irq/Kconfig | 5 ++
> kernel/irq/Makefile | 1 +
> kernel/irq/chip.c| 41
> kernel/irq/dumb-demux-chip.c | 140
> +++
> kernel/irq/handle.c | 31 -
> kernel/irq/internals.h | 3 +
> 11 files changed, 344 insertions(+), 2 deletions(-)
> create mode 100644 drivers/irqchip/irq-dumb-demux.c
> create mode 100644 kernel/irq/dumb-demux-chip.c
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index cc79d2a..8a9df88 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ
> select GENERIC_IRQ_CHIP
> select IRQ_DOMAIN
>
> +config DUMB_DEMUX_IRQ
> + bool
> + select DUMB_IRQ_DEMUX_CHIP
> +
> config DW_APB_ICTL
> bool
> select GENERIC_IRQ_CHIP
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 9516a32..77f3c51 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU) += irq-armada-370-xp.o
> obj-$(CONFIG_ARCH_MXS) += irq-mxs.o
> obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o
> obj-$(CONFIG_DW_APB_ICTL)+= irq-dw-apb-ictl.o
> +obj-$(CONFIG_DUMB_DEMUX_IRQ) += irq-dumb-demux.o
> obj-$(CONFIG_METAG) += irq-metag-ext.o
> obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o
> obj-$(CONFIG_ARCH_MOXART)+= irq-moxart.o
> diff --git a/drivers/irqchip/irq-dumb-demux.c
> b/drivers/irqchip/irq-dumb-demux.c
> new file mode 100644
> index 000..dfa05ce
> --- /dev/null
> +++ b/drivers/irqchip/irq-dumb-demux.c
> @@ -0,0 +1,70 @@
Maybe add a little file header here. It's always better.
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "irqchip.h"
> +
> +static int __init dumb_irq_demux_of_init(struct device_node *node,
> + struct device_node *parent)
> +{
> + struct irq_chip_dumb_demux *demux;
> + unsigned int irq;
> + u32 valid_irqs;
> + int ret;
> +
> + irq = irq_of_parse_and_map(node, 0);
> + if (!irq) {
> + pr_err("Failed to retrieve dumb irq demuxer source\n");
> + return -EINVAL;
> + }
> +
> + ret = of_property_read_u32(node, "irqs", _irqs);
> + if (ret) {
> + pr_err("Invalid of missing 'irqs' property\n");
> + return ret;
> + }
> +
> + demux = irq_alloc_dumb_demux_chip(irq, valid_irqs,
> + IRQ_NOREQUEST | IRQ_NOPROBE |
> + IRQ_NOAUTOEN, 0, 0);
> + if (!demux) {
> + pr_err("Failed to allocate dumb irq demuxer struct\n");
> + return -ENOMEM;
> + }
> +
> + demux->domain = irq_domain_add_linear(node, BITS_PER_LONG,
> + _dumb_demux_domain_ops,
> + demux);
> + if (!demux->domain) {
> + ret = -ENOMEM;
> + goto err_free_demux;
> + }
> +
> + ret = irq_set_handler_data(irq, demux);
> + if (ret) {
> + pr_err("Failed to assign handler data\n");
> + goto err_free_domain;
> + }
> +
> + irq_set_chained_handler(irq, irq_dumb_demux_handler);
> +
> + /*
> + * Disable the src irq (automatically enabled by
> + * irq_set_chained_handler) to prevent irqs from happening while
> + * nobody requested any of the demuxed irqs.
> + */
> + disable_irq(irq);
> +
> + return 0;
> +
> +err_free_domain:
> + irq_domain_remove(demux->domain);
> +
> +err_free_demux:
> + kfree(demux);
> +
> + return ret;
> +}
> +IRQCHIP_DECLARE(dumb_irq_demux, "irqchip-dumb-demux",
> dumb_irq_demux_of_init);
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index d09ec7a..ae8fa21 100644
> --- a/include/linux/irq.h
>
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Tue, 13 Jan 2015, Rob Herring wrote: > On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon > wrote: > > Some interrupt controllers are multiplexing several peripheral IRQs on > > a single interrupt line. > > While this is not a problem for most IRQs (as long as all peripherals > > request the interrupt with IRQF_SHARED flag set), multiplexing timers and > > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND > > and !IRQF_NO_SUSPEND is prohibited). > > > > Create a dumb irq demultiplexer which simply forwards interrupts to all > > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique > > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND > > and !IRQF_NO_SUSPEND mix on a given interrupt. > > This really seems like a work-around for how IRQF_SHARED works. It It's a workaround for a short coming of IRQF_SHARED. IRQF_SHARED has a massive short coming versus suspend and wakeup interrupts. If one of the demultiplexed interrupts is a valid wakeup source then we have no sane way to express this with IRQF_SHARED simply because the drivers need to be aware whether they run on stupid or well designed hardware. > seems like what is really desired is just per handler disabling. It is So you want a magic API like disable/enable_irq_action()? Certainly not. You'd open just another can of worms which will bring us abuse and hard to debug problems because driver writers think it's a good idea to use it for random purposes. Aside of that it would add another conditional into the interrupt delivery hotpath which is not desired either. > fragile in that devices can deadlock the system if the drivers don't > disable the interrupt source before calling disable_irq. But unlike Any misdesigned driver can do that for you. > IRQF_SHARED, there is nothing explicit in the driver indicating it is > designed to work properly with a shared interrupt line. IRQF_SHARED is a pretty bad indicator. Look at all the drivers which slap this flag onto request_irq() and have no single line of code which makes sure that the driver would ever work on a shared line. If it's just for annotational purposes, we can add a new IRQF flag, which is required to request such a interrupt line. > I see no reason to accept this into DT either. We already can support > shared lines and modeling an OR gate as an interrupt controller is > pointless. It's absolutely not pointless. All attempts to work around that have resulted in horrible bandaids so far. That's why I guided Boris to implement this dummy demultiplexing mechanism. It solves the problem at hand nicely without adding nasty hackarounds into the suspend/resume code and inflicting platform knowledge on multi-platform device drivers. If you have a proper solution for the problem at hand which - avoids the demux dummy - works straight forward with suspend/resume/wakeup - does not add horrible new APIs - does not add conditionals to the interrupt hotpath - does not inflict platform knowledge about interrupt chip details on drivers then I'm happy to take it. But as long as you can't come up with anything sane, the demux dummy is the best solution for this problem we've seen so far. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Hi Thomas,
On Tue, 13 Jan 2015 22:00:55 +0100 (CET)
Thomas Gleixner wrote:
> On Tue, 13 Jan 2015, Boris Brezillon wrote:
> > + ret = irq_set_handler_data(irq, demux);
> > + if (ret) {
> > + pr_err("Failed to assign handler data\n");
> > + goto err_free_domain;
> > + }
> > +
> > + irq_set_chained_handler(irq, irq_dumb_demux_handler);
> > +
> > + /*
> > +* Disable the src irq (automatically enabled by
> > +* irq_set_chained_handler) to prevent irqs from happening while
> > +* nobody requested any of the demuxed irqs.
> > +*/
> > + disable_irq(irq);
>
> We rather prevent the startup of the irq line right away.
Yep, the comment was here to draw the attention, since I wasn't sure
modifying core code was the best solution.
Anyway I agree that keeping the src_irq disabled when registering the
chained handler is better than doing it afterwards.
>
> enum {
> IRQ_CHAINED_NONE,
> IRQ_CHAINED_STARTUP,
> IRQ_CHAINED_NOSTARTUP,
> };
>
> -__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int
> is_chained,
> - const char *name);
> +__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int
> chained_mode,
> + const char *name);
>
> {
>
> if (handle != handle_bad_irq && chained_mode) {
> irq_settings_set_noprobe(desc);
> irq_settings_set_norequest(desc);
> irq_settings_set_nothread(desc);
> - irq_startup(desc, true);
> + if (chained_mode == IRQ_CHAINED_STARTUP)
> + irq_startup(desc, true);
> }
>
> }
>
> Hmm?
I'm fine with this approach, with a static inline helper:
static inline void
irq_set_chained_handler_nostartup(unsigned int irq,
irq_flow_handler_t handle)
{
__irq_set_handler(irq, handle, IRQ_CHAINED_NOSTARTUP, NULL);
}
Anyway, I'll wait a clear decision regarding which approach should be
taken (see my answer to Rob) before sending a new version.
Thanks for your reviews.
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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Hi Rob, On Tue, 13 Jan 2015 21:26:42 -0600 Rob Herring wrote: > On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon > wrote: > > Some interrupt controllers are multiplexing several peripheral IRQs on > > a single interrupt line. > > While this is not a problem for most IRQs (as long as all peripherals > > request the interrupt with IRQF_SHARED flag set), multiplexing timers and > > other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND > > and !IRQF_NO_SUSPEND is prohibited). > > > > Create a dumb irq demultiplexer which simply forwards interrupts to all > > peripherals (exactly what's happening with IRQ_SHARED) but keep a unique > > irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND > > and !IRQF_NO_SUSPEND mix on a given interrupt. > > This really seems like a work-around for how IRQF_SHARED works. It > seems like what is really desired is just per handler disabling. Like what I proposed here [1] ? > It is > fragile in that devices can deadlock the system if the drivers don't > disable the interrupt source before calling disable_irq. Not exactly deadlock since spurious interrupt detection is implemented, but yes, things won't work as expected. > But unlike > IRQF_SHARED, there is nothing explicit in the driver indicating it is > designed to work properly with a shared interrupt line. > > I see no reason to accept this into DT either. We already can support > shared lines and modeling an OR gate as an interrupt controller is > pointless. Okay, I guess I'll let DT and irq maintainers decide what is preferable here (I already spent much time than I first expected to remove this warning in a proper way). Best Regards, Boris [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 linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner t...@linutronix.de wrote: On Tue, 13 Jan 2015, Rob Herring wrote: On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon boris.brezil...@free-electrons.com wrote: Some interrupt controllers are multiplexing several peripheral IRQs on a single interrupt line. While this is not a problem for most IRQs (as long as all peripherals request the interrupt with IRQF_SHARED flag set), multiplexing timers and other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND is prohibited). Create a dumb irq demultiplexer which simply forwards interrupts to all peripherals (exactly what's happening with IRQ_SHARED) but keep a unique irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND mix on a given interrupt. This really seems like a work-around for how IRQF_SHARED works. It It's a workaround for a short coming of IRQF_SHARED. IRQF_SHARED has a massive short coming versus suspend and wakeup interrupts. If one of the demultiplexed interrupts is a valid wakeup source then we have no sane way to express this with IRQF_SHARED simply because the drivers need to be aware whether they run on stupid or well designed hardware. Unfortunately, the drivers will still have to know this. They cannot assume that they can call disable_irq and their device irq state does not matter. Perhaps we need a debug feature such that disable_irq/enable_irq are nops with IRQF_SHARED? seems like what is really desired is just per handler disabling. It is So you want a magic API like disable/enable_irq_action()? Certainly not. Agreed. You'd open just another can of worms which will bring us abuse and hard to debug problems because driver writers think it's a good idea to use it for random purposes. Aside of that it would add another conditional into the interrupt delivery hotpath which is not desired either. fragile in that devices can deadlock the system if the drivers don't disable the interrupt source before calling disable_irq. But unlike Any misdesigned driver can do that for you. IRQF_SHARED, there is nothing explicit in the driver indicating it is designed to work properly with a shared interrupt line. IRQF_SHARED is a pretty bad indicator. Look at all the drivers which slap this flag onto request_irq() and have no single line of code which makes sure that the driver would ever work on a shared line. If it's just for annotational purposes, we can add a new IRQF flag, which is required to request such a interrupt line. I see no reason to accept this into DT either. We already can support shared lines and modeling an OR gate as an interrupt controller is pointless. It's absolutely not pointless. All attempts to work around that have resulted in horrible bandaids so far. That's why I guided Boris to implement this dummy demultiplexing mechanism. It solves the problem at hand nicely without adding nasty hackarounds into the suspend/resume code and inflicting platform knowledge on multi-platform device drivers. This change will break on old kernels with a new dtb. Would you be happy if a BIOS update required a new kernel? Fixing this for any platform requires a dtb update which may not be possible on some platforms. I don't have a problem with this breakage for 1 platform and the at91 guys may not care, but we'd ultimately be changing how all shared irqs are specified for all DT. Maybe we decide that this is how we want to describe things, but that needs much wider discussion and agreement. If you have a proper solution for the problem at hand which - avoids the demux dummy - works straight forward with suspend/resume/wakeup - does not add horrible new APIs - does not add conditionals to the interrupt hotpath - does not inflict platform knowledge about interrupt chip details on drivers then I'm happy to take it. But as long as you can't come up with anything sane, the demux dummy is the best solution for this problem we've seen so far. What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a suspended action list? This would leave only the actions with IRQF_NO_SUSPEND set in the active action list. The cost would be a pointer in irq_desc and moving the actions during suspend and resume. There are probably ways to do this demux irqchip without a DT change. Since we can't just move Linux irq numbers to different irq_chips during request_irq, we would have to parse the DT up front to find all shared interrupts and create a demux irqchip for them. That wouldn't be very efficient, but is straight-forward. Then we'd have to handle the translation into Linux irq numbers correctly which is probably the more difficult part. Rob -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Hi Rob, On Wed, 14 Jan 2015 16:24:32 -0600 Rob Herring robherri...@gmail.com wrote: On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner t...@linutronix.de wrote: On Tue, 13 Jan 2015, Rob Herring wrote: On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon boris.brezil...@free-electrons.com wrote: Some interrupt controllers are multiplexing several peripheral IRQs on a single interrupt line. While this is not a problem for most IRQs (as long as all peripherals request the interrupt with IRQF_SHARED flag set), multiplexing timers and other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND is prohibited). Create a dumb irq demultiplexer which simply forwards interrupts to all peripherals (exactly what's happening with IRQ_SHARED) but keep a unique irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND mix on a given interrupt. This really seems like a work-around for how IRQF_SHARED works. It It's a workaround for a short coming of IRQF_SHARED. IRQF_SHARED has a massive short coming versus suspend and wakeup interrupts. If one of the demultiplexed interrupts is a valid wakeup source then we have no sane way to express this with IRQF_SHARED simply because the drivers need to be aware whether they run on stupid or well designed hardware. Unfortunately, the drivers will still have to know this. They cannot assume that they can call disable_irq and their device irq state does not matter. Perhaps we need a debug feature such that disable_irq/enable_irq are nops with IRQF_SHARED? seems like what is really desired is just per handler disabling. It is So you want a magic API like disable/enable_irq_action()? Certainly not. Agreed. You'd open just another can of worms which will bring us abuse and hard to debug problems because driver writers think it's a good idea to use it for random purposes. Aside of that it would add another conditional into the interrupt delivery hotpath which is not desired either. fragile in that devices can deadlock the system if the drivers don't disable the interrupt source before calling disable_irq. But unlike Any misdesigned driver can do that for you. IRQF_SHARED, there is nothing explicit in the driver indicating it is designed to work properly with a shared interrupt line. IRQF_SHARED is a pretty bad indicator. Look at all the drivers which slap this flag onto request_irq() and have no single line of code which makes sure that the driver would ever work on a shared line. If it's just for annotational purposes, we can add a new IRQF flag, which is required to request such a interrupt line. I see no reason to accept this into DT either. We already can support shared lines and modeling an OR gate as an interrupt controller is pointless. It's absolutely not pointless. All attempts to work around that have resulted in horrible bandaids so far. That's why I guided Boris to implement this dummy demultiplexing mechanism. It solves the problem at hand nicely without adding nasty hackarounds into the suspend/resume code and inflicting platform knowledge on multi-platform device drivers. This change will break on old kernels with a new dtb. Would you be happy if a BIOS update required a new kernel? Fixing this for any platform requires a dtb update which may not be possible on some platforms. I don't have a problem with this breakage for 1 platform and the at91 guys may not care, but we'd ultimately be changing how all shared irqs are specified for all DT. Maybe we decide that this is how we want to describe things, but that needs much wider discussion and agreement. I tried really hard on finding a DT representation that would not break the DT ABI, but didn't find any easy solution. How about keeping all platforms with the shared irq pattern, except for those that really have an irq line shared by a timer and several other devices (not sure yet, but at91 seems to be the only impacted platform so far). If you have a proper solution for the problem at hand which - avoids the demux dummy - works straight forward with suspend/resume/wakeup - does not add horrible new APIs - does not add conditionals to the interrupt hotpath - does not inflict platform knowledge about interrupt chip details on drivers then I'm happy to take it. But as long as you can't come up with anything sane, the demux dummy is the best solution for this problem we've seen so far. What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a suspended action list? This would leave only the actions with IRQF_NO_SUSPEND set in the active action list. The cost would be a pointer in irq_desc and moving the actions during suspend and resume. That really looks like what I suggested here [1]. There are probably ways to do this demux
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Le 14/01/2015 15:03, Boris Brezillon a écrit :
On Wed, 14 Jan 2015 14:36:42 +0100
Nicolas Ferre nicolas.fe...@atmel.com wrote:
Le 13/01/2015 19:46, Boris Brezillon a écrit :
Some interrupt controllers are multiplexing several peripheral IRQs on
a single interrupt line.
While this is not a problem for most IRQs (as long as all peripherals
request the interrupt with IRQF_SHARED flag set), multiplexing timers and
other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
and !IRQF_NO_SUSPEND is prohibited).
Create a dumb irq demultiplexer which simply forwards interrupts to all
peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
and !IRQF_NO_SUSPEND mix on a given interrupt.
Signed-off-by: Boris Brezillon boris.brezil...@free-electrons.com
---
drivers/irqchip/Kconfig | 4 ++
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-dumb-demux.c | 70
include/linux/irq.h | 49 ++
include/linux/irqdomain.h| 1 +
kernel/irq/Kconfig | 5 ++
kernel/irq/Makefile | 1 +
kernel/irq/chip.c| 41
kernel/irq/dumb-demux-chip.c | 140
+++
kernel/irq/handle.c | 31 -
kernel/irq/internals.h | 3 +
11 files changed, 344 insertions(+), 2 deletions(-)
create mode 100644 drivers/irqchip/irq-dumb-demux.c
create mode 100644 kernel/irq/dumb-demux-chip.c
[..]
+static void irq_dumb_demux_mask(struct irq_data *d)
+{
+ struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
+
+ clear_bit(d-hwirq, demux-unmasked);
+
+ if (!demux-unmasked)
+ disable_irq_nosync(demux-src_irq);
+}
+
+static void irq_dumb_demux_unmask(struct irq_data *d)
+{
+ struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
+ bool enable_src_irq = !demux-unmasked;
Why this additional bool unlike the other function above?
Because set_bit will modify the unmasked status and we must check if it
is equal to 0 (in other terms, all irqs are masked) before modifying it
in order to know whether we should enable the src irq or not.
pf! ok, sorry for the noise then ;-)
+
+ set_bit(d-hwirq, demux-unmasked);
+
+ if (enable_src_irq)
+ enable_irq(demux-src_irq);
+}
+
[...]
Bye,
--
Nicolas Ferre
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Hi Rob, On Tue, 13 Jan 2015 21:26:42 -0600 Rob Herring robherri...@gmail.com wrote: On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon boris.brezil...@free-electrons.com wrote: Some interrupt controllers are multiplexing several peripheral IRQs on a single interrupt line. While this is not a problem for most IRQs (as long as all peripherals request the interrupt with IRQF_SHARED flag set), multiplexing timers and other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND is prohibited). Create a dumb irq demultiplexer which simply forwards interrupts to all peripherals (exactly what's happening with IRQ_SHARED) but keep a unique irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND mix on a given interrupt. This really seems like a work-around for how IRQF_SHARED works. It seems like what is really desired is just per handler disabling. Like what I proposed here [1] ? It is fragile in that devices can deadlock the system if the drivers don't disable the interrupt source before calling disable_irq. Not exactly deadlock since spurious interrupt detection is implemented, but yes, things won't work as expected. But unlike IRQF_SHARED, there is nothing explicit in the driver indicating it is designed to work properly with a shared interrupt line. I see no reason to accept this into DT either. We already can support shared lines and modeling an OR gate as an interrupt controller is pointless. Okay, I guess I'll let DT and irq maintainers decide what is preferable here (I already spent much time than I first expected to remove this warning in a proper way). Best Regards, Boris [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 linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Hi Thomas,
On Tue, 13 Jan 2015 22:00:55 +0100 (CET)
Thomas Gleixner t...@linutronix.de wrote:
On Tue, 13 Jan 2015, Boris Brezillon wrote:
+ ret = irq_set_handler_data(irq, demux);
+ if (ret) {
+ pr_err(Failed to assign handler data\n);
+ goto err_free_domain;
+ }
+
+ irq_set_chained_handler(irq, irq_dumb_demux_handler);
+
+ /*
+* Disable the src irq (automatically enabled by
+* irq_set_chained_handler) to prevent irqs from happening while
+* nobody requested any of the demuxed irqs.
+*/
+ disable_irq(irq);
We rather prevent the startup of the irq line right away.
Yep, the comment was here to draw the attention, since I wasn't sure
modifying core code was the best solution.
Anyway I agree that keeping the src_irq disabled when registering the
chained handler is better than doing it afterwards.
enum {
IRQ_CHAINED_NONE,
IRQ_CHAINED_STARTUP,
IRQ_CHAINED_NOSTARTUP,
};
-__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int
is_chained,
- const char *name);
+__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int
chained_mode,
+ const char *name);
{
if (handle != handle_bad_irq chained_mode) {
irq_settings_set_noprobe(desc);
irq_settings_set_norequest(desc);
irq_settings_set_nothread(desc);
- irq_startup(desc, true);
+ if (chained_mode == IRQ_CHAINED_STARTUP)
+ irq_startup(desc, true);
}
}
Hmm?
I'm fine with this approach, with a static inline helper:
static inline void
irq_set_chained_handler_nostartup(unsigned int irq,
irq_flow_handler_t handle)
{
__irq_set_handler(irq, handle, IRQ_CHAINED_NOSTARTUP, NULL);
}
Anyway, I'll wait a clear decision regarding which approach should be
taken (see my answer to Rob) before sending a new version.
Thanks for your reviews.
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 linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Wed, 14 Jan 2015 14:36:42 +0100
Nicolas Ferre nicolas.fe...@atmel.com wrote:
Le 13/01/2015 19:46, Boris Brezillon a écrit :
Some interrupt controllers are multiplexing several peripheral IRQs on
a single interrupt line.
While this is not a problem for most IRQs (as long as all peripherals
request the interrupt with IRQF_SHARED flag set), multiplexing timers and
other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
and !IRQF_NO_SUSPEND is prohibited).
Create a dumb irq demultiplexer which simply forwards interrupts to all
peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
and !IRQF_NO_SUSPEND mix on a given interrupt.
Signed-off-by: Boris Brezillon boris.brezil...@free-electrons.com
---
drivers/irqchip/Kconfig | 4 ++
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-dumb-demux.c | 70
include/linux/irq.h | 49 ++
include/linux/irqdomain.h| 1 +
kernel/irq/Kconfig | 5 ++
kernel/irq/Makefile | 1 +
kernel/irq/chip.c| 41
kernel/irq/dumb-demux-chip.c | 140
+++
kernel/irq/handle.c | 31 -
kernel/irq/internals.h | 3 +
11 files changed, 344 insertions(+), 2 deletions(-)
create mode 100644 drivers/irqchip/irq-dumb-demux.c
create mode 100644 kernel/irq/dumb-demux-chip.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index cc79d2a..8a9df88 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ
select GENERIC_IRQ_CHIP
select IRQ_DOMAIN
+config DUMB_DEMUX_IRQ
+ bool
+ select DUMB_IRQ_DEMUX_CHIP
+
config DW_APB_ICTL
bool
select GENERIC_IRQ_CHIP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 9516a32..77f3c51 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU)+= irq-armada-370-xp.o
obj-$(CONFIG_ARCH_MXS) += irq-mxs.o
obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o
obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o
+obj-$(CONFIG_DUMB_DEMUX_IRQ) += irq-dumb-demux.o
obj-$(CONFIG_METAG)+= irq-metag-ext.o
obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o
obj-$(CONFIG_ARCH_MOXART) += irq-moxart.o
diff --git a/drivers/irqchip/irq-dumb-demux.c
b/drivers/irqchip/irq-dumb-demux.c
new file mode 100644
index 000..dfa05ce
--- /dev/null
+++ b/drivers/irqchip/irq-dumb-demux.c
@@ -0,0 +1,70 @@
Maybe add a little file header here. It's always better.
Sure, I just forgot it.
+#ifdef CONFIG_DUMB_IRQ_DEMUX_CHIP
+/**
+ * handle_dumb_demux_irq - Dumb demuxer irq handle function.
+ * @irq: the interrupt number
+ * @desc: the interrupt description structure for this irq
+ *
+ * Dumb demux interrupts are sent from a demultiplexing interrupt handler
+ * which is not able to decide which child interrupt interrupt handler
typo: interrupt interrupt
I'll fix that.
+ * should be called.
+ *
+ * Note: The caller is expected to handle the ack, clear, mask and
+ * unmask issues if necessary.
+ */
[...]
+
/*
* Called unconditionally from handle_level_irq() and only for oneshot
* interrupts from handle_fasteoi_irq()
diff --git a/kernel/irq/dumb-demux-chip.c b/kernel/irq/dumb-demux-chip.c
new file mode 100644
index 000..8e2de1d
--- /dev/null
+++ b/kernel/irq/dumb-demux-chip.c
@@ -0,0 +1,140 @@
+/*
+ * Library implementing common dumb irq demux chip functions
+ *
+ * Copyright (C) 2015, Boris Brezillon
License here, please.
Yep, I'll add it.
+ */
+#include linux/err.h
+#include linux/io.h
+#include linux/irq.h
+#include linux/slab.h
+#include linux/export.h
+#include linux/irq.h
+#include linux/irqdomain.h
+#include linux/irqchip/chained_irq.h
+#include linux/interrupt.h
+#include linux/kernel_stat.h
+#include linux/syscore_ops.h
+
+#include internals.h
+
+static void irq_dumb_demux_mask(struct irq_data *d)
+{
+ struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
+
+ clear_bit(d-hwirq, demux-unmasked);
+
+ if (!demux-unmasked)
+ disable_irq_nosync(demux-src_irq);
+}
+
+static void irq_dumb_demux_unmask(struct irq_data *d)
+{
+ struct irq_chip_dumb_demux *demux = irq_data_get_irq_chip_data(d);
+ bool enable_src_irq = !demux-unmasked;
Why this additional bool unlike the other function above?
Because set_bit will modify the unmasked status and we must check if it
is equal to 0 (in other terms, all irqs are masked) before modifying it
in order
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Le 13/01/2015 19:46, Boris Brezillon a écrit :
Some interrupt controllers are multiplexing several peripheral IRQs on
a single interrupt line.
While this is not a problem for most IRQs (as long as all peripherals
request the interrupt with IRQF_SHARED flag set), multiplexing timers and
other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
and !IRQF_NO_SUSPEND is prohibited).
Create a dumb irq demultiplexer which simply forwards interrupts to all
peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
and !IRQF_NO_SUSPEND mix on a given interrupt.
Signed-off-by: Boris Brezillon boris.brezil...@free-electrons.com
---
drivers/irqchip/Kconfig | 4 ++
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-dumb-demux.c | 70
include/linux/irq.h | 49 ++
include/linux/irqdomain.h| 1 +
kernel/irq/Kconfig | 5 ++
kernel/irq/Makefile | 1 +
kernel/irq/chip.c| 41
kernel/irq/dumb-demux-chip.c | 140
+++
kernel/irq/handle.c | 31 -
kernel/irq/internals.h | 3 +
11 files changed, 344 insertions(+), 2 deletions(-)
create mode 100644 drivers/irqchip/irq-dumb-demux.c
create mode 100644 kernel/irq/dumb-demux-chip.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index cc79d2a..8a9df88 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ
select GENERIC_IRQ_CHIP
select IRQ_DOMAIN
+config DUMB_DEMUX_IRQ
+ bool
+ select DUMB_IRQ_DEMUX_CHIP
+
config DW_APB_ICTL
bool
select GENERIC_IRQ_CHIP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 9516a32..77f3c51 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU) += irq-armada-370-xp.o
obj-$(CONFIG_ARCH_MXS) += irq-mxs.o
obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o
obj-$(CONFIG_DW_APB_ICTL)+= irq-dw-apb-ictl.o
+obj-$(CONFIG_DUMB_DEMUX_IRQ) += irq-dumb-demux.o
obj-$(CONFIG_METAG) += irq-metag-ext.o
obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o
obj-$(CONFIG_ARCH_MOXART)+= irq-moxart.o
diff --git a/drivers/irqchip/irq-dumb-demux.c
b/drivers/irqchip/irq-dumb-demux.c
new file mode 100644
index 000..dfa05ce
--- /dev/null
+++ b/drivers/irqchip/irq-dumb-demux.c
@@ -0,0 +1,70 @@
Maybe add a little file header here. It's always better.
+#include linux/interrupt.h
+#include linux/irq.h
+#include linux/irqdomain.h
+#include linux/of_irq.h
+#include linux/slab.h
+
+#include irqchip.h
+
+static int __init dumb_irq_demux_of_init(struct device_node *node,
+ struct device_node *parent)
+{
+ struct irq_chip_dumb_demux *demux;
+ unsigned int irq;
+ u32 valid_irqs;
+ int ret;
+
+ irq = irq_of_parse_and_map(node, 0);
+ if (!irq) {
+ pr_err(Failed to retrieve dumb irq demuxer source\n);
+ return -EINVAL;
+ }
+
+ ret = of_property_read_u32(node, irqs, valid_irqs);
+ if (ret) {
+ pr_err(Invalid of missing 'irqs' property\n);
+ return ret;
+ }
+
+ demux = irq_alloc_dumb_demux_chip(irq, valid_irqs,
+ IRQ_NOREQUEST | IRQ_NOPROBE |
+ IRQ_NOAUTOEN, 0, 0);
+ if (!demux) {
+ pr_err(Failed to allocate dumb irq demuxer struct\n);
+ return -ENOMEM;
+ }
+
+ demux-domain = irq_domain_add_linear(node, BITS_PER_LONG,
+ irq_dumb_demux_domain_ops,
+ demux);
+ if (!demux-domain) {
+ ret = -ENOMEM;
+ goto err_free_demux;
+ }
+
+ ret = irq_set_handler_data(irq, demux);
+ if (ret) {
+ pr_err(Failed to assign handler data\n);
+ goto err_free_domain;
+ }
+
+ irq_set_chained_handler(irq, irq_dumb_demux_handler);
+
+ /*
+ * Disable the src irq (automatically enabled by
+ * irq_set_chained_handler) to prevent irqs from happening while
+ * nobody requested any of the demuxed irqs.
+ */
+ disable_irq(irq);
+
+ return 0;
+
+err_free_domain:
+ irq_domain_remove(demux-domain);
+
+err_free_demux:
+ kfree(demux);
+
+ return ret;
+}
+IRQCHIP_DECLARE(dumb_irq_demux, irqchip-dumb-demux,
dumb_irq_demux_of_init);
diff --git a/include/linux/irq.h b/include/linux/irq.h
index d09ec7a..ae8fa21 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -445,6
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Tue, 13 Jan 2015, Rob Herring wrote: On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon boris.brezil...@free-electrons.com wrote: Some interrupt controllers are multiplexing several peripheral IRQs on a single interrupt line. While this is not a problem for most IRQs (as long as all peripherals request the interrupt with IRQF_SHARED flag set), multiplexing timers and other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND is prohibited). Create a dumb irq demultiplexer which simply forwards interrupts to all peripherals (exactly what's happening with IRQ_SHARED) but keep a unique irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND and !IRQF_NO_SUSPEND mix on a given interrupt. This really seems like a work-around for how IRQF_SHARED works. It It's a workaround for a short coming of IRQF_SHARED. IRQF_SHARED has a massive short coming versus suspend and wakeup interrupts. If one of the demultiplexed interrupts is a valid wakeup source then we have no sane way to express this with IRQF_SHARED simply because the drivers need to be aware whether they run on stupid or well designed hardware. seems like what is really desired is just per handler disabling. It is So you want a magic API like disable/enable_irq_action()? Certainly not. You'd open just another can of worms which will bring us abuse and hard to debug problems because driver writers think it's a good idea to use it for random purposes. Aside of that it would add another conditional into the interrupt delivery hotpath which is not desired either. fragile in that devices can deadlock the system if the drivers don't disable the interrupt source before calling disable_irq. But unlike Any misdesigned driver can do that for you. IRQF_SHARED, there is nothing explicit in the driver indicating it is designed to work properly with a shared interrupt line. IRQF_SHARED is a pretty bad indicator. Look at all the drivers which slap this flag onto request_irq() and have no single line of code which makes sure that the driver would ever work on a shared line. If it's just for annotational purposes, we can add a new IRQF flag, which is required to request such a interrupt line. I see no reason to accept this into DT either. We already can support shared lines and modeling an OR gate as an interrupt controller is pointless. It's absolutely not pointless. All attempts to work around that have resulted in horrible bandaids so far. That's why I guided Boris to implement this dummy demultiplexing mechanism. It solves the problem at hand nicely without adding nasty hackarounds into the suspend/resume code and inflicting platform knowledge on multi-platform device drivers. If you have a proper solution for the problem at hand which - avoids the demux dummy - works straight forward with suspend/resume/wakeup - does not add horrible new APIs - does not add conditionals to the interrupt hotpath - does not inflict platform knowledge about interrupt chip details on drivers then I'm happy to take it. But as long as you can't come up with anything sane, the demux dummy is the best solution for this problem we've seen so far. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon
wrote:
> Some interrupt controllers are multiplexing several peripheral IRQs on
> a single interrupt line.
> While this is not a problem for most IRQs (as long as all peripherals
> request the interrupt with IRQF_SHARED flag set), multiplexing timers and
> other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
> and !IRQF_NO_SUSPEND is prohibited).
>
> Create a dumb irq demultiplexer which simply forwards interrupts to all
> peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
> irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
> and !IRQF_NO_SUSPEND mix on a given interrupt.
This really seems like a work-around for how IRQF_SHARED works. It
seems like what is really desired is just per handler disabling. It is
fragile in that devices can deadlock the system if the drivers don't
disable the interrupt source before calling disable_irq. But unlike
IRQF_SHARED, there is nothing explicit in the driver indicating it is
designed to work properly with a shared interrupt line.
I see no reason to accept this into DT either. We already can support
shared lines and modeling an OR gate as an interrupt controller is
pointless.
Rob
>
> Signed-off-by: Boris Brezillon
> ---
> drivers/irqchip/Kconfig | 4 ++
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-dumb-demux.c | 70
> include/linux/irq.h | 49 ++
> include/linux/irqdomain.h| 1 +
> kernel/irq/Kconfig | 5 ++
> kernel/irq/Makefile | 1 +
> kernel/irq/chip.c| 41
> kernel/irq/dumb-demux-chip.c | 140
> +++
> kernel/irq/handle.c | 31 -
> kernel/irq/internals.h | 3 +
> 11 files changed, 344 insertions(+), 2 deletions(-)
> create mode 100644 drivers/irqchip/irq-dumb-demux.c
> create mode 100644 kernel/irq/dumb-demux-chip.c
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index cc79d2a..8a9df88 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ
> select GENERIC_IRQ_CHIP
> select IRQ_DOMAIN
>
> +config DUMB_DEMUX_IRQ
> + bool
> + select DUMB_IRQ_DEMUX_CHIP
> +
> config DW_APB_ICTL
> bool
> select GENERIC_IRQ_CHIP
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 9516a32..77f3c51 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU)+= irq-armada-370-xp.o
> obj-$(CONFIG_ARCH_MXS) += irq-mxs.o
> obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o
> obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o
> +obj-$(CONFIG_DUMB_DEMUX_IRQ) += irq-dumb-demux.o
> obj-$(CONFIG_METAG)+= irq-metag-ext.o
> obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o
> obj-$(CONFIG_ARCH_MOXART) += irq-moxart.o
> diff --git a/drivers/irqchip/irq-dumb-demux.c
> b/drivers/irqchip/irq-dumb-demux.c
> new file mode 100644
> index 000..dfa05ce
> --- /dev/null
> +++ b/drivers/irqchip/irq-dumb-demux.c
> @@ -0,0 +1,70 @@
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "irqchip.h"
> +
> +static int __init dumb_irq_demux_of_init(struct device_node *node,
> +struct device_node *parent)
> +{
> + struct irq_chip_dumb_demux *demux;
> + unsigned int irq;
> + u32 valid_irqs;
> + int ret;
> +
> + irq = irq_of_parse_and_map(node, 0);
> + if (!irq) {
> + pr_err("Failed to retrieve dumb irq demuxer source\n");
> + return -EINVAL;
> + }
> +
> + ret = of_property_read_u32(node, "irqs", _irqs);
> + if (ret) {
> + pr_err("Invalid of missing 'irqs' property\n");
> + return ret;
> + }
> +
> + demux = irq_alloc_dumb_demux_chip(irq, valid_irqs,
> + IRQ_NOREQUEST | IRQ_NOPROBE |
> + IRQ_NOAUTOEN, 0, 0);
> + if (!demux) {
> + pr_err("Failed to allocate dumb irq demuxer struct\n");
> + return -ENOMEM;
> + }
> +
> + demux->domain = irq_domain_add_linear(node, BITS_PER_LONG,
> + _dumb_demux_domain_ops,
> + demux);
> + if (!demux->domain) {
> + ret = -ENOMEM;
> + goto err_free_demux;
> + }
> +
> + ret = irq_set_handler_data(irq, demux);
> + if (ret) {
> + pr_err("Failed to assign handler data\n");
> + goto err_free_domain;
> + }
> +
> + irq_set_chained_handler(irq, irq_dumb_demux_handler);
> +
> +
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Tue, 13 Jan 2015, Boris Brezillon wrote:
> + ret = irq_set_handler_data(irq, demux);
> + if (ret) {
> + pr_err("Failed to assign handler data\n");
> + goto err_free_domain;
> + }
> +
> + irq_set_chained_handler(irq, irq_dumb_demux_handler);
> +
> + /*
> + * Disable the src irq (automatically enabled by
> + * irq_set_chained_handler) to prevent irqs from happening while
> + * nobody requested any of the demuxed irqs.
> + */
> + disable_irq(irq);
We rather prevent the startup of the irq line right away.
enum {
IRQ_CHAINED_NONE,
IRQ_CHAINED_STARTUP,
IRQ_CHAINED_NOSTARTUP,
};
-__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
- const char *name);
+__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int
chained_mode,
+ const char *name);
{
if (handle != handle_bad_irq && chained_mode) {
irq_settings_set_noprobe(desc);
irq_settings_set_norequest(desc);
irq_settings_set_nothread(desc);
- irq_startup(desc, true);
+ if (chained_mode == IRQ_CHAINED_STARTUP)
+ irq_startup(desc, true);
}
}
Hmm?
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Some interrupt controllers are multiplexing several peripheral IRQs on
a single interrupt line.
While this is not a problem for most IRQs (as long as all peripherals
request the interrupt with IRQF_SHARED flag set), multiplexing timers and
other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
and !IRQF_NO_SUSPEND is prohibited).
Create a dumb irq demultiplexer which simply forwards interrupts to all
peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
and !IRQF_NO_SUSPEND mix on a given interrupt.
Signed-off-by: Boris Brezillon
---
drivers/irqchip/Kconfig | 4 ++
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-dumb-demux.c | 70
include/linux/irq.h | 49 ++
include/linux/irqdomain.h| 1 +
kernel/irq/Kconfig | 5 ++
kernel/irq/Makefile | 1 +
kernel/irq/chip.c| 41
kernel/irq/dumb-demux-chip.c | 140 +++
kernel/irq/handle.c | 31 -
kernel/irq/internals.h | 3 +
11 files changed, 344 insertions(+), 2 deletions(-)
create mode 100644 drivers/irqchip/irq-dumb-demux.c
create mode 100644 kernel/irq/dumb-demux-chip.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index cc79d2a..8a9df88 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ
select GENERIC_IRQ_CHIP
select IRQ_DOMAIN
+config DUMB_DEMUX_IRQ
+ bool
+ select DUMB_IRQ_DEMUX_CHIP
+
config DW_APB_ICTL
bool
select GENERIC_IRQ_CHIP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 9516a32..77f3c51 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU)+= irq-armada-370-xp.o
obj-$(CONFIG_ARCH_MXS) += irq-mxs.o
obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o
obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o
+obj-$(CONFIG_DUMB_DEMUX_IRQ) += irq-dumb-demux.o
obj-$(CONFIG_METAG)+= irq-metag-ext.o
obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o
obj-$(CONFIG_ARCH_MOXART) += irq-moxart.o
diff --git a/drivers/irqchip/irq-dumb-demux.c b/drivers/irqchip/irq-dumb-demux.c
new file mode 100644
index 000..dfa05ce
--- /dev/null
+++ b/drivers/irqchip/irq-dumb-demux.c
@@ -0,0 +1,70 @@
+#include
+#include
+#include
+#include
+#include
+
+#include "irqchip.h"
+
+static int __init dumb_irq_demux_of_init(struct device_node *node,
+struct device_node *parent)
+{
+ struct irq_chip_dumb_demux *demux;
+ unsigned int irq;
+ u32 valid_irqs;
+ int ret;
+
+ irq = irq_of_parse_and_map(node, 0);
+ if (!irq) {
+ pr_err("Failed to retrieve dumb irq demuxer source\n");
+ return -EINVAL;
+ }
+
+ ret = of_property_read_u32(node, "irqs", _irqs);
+ if (ret) {
+ pr_err("Invalid of missing 'irqs' property\n");
+ return ret;
+ }
+
+ demux = irq_alloc_dumb_demux_chip(irq, valid_irqs,
+ IRQ_NOREQUEST | IRQ_NOPROBE |
+ IRQ_NOAUTOEN, 0, 0);
+ if (!demux) {
+ pr_err("Failed to allocate dumb irq demuxer struct\n");
+ return -ENOMEM;
+ }
+
+ demux->domain = irq_domain_add_linear(node, BITS_PER_LONG,
+ _dumb_demux_domain_ops,
+ demux);
+ if (!demux->domain) {
+ ret = -ENOMEM;
+ goto err_free_demux;
+ }
+
+ ret = irq_set_handler_data(irq, demux);
+ if (ret) {
+ pr_err("Failed to assign handler data\n");
+ goto err_free_domain;
+ }
+
+ irq_set_chained_handler(irq, irq_dumb_demux_handler);
+
+ /*
+* Disable the src irq (automatically enabled by
+* irq_set_chained_handler) to prevent irqs from happening while
+* nobody requested any of the demuxed irqs.
+*/
+ disable_irq(irq);
+
+ return 0;
+
+err_free_domain:
+ irq_domain_remove(demux->domain);
+
+err_free_demux:
+ kfree(demux);
+
+ return ret;
+}
+IRQCHIP_DECLARE(dumb_irq_demux, "irqchip-dumb-demux", dumb_irq_demux_of_init);
diff --git a/include/linux/irq.h b/include/linux/irq.h
index d09ec7a..ae8fa21 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -445,6 +445,10 @@ extern void handle_fasteoi_irq(unsigned int irq, struct
irq_desc *desc);
extern void handle_edge_irq(unsigned int irq, struct irq_desc *desc);
extern void handle_edge_eoi_irq(unsigned int irq, struct irq_desc *desc);
extern
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Tue, Jan 13, 2015 at 12:46 PM, Boris Brezillon
boris.brezil...@free-electrons.com wrote:
Some interrupt controllers are multiplexing several peripheral IRQs on
a single interrupt line.
While this is not a problem for most IRQs (as long as all peripherals
request the interrupt with IRQF_SHARED flag set), multiplexing timers and
other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
and !IRQF_NO_SUSPEND is prohibited).
Create a dumb irq demultiplexer which simply forwards interrupts to all
peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
and !IRQF_NO_SUSPEND mix on a given interrupt.
This really seems like a work-around for how IRQF_SHARED works. It
seems like what is really desired is just per handler disabling. It is
fragile in that devices can deadlock the system if the drivers don't
disable the interrupt source before calling disable_irq. But unlike
IRQF_SHARED, there is nothing explicit in the driver indicating it is
designed to work properly with a shared interrupt line.
I see no reason to accept this into DT either. We already can support
shared lines and modeling an OR gate as an interrupt controller is
pointless.
Rob
Signed-off-by: Boris Brezillon boris.brezil...@free-electrons.com
---
drivers/irqchip/Kconfig | 4 ++
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-dumb-demux.c | 70
include/linux/irq.h | 49 ++
include/linux/irqdomain.h| 1 +
kernel/irq/Kconfig | 5 ++
kernel/irq/Makefile | 1 +
kernel/irq/chip.c| 41
kernel/irq/dumb-demux-chip.c | 140
+++
kernel/irq/handle.c | 31 -
kernel/irq/internals.h | 3 +
11 files changed, 344 insertions(+), 2 deletions(-)
create mode 100644 drivers/irqchip/irq-dumb-demux.c
create mode 100644 kernel/irq/dumb-demux-chip.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index cc79d2a..8a9df88 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ
select GENERIC_IRQ_CHIP
select IRQ_DOMAIN
+config DUMB_DEMUX_IRQ
+ bool
+ select DUMB_IRQ_DEMUX_CHIP
+
config DW_APB_ICTL
bool
select GENERIC_IRQ_CHIP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 9516a32..77f3c51 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU)+= irq-armada-370-xp.o
obj-$(CONFIG_ARCH_MXS) += irq-mxs.o
obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o
obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o
+obj-$(CONFIG_DUMB_DEMUX_IRQ) += irq-dumb-demux.o
obj-$(CONFIG_METAG)+= irq-metag-ext.o
obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o
obj-$(CONFIG_ARCH_MOXART) += irq-moxart.o
diff --git a/drivers/irqchip/irq-dumb-demux.c
b/drivers/irqchip/irq-dumb-demux.c
new file mode 100644
index 000..dfa05ce
--- /dev/null
+++ b/drivers/irqchip/irq-dumb-demux.c
@@ -0,0 +1,70 @@
+#include linux/interrupt.h
+#include linux/irq.h
+#include linux/irqdomain.h
+#include linux/of_irq.h
+#include linux/slab.h
+
+#include irqchip.h
+
+static int __init dumb_irq_demux_of_init(struct device_node *node,
+struct device_node *parent)
+{
+ struct irq_chip_dumb_demux *demux;
+ unsigned int irq;
+ u32 valid_irqs;
+ int ret;
+
+ irq = irq_of_parse_and_map(node, 0);
+ if (!irq) {
+ pr_err(Failed to retrieve dumb irq demuxer source\n);
+ return -EINVAL;
+ }
+
+ ret = of_property_read_u32(node, irqs, valid_irqs);
+ if (ret) {
+ pr_err(Invalid of missing 'irqs' property\n);
+ return ret;
+ }
+
+ demux = irq_alloc_dumb_demux_chip(irq, valid_irqs,
+ IRQ_NOREQUEST | IRQ_NOPROBE |
+ IRQ_NOAUTOEN, 0, 0);
+ if (!demux) {
+ pr_err(Failed to allocate dumb irq demuxer struct\n);
+ return -ENOMEM;
+ }
+
+ demux-domain = irq_domain_add_linear(node, BITS_PER_LONG,
+ irq_dumb_demux_domain_ops,
+ demux);
+ if (!demux-domain) {
+ ret = -ENOMEM;
+ goto err_free_demux;
+ }
+
+ ret = irq_set_handler_data(irq, demux);
+ if (ret) {
+ pr_err(Failed to assign handler data\n);
+ goto err_free_domain;
+ }
+
+ irq_set_chained_handler(irq,
[PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
Some interrupt controllers are multiplexing several peripheral IRQs on
a single interrupt line.
While this is not a problem for most IRQs (as long as all peripherals
request the interrupt with IRQF_SHARED flag set), multiplexing timers and
other type of peripherals will generate a WARNING (mixing IRQF_NO_SUSPEND
and !IRQF_NO_SUSPEND is prohibited).
Create a dumb irq demultiplexer which simply forwards interrupts to all
peripherals (exactly what's happening with IRQ_SHARED) but keep a unique
irq number for each peripheral, thus preventing the IRQF_NO_SUSPEND
and !IRQF_NO_SUSPEND mix on a given interrupt.
Signed-off-by: Boris Brezillon boris.brezil...@free-electrons.com
---
drivers/irqchip/Kconfig | 4 ++
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-dumb-demux.c | 70
include/linux/irq.h | 49 ++
include/linux/irqdomain.h| 1 +
kernel/irq/Kconfig | 5 ++
kernel/irq/Makefile | 1 +
kernel/irq/chip.c| 41
kernel/irq/dumb-demux-chip.c | 140 +++
kernel/irq/handle.c | 31 -
kernel/irq/internals.h | 3 +
11 files changed, 344 insertions(+), 2 deletions(-)
create mode 100644 drivers/irqchip/irq-dumb-demux.c
create mode 100644 kernel/irq/dumb-demux-chip.c
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index cc79d2a..8a9df88 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -70,6 +70,10 @@ config BRCMSTB_L2_IRQ
select GENERIC_IRQ_CHIP
select IRQ_DOMAIN
+config DUMB_DEMUX_IRQ
+ bool
+ select DUMB_IRQ_DEMUX_CHIP
+
config DW_APB_ICTL
bool
select GENERIC_IRQ_CHIP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 9516a32..77f3c51 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_MVEBU)+= irq-armada-370-xp.o
obj-$(CONFIG_ARCH_MXS) += irq-mxs.o
obj-$(CONFIG_ARCH_S3C24XX) += irq-s3c24xx.o
obj-$(CONFIG_DW_APB_ICTL) += irq-dw-apb-ictl.o
+obj-$(CONFIG_DUMB_DEMUX_IRQ) += irq-dumb-demux.o
obj-$(CONFIG_METAG)+= irq-metag-ext.o
obj-$(CONFIG_METAG_PERFCOUNTER_IRQS) += irq-metag.o
obj-$(CONFIG_ARCH_MOXART) += irq-moxart.o
diff --git a/drivers/irqchip/irq-dumb-demux.c b/drivers/irqchip/irq-dumb-demux.c
new file mode 100644
index 000..dfa05ce
--- /dev/null
+++ b/drivers/irqchip/irq-dumb-demux.c
@@ -0,0 +1,70 @@
+#include linux/interrupt.h
+#include linux/irq.h
+#include linux/irqdomain.h
+#include linux/of_irq.h
+#include linux/slab.h
+
+#include irqchip.h
+
+static int __init dumb_irq_demux_of_init(struct device_node *node,
+struct device_node *parent)
+{
+ struct irq_chip_dumb_demux *demux;
+ unsigned int irq;
+ u32 valid_irqs;
+ int ret;
+
+ irq = irq_of_parse_and_map(node, 0);
+ if (!irq) {
+ pr_err(Failed to retrieve dumb irq demuxer source\n);
+ return -EINVAL;
+ }
+
+ ret = of_property_read_u32(node, irqs, valid_irqs);
+ if (ret) {
+ pr_err(Invalid of missing 'irqs' property\n);
+ return ret;
+ }
+
+ demux = irq_alloc_dumb_demux_chip(irq, valid_irqs,
+ IRQ_NOREQUEST | IRQ_NOPROBE |
+ IRQ_NOAUTOEN, 0, 0);
+ if (!demux) {
+ pr_err(Failed to allocate dumb irq demuxer struct\n);
+ return -ENOMEM;
+ }
+
+ demux-domain = irq_domain_add_linear(node, BITS_PER_LONG,
+ irq_dumb_demux_domain_ops,
+ demux);
+ if (!demux-domain) {
+ ret = -ENOMEM;
+ goto err_free_demux;
+ }
+
+ ret = irq_set_handler_data(irq, demux);
+ if (ret) {
+ pr_err(Failed to assign handler data\n);
+ goto err_free_domain;
+ }
+
+ irq_set_chained_handler(irq, irq_dumb_demux_handler);
+
+ /*
+* Disable the src irq (automatically enabled by
+* irq_set_chained_handler) to prevent irqs from happening while
+* nobody requested any of the demuxed irqs.
+*/
+ disable_irq(irq);
+
+ return 0;
+
+err_free_domain:
+ irq_domain_remove(demux-domain);
+
+err_free_demux:
+ kfree(demux);
+
+ return ret;
+}
+IRQCHIP_DECLARE(dumb_irq_demux, irqchip-dumb-demux, dumb_irq_demux_of_init);
diff --git a/include/linux/irq.h b/include/linux/irq.h
index d09ec7a..ae8fa21 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -445,6 +445,10 @@ extern void handle_fasteoi_irq(unsigned int irq, struct
irq_desc *desc);
extern void handle_edge_irq(unsigned int irq, struct
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Tue, 13 Jan 2015, Boris Brezillon wrote:
+ ret = irq_set_handler_data(irq, demux);
+ if (ret) {
+ pr_err(Failed to assign handler data\n);
+ goto err_free_domain;
+ }
+
+ irq_set_chained_handler(irq, irq_dumb_demux_handler);
+
+ /*
+ * Disable the src irq (automatically enabled by
+ * irq_set_chained_handler) to prevent irqs from happening while
+ * nobody requested any of the demuxed irqs.
+ */
+ disable_irq(irq);
We rather prevent the startup of the irq line right away.
enum {
IRQ_CHAINED_NONE,
IRQ_CHAINED_STARTUP,
IRQ_CHAINED_NOSTARTUP,
};
-__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
- const char *name);
+__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int
chained_mode,
+ const char *name);
{
if (handle != handle_bad_irq chained_mode) {
irq_settings_set_noprobe(desc);
irq_settings_set_norequest(desc);
irq_settings_set_nothread(desc);
- irq_startup(desc, true);
+ if (chained_mode == IRQ_CHAINED_STARTUP)
+ irq_startup(desc, true);
}
}
Hmm?
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
