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!
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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);
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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;
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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;
}
On Wed, Nov 29, 2023 at 02:50:54PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Wed, Nov 29, 2023 at 02:52:22PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Wed, Nov 29, 2023 at 02:48:22PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Wed, Nov 29, 2023 at 3:34 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Wed, Nov 29, 2023 at 02:50:54PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > 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.
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
On Sat, Dec 2, 2023 at 3:31 PM Hans de Goede <[email protected]> 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!
On Wed, Nov 29, 2023 at 3:33 PM Rafael J. Wysocki <[email protected]> wrote:
>
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> ---
> 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;
>
>
>
>
>
Hi Hans,
On Sat, Dec 2, 2023 at 3:31 PM Hans de Goede <[email protected]> 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!
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 <[email protected]> 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