Re: [PATCH v2 08/12] thermal/of: Rename the devm_thermal_of_cooling_device_register() function

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [Critical] Double call to thermal_cooling_device_init_complete() corrupts thermal_cdev_list.
- [High] cdev->np is stored without acquiring a reference, leading to a dangling pointer after the OF iterator loops finish.
- [Low] The mass-rename forces standalone cooling devices to use an API explicitly named and documented for child nodes.
- [Low] The non-devres equivalent of the API was not renamed, resulting in an inconsistent subsystem interface.
--

commit f1de025111783e26ec2bab8fd8078236f785c8b4
Author: Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>

thermal/of: Rename the devm_thermal_of_cooling_device_register() function

This commit renames devm_thermal_of_cooling_device_register() to
devm_thermal_of_child_cooling_device_register() to differentiate between
cooling devices bound via child nodes and a proposed new binding. The
rename was applied globally across drivers using a sed script.

> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 37269db2de84f..e6a567d58579d 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -685,8 +685,9 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  
>  	ctx->pwm_fan_state = ctx->pwm_fan_max_state;
>  	if (IS_ENABLED(CONFIG_THERMAL)) {
> -		cdev = devm_thermal_of_cooling_device_register(dev,
> -			dev->of_node, "pwm-fan", ctx, &pwm_fan_cooling_ops);
> +		cdev = devm_thermal_of_child_cooling_device_register(dev, dev->of_node,
> +								     "pwm-fan", ctx,
> +								     &pwm_fan_cooling_ops);

This isn't a bug, but does it make sense to use an API explicitly named and
documented for child nodes here?

Many standalone drivers like pwm-fan, gpio-fan, and max6650 simply pass their
main device node (dev->of_node) rather than a child node, which creates a
semantic contradiction with the new name.

[ ... ]

> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
> index ee9776d0e5be7..d06487bb5e675 100644
> --- a/drivers/thermal/thermal_of.c
> +++ b/drivers/thermal/thermal_of.c
[ ... ]
> @@ -563,14 +563,17 @@ static void thermal_cooling_device_release(struct device *dev, void *res)
>   * to /sys/class/thermal/ folder as cooling_device[0-*]. It tries to bind itself
>   * to all the thermal zone devices registered at the same time.
>   *
> + * This function should be used when a cooling controller has child
> + * nodes which are referenced in the thermal zone cooling map.
> + *
>   * Return: a pointer to the created struct thermal_cooling_device or an
>   * ERR_PTR. Caller must check return value with IS_ERR*() helpers.
>   */
>  struct thermal_cooling_device *
> -devm_thermal_of_cooling_device_register(struct device *dev,
> -					struct device_node *np,
> -					const char *type, void *devdata,
> -					const struct thermal_cooling_device_ops *ops)
> +devm_thermal_of_child_cooling_device_register(struct device *dev,
> +					      struct device_node *np,
> +					      const char *type, void *devdata,
> +					      const struct thermal_cooling_device_ops *ops)
>  {

This isn't a bug, but could this mass-rename cause confusion since the
non-devres counterpart (thermal_of_cooling_device_register) was not renamed?
Leaving one unchanged while renaming the other creates an inconsistent
subsystem API.

Also, since this API is now explicitly targeted at child nodes, which are
often iterated over using macros like for_each_child_of_node() or
for_each_child_of_node_scoped(), could this promote a use-after-free issue?

These iterators automatically drop the node reference when advancing or going
out of scope. If thermal_of_cooling_device_register() stores cdev->np = np
without acquiring a reference via of_node_get(), cdev->np will become a
dangling pointer once the loop finishes.

>  	struct thermal_cooling_device **ptr, *tcd;
>  
> @@ -592,4 +595,4 @@ devm_thermal_of_cooling_device_register(struct device *dev,
>  
>  	return tcd;
>  }
> -EXPORT_SYMBOL_GPL(devm_thermal_of_cooling_device_register);
> +EXPORT_SYMBOL_GPL(devm_thermal_of_child_cooling_device_register);

Can the double call to thermal_cooling_device_init_complete() cause an
infinite loop during list traversal?

Looking at the code just above the return in this function (affected by
earlier commits):

	tcd = thermal_of_cooling_device_register(np, type, devdata, ops);
	if (IS_ERR(tcd)) {
		devres_free(ptr);
		return tcd;
	}

	*ptr = tcd;
	devres_add(dev, ptr);

	thermal_cooling_device_init_complete(tcd);

thermal_of_cooling_device_register() already calls
thermal_cooling_device_init_complete(), which adds the cooling device to
thermal_cdev_list via list_add(). Calling it a second time here overwrites
the list pointers, establishing a cyclic reference.

I noticed this regression was introduced by an earlier commit
("thermal/core: Make thermal_cooling_device_init_complete() non static")
and a fix was provided later in the series in commit
("thermal/of: Pass the of_index and add a function to register with an
index").

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260422174305.2899095-1-daniel.lezcano@oss.qualcomm.com?part=8