[PATCH] thermal: of: Fix logic in thermal_of_should_bind

[PATCH] thermal: of: Fix logic in thermal_of_should_bind

[Yu-Che Cheng]

The current thermal_of_should_bind will stop iterating cooling-maps on
the first matched trip point, leading to subsequent cooling devices
binding to the same trip point failing to find the cooling spec.

The iteration should continue enumerating subsequent cooling-maps if the
target cooling device is not found.

Fix the logic to break only when a matched cooling device is found.

Fixes: 94c6110b0b13 ("thermal/of: Use the .should_bind() thermal zone callback")
Signed-off-by: Yu-Che Cheng <giver@chromium.org>
---
 drivers/thermal/thermal_of.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index 5ab4ce4daaeb..69c530e38574 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -312,7 +312,8 @@ static bool thermal_of_should_bind(struct thermal_zone_device *tz,
 				break;
 		}
 
-		break;
+		if (result)
+			break;
 	}
 
 	of_node_put(cm_np);

---
base-commit: 2408a807bfc3f738850ef5ad5e3fd59d66168996
change-id: 20250218-fix-thermal-of-247b71be0faa

Best regards,
-- 
Yu-Che Cheng <giver@chromium.org>

Re: [PATCH] thermal: of: Fix logic in thermal_of_should_bind

[Rafael J. Wysocki]

On Wed, Feb 19, 2025 at 8:06 AM Yu-Che Cheng <giver@chromium.org> wrote:
>
> The current thermal_of_should_bind will stop iterating cooling-maps on
> the first matched trip point, leading to subsequent cooling devices
> binding to the same trip point failing to find the cooling spec.
>
> The iteration should continue enumerating subsequent cooling-maps if the
> target cooling device is not found.
>
> Fix the logic to break only when a matched cooling device is found.

OK, but ->

> Fixes: 94c6110b0b13 ("thermal/of: Use the .should_bind() thermal zone callback")
> Signed-off-by: Yu-Che Cheng <giver@chromium.org>
> ---
>  drivers/thermal/thermal_of.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index 5ab4ce4daaeb..69c530e38574 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
> @@ -312,7 +312,8 @@ static bool thermal_of_should_bind(struct thermal_zone_device *tz,
>                                 break;

-> I'd prefer to do a jump from here, that is

-                                 break;
+                                goto put_cm_np;
>                 }
>
> -               break;

and remove the break statement above altogether.

> +               if (result)
> +                       break;
>         }
>

And of course the label needs to be added too:

+put_cm_np:
>         of_node_put(cm_np);
>
> ---

Re: [PATCH] thermal: of: Fix logic in thermal_of_should_bind

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 1914 bytes --]

On Wed, Feb 19, 2025 at 10:40 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Feb 19, 2025 at 8:06 AM Yu-Che Cheng <giver@chromium.org> wrote:
> >
> > The current thermal_of_should_bind will stop iterating cooling-maps on
> > the first matched trip point, leading to subsequent cooling devices
> > binding to the same trip point failing to find the cooling spec.
> >
> > The iteration should continue enumerating subsequent cooling-maps if the
> > target cooling device is not found.
> >
> > Fix the logic to break only when a matched cooling device is found.
>
> OK, but ->
>
> > Fixes: 94c6110b0b13 ("thermal/of: Use the .should_bind() thermal zone callback")
> > Signed-off-by: Yu-Che Cheng <giver@chromium.org>
> > ---
> >  drivers/thermal/thermal_of.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> > index 5ab4ce4daaeb..69c530e38574 100644
> > --- a/drivers/thermal/thermal_of.c
> > +++ b/drivers/thermal/thermal_of.c
> > @@ -312,7 +312,8 @@ static bool thermal_of_should_bind(struct thermal_zone_device *tz,
> >                                 break;
>
> -> I'd prefer to do a jump from here, that is
>
> -                                 break;
> +                                goto put_cm_np;
> >                 }
> >
> > -               break;
>
> and remove the break statement above altogether.
>
> > +               if (result)
> > +                       break;
> >         }
> >
>
> And of course the label needs to be added too:
>
> +put_cm_np:
> >         of_node_put(cm_np);
> >
> > ---

Or even, to avoid adding a new label, move the loop from
thermal_of_should_bind() into a new function that will be called by it
do carry out the cooling-maps lookup, like in the attached patch.

Can you check if it works for you, please?

[-- Attachment #2: thermal-of-fix.patch --]
[-- Type: text/x-patch, Size: 1880 bytes --]

---
 drivers/thermal/thermal_of.c |   50 ++++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 21 deletions(-)

--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -274,6 +274,34 @@
 	return true;
 }
 
+static bool thermal_of_cm_lookup(struct device_node *cm_np,
+				 const struct thermal_trip *trip,
+				 struct thermal_cooling_device *cdev,
+				 struct cooling_spec *c)
+{
+	for_each_child_of_node_scoped(cm_np, child) {
+		struct device_node *tr_np;
+		int count, i;
+
+		tr_np = of_parse_phandle(child, "trip", 0);
+		if (tr_np != trip->priv)
+			continue;
+
+		/* The trip has been found, look up the cdev. */
+		count = of_count_phandle_with_args(child, "cooling-device",
+						   "#cooling-cells");
+		if (count <= 0)
+			pr_err("Add a cooling_device property with at least one device\n");
+
+		for (i = 0; i < count; i++) {
+			if (thermal_of_get_cooling_spec(child, i, cdev, c))
+				return true;
+		}
+	}
+
+	return false;
+}
+
 static bool thermal_of_should_bind(struct thermal_zone_device *tz,
 				   const struct thermal_trip *trip,
 				   struct thermal_cooling_device *cdev,
@@ -293,27 +321,7 @@
 		goto out;
 
 	/* Look up the trip and the cdev in the cooling maps. */
-	for_each_child_of_node_scoped(cm_np, child) {
-		struct device_node *tr_np;
-		int count, i;
-
-		tr_np = of_parse_phandle(child, "trip", 0);
-		if (tr_np != trip->priv)
-			continue;
-
-		/* The trip has been found, look up the cdev. */
-		count = of_count_phandle_with_args(child, "cooling-device", "#cooling-cells");
-		if (count <= 0)
-			pr_err("Add a cooling_device property with at least one device\n");
-
-		for (i = 0; i < count; i++) {
-			result = thermal_of_get_cooling_spec(child, i, cdev, c);
-			if (result)
-				break;
-		}
-
-		break;
-	}
+	result = thermal_of_cm_lookup(cm_np, trip, cdev, c);
 
 	of_node_put(cm_np);
 out:

Re: [PATCH] thermal: of: Fix logic in thermal_of_should_bind

[Yu-Che Cheng]

Yes, it works on my side, as they look functionally equivalent.
Moving into a new function makes the control flow look more intuitive indeed.





On Fri, Feb 21, 2025 at 4:22 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Feb 19, 2025 at 10:40 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Feb 19, 2025 at 8:06 AM Yu-Che Cheng <giver@chromium.org> wrote:
> > >
> > > The current thermal_of_should_bind will stop iterating cooling-maps on
> > > the first matched trip point, leading to subsequent cooling devices
> > > binding to the same trip point failing to find the cooling spec.
> > >
> > > The iteration should continue enumerating subsequent cooling-maps if the
> > > target cooling device is not found.
> > >
> > > Fix the logic to break only when a matched cooling device is found.
> >
> > OK, but ->
> >
> > > Fixes: 94c6110b0b13 ("thermal/of: Use the .should_bind() thermal zone callback")
> > > Signed-off-by: Yu-Che Cheng <giver@chromium.org>
> > > ---
> > >  drivers/thermal/thermal_of.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> > > index 5ab4ce4daaeb..69c530e38574 100644
> > > --- a/drivers/thermal/thermal_of.c
> > > +++ b/drivers/thermal/thermal_of.c
> > > @@ -312,7 +312,8 @@ static bool thermal_of_should_bind(struct thermal_zone_device *tz,
> > >                                 break;
> >
> > -> I'd prefer to do a jump from here, that is
> >
> > -                                 break;
> > +                                goto put_cm_np;
> > >                 }
> > >
> > > -               break;
> >
> > and remove the break statement above altogether.
> >
> > > +               if (result)
> > > +                       break;
> > >         }
> > >
> >
> > And of course the label needs to be added too:
> >
> > +put_cm_np:
> > >         of_node_put(cm_np);
> > >
> > > ---
>
> Or even, to avoid adding a new label, move the loop from
> thermal_of_should_bind() into a new function that will be called by it
> do carry out the cooling-maps lookup, like in the attached patch.
>
> Can you check if it works for you, please?

Re: [PATCH] thermal: of: Fix logic in thermal_of_should_bind

[Lukasz Luba]

Hi Rafael,

On 2/20/25 20:22, Rafael J. Wysocki wrote:
> On Wed, Feb 19, 2025 at 10:40 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>
>> On Wed, Feb 19, 2025 at 8:06 AM Yu-Che Cheng <giver@chromium.org> wrote:
>>>
>>> The current thermal_of_should_bind will stop iterating cooling-maps on
>>> the first matched trip point, leading to subsequent cooling devices
>>> binding to the same trip point failing to find the cooling spec.
>>>
>>> The iteration should continue enumerating subsequent cooling-maps if the
>>> target cooling device is not found.
>>>
>>> Fix the logic to break only when a matched cooling device is found.
>>
>> OK, but ->
>>
>>> Fixes: 94c6110b0b13 ("thermal/of: Use the .should_bind() thermal zone callback")
>>> Signed-off-by: Yu-Che Cheng <giver@chromium.org>
>>> ---
>>>   drivers/thermal/thermal_of.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
>>> index 5ab4ce4daaeb..69c530e38574 100644
>>> --- a/drivers/thermal/thermal_of.c
>>> +++ b/drivers/thermal/thermal_of.c
>>> @@ -312,7 +312,8 @@ static bool thermal_of_should_bind(struct thermal_zone_device *tz,
>>>                                  break;
>>
>> -> I'd prefer to do a jump from here, that is
>>
>> -                                 break;
>> +                                goto put_cm_np;
>>>                  }
>>>
>>> -               break;
>>
>> and remove the break statement above altogether.
>>
>>> +               if (result)
>>> +                       break;
>>>          }
>>>
>>
>> And of course the label needs to be added too:
>>
>> +put_cm_np:
>>>          of_node_put(cm_np);
>>>
>>> ---
> 
> Or even, to avoid adding a new label, move the loop from
> thermal_of_should_bind() into a new function that will be called by it
> do carry out the cooling-maps lookup, like in the attached patch.
> 
> Can you check if it works for you, please?


I have experimented with your proposed patch with my DT setup.
It looks OK to me and much more clean.

If you have any super-edge-case scenario DT config to try, please let me
know. I will try to create such on my end...

If you would like to go forward with your patch, feel free to add:

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Tested-by: Lukasz Luba <lukasz.luba@arm.com>

Regards,
Lukasz