Received-SPF: pass (google.com: domain of linux-kernel+bounces-965-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 15 Dec 2023 15:57:51 +0300
From: Serge Semin <fancer.lancer@gmail.com>
To: Thomas Gleixner <tglx@linutronix.de>, xiongxin <xiongxin@kylinos.cn>
Cc: Linus Walleij <linus.walleij@linaro.org>, 
	Luo Jiaxing <luojiaxing@huawei.com>, Marc Zyngier <maz@kernel.org>, 
	Bartosz Golaszewski <bgolaszewski@baylibre.com>, Serge Semin <Sergey.Semin@baikalelectronics.ru>, 
	Andy Shevchenko <andy.shevchenko@gmail.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, 
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, 
	Linuxarm <linuxarm@huawei.com>
Subject: Re: [PATCH v1] gpio: dwapb: mask/unmask IRQ when disable/enable it
Message-ID: <osesb4frbdajs5cdfzwcbdpap44r6gry4tcy57xoms2nabo7eg@phehazlr6cm5>
References: <1606728979-44259-1-git-send-email-luojiaxing@huawei.com>
 <CACRpkdYHAecm3ygaze80SsXzNMYSA9p+p6JY4BKO2D+vArb-WA@mail.gmail.com>
 <87fs03opju.ffs@tglx>
 <cbgdeksaulqu65nbnz7l4wpuwensllkdlyi5babhpngclze4df@zxetbohgpfvg>
 <87a5qbohtp.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87a5qbohtp.ffs@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