Re: [PATCH v1] thermal/of: Fix cdev lookup in thermal_of_should_bind()

From: Yu-Che Cheng
Date: Fri Feb 21 2025 - 18:28:26 EST


Reviewed-by: Yu-Che Cheng <giver@xxxxxxxxxxxx>
Tested-by: Yu-Che Cheng <giver@xxxxxxxxxxxx>

On Sat, Feb 22, 2025 at 12:57 AM Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> Since thermal_of_should_bind() terminates the loop after processing
> the first child found in cooling-maps, it will never match more than
> one cdev to a given trip point which is incorrect, as there may be
> cooling-maps associating one trip point with multiple cooling devices.
>
> Address this by letting the loop continue until either all
> children have been processed or a matching one has been found.
>
> To avoid adding conditionals or goto statements, put the loop in
> question into a separate function and make that function return
> right away after finding a matching cooling-maps entry.
>
> Fixes: 94c6110b0b13 ("thermal/of: Use the .should_bind() thermal zone callback")
> Link: https://lore.kernel.org/linux-pm/20250219-fix-thermal-of-v1-1-de36e7a590c4@xxxxxxxxxxxx/
> Reported-by: Yu-Che Cheng <giver@xxxxxxxxxxxx>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
> 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:
>
>
>