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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <osesb4frbdajs5cdfzwcbdpap44r6gry4tcy57xoms2nabo7eg@phehazlr6cm5>
Date: Fri, 15 Dec 2023 15:57:51 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Thomas Gleixner <tglx@...utronix.de>, xiongxin <xiongxin@...inos.cn>
Cc: Linus Walleij <linus.walleij@...aro.org>, 
	Luo Jiaxing <luojiaxing@...wei.com>, Marc Zyngier <maz@...nel.org>, 
	Bartosz Golaszewski <bgolaszewski@...libre.com>, Serge Semin <Sergey.Semin@...kalelectronics.ru>, 
	Andy Shevchenko <andy.shevchenko@...il.com>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	Linuxarm <linuxarm@...wei.com>
Subject: Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it

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