linux-kernel - Re: [PATCH v2 3/5] ACPI: scan: Make acpi_processor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240227092844.00006d49@Huawei.com>
Date: Tue, 27 Feb 2024 09:28:44 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
CC: Linux ACPI <linux-acpi@...r.kernel.org>, LKML
	<linux-kernel@...r.kernel.org>, Mika Westerberg
	<mika.westerberg@...ux.intel.com>, "Rafael J. Wysocki" <rafael@...nel.org>,
	"Russell King (Oracle)" <linux@...linux.org.uk>,
	<kangkang.shen@...urewei.com>
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" <rjw@...ysocki.net> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> 
> 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 <rafael.j.wysocki@...el.com>

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 <Jonathan.Cameron@...wei.com>

> ---
> 
> 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)
> 
> 
>