[PATCH v1 0/4] ACPI: OSL: acpi_os_execute() improvements

[PATCH v1 0/4] ACPI: OSL: acpi_os_execute() improvements

[Rafael J. Wysocki]

Hi Everyone,

This series improves acpi_os_execute() on top of

https://patchwork.kernel.org/project/linux-acpi/patch/5745568.DvuYhMxLoT@kreacher/

but only the last patch really depends on it.

The first two patches clean up the code somewhat and the third one modifies
the function to allow Notify () handlers to run on all CPUs (not on CPU0 only).

The last patch changes it to use GFP_KERNEL for memory allocations, as it does
not run in interrupt context any more after the change linked above.

Thanks!

[PATCH v1 1/4] ACPI: OSL: Rework error handling in acpi_os_execute()

[Rafael J. Wysocki]

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

Reduce the number of checks and goto labels related to error handling
in acpi_os_execute() and drop the status local variable, which turns
out to be redundant, from it.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/osl.c |   23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/acpi/osl.c
===================================================================
--- linux-pm.orig/drivers/acpi/osl.c
+++ linux-pm/drivers/acpi/osl.c
@@ -1060,7 +1060,6 @@ int __init acpi_debugger_init(void)
 acpi_status acpi_os_execute(acpi_execute_type type,
 			    acpi_osd_exec_callback function, void *context)
 {
-	acpi_status status = AE_OK;
 	struct acpi_os_dpc *dpc;
 	struct workqueue_struct *queue;
 	int ret;
@@ -1073,9 +1072,9 @@ acpi_status acpi_os_execute(acpi_execute
 		ret = acpi_debugger_create_thread(function, context);
 		if (ret) {
 			pr_err("Kernel thread creation failed\n");
-			status = AE_ERROR;
+			return AE_ERROR;
 		}
-		goto out_thread;
+		return AE_OK;
 	}
 
 	/*
@@ -1107,12 +1106,9 @@ acpi_status acpi_os_execute(acpi_execute
 		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
 	} else {
 		pr_err("Unsupported os_execute type %d.\n", type);
-		status = AE_ERROR;
+		goto err;
 	}
 
-	if (ACPI_FAILURE(status))
-		goto err_workqueue;
-
 	/*
 	 * On some machines, a software-initiated SMI causes corruption unless
 	 * the SMI runs on CPU 0.  An SMI can be initiated by any AML, but
@@ -1123,13 +1119,14 @@ acpi_status acpi_os_execute(acpi_execute
 	ret = queue_work_on(0, queue, &dpc->work);
 	if (!ret) {
 		pr_err("Unable to queue work\n");
-		status = AE_ERROR;
+		goto err;
 	}
-err_workqueue:
-	if (ACPI_FAILURE(status))
-		kfree(dpc);
-out_thread:
-	return status;
+
+	return AE_OK;
+
+err:
+	kfree(dpc);
+	return AE_ERROR;
 }
 EXPORT_SYMBOL(acpi_os_execute);
 

Re: [PATCH v1 1/4] ACPI: OSL: Rework error handling in acpi_os_execute()

[Andy Shevchenko]

On Wed, Nov 29, 2023 at 02:46:51PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Reduce the number of checks and goto labels related to error handling
> in acpi_os_execute() and drop the status local variable, which turns
> out to be redundant, from it.
> 
> No intentional functional impact.

LGTM,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 2/4] ACPI: OSL: Rearrange workqueue selection in acpi_os_execute()

[Rafael J. Wysocki]

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

Replace the 3-branch if () statement used for selecting the target
workqueue in acpi_os_execute() with a switch () one that is more
suitable for this purpose and carry out the work item initialization
before it to avoid code duplication.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/osl.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/acpi/osl.c
===================================================================
--- linux-pm.orig/drivers/acpi/osl.c
+++ linux-pm/drivers/acpi/osl.c
@@ -1092,19 +1092,21 @@ acpi_status acpi_os_execute(acpi_execute
 
 	dpc->function = function;
 	dpc->context = context;
+	INIT_WORK(&dpc->work, acpi_os_execute_deferred);
 
 	/*
 	 * To prevent lockdep from complaining unnecessarily, make sure that
 	 * there is a different static lockdep key for each workqueue by using
 	 * INIT_WORK() for each of them separately.
 	 */
-	if (type == OSL_NOTIFY_HANDLER) {
+	switch (type) {
+	case OSL_NOTIFY_HANDLER:
 		queue = kacpi_notify_wq;
-		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
-	} else if (type == OSL_GPE_HANDLER) {
+		break;
+	case OSL_GPE_HANDLER:
 		queue = kacpid_wq;
-		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
-	} else {
+		break;
+	default:
 		pr_err("Unsupported os_execute type %d.\n", type);
 		goto err;
 	}

Re: [PATCH v1 2/4] ACPI: OSL: Rearrange workqueue selection in acpi_os_execute()

[Andy Shevchenko]

On Wed, Nov 29, 2023 at 02:48:22PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Replace the 3-branch if () statement used for selecting the target
> workqueue in acpi_os_execute() with a switch () one that is more
> suitable for this purpose and carry out the work item initialization
> before it to avoid code duplication.
> 
> No intentional functional impact.

LGTM,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 3/4] ACPI: OSL: Allow Notify () handlers to run on all CPUs

[Rafael J. Wysocki]

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

Notify () handlers, like GPE handlers, are only allowed to run on CPU0
now out of the concern that they might trigger an SMM trap and that (in
some cases) the SMM code running as a result of that might corrupt
memory if not run on CPU0.

However, Notify () handlers are registered by kernel code and they
are not likely to evaluate AML that would trigger an SMM trap.  In
fact, many of them don't even evaluate any AML at all and even if
they do, that AML may as well be evaluated in other code paths.  In
other words, they are not special from the AML evaluation perspective,
so there is no real reason to treat them in any special way.

Accordingly, allow Notify () handlers, unlike GPE handlers, to be
executed by all CPUs in the system.

Also adjust the allocation of the "notify" workqueue to allow multiple
handlers to be executed at the same time, because they need not be
serialized.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/osl.c |   23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/acpi/osl.c
===================================================================
--- linux-pm.orig/drivers/acpi/osl.c
+++ linux-pm/drivers/acpi/osl.c
@@ -1061,7 +1061,6 @@ acpi_status acpi_os_execute(acpi_execute
 			    acpi_osd_exec_callback function, void *context)
 {
 	struct acpi_os_dpc *dpc;
-	struct workqueue_struct *queue;
 	int ret;
 
 	ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
@@ -1101,24 +1100,22 @@ acpi_status acpi_os_execute(acpi_execute
 	 */
 	switch (type) {
 	case OSL_NOTIFY_HANDLER:
-		queue = kacpi_notify_wq;
+		ret = queue_work(kacpi_notify_wq, &dpc->work);
 		break;
 	case OSL_GPE_HANDLER:
-		queue = kacpid_wq;
+		/*
+		 * On some machines, a software-initiated SMI causes corruption
+		 * unless the SMI runs on CPU 0.  An SMI can be initiated by
+		 * any AML, but typically it's done in GPE-related methods that
+		 * are run via workqueues, so we can avoid the known corruption
+		 * cases by always queueing on CPU 0.
+		 */
+		ret = queue_work_on(0, kacpid_wq, &dpc->work);
 		break;
 	default:
 		pr_err("Unsupported os_execute type %d.\n", type);
 		goto err;
 	}
-
-	/*
-	 * On some machines, a software-initiated SMI causes corruption unless
-	 * the SMI runs on CPU 0.  An SMI can be initiated by any AML, but
-	 * typically it's done in GPE-related methods that are run via
-	 * workqueues, so we can avoid the known corruption cases by always
-	 * queueing on CPU 0.
-	 */
-	ret = queue_work_on(0, queue, &dpc->work);
 	if (!ret) {
 		pr_err("Unable to queue work\n");
 		goto err;
@@ -1668,7 +1665,7 @@ acpi_status __init acpi_os_initialize(vo
 acpi_status __init acpi_os_initialize1(void)
 {
 	kacpid_wq = alloc_workqueue("kacpid", 0, 1);
-	kacpi_notify_wq = alloc_workqueue("kacpi_notify", 0, 1);
+	kacpi_notify_wq = alloc_workqueue("kacpi_notify", 0, 0);
 	kacpi_hotplug_wq = alloc_ordered_workqueue("kacpi_hotplug", 0);
 	BUG_ON(!kacpid_wq);
 	BUG_ON(!kacpi_notify_wq);

Re: [PATCH v1 3/4] ACPI: OSL: Allow Notify () handlers to run on all CPUs

[Andy Shevchenko]

On Wed, Nov 29, 2023 at 02:50:54PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Notify () handlers, like GPE handlers, are only allowed to run on CPU0
> now out of the concern that they might trigger an SMM trap and that (in
> some cases) the SMM code running as a result of that might corrupt
> memory if not run on CPU0.

Pardon my French, but I'm a bit lost in the semantics of all those "that".
Maybe the above can be simplified?

> However, Notify () handlers are registered by kernel code and they
> are not likely to evaluate AML that would trigger an SMM trap.  In
> fact, many of them don't even evaluate any AML at all and even if
> they do, that AML may as well be evaluated in other code paths.  In
> other words, they are not special from the AML evaluation perspective,
> so there is no real reason to treat them in any special way.
> 
> Accordingly, allow Notify () handlers, unlike GPE handlers, to be
> executed by all CPUs in the system.
> 
> Also adjust the allocation of the "notify" workqueue to allow multiple
> handlers to be executed at the same time, because they need not be
> serialized.

Code wise LGTM,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/4] ACPI: OSL: Allow Notify () handlers to run on all CPUs

[Rafael J. Wysocki]

On Wed, Nov 29, 2023 at 3:34 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Nov 29, 2023 at 02:50:54PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Notify () handlers, like GPE handlers, are only allowed to run on CPU0
> > now out of the concern that they might trigger an SMM trap and that (in
> > some cases) the SMM code running as a result of that might corrupt
> > memory if not run on CPU0.
>
> Pardon my French, but I'm a bit lost in the semantics of all those "that".

Well, fair enough.

> Maybe the above can be simplified?

It can be rephrased.

[PATCH v1 4/4] ACPI: OSL: Use GFP_KERNEL for work item allocations

[Rafael J. Wysocki]

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

After the recent modification changing the ACPI SCI interrupt handler
into a threaded one, the SCI interrupt handler code does not run in
interrupt context any more and acpi_os_execute(), that may be invoked
by it, need not use GFP_ATOMIC for allocating work items.

Make it use GFP_KERNEL instead.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/osl.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Index: linux-pm/drivers/acpi/osl.c
===================================================================
--- linux-pm.orig/drivers/acpi/osl.c
+++ linux-pm/drivers/acpi/osl.c
@@ -1084,8 +1084,7 @@ acpi_status acpi_os_execute(acpi_execute
 	 * parameters we can't use the approach some kernel code uses of
 	 * having a static work_struct.
 	 */
-
-	dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_ATOMIC);
+	dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_KERNEL);
 	if (!dpc)
 		return AE_NO_MEMORY;
 

Re: [PATCH v1 4/4] ACPI: OSL: Use GFP_KERNEL for work item allocations

[Andy Shevchenko]

On Wed, Nov 29, 2023 at 02:52:22PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After the recent modification changing the ACPI SCI interrupt handler
> into a threaded one, the SCI interrupt handler code does not run in
> interrupt context any more and acpi_os_execute(), that may be invoked
> by it, need not use GFP_ATOMIC for allocating work items.
> 
> Make it use GFP_KERNEL instead.

True, threader IRQ handler can sleep.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/4] ACPI: OSL: Use GFP_KERNEL for work item allocations

[Rafael J. Wysocki]

On Wed, Nov 29, 2023 at 3:33 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> After the recent modification changing the ACPI SCI interrupt handler
> into a threaded one, the SCI interrupt handler code does not run in
> interrupt context any more and acpi_os_execute(), that may be invoked
> by it, need not use GFP_ATOMIC for allocating work items.
>
> Make it use GFP_KERNEL instead.

This change is premature, because acpi_ev_detect_gpe() still disables
local interrupts around acpi_os_execute() calls, even though it runs
from a kernel thread now.

Withdrawing.

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/osl.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> Index: linux-pm/drivers/acpi/osl.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/osl.c
> +++ linux-pm/drivers/acpi/osl.c
> @@ -1084,8 +1084,7 @@ acpi_status acpi_os_execute(acpi_execute
>          * parameters we can't use the approach some kernel code uses of
>          * having a static work_struct.
>          */
> -
> -       dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_ATOMIC);
> +       dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_KERNEL);
>         if (!dpc)
>                 return AE_NO_MEMORY;
>
>
>
>
>

Re: [PATCH v1 0/4] ACPI: OSL: acpi_os_execute() improvements

[Hans de Goede]

Hi,

On 11/29/23 14:45, Rafael J. Wysocki wrote:
> Hi Everyone,
> 
> This series improves acpi_os_execute() on top of
> 
> https://patchwork.kernel.org/project/linux-acpi/patch/5745568.DvuYhMxLoT@kreacher/
> 
> but only the last patch really depends on it.
> 
> The first two patches clean up the code somewhat and the third one modifies
> the function to allow Notify () handlers to run on all CPUs (not on CPU0 only).
> 
> The last patch changes it to use GFP_KERNEL for memory allocations, as it does
> not run in interrupt context any more after the change linked above.

I have added this series, as well as the preceding
"ACPI: OSL: Use a threaded interrupt handler for SCI"
patch to my personal tree now, so that it will get tested on various
devices when I run my personal tree on them.

I'll let you know if I hit any issues caused by this series.

Regards,

Hans

Re: [PATCH v1 0/4] ACPI: OSL: acpi_os_execute() improvements

[Rafael J. Wysocki]

On Sat, Dec 2, 2023 at 3:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11/29/23 14:45, Rafael J. Wysocki wrote:
> > Hi Everyone,
> >
> > This series improves acpi_os_execute() on top of
> >
> > https://patchwork.kernel.org/project/linux-acpi/patch/5745568.DvuYhMxLoT@kreacher/
> >
> > but only the last patch really depends on it.
> >
> > The first two patches clean up the code somewhat and the third one modifies
> > the function to allow Notify () handlers to run on all CPUs (not on CPU0 only).
> >
> > The last patch changes it to use GFP_KERNEL for memory allocations, as it does
> > not run in interrupt context any more after the change linked above.
>
> I have added this series, as well as the preceding
> "ACPI: OSL: Use a threaded interrupt handler for SCI"
> patch to my personal tree now, so that it will get tested on various
> devices when I run my personal tree on them.
>
> I'll let you know if I hit any issues caused by this series.

Thank you!

Re: [PATCH v1 0/4] ACPI: OSL: acpi_os_execute() improvements

[Rafael J. Wysocki]

Hi Hans,

On Sat, Dec 2, 2023 at 3:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11/29/23 14:45, Rafael J. Wysocki wrote:
> > Hi Everyone,
> >
> > This series improves acpi_os_execute() on top of
> >
> > https://patchwork.kernel.org/project/linux-acpi/patch/5745568.DvuYhMxLoT@kreacher/
> >
> > but only the last patch really depends on it.
> >
> > The first two patches clean up the code somewhat and the third one modifies
> > the function to allow Notify () handlers to run on all CPUs (not on CPU0 only).
> >
> > The last patch changes it to use GFP_KERNEL for memory allocations, as it does
> > not run in interrupt context any more after the change linked above.
>
> I have added this series, as well as the preceding
> "ACPI: OSL: Use a threaded interrupt handler for SCI"
> patch to my personal tree now, so that it will get tested on various
> devices when I run my personal tree on them.
>
> I'll let you know if I hit any issues caused by this series.

As stated here

https://lore.kernel.org/linux-acpi/CAJZ5v0jkHLGa2XxB4TMqzrBBdZYXY79+sh1Z0ZF6keYdLDyfkg@mail.gmail.com/

the last patch in this series is not really a good idea just yet, so
please drop it.

Thanks!

Re: [PATCH v1 0/4] ACPI: OSL: acpi_os_execute() improvements

[Hans de Goede]

Hi,

On 12/4/23 17:32, Rafael J. Wysocki wrote:
> Hi Hans,
> 
> On Sat, Dec 2, 2023 at 3:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 11/29/23 14:45, Rafael J. Wysocki wrote:
>>> Hi Everyone,
>>>
>>> This series improves acpi_os_execute() on top of
>>>
>>> https://patchwork.kernel.org/project/linux-acpi/patch/5745568.DvuYhMxLoT@kreacher/
>>>
>>> but only the last patch really depends on it.
>>>
>>> The first two patches clean up the code somewhat and the third one modifies
>>> the function to allow Notify () handlers to run on all CPUs (not on CPU0 only).
>>>
>>> The last patch changes it to use GFP_KERNEL for memory allocations, as it does
>>> not run in interrupt context any more after the change linked above.
>>
>> I have added this series, as well as the preceding
>> "ACPI: OSL: Use a threaded interrupt handler for SCI"
>> patch to my personal tree now, so that it will get tested on various
>> devices when I run my personal tree on them.
>>
>> I'll let you know if I hit any issues caused by this series.
> 
> As stated here
> 
> https://lore.kernel.org/linux-acpi/CAJZ5v0jkHLGa2XxB4TMqzrBBdZYXY79+sh1Z0ZF6keYdLDyfkg@mail.gmail.com/
> 
> the last patch in this series is not really a good idea just yet, so
> please drop it.

Ack, done.

Regards,

Hans