Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

[Serge Semin <fancer.lancer@xxxxxxxxx>]

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