Re: [PATCH 4/4] hwmon: (adt7470) Add thermal zone sensor support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The thermal zone callback `adt7470_get_temp()` relies on a cached temperature array (`data->temp`) that is never updated by the background thread, resulting in permanently stale temperature readings.
- [High] Severe lock contention on `data->lock` stalling the thermal polling worker for up to 2 seconds.
- [High] Unprotected call to `adt7470_update_sensors()` during probe races with sysfs reads, allowing concurrent execution and hardware state corruption.
- [High] Cooling device ID collision prevents independent PWM channel control because `devm_thermal_of_child_cooling_device_register()` hardcodes `cdev_id = 0`.
- [Medium] The newly added thermal callback violates the hwmon subsystem locking guidelines by using a private mutex instead of `hwmon_lock()`.
--

--- Patch [4]: [PATCH 4/4] hwmon: (adt7470) Add thermal zone sensor support ---
commit 76a97c2b3fe41c135377aaeddcae6f2428b2ba1f
Author: Luiz Angelo Daros de Luca <luizluca@gmail.com>

hwmon: (adt7470) Add thermal zone sensor support
    
Expose the ADT7470 external temperature sensors to the thermal framework
via Device Tree. The thermal callbacks use the driver's cached sensor data,
avoiding any I2C bus traffic during frequent polling by the thermal core.

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

[ ... ]

> @@ -957,6 +963,47 @@ static int adt7470_register_cooling_devices(struct device *dev,
>  	return 0;
>  }
>  
> +static int adt7470_get_temp(struct thermal_zone_device *tz, int *temp)
> +{
> +	struct adt7470_thermal_sensor *sensor = thermal_zone_device_priv(tz);
> +	struct adt7470_data *data = sensor->data;
> +
> +	mutex_lock(&data->lock);

[Severity: Medium]
Does using this private mutex violate the hwmon subsystem locking guidelines?

The documentation specifies that drivers should use hwmon_lock() and
hwmon_unlock() for attributes registered outside the hwmon core, rather than
their internal lock.

[Severity: High]
Can taking this lock cause the thermal polling worker to stall?

The background thread adt7470_update_thread() acquires data->lock and then
sleeps for up to 2 seconds while waiting for hardware measurements:

adt7470_read_temperatures()
    res = msleep_interruptible((data->num_temp_sensors >= 0 ?
                                data->num_temp_sensors * 200 :
                                TEMP_COLLECTION_TIME));

Any concurrent attempt by the thermal core to call adt7470_get_temp() will
block on this exact same mutex for the duration of that sleep, potentially
starving the system thermal workqueue.

> +	*temp = 1000 * data->temp[sensor->id];

[Severity: High]
Will this temperature reading remain permanently stale?

The background thread adt7470_update_thread() calls
adt7470_read_temperatures(), which initiates conversions but returns early
without reading the values:

adt7470_read_temperatures()
    if (data->num_temp_sensors >= 0)
        return 0;

Since it bypasses reading the results into the data->temp cache, and the cache
is exclusively updated when userspace reads sysfs attributes via
adt7470_update_sensors(), relying entirely on the thermal core for polling
will prevent the reported temperature from ever changing.

> +	mutex_unlock(&data->lock);
> +
> +	return 0;
> +}

[ ... ]

> @@ -1401,6 +1448,10 @@ static int adt7470_probe(struct i2c_client *client)
>  		err = adt7470_register_cooling_devices(dev, data);

[Severity: High]
Does this cause a cooling device ID collision?

In adt7470_register_cooling_devices(), the driver loops to register 4 separate
cooling devices using devm_thermal_of_child_cooling_device_register(), passing
the exact same device node:

adt7470_register_cooling_devices()
    cdev = devm_thermal_of_child_cooling_device_register(dev,
                                                         dev->of_node,
                                                         ...);

Because this child API internally hardcodes cdev_id = 0, all 4 channels end up
sharing identical matching criteria. This prevents the thermal framework from
distinguishing between the PWM cooling devices, destroying independent channel
control.

[Severity: High]
Is there a race condition with sysfs reads during probe?

Earlier in adt7470_probe(), the hwmon device is registered with
devm_hwmon_device_register_with_info(), exposing sysfs attributes to userspace.
Immediately after, adt7470_update_sensors(data) is called without holding
data->lock:

adt7470_probe()
    hwmon_dev = devm_hwmon_device_register_with_info(...)
    ...
    err = adt7470_update_sensors(data);

A concurrent sysfs read can trigger adt7470_update_device(), which takes the
lock and calls adt7470_update_sensors() simultaneously. This interleaved
execution inside adt7470_read_temperatures() could corrupt hardware sequences.

>  		if (err)
>  			return err;
> +
> +		err = adt7470_register_thermal_sensors(dev, data);
> +		if (err)
> +			return err;
>  	}

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