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

[Rafael J. Wysocki]

On Wed, Feb 19, 2025 at 10:40 PM Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
>
> On Wed, Feb 19, 2025 at 8:06 AM Yu-Che Cheng <giver@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> >  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?
---
 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: