Received-SPF: pass (google.com: domain of linux-kernel+bounces-82957-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Tue, 27 Feb 2024 09:28:44 +0000
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
CC: Linux ACPI <linux-acpi@vger.kernel.org>, LKML
	<linux-kernel@vger.kernel.org>, Mika Westerberg
	<mika.westerberg@linux.intel.com>, "Rafael J. Wysocki" <rafael@kernel.org>,
	"Russell King (Oracle)" <linux@armlinux.org.uk>,
	<kangkang.shen@futurewei.com>
Subject: Re: [PATCH v2 3/5] ACPI: scan: Make acpi_processor_add() check the
 device enabled bit
Message-ID: <20240227092844.00006d49@Huawei.com>
In-Reply-To: <3283809.44csPzL39Z@kreacher>
References: <6021126.lOV4Wx5bFT@kreacher>
	<3283809.44csPzL39Z@kreacher>
Organization: Huawei Technologies Research and Development (UK) Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit

On Mon, 26 Feb 2024 17:40:52 +0100
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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@intel.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@huawei.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)
> 
> 
>