From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There is a problem when a driver requests a shared IRQ line to run
a threaded handler on it without IRQF_ONESHOT set if that flag has
been set already for the IRQ in question by somebody else.  Namely,
the request fails which usually leads to a probe failure even though
the driver might have worked just fine with IRQF_ONESHOT, but it does
not want to use it by default.  Currently, the only way to handle this
is to try to request the IRQ without IRQF_ONESHOT, but with
IRQF_PROBE_SHARED set and if this fails, try again with IRQF_ONESHOT
set.  However, this is a bit cumbersome and not very clean.

When commit 7a36b901a6eb ("ACPI: OSL: Use a threaded interrupt handler
for SCI") switched over the ACPI subsystem to using a threaded interrupt
handler for the SCI, it had to use IRQF_ONESHOT for it because that's
required due to the way the SCI handler works (it needs to walk all of
the enabled GPEs before IRQ line can be unmasked).  The SCI IRQ line is
not shared with other users very often due to the SCI handling overhead,
but on sone systems it is shared and when the other user of it attempts
to install a threaded handler, a flags mismatch related to IRQF_ONESHOT
may occur.  As it turned out, that happened to the pinctrl-amd driver
and so commit 4451e8e8415e ("pinctrl: amd: Add IRQF_ONESHOT to the
interrupt request") attempted to address the issue by adding
IRQF_ONESHOT to the interrupt flags in that driver, but this is now
causing an IRQF_ONESHOT-related mismatch to occur on another system
which cannot boot as a result of it.

Clearly, pinctrl-amd can work with IRQF_ONESHOT if need be, but it
should not set that flag by default, so it needs a way to indicate that
to the IRQ subsystem.

To that end, introdcuce a new interrupt flag, IRQF_COND_ONESHOT, which
will only have effect when the IRQ line is shared and IRQF_ONESHOT has
been set for it already, in which case it will be promoted to the
latter.

This is sufficient for drivers sharing the IRQ line with the SCI as it
is requested by the ACPI subsystem before any drivers are probed, so
they will always see IRQF_ONESHOT set for the IRQ in question.

Closes: https://lore.kernel.org/lkml/CAN-StX1HqWqi+YW=t+V52-38Mfp5fAz7YHx4aH-CQjgyNiKx3g@mail.gmail.com/
Fixes: 4451e8e8415e ("pinctrl: amd: Add IRQF_ONESHOT to the interrupt request")
Cc: 6.8+ <stable@vger.kernel.org> # 6.8+
Reported-by: Francisco Ayala Le Brun <francisco@videowindow.eu>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pinctrl/pinctrl-amd.c |    2 +-
 include/linux/interrupt.h     |    5 ++++-
 kernel/irq/manage.c           |    9 +++++++--
 3 files changed, 12 insertions(+), 4 deletions(-)

Index: linux-pm/include/linux/interrupt.h
===================================================================
--- linux-pm.orig/include/linux/interrupt.h
+++ linux-pm/include/linux/interrupt.h
@@ -67,6 +67,8 @@
  *                later.
  * IRQF_NO_DEBUG - Exclude from runnaway detection for IPI and similar handlers,
  *		   depends on IRQF_PERCPU.
+ * IRQF_COND_ONESHOT - Agree to do IRQF_ONESHOT if already set for a shared
+ *                 interrupt.
  */
 #define IRQF_SHARED		0x00000080
 #define IRQF_PROBE_SHARED	0x00000100
@@ -82,6 +84,7 @@
 #define IRQF_COND_SUSPEND	0x00040000
 #define IRQF_NO_AUTOEN		0x00080000
 #define IRQF_NO_DEBUG		0x00100000
+#define IRQF_COND_ONESHOT	0x00200000
 
 #define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
 
@@ -784,7 +787,7 @@ extern void tasklet_setup(struct tasklet
  * if more than one irq occurred.
  */
 
-#if !defined(CONFIG_GENERIC_IRQ_PROBE) 
+#if !defined(CONFIG_GENERIC_IRQ_PROBE)
 static inline unsigned long probe_irq_on(void)
 {
 	return 0;
Index: linux-pm/kernel/irq/manage.c
===================================================================
--- linux-pm.orig/kernel/irq/manage.c
+++ linux-pm/kernel/irq/manage.c
@@ -1643,8 +1643,13 @@ __setup_irq(unsigned int irq, struct irq
 		}
 
 		if (!((old->flags & new->flags) & IRQF_SHARED) ||
-		    (oldtype != (new->flags & IRQF_TRIGGER_MASK)) ||
-		    ((old->flags ^ new->flags) & IRQF_ONESHOT))
+		    (oldtype != (new->flags & IRQF_TRIGGER_MASK)))
+			goto mismatch;
+
+		if ((old->flags & IRQF_ONESHOT) &&
+		    (new->flags & IRQF_COND_ONESHOT))
+			new->flags |= IRQF_ONESHOT;
+		else if ((old->flags ^ new->flags) & IRQF_ONESHOT)
 			goto mismatch;
 
 		/* All handlers must agree on per-cpuness */
Index: linux-pm/drivers/pinctrl/pinctrl-amd.c
===================================================================
--- linux-pm.orig/drivers/pinctrl/pinctrl-amd.c
+++ linux-pm/drivers/pinctrl/pinctrl-amd.c
@@ -1159,7 +1159,7 @@ static int amd_gpio_probe(struct platfor
 	}
 
 	ret = devm_request_irq(&pdev->dev, gpio_dev->irq, amd_gpio_irq_handler,
-			       IRQF_SHARED | IRQF_ONESHOT, KBUILD_MODNAME, gpio_dev);
+			       IRQF_SHARED | IRQF_COND_ONESHOT, KBUILD_MODNAME, gpio_dev);
 	if (ret)
 		goto out2;

On Mon, Mar 25, 2024 at 1:58 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> There is a problem when a driver requests a shared IRQ line to run
> a threaded handler on it without IRQF_ONESHOT set if that flag has
> been set already for the IRQ in question by somebody else.  Namely,
> the request fails which usually leads to a probe failure even though
> the driver might have worked just fine with IRQF_ONESHOT, but it does
> not want to use it by default.  Currently, the only way to handle this
> is to try to request the IRQ without IRQF_ONESHOT, but with
> IRQF_PROBE_SHARED set and if this fails, try again with IRQF_ONESHOT
> set.  However, this is a bit cumbersome and not very clean.
>
> When commit 7a36b901a6eb ("ACPI: OSL: Use a threaded interrupt handler
> for SCI") switched over the ACPI subsystem to using a threaded interrupt
> handler for the SCI, it had to use IRQF_ONESHOT for it because that's
> required due to the way the SCI handler works (it needs to walk all of
> the enabled GPEs before IRQ line can be unmasked).  The SCI IRQ line is
> not shared with other users very often due to the SCI handling overhead,
> but on sone systems it is shared and when the other user of it attempts
> to install a threaded handler, a flags mismatch related to IRQF_ONESHOT
> may occur.  As it turned out, that happened to the pinctrl-amd driver
> and so commit 4451e8e8415e ("pinctrl: amd: Add IRQF_ONESHOT to the
> interrupt request") attempted to address the issue by adding
> IRQF_ONESHOT to the interrupt flags in that driver, but this is now
> causing an IRQF_ONESHOT-related mismatch to occur on another system
> which cannot boot as a result of it.
>
> Clearly, pinctrl-amd can work with IRQF_ONESHOT if need be, but it
> should not set that flag by default, so it needs a way to indicate that
> to the IRQ subsystem.
>
> To that end, introdcuce a new interrupt flag, IRQF_COND_ONESHOT, which
> will only have effect when the IRQ line is shared and IRQF_ONESHOT has
> been set for it already, in which case it will be promoted to the
> latter.
>
> This is sufficient for drivers sharing the IRQ line with the SCI as it
> is requested by the ACPI subsystem before any drivers are probed, so
> they will always see IRQF_ONESHOT set for the IRQ in question.
>
> Closes: https://lore.kernel.org/lkml/CAN-StX1HqWqi+YW=t+V52-38Mfp5fAz7YHx4aH-CQjgyNiKx3g@mail.gmail.com/
> Fixes: 4451e8e8415e ("pinctrl: amd: Add IRQF_ONESHOT to the interrupt request")
> Cc: 6.8+ <stable@vger.kernel.org> # 6.8+
> Reported-by: Francisco Ayala Le Brun <francisco@videowindow.eu>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> -#if !defined(CONFIG_GENERIC_IRQ_PROBE)
> +#if !defined(CONFIG_GENERIC_IRQ_PROBE)

Is that some whitespace fix? Not that it matters to me, so:

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I expect that Thomas want to apply this one.

Yours,
Linus Walleij

On Mon, Mar 25, 2024 at 2:32 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Mon, Mar 25, 2024 at 1:58 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > There is a problem when a driver requests a shared IRQ line to run
> > a threaded handler on it without IRQF_ONESHOT set if that flag has
> > been set already for the IRQ in question by somebody else.  Namely,
> > the request fails which usually leads to a probe failure even though
> > the driver might have worked just fine with IRQF_ONESHOT, but it does
> > not want to use it by default.  Currently, the only way to handle this
> > is to try to request the IRQ without IRQF_ONESHOT, but with
> > IRQF_PROBE_SHARED set and if this fails, try again with IRQF_ONESHOT
> > set.  However, this is a bit cumbersome and not very clean.
> >
> > When commit 7a36b901a6eb ("ACPI: OSL: Use a threaded interrupt handler
> > for SCI") switched over the ACPI subsystem to using a threaded interrupt
> > handler for the SCI, it had to use IRQF_ONESHOT for it because that's
> > required due to the way the SCI handler works (it needs to walk all of
> > the enabled GPEs before IRQ line can be unmasked).  The SCI IRQ line is
> > not shared with other users very often due to the SCI handling overhead,
> > but on sone systems it is shared and when the other user of it attempts
> > to install a threaded handler, a flags mismatch related to IRQF_ONESHOT
> > may occur.  As it turned out, that happened to the pinctrl-amd driver
> > and so commit 4451e8e8415e ("pinctrl: amd: Add IRQF_ONESHOT to the
> > interrupt request") attempted to address the issue by adding
> > IRQF_ONESHOT to the interrupt flags in that driver, but this is now
> > causing an IRQF_ONESHOT-related mismatch to occur on another system
> > which cannot boot as a result of it.
> >
> > Clearly, pinctrl-amd can work with IRQF_ONESHOT if need be, but it
> > should not set that flag by default, so it needs a way to indicate that
> > to the IRQ subsystem.
> >
> > To that end, introdcuce a new interrupt flag, IRQF_COND_ONESHOT, which
> > will only have effect when the IRQ line is shared and IRQF_ONESHOT has
> > been set for it already, in which case it will be promoted to the
> > latter.
> >
> > This is sufficient for drivers sharing the IRQ line with the SCI as it
> > is requested by the ACPI subsystem before any drivers are probed, so
> > they will always see IRQF_ONESHOT set for the IRQ in question.
> >
> > Closes: https://lore.kernel.org/lkml/CAN-StX1HqWqi+YW=t+V52-38Mfp5fAz7YHx4aH-CQjgyNiKx3g@mail.gmail.com/
> > Fixes: 4451e8e8415e ("pinctrl: amd: Add IRQF_ONESHOT to the interrupt request")
> > Cc: 6.8+ <stable@vger.kernel.org> # 6.8+
> > Reported-by: Francisco Ayala Le Brun <francisco@videowindow.eu>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> > -#if !defined(CONFIG_GENERIC_IRQ_PROBE)
> > +#if !defined(CONFIG_GENERIC_IRQ_PROBE)
>
> Is that some whitespace fix? Not that it matters to me, so:

Well, incidental, but yes (trailing whitespace).  I actually forgot to
remove this change from the patch before sending it.

> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> I expect that Thomas want to apply this one.

Thank you!

The following commit has been merged into the irq/urgent branch of tip:

Commit-ID:     c2ddeb29612f7ca84ed10c6d4f3ac99705135447
Gitweb:        https://git.kernel.org/tip/c2ddeb29612f7ca84ed10c6d4f3ac99705135447
Author:        Rafael J. Wysocki <rafael.j.wysocki@intel.com>
AuthorDate:    Mon, 25 Mar 2024 13:58:08 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 25 Mar 2024 23:45:21 +01:00

genirq: Introduce IRQF_COND_ONESHOT and use it in pinctrl-amd

There is a problem when a driver requests a shared interrupt line to run a
threaded handler on it without IRQF_ONESHOT set if that flag has been set
already for the IRQ in question by somebody else.  Namely, the request
fails which usually leads to a probe failure even though the driver might
have worked just fine with IRQF_ONESHOT, but it does not want to use it by
default.  Currently, the only way to handle this is to try to request the
IRQ without IRQF_ONESHOT, but with IRQF_PROBE_SHARED set and if this fails,
try again with IRQF_ONESHOT set.  However, this is a bit cumbersome and not
very clean.

When commit 7a36b901a6eb ("ACPI: OSL: Use a threaded interrupt handler for
SCI") switched the ACPI subsystem over to using a threaded interrupt
handler for the SCI, it had to use IRQF_ONESHOT for it because that's
required due to the way the SCI handler works (it needs to walk all of the
enabled GPEs before the interrupt line can be unmasked). The SCI interrupt
line is not shared with other users very often due to the SCI handling
overhead, but on sone systems it is shared and when the other user of it
attempts to install a threaded handler, a flags mismatch related to
IRQF_ONESHOT may occur.

As it turned out, that happened to the pinctrl-amd driver and so commit
4451e8e8415e ("pinctrl: amd: Add IRQF_ONESHOT to the interrupt request")
attempted to address the issue by adding IRQF_ONESHOT to the interrupt
flags in that driver, but this is now causing an IRQF_ONESHOT-related
mismatch to occur on another system which cannot boot as a result of it.

Clearly, pinctrl-amd can work with IRQF_ONESHOT if need be, but it should
not set that flag by default, so it needs a way to indicate that to the
interrupt subsystem.

To that end, introdcuce a new interrupt flag, IRQF_COND_ONESHOT, which will
only have effect when the IRQ line is shared and IRQF_ONESHOT has been set
for it already, in which case it will be promoted to the latter.

This is sufficient for drivers sharing the interrupt line with the SCI as
it is requested by the ACPI subsystem before any drivers are probed, so
they will always see IRQF_ONESHOT set for the interrupt in question.

Fixes: 4451e8e8415e ("pinctrl: amd: Add IRQF_ONESHOT to the interrupt request")
Reported-by: Francisco Ayala Le Brun <francisco@videowindow.eu>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Cc: 6.8+ <stable@vger.kernel.org> # 6.8+
Closes: https://lore.kernel.org/lkml/CAN-StX1HqWqi+YW=t+V52-38Mfp5fAz7YHx4aH-CQjgyNiKx3g@mail.gmail.com/
Link: https://lore.kernel.org/r/12417336.O9o76ZdvQC@kreacher

---
 drivers/pinctrl/pinctrl-amd.c |  2 +-
 include/linux/interrupt.h     |  3 +++
 kernel/irq/manage.c           |  9 +++++++--
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 49f89b7..7f66ec7 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -1159,7 +1159,7 @@ static int amd_gpio_probe(struct platform_device *pdev)
 	}
 
 	ret = devm_request_irq(&pdev->dev, gpio_dev->irq, amd_gpio_irq_handler,
-			       IRQF_SHARED | IRQF_ONESHOT, KBUILD_MODNAME, gpio_dev);
+			       IRQF_SHARED | IRQF_COND_ONESHOT, KBUILD_MODNAME, gpio_dev);
 	if (ret)
 		goto out2;
 
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 76121c2..5c9bdd3 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -67,6 +67,8 @@
  *                later.
  * IRQF_NO_DEBUG - Exclude from runnaway detection for IPI and similar handlers,
  *		   depends on IRQF_PERCPU.
+ * IRQF_COND_ONESHOT - Agree to do IRQF_ONESHOT if already set for a shared
+ *                 interrupt.
  */
 #define IRQF_SHARED		0x00000080
 #define IRQF_PROBE_SHARED	0x00000100
@@ -82,6 +84,7 @@
 #define IRQF_COND_SUSPEND	0x00040000
 #define IRQF_NO_AUTOEN		0x00080000
 #define IRQF_NO_DEBUG		0x00100000
+#define IRQF_COND_ONESHOT	0x00200000
 
 #define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index ad3eaf2..bf9ae8a 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1643,8 +1643,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		}
 
 		if (!((old->flags & new->flags) & IRQF_SHARED) ||
-		    (oldtype != (new->flags & IRQF_TRIGGER_MASK)) ||
-		    ((old->flags ^ new->flags) & IRQF_ONESHOT))
+		    (oldtype != (new->flags & IRQF_TRIGGER_MASK)))
+			goto mismatch;
+
+		if ((old->flags & IRQF_ONESHOT) &&
+		    (new->flags & IRQF_COND_ONESHOT))
+			new->flags |= IRQF_ONESHOT;
+		else if ((old->flags ^ new->flags) & IRQF_ONESHOT)
 			goto mismatch;
 
 		/* All handlers must agree on per-cpuness */