Re: [PATCH 3/4] hwmon: (adt7470) Add thermal cooling device support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] All 4 PWM cooling devices are registered with identical device tree nodes and cdev_id, preventing independent thermal zone binding.
- [High] `adt7470_update_sensors()` is called locklessly in `adt7470_probe()` after the hwmon device is registered, leading to a severe data race.
- [Medium] The driver unconditionally registers all 4 cooling devices, failing to make them optional based on `#cooling-cells`, contrary to the commit message.
--

--- Patch [3]: [PATCH 3/4] hwmon: (adt7470) Add thermal cooling device support ---
commit f3b7f3fcc935f749dd7f53e14d3533656b277e5b
Author: Luiz Angelo Daros de Luca <luizluca@gmail.com>

hwmon: (adt7470) Add thermal cooling device support

The ADT7470 has four PWM outputs that can be used to control fans.
Register each PWM output as an optional thermal cooling device via
Device Tree so it can be referenced by thermal zones.

When the thermal subsystem sets a cooling state, the driver switches
the corresponding PWM channel to manual mode so that the requested duty
cycle takes effect.

> Registration is optional: when the "#cooling-cells" property is absent,
> devm_thermal_of_child_cooling_device_register() returns -ENODEV and the
> driver silently skips that PWM channel.

[Severity: Medium]
Is this description accurate? Looking at thermal_of_cooling_device_register(),
it does not appear to parse the device tree for #cooling-cells during
registration. Since it doesn't return -ENODEV for a missing property,
doesn't this result in unconditionally registering all 4 cooling devices
even if the device tree does not specify them?

[ ... ]

> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
> index 4ac292cd7f47e..b865f4bd50f87 100644
> --- a/drivers/hwmon/adt7470.c
> +++ b/drivers/hwmon/adt7470.c

[ ... ]

> @@ -846,6 +856,107 @@ static int adt7470_pwm_write(struct device *dev, u32 attr, int channel, long val
[ ... ]
> +static int adt7470_register_cooling_devices(struct device *dev,
> +					    struct adt7470_data *data)
> +{
> +	int i;
> +
> +	for (i = 0; i < ADT7470_PWM_COUNT; i++) {
> +		struct thermal_cooling_device *cdev;
> +		char cdev_name[THERMAL_NAME_LENGTH];
> +
> +		data->cooling_devices[i].data = data;
> +		data->cooling_devices[i].pwm_index = i;
> +
> +		snprintf(cdev_name, sizeof(cdev_name), "adt7470-pwm%d", i);
> +		cdev = devm_thermal_of_child_cooling_device_register(dev,
> +								     dev->of_node, cdev_name,
> +								     &data->cooling_devices[i],
> +								     &adt7470_cooling_ops);

[Severity: High]
When registering the cooling devices in a loop, does passing the same parent
dev->of_node for every PWM channel prevent independent thermal zone binding?

Since __devm_thermal_of_cooling_device_register() binds them all with the
same node, they will all get assigned cdev_id = 0. When the thermal core
parses a DT cooling map requiring a specific cdev_id, will it fail to match
the individual channels since they are indistinguishable?

> +
> +		if (IS_ERR(cdev)) {
> +			if (PTR_ERR(cdev) == -ENODEV)
> +				continue;

[Severity: Medium]
Will this check ever be hit? As noted in the commit message review above,
the thermal registration API does not return -ENODEV for missing device
tree properties, which causes all PWM channels to be registered
unconditionally.

> +			return dev_warn_probe(dev, PTR_ERR(cdev),
> +					      "failed to register cooling device %s\n",
> +					      cdev_name);
> +		}
> +	}
> +
> +	return 0;
> +}

[ ... ]

> @@ -1281,6 +1392,17 @@ static int adt7470_probe(struct i2c_client *client)
>  	if (IS_ERR(hwmon_dev))
>  		return PTR_ERR(hwmon_dev);
>  
> +	if (IS_ENABLED(CONFIG_THERMAL)) {
> +		/* fill the cache before registering the cooling devices */
> +		err = adt7470_update_sensors(data);

[Severity: High]
Can this call to adt7470_update_sensors() cause hardware configuration
corruption due to a missing lock?

This is executed in adt7470_probe() immediately after the hwmon device is
registered, meaning sysfs attributes are already exposed to userspace.

Since adt7470_update_sensors() calls adt7470_read_temperatures(), which
explicitly assumes data->lock is held while performing multi-step register
manipulations and a 2-second sleep, what prevents a concurrent userspace sysfs
access from racing during this window and corrupting the PWM configuration?

> +		if (err)
> +			return err;
> +
> +		err = adt7470_register_cooling_devices(dev, data);
> +		if (err)
> +			return err;
> +	}
> +
>  	data->auto_update = kthread_run(adt7470_update_thread, client, "%s",
>  					dev_name(hwmon_dev));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-adt7470_thermalzone-v1-0-cc55ef35edde@gmail.com?part=3