2024-02-26 16:47:46

by Rafael J. Wysocki

[permalink] [raw]
Subject: [PATCH v2 3/5] ACPI: scan: Make acpi_processor_add() check the device enabled bit

From: Rafael J. Wysocki <[email protected]>

Modify acpi_processor_add() return an error if _STA returns the enabled
bit clear for the given processor device, so as to avoid using processors
that don't decode their resources, as per the ACPI specification. [1]

Link: https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html#sta-device-status # [1]
Signed-off-by: Rafael J. Wysocki <[email protected]>
---

v1 -> v2:
* Move acpi_device_is_enabled() to this patch.
* Change patch ordering.
* Do not check the "functional" _STA bit in acpi_device_is_present().

---
drivers/acpi/acpi_processor.c | 3 +++
drivers/acpi/internal.h | 1 +
drivers/acpi/scan.c | 5 +++++
3 files changed, 9 insertions(+)

Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -121,6 +121,7 @@ int acpi_device_setup_files(struct acpi_
void acpi_device_remove_files(struct acpi_device *dev);
void acpi_device_add_finalize(struct acpi_device *device);
void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
+bool acpi_device_is_enabled(const struct acpi_device *adev);
bool acpi_device_is_present(const struct acpi_device *adev);
bool acpi_device_is_battery(struct acpi_device *adev);
bool acpi_device_is_first_physical_node(struct acpi_device *adev,
Index: linux-pm/drivers/acpi/acpi_processor.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_processor.c
+++ linux-pm/drivers/acpi/acpi_processor.c
@@ -381,6 +381,9 @@ static int acpi_processor_add(struct acp
struct device *dev;
int result = 0;

+ if (!acpi_device_is_enabled(device))
+ return -ENODEV;
+
pr = kzalloc(sizeof(struct acpi_processor), GFP_KERNEL);
if (!pr)
return -ENOMEM;
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1945,6 +1945,11 @@ bool acpi_device_is_present(const struct
return adev->status.present || adev->status.functional;
}

+bool acpi_device_is_enabled(const struct acpi_device *adev)
+{
+ return adev->status.present && adev->status.enabled;
+}
+
static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
const char *idstr,
const struct acpi_device_id **matchid)





2024-02-27 09:54:46

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v2 3/5] ACPI: scan: Make acpi_processor_add() check the device enabled bit

On Mon, 26 Feb 2024 17:40:52 +0100
"Rafael J. Wysocki" <[email protected]> wrote:

> From: Rafael J. Wysocki <[email protected]>
>
> Modify acpi_processor_add() return an error if _STA returns the enabled
> bit clear for the given processor device, so as to avoid using processors
> that don't decode their resources, as per the ACPI specification. [1]
>
> Link: https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html#sta-device-status # [1]
> Signed-off-by: Rafael J. Wysocki <[email protected]>

Sorry for lack of reply on discussion. Your follow up mails never reached my
inbox for some reason so I just caught up on lore. I'll keep an eye on
the archives to make sure I don't miss further discussion.

Agreed that functional isn't relevant here so this patch is correct.
Also agree that it would be nice to clarify the spec as you mentioned
to say that bit 1 is reserved if bit 0 of _STA result is clear.
Depending on interpretation it's either a clarification or a relaxation
of current statements, so should be uncontroversial (famous last words ;)

+CC kangkang so this is on his radar as an ACPI cleanup suggestion.
For his reference, discussion is here:
https://lore.kernel.org/linux-acpi/CAJZ5v0jjD=KN0pOuWZZ8DT5yHdu03KgOSHYe3wB7h2vafNa44w@mail.gmail.com/

Reviewed-by: Jonathan Cameron <[email protected]>

> ---
>
> v1 -> v2:
> * Move acpi_device_is_enabled() to this patch.
> * Change patch ordering.
> * Do not check the "functional" _STA bit in acpi_device_is_present().
>
> ---
> drivers/acpi/acpi_processor.c | 3 +++
> drivers/acpi/internal.h | 1 +
> drivers/acpi/scan.c | 5 +++++
> 3 files changed, 9 insertions(+)
>
> Index: linux-pm/drivers/acpi/internal.h
> ===================================================================
> --- linux-pm.orig/drivers/acpi/internal.h
> +++ linux-pm/drivers/acpi/internal.h
> @@ -121,6 +121,7 @@ int acpi_device_setup_files(struct acpi_
> void acpi_device_remove_files(struct acpi_device *dev);
> void acpi_device_add_finalize(struct acpi_device *device);
> void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
> +bool acpi_device_is_enabled(const struct acpi_device *adev);
> bool acpi_device_is_present(const struct acpi_device *adev);
> bool acpi_device_is_battery(struct acpi_device *adev);
> bool acpi_device_is_first_physical_node(struct acpi_device *adev,
> Index: linux-pm/drivers/acpi/acpi_processor.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpi_processor.c
> +++ linux-pm/drivers/acpi/acpi_processor.c
> @@ -381,6 +381,9 @@ static int acpi_processor_add(struct acp
> struct device *dev;
> int result = 0;
>
> + if (!acpi_device_is_enabled(device))
> + return -ENODEV;
> +
> pr = kzalloc(sizeof(struct acpi_processor), GFP_KERNEL);
> if (!pr)
> return -ENOMEM;
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -1945,6 +1945,11 @@ bool acpi_device_is_present(const struct
> return adev->status.present || adev->status.functional;
> }
>
> +bool acpi_device_is_enabled(const struct acpi_device *adev)
> +{
> + return adev->status.present && adev->status.enabled;
> +}
> +
> static bool acpi_scan_handler_matching(struct acpi_scan_handler *handler,
> const char *idstr,
> const struct acpi_device_id **matchid)
>
>
>


2024-02-27 11:14:13

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH v2 3/5] ACPI: scan: Make acpi_processor_add() check the device enabled bit

On Tue, Feb 27, 2024 at 10:28 AM Jonathan Cameron
<[email protected]> wrote:
>
> On Mon, 26 Feb 2024 17:40:52 +0100
> "Rafael J. Wysocki" <[email protected]> wrote:
>
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Modify acpi_processor_add() return an error if _STA returns the enabled
> > bit clear for the given processor device, so as to avoid using processors
> > that don't decode their resources, as per the ACPI specification. [1]
> >
> > Link: https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html#sta-device-status # [1]
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
>
> Sorry for lack of reply on discussion.

No worries.

> Your follow up mails never reached my inbox for some reason

/me blames spam filters somewhere.

> so I just caught up on lore. I'll keep an eye on
> the archives to make sure I don't miss further discussion.

Thanks!

> Agreed that functional isn't relevant here so this patch is correct.
> Also agree that it would be nice to clarify the spec as you mentioned
> to say that bit 1 is reserved if bit 0 of _STA result is clear.
> Depending on interpretation it's either a clarification or a relaxation
> of current statements, so should be uncontroversial (famous last words ;)

Right.

> +CC kangkang so this is on his radar as an ACPI cleanup suggestion.
> For his reference, discussion is here:
> https://lore.kernel.org/linux-acpi/CAJZ5v0jjD=KN0pOuWZZ8DT5yHdu03KgOSHYe3wB7h2vafNa44w@mail.gmail.com/
>
> Reviewed-by: Jonathan Cameron <[email protected]>

Thanks for all of the reviews!