| Message ID | 1606728979-44259-1-git-send-email-luojiaxing@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] gpio: dwapb: mask/unmask IRQ when disable/enable it | expand |
On Mon, Nov 30, 2020 at 05:36:19PM +0800, Luo Jiaxing wrote: > The mask and unmask registers are not configured in dwapb_irq_enable() and > dwapb_irq_disable(). In the following situations, the IRQ will be masked by > default after the IRQ is enabled: > > mask IRQ -> disable IRQ -> enable IRQ > > In this case, the IRQ status of GPIO controller is inconsistent with it's > irq_data too. For example, in __irq_enable(), IRQD_IRQ_DISABLED and > IRQD_IRQ_MASKED are both clear, but GPIO controller do not perform unmask. Sounds a bit like a papering over the issue which is slightly different. Can you elaborate more, why ->irq_mask() / ->irq_unmask() are not being called?
On 2020/11/30 19:22, Andy Shevchenko wrote: > On Mon, Nov 30, 2020 at 05:36:19PM +0800, Luo Jiaxing wrote: >> The mask and unmask registers are not configured in dwapb_irq_enable() and >> dwapb_irq_disable(). In the following situations, the IRQ will be masked by >> default after the IRQ is enabled: >> >> mask IRQ -> disable IRQ -> enable IRQ >> >> In this case, the IRQ status of GPIO controller is inconsistent with it's >> irq_data too. For example, in __irq_enable(), IRQD_IRQ_DISABLED and >> IRQD_IRQ_MASKED are both clear, but GPIO controller do not perform unmask. > Sounds a bit like a papering over the issue which is slightly different. > Can you elaborate more, why ->irq_mask() / ->irq_unmask() are not being called? Sure, The basic software invoking process is as follows: Release IRQ: free_irq() -> __free_irq() -> irq_shutdown() ->__irq_disable() Disable IRQ: disable_irq() -> __disable_irq_nosync() -> __disable_irq -> irq_disable -> __irq_disable() As shown before, both will call __irq_disable(). The code of it is as follows: if (irqd_irq_disabled(&desc->irq_data)) { if (mask) mask_irq(desc); } else { irq_state_set_disabled(desc); if (desc->irq_data.chip->irq_disable) { desc->irq_data.chip->irq_disable(&desc->irq_data); irq_state_set_masked(desc); } else if (mask) { mask_irq(desc); } } Because gpio-dwapb.c provides the hook function of irq_disable, __irq_disable() will directly calls chip->irq_disable() instead of mask_irq(). For irq_enable(), it's similar and the code is as follows: if (!irqd_irq_disabled(&desc->irq_data)) { unmask_irq(desc); } else { irq_state_clr_disabled(desc); if (desc->irq_data.chip->irq_enable) { desc->irq_data.chip->irq_enable(&desc->irq_data); irq_state_clr_masked(desc); } else { unmask_irq(desc); } } Similarly, because gpio-dwapb.c provides the hook function of irq_enable, irq_enable() will directly calls chip->irq_enable() but does not call unmask_irq(). Therefore, the current handle is as follows: API of IRQ: | mask_irq() | disable_irq() | enable_irq() gpio-dwapb.c: | chip->irq_mask() | chip->irq_diable() | chip->irq_enable() I do not know why irq_enable() only calls chip->irq_enable(). However, the code shows that irq_enable() clears the disable and masked flags in the irq_data state. Therefore, for gpio-dwapb.c, I thinks ->irq_enable also needs to clear the disable and masked flags in the hardware register.
Sorry for top posting but I need the help of the irqchip maintainer Marc Z to hash this out. The mask/unmask/disable/enable semantics is something that you need to work with every day to understand right. Yours, Linus Walleij On Mon, Nov 30, 2020 at 10:36 AM Luo Jiaxing <luojiaxing@huawei.com> wrote: > > The mask and unmask registers are not configured in dwapb_irq_enable() and > dwapb_irq_disable(). In the following situations, the IRQ will be masked by > default after the IRQ is enabled: > > mask IRQ -> disable IRQ -> enable IRQ > > In this case, the IRQ status of GPIO controller is inconsistent with it's > irq_data too. For example, in __irq_enable(), IRQD_IRQ_DISABLED and > IRQD_IRQ_MASKED are both clear, but GPIO controller do not perform unmask. > > Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com> > --- > drivers/gpio/gpio-dwapb.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 2a9046c..ca654eb 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -270,6 +270,8 @@ static void dwapb_irq_enable(struct irq_data *d) > u32 val; > > spin_lock_irqsave(&gc->bgpio_lock, flags); > + val = dwapb_read(gpio, GPIO_INTMASK) & ~BIT(irqd_to_hwirq(d)); > + dwapb_write(gpio, GPIO_INTMASK, val); > val = dwapb_read(gpio, GPIO_INTEN); > val |= BIT(irqd_to_hwirq(d)); > dwapb_write(gpio, GPIO_INTEN, val); > @@ -284,6 +286,8 @@ static void dwapb_irq_disable(struct irq_data *d) > u32 val; > > spin_lock_irqsave(&gc->bgpio_lock, flags); > + val = dwapb_read(gpio, GPIO_INTMASK) | BIT(irqd_to_hwirq(d)); > + dwapb_write(gpio, GPIO_INTMASK, val); > val = dwapb_read(gpio, GPIO_INTEN); > val &= ~BIT(irqd_to_hwirq(d)); > dwapb_write(gpio, GPIO_INTEN, val); > -- > 2.7.4 >
On Tue, Dec 01, 2020 at 04:59:21PM +0800, luojiaxing wrote: > > On 2020/11/30 19:22, Andy Shevchenko wrote: > > On Mon, Nov 30, 2020 at 05:36:19PM +0800, Luo Jiaxing wrote: > > > The mask and unmask registers are not configured in dwapb_irq_enable() and > > > dwapb_irq_disable(). In the following situations, the IRQ will be masked by > > > default after the IRQ is enabled: > > > > > > mask IRQ -> disable IRQ -> enable IRQ > > > > > > In this case, the IRQ status of GPIO controller is inconsistent with it's > > > irq_data too. For example, in __irq_enable(), IRQD_IRQ_DISABLED and > > > IRQD_IRQ_MASKED are both clear, but GPIO controller do not perform unmask. > > Sounds a bit like a papering over the issue which is slightly different. > > Can you elaborate more, why ->irq_mask() / ->irq_unmask() are not being called? > > > Sure, The basic software invoking process is as follows: > > Release IRQ: > free_irq() -> __free_irq() -> irq_shutdown() ->__irq_disable() > > Disable IRQ: > disable_irq() -> __disable_irq_nosync() -> __disable_irq -> irq_disable -> > __irq_disable() > > As shown before, both will call __irq_disable(). The code of it is as > follows: > > if (irqd_irq_disabled(&desc->irq_data)) { > if (mask) > mask_irq(desc); > > } else { > irq_state_set_disabled(desc); > if (desc->irq_data.chip->irq_disable) { > desc->irq_data.chip->irq_disable(&desc->irq_data); > irq_state_set_masked(desc); > } else if (mask) { > mask_irq(desc); > } > } > > Because gpio-dwapb.c provides the hook function of irq_disable, > __irq_disable() will directly calls chip->irq_disable() instead of > mask_irq(). > > For irq_enable(), it's similar and the code is as follows: > > if (!irqd_irq_disabled(&desc->irq_data)) { > unmask_irq(desc); > } else { > irq_state_clr_disabled(desc); > if (desc->irq_data.chip->irq_enable) { > desc->irq_data.chip->irq_enable(&desc->irq_data); > irq_state_clr_masked(desc); > } else { > unmask_irq(desc); > } > } > > Similarly, because gpio-dwapb.c provides the hook function of irq_enable, > irq_enable() will directly calls chip->irq_enable() but does not call > unmask_irq(). > > > Therefore, the current handle is as follows: > > API of IRQ: | mask_irq() | disable_irq() > | enable_irq() > > gpio-dwapb.c: | chip->irq_mask() | chip->irq_diable() | > chip->irq_enable() > > I do not know why irq_enable() only calls chip->irq_enable(). However, the > code shows that irq_enable() clears the disable and masked flags in the > irq_data state. > > Therefore, for gpio-dwapb.c, I thinks ->irq_enable also needs to clear the > disable and masked flags in the hardware register. > Hmm, that sounds like a problem, but the explanation is a bit unclear to me. AFAICS you are saying that the only callbacks which are called during the IRQ request/release are the irq_enable(), right? If so then the only reason why we haven't got a problem reported due to that so far is that the IRQs actually unmasked by default. In anyway I'd suggest to join someone from the kernel IRQs-related subsystem to this discussion to ask their opinion whether the IRQs setup procedure is supposed to work like you say and the irq_enable shall actually also unmask IRQs. Thomas, Jason, Mark, could you give us your comment about the issue? -Sergey > > >
On Sat, Dec 5, 2020 at 11:15 PM Serge Semin <fancer.lancer@gmail.com> wrote: > Hmm, that sounds like a problem, but the explanation is a bit unclear > to me. AFAICS you are saying that the only callbacks which are > called during the IRQ request/release are the irq_enable(), right? If > so then the only reason why we haven't got a problem reported due to > that so far is that the IRQs actually unmasked by default. What we usually do in cases like that (and I have discussed this with tglx in the past I think) is to simply mask off all IRQs in probe(). Then they will be unmasked when requested by drivers. See e.g. gpio-pl061 that has this line in probe(): writeb(0, pl061->base + GPIOIE); /* disable irqs */ Yours, Linus Walleij
On 2020-12-06 15:02, Linus Walleij wrote: > On Sat, Dec 5, 2020 at 11:15 PM Serge Semin <fancer.lancer@gmail.com> > wrote: > >> Hmm, that sounds like a problem, but the explanation is a bit unclear >> to me. AFAICS you are saying that the only callbacks which are >> called during the IRQ request/release are the irq_enable(), right? If >> so then the only reason why we haven't got a problem reported due to >> that so far is that the IRQs actually unmasked by default. > > What we usually do in cases like that (and I have discussed this > with tglx in the past I think) is to simply mask off all IRQs in > probe(). > Then they will be unmasked when requested by drivers. > > See e.g. gpio-pl061 that has this line in probe(): > writeb(0, pl061->base + GPIOIE); /* disable irqs */ This should definitely be the default behaviour. The code code expects all interrupt sources to be masked until actively enabled, usually with the IRQ being requested. Thanks, M.
On 2020/12/6 6:15, Serge Semin wrote: > On Tue, Dec 01, 2020 at 04:59:21PM +0800, luojiaxing wrote: >> On 2020/11/30 19:22, Andy Shevchenko wrote: >>> On Mon, Nov 30, 2020 at 05:36:19PM +0800, Luo Jiaxing wrote: >>>> The mask and unmask registers are not configured in dwapb_irq_enable() and >>>> dwapb_irq_disable(). In the following situations, the IRQ will be masked by >>>> default after the IRQ is enabled: >>>> >>>> mask IRQ -> disable IRQ -> enable IRQ >>>> >>>> In this case, the IRQ status of GPIO controller is inconsistent with it's >>>> irq_data too. For example, in __irq_enable(), IRQD_IRQ_DISABLED and >>>> IRQD_IRQ_MASKED are both clear, but GPIO controller do not perform unmask. >>> Sounds a bit like a papering over the issue which is slightly different. >>> Can you elaborate more, why ->irq_mask() / ->irq_unmask() are not being called? >> >> Sure, The basic software invoking process is as follows: >> >> Release IRQ: >> free_irq() -> __free_irq() -> irq_shutdown() ->__irq_disable() >> >> Disable IRQ: >> disable_irq() -> __disable_irq_nosync() -> __disable_irq -> irq_disable -> >> __irq_disable() >> >> As shown before, both will call __irq_disable(). The code of it is as >> follows: >> >> if (irqd_irq_disabled(&desc->irq_data)) { >> if (mask) >> mask_irq(desc); >> >> } else { >> irq_state_set_disabled(desc); >> if (desc->irq_data.chip->irq_disable) { >> desc->irq_data.chip->irq_disable(&desc->irq_data); >> irq_state_set_masked(desc); >> } else if (mask) { >> mask_irq(desc); >> } >> } >> >> Because gpio-dwapb.c provides the hook function of irq_disable, >> __irq_disable() will directly calls chip->irq_disable() instead of >> mask_irq(). >> >> For irq_enable(), it's similar and the code is as follows: >> >> if (!irqd_irq_disabled(&desc->irq_data)) { >> unmask_irq(desc); >> } else { >> irq_state_clr_disabled(desc); >> if (desc->irq_data.chip->irq_enable) { >> desc->irq_data.chip->irq_enable(&desc->irq_data); >> irq_state_clr_masked(desc); >> } else { >> unmask_irq(desc); >> } >> } >> >> Similarly, because gpio-dwapb.c provides the hook function of irq_enable, >> irq_enable() will directly calls chip->irq_enable() but does not call >> unmask_irq(). >> >> >> Therefore, the current handle is as follows: >> >> API of IRQ: | mask_irq() | disable_irq() >> | enable_irq() >> >> gpio-dwapb.c: | chip->irq_mask() | chip->irq_diable() | >> chip->irq_enable() >> >> I do not know why irq_enable() only calls chip->irq_enable(). However, the >> code shows that irq_enable() clears the disable and masked flags in the >> irq_data state. >> >> Therefore, for gpio-dwapb.c, I thinks ->irq_enable also needs to clear the >> disable and masked flags in the hardware register. >> > Hmm, that sounds like a problem, but the explanation is a bit unclear > to me. AFAICS you are saying that the only callbacks which are > called during the IRQ request/release are the irq_enable(), right? Yes, but one point needs to be clarified, for IRQ requests, it calls irq_enable(); for IRQ release, it calls irq_disable(). Actually I am thinking that why only irq_enable()/irq_disable() is called since the mask and enable flags of irq_data are both set. Does IRQ subsystem expect irq_enable to set both mask and enable? If we didn't do that, the state machine of the software is different from hardware, at least for mask bit. > If > so then the only reason why we haven't got a problem reported due to > that so far is that the IRQs actually unmasked by default. yes, I think so, Common drivers do not mask the IRQ before releasing it. But that's possible. > > In anyway I'd suggest to join someone from the kernel IRQs-related > subsystem to this discussion to ask their opinion whether the IRQs > setup procedure is supposed to work like you say and the irq_enable > shall actually also unmask IRQs. > > Thomas, Jason, Mark, could you give us your comment about the issue? > > -Sergey > >> >> > . >
On 2020/12/6 23:02, Linus Walleij wrote: > On Sat, Dec 5, 2020 at 11:15 PM Serge Semin <fancer.lancer@gmail.com> wrote: > >> Hmm, that sounds like a problem, but the explanation is a bit unclear >> to me. AFAICS you are saying that the only callbacks which are >> called during the IRQ request/release are the irq_enable(), right? If >> so then the only reason why we haven't got a problem reported due to >> that so far is that the IRQs actually unmasked by default. > What we usually do in cases like that (and I have discussed this > with tglx in the past I think) is to simply mask off all IRQs in probe(). > Then they will be unmasked when requested by drivers. Yes, I agree. but in this case I mean that they will not unmasked when drivers request IRQ. Drivers request IRQ will only call irq_enable(), so if we mask all IRQ in gpio-dwab.'s probe(), no one will unmask the IRQ for drivers. Thanks Jiaxing > > See e.g. gpio-pl061 that has this line in probe(): > writeb(0, pl061->base + GPIOIE); /* disable irqs */ > > Yours, > Linus Walleij > > . >
On 2020/12/7 2:50, Marc Zyngier wrote: > On 2020-12-06 15:02, Linus Walleij wrote: >> On Sat, Dec 5, 2020 at 11:15 PM Serge Semin <fancer.lancer@gmail.com> >> wrote: >> >>> Hmm, that sounds like a problem, but the explanation is a bit unclear >>> to me. AFAICS you are saying that the only callbacks which are >>> called during the IRQ request/release are the irq_enable(), right? If >>> so then the only reason why we haven't got a problem reported due to >>> that so far is that the IRQs actually unmasked by default. >> >> What we usually do in cases like that (and I have discussed this >> with tglx in the past I think) is to simply mask off all IRQs in >> probe(). >> Then they will be unmasked when requested by drivers. >> >> See e.g. gpio-pl061 that has this line in probe(): >> writeb(0, pl061->base + GPIOIE); /* disable irqs */ > > This should definitely be the default behaviour. The code code > expects all interrupt sources to be masked until actively enabled, > usually with the IRQ being requested. I think this patch is used for that purpose. I do two things in irq_enable(): unmask irq and then enable IRQ; and for irq_disable(), it's similar; mask IRQ then disable it. Thanks Jiaxing > > Thanks, > > M.
On Mon, Dec 7, 2020 at 2:10 PM luojiaxing <luojiaxing@huawei.com> wrote: > > > On 2020/12/7 2:50, Marc Zyngier wrote: > > On 2020-12-06 15:02, Linus Walleij wrote: > >> On Sat, Dec 5, 2020 at 11:15 PM Serge Semin <fancer.lancer@gmail.com> > >> wrote: > >> > >>> Hmm, that sounds like a problem, but the explanation is a bit unclear > >>> to me. AFAICS you are saying that the only callbacks which are > >>> called during the IRQ request/release are the irq_enable(), right? If > >>> so then the only reason why we haven't got a problem reported due to > >>> that so far is that the IRQs actually unmasked by default. > >> > >> What we usually do in cases like that (and I have discussed this > >> with tglx in the past I think) is to simply mask off all IRQs in > >> probe(). > >> Then they will be unmasked when requested by drivers. > >> > >> See e.g. gpio-pl061 that has this line in probe(): > >> writeb(0, pl061->base + GPIOIE); /* disable irqs */ > > > > This should definitely be the default behaviour. The code code > > expects all interrupt sources to be masked until actively enabled, > > usually with the IRQ being requested. > > > I think this patch is used for that purpose. I do two things in > irq_enable(): unmask irq and then enable IRQ; > > and for irq_disable(), it's similar; mask IRQ then disable it. Hi! Could you please resend this patch rebased on top of v5.11-rc2 and with the detailed explanation you responded with to Andy as part of the commit message? Thanks! Bart
Hello folks, My comments are below. On Wed, Jan 06, 2021 at 01:44:28PM +0200, Andy Shevchenko wrote: > On Wednesday, January 6, 2021, Bartosz Golaszewski < > bgolaszewski@baylibre.com> wrote: > > > On Mon, Dec 7, 2020 at 2:10 PM luojiaxing <luojiaxing@huawei.com> wrote: > > > > > > > > > On 2020/12/7 2:50, Marc Zyngier wrote: > > > > On 2020-12-06 15:02, Linus Walleij wrote: > > > >> On Sat, Dec 5, 2020 at 11:15 PM Serge Semin <fancer.lancer@gmail.com> > > > >> wrote: > > > >> > > > >>> Hmm, that sounds like a problem, but the explanation is a bit unclear > > > >>> to me. AFAICS you are saying that the only callbacks which are > > > >>> called during the IRQ request/release are the irq_enable(), right? If > > > >>> so then the only reason why we haven't got a problem reported due to > > > >>> that so far is that the IRQs actually unmasked by default. As I said the problem explanation stated in the log is a bit unclear to me. It needs elaboration at the very least in v2 with more details why masking and masking needs to be performed in the IRQ disable/enable callback. But AFAICS from the code invocation stack and the Luo further messages the problem with having both mask/unmask and disable/enable IRQ-chip functionality may indeed exist. Judging by the irq_enable() and irq_disable() functions code both of them use only one pair of the IRQ switchers with giving more favor to the IRQ disable/enable methods. So if the later are specified for an IRQ chip, then the IRQ mask/unmask functions just won't be called. (Though I might be wrong in this matter. Marc, please correct me if I am.) In our case if for some reason any of the GPIO lane IRQ has been masked for instance by a bootloader or the default state has been changed on IP-core level, then the corresponding IRQ just won't be activated by the kernel IRQs subsystem. In case of my DW APB core and most likely in the most of the cases all the IRQs are unmasked, but disabled by default. That's why we haven't got any report about the problem until now. > > > >> > > > >> What we usually do in cases like that (and I have discussed this > > > >> with tglx in the past I think) is to simply mask off all IRQs in > > > >> probe(). > > > >> Then they will be unmasked when requested by drivers. > > > >> > > > >> See e.g. gpio-pl061 that has this line in probe(): > > > >> writeb(0, pl061->base + GPIOIE); /* disable irqs */ > > > > > > > > This should definitely be the default behaviour. The code code > > > > expects all interrupt sources to be masked until actively enabled, > > > > usually with the IRQ being requested. What DW APB driver has different with respect to the others is that it provides both IRQ mask/unmask and disable/enable functionality. So if we get to mask all the IRQs in the probe method, then according to the irq_enable()/irq_disable() code semantics and as Luo said the corresponding IRQ won't be unmasked in request_irq(), but will be just enabled. So effectively the IRQ will be left masked, which of course we don't want to happen after the successful request_irq() method return. As I see it the problem either needs to be worked around in the local driver (for instance in a way Luo suggests) or fixed in the IRQ subsystem level. > > > > > > > > > I think this patch is used for that purpose. I do two things in > > > irq_enable(): unmask irq and then enable IRQ; > > > > > > and for irq_disable(), it's similar; mask IRQ then disable it. Yeah, this patch provides a work around for the problem. So the chip->irq_enable() callback enables and unmasks the corresponding GPIO IRQ, while chip->irq_disable() masks and disables it. In this case the irq_mask()/irq_unmask() methods just get to be redundant since won't be used by the core anyway. But before accepting the solution in my opinion it would be better to at least discuss whether it is possible to fix the irq_enable()/irq_disable() methods so the similar problem wouldn't happen for the hardware like DW APB. Marc, Linus, Andy, Bartosz, Luo what do you think? Wouldn't that be better to fix the IRQ subsystem code instead seeing it doesn't cover all the possible hardware like with both types of IRQ enable/mask callback provided? > > > > Hi! > > > > Could you please resend this patch rebased on top of v5.11-rc2 and > > with the detailed explanation you responded with to Andy as part of > > the commit message? > > > > > I guess it’s more than that. What’s the driver maintainer position here? Andy, thanks for sending a notification about this patch. Please see my comments above. -Sergey > > > > > Thanks! > > Bart > > > > > -- > With Best Regards, > Andy Shevchenko
On Sat, Dec 05 2020 at 22:58, Linus Walleij wrote: > Sorry for top posting but I need the help of the irqchip maintainer > Marc Z to hash this out. > > The mask/unmask/disable/enable semantics is something that > you need to work with every day to understand right. The patch is correct. The irq_enable() callback is required to be a superset of irq_unmask(). I.e. the core code expects it to do: 1) Some preparatory work to enable the interrupt line 2) Unmask the interrupt, which is why the masked state is cleared by the core after invoking the irq_enable() callback. #2 is pretty obvious because if an interrupt chip does not implement the irq_enable() callback the core defaults to irq_unmask() Correspondingly the core expects from the irq_disable() callback: 1) To mask the interrupt 2) To do some extra work to disable the interrupt line Same reasoning as above vs. #1 as the core fallback is to invoke the irq_unmask() callback when the irq_disable() callback is not implemented. Thanks, tglx
Hi Thomas On Fri, Dec 15, 2023 at 09:09:09AM +0100, Thomas Gleixner wrote: > On Sat, Dec 05 2020 at 22:58, Linus Walleij wrote: > > Sorry for top posting but I need the help of the irqchip maintainer > > Marc Z to hash this out. > > > > The mask/unmask/disable/enable semantics is something that > > you need to work with every day to understand right. > > The patch is correct. > > The irq_enable() callback is required to be a superset of > irq_unmask(). I.e. the core code expects it to do: > > 1) Some preparatory work to enable the interrupt line > > 2) Unmask the interrupt, which is why the masked state is cleared > by the core after invoking the irq_enable() callback. > > #2 is pretty obvious because if an interrupt chip does not implement the > irq_enable() callback the core defaults to irq_unmask() > > Correspondingly the core expects from the irq_disable() callback: > > 1) To mask the interrupt > > 2) To do some extra work to disable the interrupt line > > Same reasoning as above vs. #1 as the core fallback is to invoke the > irq_unmask() callback when the irq_disable() callback is not > implemented. Just curious. Wouldn't that be more correct/portable for the core to call both callbacks when it's required and if both are provided? So the supersetness requirement would be no longer applied to the IRQ enable/disable callbacks implementation thus avoiding the code duplications in the low-level drivers. -Serge(y) > > Thanks, > > tglx >
On Fri, Dec 15 2023 at 13:24, Serge Semin wrote: > On Fri, Dec 15, 2023 at 09:09:09AM +0100, Thomas Gleixner wrote: >> On Sat, Dec 05 2020 at 22:58, Linus Walleij wrote: >> > Sorry for top posting but I need the help of the irqchip maintainer >> > Marc Z to hash this out. >> > >> > The mask/unmask/disable/enable semantics is something that >> > you need to work with every day to understand right. >> >> The patch is correct. >> >> The irq_enable() callback is required to be a superset of >> irq_unmask(). I.e. the core code expects it to do: >> >> 1) Some preparatory work to enable the interrupt line >> >> 2) Unmask the interrupt, which is why the masked state is cleared >> by the core after invoking the irq_enable() callback. >> >> #2 is pretty obvious because if an interrupt chip does not implement the >> irq_enable() callback the core defaults to irq_unmask() >> >> Correspondingly the core expects from the irq_disable() callback: >> >> 1) To mask the interrupt >> >> 2) To do some extra work to disable the interrupt line >> >> Same reasoning as above vs. #1 as the core fallback is to invoke the >> irq_unmask() callback when the irq_disable() callback is not >> implemented. > > Just curious. Wouldn't that be more correct/portable for the core to > call both callbacks when it's required and if both are provided? So > the supersetness requirement would be no longer applied to the > IRQ enable/disable callbacks implementation thus avoiding the code > duplications in the low-level drivers. We could do that, but there are chips which require atomicity of the operations (#1/#2). Not sure whether it safes much. Thanks, tglx
On Fri, Dec 15, 2023 at 11:56:02AM +0100, Thomas Gleixner wrote: > On Fri, Dec 15 2023 at 13:24, Serge Semin wrote: > > On Fri, Dec 15, 2023 at 09:09:09AM +0100, Thomas Gleixner wrote: > >> On Sat, Dec 05 2020 at 22:58, Linus Walleij wrote: > >> > Sorry for top posting but I need the help of the irqchip maintainer > >> > Marc Z to hash this out. > >> > > >> > The mask/unmask/disable/enable semantics is something that > >> > you need to work with every day to understand right. > >> > >> The patch is correct. > >> > >> The irq_enable() callback is required to be a superset of > >> irq_unmask(). I.e. the core code expects it to do: > >> > >> 1) Some preparatory work to enable the interrupt line > >> > >> 2) Unmask the interrupt, which is why the masked state is cleared > >> by the core after invoking the irq_enable() callback. > >> > >> #2 is pretty obvious because if an interrupt chip does not implement the > >> irq_enable() callback the core defaults to irq_unmask() > >> > >> Correspondingly the core expects from the irq_disable() callback: > >> > >> 1) To mask the interrupt > >> > >> 2) To do some extra work to disable the interrupt line > >> > >> Same reasoning as above vs. #1 as the core fallback is to invoke the > >> irq_unmask() callback when the irq_disable() callback is not > >> implemented. > > > > Just curious. Wouldn't that be more correct/portable for the core to > > call both callbacks when it's required and if both are provided? So > > the supersetness requirement would be no longer applied to the > > IRQ enable/disable callbacks implementation thus avoiding the code > > duplications in the low-level drivers. > > We could do that, but there are chips which require atomicity of the > operations (#1/#2). Not sure whether it safes much. I see. Thanks for the answer. Right, seeing there are only three GPIO drivers have such problem: drivers/gpio/gpio-ml-ioh.c drivers/gpio/gpio-dwapb.c drivers/gpio/gpio-hisi.c it's better to leave the semantics as is. It just isn't worth it to risk breaking so many platforms due to several drivers. Regarding this patch implementation. It can be optimized a bit: diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index 4a4f61bf6c58..15943f67758c 100644 --- a/drivers/gpio/gpio-dwapb.c +++ b/drivers/gpio/gpio-dwapb.c @@ -282,13 +282,15 @@ static void dwapb_irq_enable(struct irq_data *d) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct dwapb_gpio *gpio = to_dwapb_gpio(gc); + u32 mask = BIT(irqd_to_hwirq(d)); unsigned long flags; u32 val; raw_spin_lock_irqsave(&gc->bgpio_lock, flags); - val = dwapb_read(gpio, GPIO_INTEN); - val |= BIT(irqd_to_hwirq(d)); + val = dwapb_read(gpio, GPIO_INTEN) | mask; dwapb_write(gpio, GPIO_INTEN, val); + val = dwapb_read(gpio, GPIO_INTMASK) & ~mask; + dwapb_write(gpio, GPIO_INTMASK, val); raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags); } @@ -296,12 +298,14 @@ static void dwapb_irq_disable(struct irq_data *d) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d); struct dwapb_gpio *gpio = to_dwapb_gpio(gc); + u32 mask = BIT(irqd_to_hwirq(d)); unsigned long flags; u32 val; raw_spin_lock_irqsave(&gc->bgpio_lock, flags); - val = dwapb_read(gpio, GPIO_INTEN); - val &= ~BIT(irqd_to_hwirq(d)); + val = dwapb_read(gpio, GPIO_INTMASK) | mask; + dwapb_write(gpio, GPIO_INTMASK, val); + val = dwapb_read(gpio, GPIO_INTEN) & ~mask; dwapb_write(gpio, GPIO_INTEN, val); raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags); } Seeing Luo' email bounces back, Xiang (sorry if I spell your name incorrectly), could you please test the patch above out whether it fixes your problem, and then resubmit it? Please don't forget to add a Fixes tag. I guess the problem has been here since the driver birthday: Fixes: 7779b3455697 ("gpio: add a driver for the Synopsys DesignWare APB GPIO block") -Serge(y) > > Thanks, > > tglx
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index 2a9046c..ca654eb 100644 --- a/drivers/gpio/gpio-dwapb.c +++ b/drivers/gpio/gpio-dwapb.c @@ -270,6 +270,8 @@ static void dwapb_irq_enable(struct irq_data *d) u32 val; spin_lock_irqsave(&gc->bgpio_lock, flags); + val = dwapb_read(gpio, GPIO_INTMASK) & ~BIT(irqd_to_hwirq(d)); + dwapb_write(gpio, GPIO_INTMASK, val); val = dwapb_read(gpio, GPIO_INTEN); val |= BIT(irqd_to_hwirq(d)); dwapb_write(gpio, GPIO_INTEN, val); @@ -284,6 +286,8 @@ static void dwapb_irq_disable(struct irq_data *d) u32 val; spin_lock_irqsave(&gc->bgpio_lock, flags); + val = dwapb_read(gpio, GPIO_INTMASK) | BIT(irqd_to_hwirq(d)); + dwapb_write(gpio, GPIO_INTMASK, val); val = dwapb_read(gpio, GPIO_INTEN); val &= ~BIT(irqd_to_hwirq(d)); dwapb_write(gpio, GPIO_INTEN, val);
The mask and unmask registers are not configured in dwapb_irq_enable() and dwapb_irq_disable(). In the following situations, the IRQ will be masked by default after the IRQ is enabled: mask IRQ -> disable IRQ -> enable IRQ In this case, the IRQ status of GPIO controller is inconsistent with it's irq_data too. For example, in __irq_enable(), IRQD_IRQ_DISABLED and IRQD_IRQ_MASKED are both clear, but GPIO controller do not perform unmask. Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com> --- drivers/gpio/gpio-dwapb.c | 4 ++++ 1 file changed, 4 insertions(+)