diff mbox series

[RFT,v1] ACPI: OSL: Use a threaded interrupt handler for SCI

Message ID 5745568.DvuYhMxLoT@kreacher
State New
Headers show
Series [RFT,v1] ACPI: OSL: Use a threaded interrupt handler for SCI | expand

Commit Message

Rafael J. Wysocki Nov. 27, 2023, 7:57 p.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

In the current arrangement, all of the acpi_ev_sci_xrupt_handler() code
is run as an interrupt handler for the SCI, in interrupt context.  Among
other things, this causes it to run with local interrupts off which
can be problematic if many GPEs are enabled and they are located in the
I/O address space, for example (because in that case local interrupts
will be off for the duration of all of the GPE hardware accesses carried
out while handling an SCI combined and that may be quite a bit of time
in extreme scenarios).

However, there is no particular reason why the code in question really
needs to run in interrupt context and in particular, it has no specific
reason to run with local interrupts off.  The only real requirement is
to prevent multiple instences of it from running in parallel with each
other, but that can be achieved regardless.

For this reason, use request_threaded_irq() instead of request_irq() for
the ACPI SCI and pass IRQF_ONESHOT to it in flags to indicate that the
interrupt needs to be masked while its handling thread is running so as
to prevent it from re-triggering while it is being handled (and in
particular until the final handled/not handled outcome is determined).

While at it, drop a redundant local variable from acpi_irq().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

The code inspection and (necessarily limited) testing carried out by me
are good indications that this should just always work, but there may
be still some really odd platform configurations I'm overlooking, so if
you have a way to give it a go, please do so.

---
 drivers/acpi/osl.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

Mario Limonciello Nov. 28, 2023, 2:27 p.m. UTC | #1 
On 11/27/2023 13:57, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> In the current arrangement, all of the acpi_ev_sci_xrupt_handler() code
> is run as an interrupt handler for the SCI, in interrupt context.  Among
> other things, this causes it to run with local interrupts off which
> can be problematic if many GPEs are enabled and they are located in the
> I/O address space, for example (because in that case local interrupts
> will be off for the duration of all of the GPE hardware accesses carried
> out while handling an SCI combined and that may be quite a bit of time
> in extreme scenarios).
> 
> However, there is no particular reason why the code in question really
> needs to run in interrupt context and in particular, it has no specific
> reason to run with local interrupts off.  The only real requirement is
> to prevent multiple instences of it from running in parallel with each
> other, but that can be achieved regardless.
> 
> For this reason, use request_threaded_irq() instead of request_irq() for
> the ACPI SCI and pass IRQF_ONESHOT to it in flags to indicate that the
> interrupt needs to be masked while its handling thread is running so as
> to prevent it from re-triggering while it is being handled (and in
> particular until the final handled/not handled outcome is determined).
> 
> While at it, drop a redundant local variable from acpi_irq().
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> The code inspection and (necessarily limited) testing carried out by me
> are good indications that this should just always work, but there may
> be still some really odd platform configurations I'm overlooking, so if
> you have a way to give it a go, please do so.

Thanks for looping me in.

I tested it on a few different laptops I have on hand from different SoC 
generations and manufacturers and ensured that SCI was working correctly 
for usage and wakeup.  My base kernel was 6.7-rc3 plus some unrelated 
RTC timeout handling patches.

Tested-by: Mario Limonciello <mario.limonciello@amd.com>

> 
> ---
>   drivers/acpi/osl.c |    9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/acpi/osl.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/osl.c
> +++ linux-pm/drivers/acpi/osl.c
> @@ -544,11 +544,7 @@ acpi_os_predefined_override(const struct
>   
>   static irqreturn_t acpi_irq(int irq, void *dev_id)
>   {
> -	u32 handled;
> -
> -	handled = (*acpi_irq_handler) (acpi_irq_context);
> -
> -	if (handled) {
> +	if ((*acpi_irq_handler)(acpi_irq_context)) {
>   		acpi_irq_handled++;
>   		return IRQ_HANDLED;
>   	} else {
> @@ -582,7 +578,8 @@ acpi_os_install_interrupt_handler(u32 gs
>   
>   	acpi_irq_handler = handler;
>   	acpi_irq_context = context;
> -	if (request_irq(irq, acpi_irq, IRQF_SHARED, "acpi", acpi_irq)) {
> +	if (request_threaded_irq(irq, NULL, acpi_irq, IRQF_SHARED | IRQF_ONESHOT,
> +			         "acpi", acpi_irq)) {
>   		pr_err("SCI (IRQ%d) allocation failed\n", irq);
>   		acpi_irq_handler = NULL;
>   		return AE_NOT_ACQUIRED;
> 
> 
>
Mika Westerberg Nov. 29, 2023, 8:56 a.m. UTC | #2 
Hi Rafael,

On Mon, Nov 27, 2023 at 08:57:43PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> In the current arrangement, all of the acpi_ev_sci_xrupt_handler() code
> is run as an interrupt handler for the SCI, in interrupt context.  Among
> other things, this causes it to run with local interrupts off which
> can be problematic if many GPEs are enabled and they are located in the
> I/O address space, for example (because in that case local interrupts
> will be off for the duration of all of the GPE hardware accesses carried
> out while handling an SCI combined and that may be quite a bit of time
> in extreme scenarios).
> 
> However, there is no particular reason why the code in question really
> needs to run in interrupt context and in particular, it has no specific
> reason to run with local interrupts off.  The only real requirement is
> to prevent multiple instences of it from running in parallel with each
> other, but that can be achieved regardless.
> 
> For this reason, use request_threaded_irq() instead of request_irq() for
> the ACPI SCI and pass IRQF_ONESHOT to it in flags to indicate that the
> interrupt needs to be masked while its handling thread is running so as
> to prevent it from re-triggering while it is being handled (and in
> particular until the final handled/not handled outcome is determined).
> 
> While at it, drop a redundant local variable from acpi_irq().
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> The code inspection and (necessarily limited) testing carried out by me
> are good indications that this should just always work, but there may
> be still some really odd platform configurations I'm overlooking, so if
> you have a way to give it a go, please do so.

Tried this on ADL-S and ADL-P systems that I have here and both work
just fine with the patch applied. I can see SCI interrupt count
increases in /proc/interrupts as expected. Did a couple of s2idle cycles
too, all good.

Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Rafael J. Wysocki Nov. 30, 2023, 1:28 p.m. UTC | #3 
Hi Mika,
Hi Mario,

On Wed, Nov 29, 2023 at 11:39 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Mon, Nov 27, 2023 at 08:57:43PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > In the current arrangement, all of the acpi_ev_sci_xrupt_handler() code
> > is run as an interrupt handler for the SCI, in interrupt context.  Among
> > other things, this causes it to run with local interrupts off which
> > can be problematic if many GPEs are enabled and they are located in the
> > I/O address space, for example (because in that case local interrupts
> > will be off for the duration of all of the GPE hardware accesses carried
> > out while handling an SCI combined and that may be quite a bit of time
> > in extreme scenarios).
> >
> > However, there is no particular reason why the code in question really
> > needs to run in interrupt context and in particular, it has no specific
> > reason to run with local interrupts off.  The only real requirement is
> > to prevent multiple instences of it from running in parallel with each
> > other, but that can be achieved regardless.
> >
> > For this reason, use request_threaded_irq() instead of request_irq() for
> > the ACPI SCI and pass IRQF_ONESHOT to it in flags to indicate that the
> > interrupt needs to be masked while its handling thread is running so as
> > to prevent it from re-triggering while it is being handled (and in
> > particular until the final handled/not handled outcome is determined).
> >
> > While at it, drop a redundant local variable from acpi_irq().
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > The code inspection and (necessarily limited) testing carried out by me
> > are good indications that this should just always work, but there may
> > be still some really odd platform configurations I'm overlooking, so if
> > you have a way to give it a go, please do so.
>
> Tried this on ADL-S and ADL-P systems that I have here and both work
> just fine with the patch applied. I can see SCI interrupt count
> increases in /proc/interrupts as expected. Did a couple of s2idle cycles
> too, all good.
>
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks for your replies and tags!

Given the lack of response from anyone else I'm going to move this
towards linux-next with 6.8 as the target.

Thank you!
Christian Heusel Jan. 23, 2024, 4:39 p.m. UTC | #4 
On 23/11/30 02:28PM, Rafael J. Wysocki wrote:
> Hi Mika, Hi Mario,
> Thanks for your replies and tags!
> 
> Given the lack of response from anyone else I'm going to move this
> towards linux-next with 6.8 as the target.
> 
> Thank you!

I got a stack trace in dmesg with linux6.8-rc1 that seems to be caused
by this commit according to my bisection, see the bugzilla report for
details / further discussion:

https://bugzilla.kernel.org/show_bug.cgi?id=218407

Cheers,
Christian
Rafael J. Wysocki Jan. 23, 2024, 5:51 p.m. UTC | #5 
On Tue, Jan 23, 2024 at 5:39 PM Christian Heusel <christian@heusel.eu> wrote:
>
> On 23/11/30 02:28PM, Rafael J. Wysocki wrote:
> > Hi Mika, Hi Mario,
> > Thanks for your replies and tags!
> >
> > Given the lack of response from anyone else I'm going to move this
> > towards linux-next with 6.8 as the target.
> >
> > Thank you!
>
> I got a stack trace in dmesg with linux6.8-rc1 that seems to be caused
> by this commit according to my bisection, see the bugzilla report for
> details / further discussion:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=218407

Mario's suggestion to add IRQF_ONESHOT to pinctrl-amd in
amd_gpio_probe() looks right to me.
diff mbox series

Patch

Index: linux-pm/drivers/acpi/osl.c
===================================================================
--- linux-pm.orig/drivers/acpi/osl.c
+++ linux-pm/drivers/acpi/osl.c
@@ -544,11 +544,7 @@  acpi_os_predefined_override(const struct
 
 static irqreturn_t acpi_irq(int irq, void *dev_id)
 {
-	u32 handled;
-
-	handled = (*acpi_irq_handler) (acpi_irq_context);
-
-	if (handled) {
+	if ((*acpi_irq_handler)(acpi_irq_context)) {
 		acpi_irq_handled++;
 		return IRQ_HANDLED;
 	} else {
@@ -582,7 +578,8 @@  acpi_os_install_interrupt_handler(u32 gs
 
 	acpi_irq_handler = handler;
 	acpi_irq_context = context;
-	if (request_irq(irq, acpi_irq, IRQF_SHARED, "acpi", acpi_irq)) {
+	if (request_threaded_irq(irq, NULL, acpi_irq, IRQF_SHARED | IRQF_ONESHOT,
+			         "acpi", acpi_irq)) {
 		pr_err("SCI (IRQ%d) allocation failed\n", irq);
 		acpi_irq_handler = NULL;
 		return AE_NOT_ACQUIRED;