[PATCH v1 07/10] ACPI: thermal: Use cooling device parent for thermal zone binding

Posted by Rafael J. Wysocki 2 weeks ago

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

The ACPI thermal zone .should_bind() callback function,
acpi_thermal_should_bind_cdev(), expects the given cooling device's
devdata to point to an ACPI device object whose ACPI handle should be
compared with ACPI handles in a list associated with the given trip
point.  That is not particularly straightforward and it effectively
requires the drivers of ACPI cooling devices to populate the devdata
with addresses of the ACPI companions of the devices they bind to.
Consequently, the devdata cannot be used by the driver for its own
needs which is its intended purpose.

That can be overcome with the help of the observation that the
ACPI device objects to be matched against the lists of ACPI handles
associated with trip points are in fact the ACPI companions of the
parents of cooling devices.  Thus instead of using the given cooling
device's devdata, it is sufficient to obtain the ACPI handle of its
parent and compare that ACPI handle with the ones in the list
associated with the given trip point.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/thermal.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index dd7666c176a0..dea28d674407 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -564,17 +564,18 @@ static bool acpi_thermal_should_bind_cdev(struct thermal_zone_device *thermal,
 					  struct cooling_spec *c)
 {
 	struct acpi_thermal_trip *acpi_trip = trip->priv;
-	struct acpi_device *cdev_adev = cdev->devdata;
+	struct device *parent = cdev->device.parent;
+	acpi_handle parent_handle;
 	int i;
 
-	/* Skip critical and hot trips. */
-	if (!acpi_trip)
+	/* Skip critical and hot trips and parentless cooling devices. */
+	if (!acpi_trip || !parent)
 		return false;
 
-	for (i = 0; i < acpi_trip->devices.count; i++) {
-		acpi_handle handle = acpi_trip->devices.handles[i];
+	parent_handle = ACPI_HANDLE(parent);
 
-		if (acpi_fetch_acpi_dev(handle) == cdev_adev)
+	for (i = 0; i < acpi_trip->devices.count; i++) {
+		if (acpi_trip->devices.handles[i] == parent_handle)
 			return true;
 	}
 
-- 
2.51.0

Re: [PATCH v1 07/10] ACPI: thermal: Use cooling device parent for thermal zone binding

Posted by Armin Wolf 1 week, 6 days ago

Am 11.09.26 um 15:05 schrieb Rafael J. Wysocki:

> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>
> The ACPI thermal zone .should_bind() callback function,
> acpi_thermal_should_bind_cdev(), expects the given cooling device's
> devdata to point to an ACPI device object whose ACPI handle should be
> compared with ACPI handles in a list associated with the given trip
> point.  That is not particularly straightforward and it effectively
> requires the drivers of ACPI cooling devices to populate the devdata
> with addresses of the ACPI companions of the devices they bind to.
> Consequently, the devdata cannot be used by the driver for its own
> needs which is its intended purpose.
>
> That can be overcome with the help of the observation that the
> ACPI device objects to be matched against the lists of ACPI handles
> associated with trip points are in fact the ACPI companions of the
> parents of cooling devices.  Thus instead of using the given cooling
> device's devdata, it is sufficient to obtain the ACPI handle of its
> parent and compare that ACPI handle with the ones in the list
> associated with the given trip point.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/acpi/thermal.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index dd7666c176a0..dea28d674407 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -564,17 +564,18 @@ static bool acpi_thermal_should_bind_cdev(struct thermal_zone_device *thermal,
>   					  struct cooling_spec *c)
>   {
>   	struct acpi_thermal_trip *acpi_trip = trip->priv;
> -	struct acpi_device *cdev_adev = cdev->devdata;
> +	struct device *parent = cdev->device.parent;
> +	acpi_handle parent_handle;
>   	int i;
>   
> -	/* Skip critical and hot trips. */
> -	if (!acpi_trip)
> +	/* Skip critical and hot trips and parentless cooling devices. */
> +	if (!acpi_trip || !parent)
>   		return false;
>   
> -	for (i = 0; i < acpi_trip->devices.count; i++) {
> -		acpi_handle handle = acpi_trip->devices.handles[i];
> +	parent_handle = ACPI_HANDLE(parent);
>   

Please check parent_handle for NULL here so we can return early. With this being fixed:

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> -		if (acpi_fetch_acpi_dev(handle) == cdev_adev)
> +	for (i = 0; i < acpi_trip->devices.count; i++) {
> +		if (acpi_trip->devices.handles[i] == parent_handle)
>   			return true;
>   	}
>

Re: [PATCH v1 07/10] ACPI: thermal: Use cooling device parent for thermal zone binding

Posted by Andy Shevchenko 1 week, 6 days ago

On Fri, Sep 11, 2026 at 03:05:07PM +0200, Rafael J. Wysocki wrote:

> The ACPI thermal zone .should_bind() callback function,
> acpi_thermal_should_bind_cdev(), expects the given cooling device's
> devdata to point to an ACPI device object whose ACPI handle should be
> compared with ACPI handles in a list associated with the given trip
> point.  That is not particularly straightforward and it effectively
> requires the drivers of ACPI cooling devices to populate the devdata
> with addresses of the ACPI companions of the devices they bind to.
> Consequently, the devdata cannot be used by the driver for its own
> needs which is its intended purpose.
> 
> That can be overcome with the help of the observation that the
> ACPI device objects to be matched against the lists of ACPI handles
> associated with trip points are in fact the ACPI companions of the
> parents of cooling devices.  Thus instead of using the given cooling
> device's devdata, it is sufficient to obtain the ACPI handle of its
> parent and compare that ACPI handle with the ones in the list
> associated with the given trip point.

...

> +	parent_handle = ACPI_HANDLE(parent);
>  
> -		if (acpi_fetch_acpi_dev(handle) == cdev_adev)
> +	for (i = 0; i < acpi_trip->devices.count; i++) {
> +		if (acpi_trip->devices.handles[i] == parent_handle)

device_match_acpi_handle() ?

		if (device_match_acpi_handle(parent, acpi_trip->devices.handles[i]))

>  			return true;
>  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 07/10] ACPI: thermal: Use cooling device parent for thermal zone binding

Posted by Rafael J. Wysocki (Intel) 1 week, 6 days ago

On Fri, Sep 11, 2026 at 6:36 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Sep 11, 2026 at 03:05:07PM +0200, Rafael J. Wysocki wrote:
>
> > The ACPI thermal zone .should_bind() callback function,
> > acpi_thermal_should_bind_cdev(), expects the given cooling device's
> > devdata to point to an ACPI device object whose ACPI handle should be
> > compared with ACPI handles in a list associated with the given trip
> > point.  That is not particularly straightforward and it effectively
> > requires the drivers of ACPI cooling devices to populate the devdata
> > with addresses of the ACPI companions of the devices they bind to.
> > Consequently, the devdata cannot be used by the driver for its own
> > needs which is its intended purpose.
> >
> > That can be overcome with the help of the observation that the
> > ACPI device objects to be matched against the lists of ACPI handles
> > associated with trip points are in fact the ACPI companions of the
> > parents of cooling devices.  Thus instead of using the given cooling
> > device's devdata, it is sufficient to obtain the ACPI handle of its
> > parent and compare that ACPI handle with the ones in the list
> > associated with the given trip point.
>
> ...
>
> > +     parent_handle = ACPI_HANDLE(parent);
> >
> > -             if (acpi_fetch_acpi_dev(handle) == cdev_adev)
> > +     for (i = 0; i < acpi_trip->devices.count; i++) {
> > +             if (acpi_trip->devices.handles[i] == parent_handle)
>
> device_match_acpi_handle() ?

That would cause ACPI_HANDLE() to be evaluated
acpi_trip->devices.count times for the parent whereas only one
evaluation is necessary, so not really.

>                 if (device_match_acpi_handle(parent, acpi_trip->devices.handles[i]))
>
> >                       return true;
> >       }