Received-SPF: pass (google.com: domain of linux-kernel+bounces-138567-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 10 Apr 2024 14:13:20 +0100
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Miguel Luis <miguel.luis@oracle.com>
CC: "Rafael J. Wysocki" <rafael@kernel.org>, Len Brown <lenb@kernel.org>,
	<linux-acpi@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<rmk+kernel@armlinux.org.uk>
Subject: Re: [RFC PATCH 1/4] ACPI: processor: refactor
 acpi_processor_get_info: evaluation of processor declaration
Message-ID: <20240410141320.00004199@Huawei.com>
In-Reply-To: <20240409150536.9933-2-miguel.luis@oracle.com>
References: <20240409150536.9933-1-miguel.luis@oracle.com>
	<20240409150536.9933-2-miguel.luis@oracle.com>
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 Tue,  9 Apr 2024 15:05:30 +0000
Miguel Luis <miguel.luis@oracle.com> wrote:

> Isolate the evaluation of processor declaration into its own function.
> 
> No functional changes.
> 
> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>

Hi Miguel,

I'd like more description in each patch of 'why' the change is useful. 

A few comments inline.

Jonathan

> ---
>  drivers/acpi/acpi_processor.c | 78 +++++++++++++++++++++++------------
>  1 file changed, 51 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 7a0dd35d62c9..37e8b69113dd 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -230,15 +230,59 @@ static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
>  }
>  #endif /* CONFIG_ACPI_HOTPLUG_CPU */
>  
> +static int acpi_evaluate_processor(struct acpi_device *device,
> +				   struct acpi_processor *pr,
> +				   union acpi_object *object,
> +				   int *device_declaration)

I'd use a bool * for device_declaration.

> +{
> +	struct acpi_buffer buffer = { sizeof(union acpi_object), object };
> +	acpi_status status = AE_OK;

Status always written so don't initialize it.

> +	unsigned long long value;
> +
> +	/*
> +	 * Declarations via the ASL "Processor" statement are deprecated.

Be clear where they are deprecated. i.e. the ACPI spec and which version and
under what circumstances. 

Or don't say it. From Linux kernel point of view we need to support this anyway
for a long long time, so knowing they are deprecated in the ACPI spec
isn't really of interest.

> +	 */
> +	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID)) {
> +		/* Declared with "Processor" statement; match ProcessorID */
> +		status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
> +		if (ACPI_FAILURE(status)) {
> +			dev_err(&device->dev,
> +				"Failed to evaluate processor object (0x%x)\n",
> +				status);
> +			return -ENODEV;
> +		}
> +
> +		value = object->processor.proc_id;
> +		goto out;

I'd keep the if / else form of the original code. I think it's easier to follow given
this really is choosing between 2 options.

> +	}
> +
> +	/*
> +	 * Declared with "Device" statement; match _UID.
> +	 */
> +	status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
> +					NULL, &value);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(&device->dev,
> +			"Failed to evaluate processor _UID (0x%x)\n",
> +			status);
> +		return -ENODEV;
> +	}
> +
> +	*device_declaration = 1;
> +out:
> +	pr->acpi_id = value;

Maybe better to pass in the pr->handle, and return value so
pr->acpi_id is set at the caller rather than setting it in
this helper function?  That will keep the pr->x setting
all in one place.

> +	return 0;
> +}
> +
>  static int acpi_processor_get_info(struct acpi_device *device)
>  {
>  	union acpi_object object = { 0 };
> -	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
>  	struct acpi_processor *pr = acpi_driver_data(device);
>  	int device_declaration = 0;
>  	acpi_status status = AE_OK;
>  	static int cpu0_initialized;
>  	unsigned long long value;
> +	int ret;
>  
>  	acpi_processor_errata();
>  
> @@ -252,32 +296,12 @@ static int acpi_processor_get_info(struct acpi_device *device)
>  	} else
>  		dev_dbg(&device->dev, "No bus mastering arbitration control\n");
>  
> -	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID)) {
> -		/* Declared with "Processor" statement; match ProcessorID */
> -		status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
> -		if (ACPI_FAILURE(status)) {
> -			dev_err(&device->dev,
> -				"Failed to evaluate processor object (0x%x)\n",
> -				status);
> -			return -ENODEV;
> -		}
> -
> -		pr->acpi_id = object.processor.proc_id;
> -	} else {
> -		/*
> -		 * Declared with "Device" statement; match _UID.
> -		 */
> -		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
> -						NULL, &value);
> -		if (ACPI_FAILURE(status)) {
> -			dev_err(&device->dev,
> -				"Failed to evaluate processor _UID (0x%x)\n",
> -				status);
> -			return -ENODEV;
> -		}
> -		device_declaration = 1;
> -		pr->acpi_id = value;
> -	}
> +	/*
> +	 * Evaluate processor declaration.
Given function name (which is well named!) I don't see the comment adding anything.
So I'd drop the comment.
> +	 */
> +	ret = acpi_evaluate_processor(device, pr, &object, &device_declaration);
> +	if (ret)
> +		return ret;
>  
>  	if (acpi_duplicate_processor_id(pr->acpi_id)) {
>  		if (pr->acpi_id == 0xff)