Re: [PATCH v2 12/16] platform/x86/thinkpad_acpi: Use acpi_dev_for_each_child()

Andy Shevchenko
2022-06-13T20:17:14Z

On Mon, Jun 13, 2022 at 08:30:19PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Instead of walking the list of children of an ACPI device directly,
> use acpi_dev_for_each_child() to carry out an action for all of
> the given ACPI device's children.
>
> This will help to eliminate the children list head from struct
> acpi_device as it is redundant and it is used in questionable ways
> in some places (in particular, locking is needed for walking the
> list pointed to it safely, but it is often missing).

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> v1 -> v2:
> * Eliminate unnecessary branch (Andy).
>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 53 +++++++++++++++++------------------
> 1 file changed, 27 insertions(+), 26 deletions(-)
>
> Index: linux-pm/drivers/platform/x86/thinkpad_acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/platform/x86/thinkpad_acpi.c
> +++ linux-pm/drivers/platform/x86/thinkpad_acpi.c
> @@ -6841,6 +6841,31 @@ static const struct backlight_ops ibm_ba
>
> /* --------------------------------------------------------------------- */
>
> +static int __init tpacpi_evaluate_bcl(struct acpi_device *adev, void *not_used)
> +{
> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> + union acpi_object *obj;
> + acpi_status status;
> + int rc;
> +
> + status = acpi_evaluate_object(adev->handle, "_BCL", NULL, &buffer);
> + if (ACPI_FAILURE(status))
> + return 0;
> +
> + obj = buffer.pointer;
> + if (!obj || obj->type != ACPI_TYPE_PACKAGE) {
> + acpi_handle_info(adev->handle,
> + "Unknown _BCL data, please report this to %s\n",
> + TPACPI_MAIL);
> + rc = 0;
> + } else {
> + rc = obj->package.count;
> + }
> + kfree(obj);
> +
> + return rc;
> +}
> +
> /*
> * Call _BCL method of video device. On some ThinkPads this will
> * switch the firmware to the ACPI brightness control mode.
> @@ -6848,37 +6873,13 @@ static const struct backlight_ops ibm_ba
>
> static int __init tpacpi_query_bcl_levels(acpi_handle handle)
> {
> - struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> - union acpi_object *obj;
> - struct acpi_device *device, *child;
> - int rc;
> + struct acpi_device *device;
>
> device = acpi_fetch_acpi_dev(handle);
> if (!device)
> return 0;
>
> - rc = 0;
> - list_for_each_entry(child, &device->children, node) {
> - acpi_status status = acpi_evaluate_object(child->handle, "_BCL",
> - NULL, &buffer);
> - if (ACPI_FAILURE(status)) {
> - buffer.length = ACPI_ALLOCATE_BUFFER;
> - continue;
> - }
> -
> - obj = (union acpi_object *)buffer.pointer;
> - if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
> - pr_err("Unknown _BCL data, please report this to %s\n",
> - TPACPI_MAIL);
> - rc = 0;
> - } else {
> - rc = obj->package.count;
> - }
> - break;
> - }
> -
> - kfree(buffer.pointer);
> - return rc;
> + return acpi_dev_for_each_child(device, tpacpi_evaluate_bcl, NULL);
> }
>
>
>
>
>

--
With Best Regards,
Andy Shevchenko