Re: [PATCH v7 1/3] hwmon: emc2305: Fix fan channel index handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing hwmon locking serialization between sysfs operations and thermal cooling callbacks.
- [High] Incomplete channel separation causing independent thermal updates to silently alias to channel 0.
- [High] Division by zero in `emc2305_show_fan()` when hardware returns a small tachometer value.
--

commit 3aaf7523e2096af769a25d6788cd08adc306d559
Author: Florin Leotescu <florin.leotescu@nxp.com>

hwmon: emc2305: Fix fan channel index handling

The commit fixes the fan channel index used for accessing per-channel data
structures. It validates the index against the available channels and uses
the DT reg property for thermal registration.

> diff --git a/drivers/hwmon/emc2305.c b/drivers/hwmon/emc2305.c
> index 64b213e1451e..f71a0e265924 100644
> --- a/drivers/hwmon/emc2305.c
> +++ b/drivers/hwmon/emc2305.c

[ ... ]

> @@ -680,12 +687,18 @@ static int emc2305_probe(struct i2c_client *client)
>  	if (IS_REACHABLE(CONFIG_THERMAL)) {
>  		/* Parse and check for the available PWM child nodes */
>  		if (pwm_childs > 0) {
> -			i = 0;
>  			for_each_child_of_node_scoped(dev->of_node, child) {
> -				ret = emc2305_set_single_tz(dev, child, i);
> +				ret = of_property_read_u32(child, "reg", &ch);
> +				if (ret || ch >= data->pwm_num)
> +					continue;
> +
> +				/*
> +				 * emc2305_set_single_tz() uses 0 for the common cooling
> +				 * device and 1..pwm_num for individual fan channels.
> +				 */
> +				ret = emc2305_set_single_tz(dev, child, ch + 1);

Since data->pwm_separate is hardcoded to false for all DT configurations,
doesn't emc2305_get_cdev_idx() unconditionally return 0 at runtime for all
registered cooling devices?

If the thermal subsystem independently updates a specific fan channel, does
this cause the driver to overwrite channel 0's internal state and broadcast
the PWM change to all hardware channels instead of separating them?

>  				if (ret != 0)
>  					return ret;
> -				i++;
>  			}
>  		} else {
>  			ret = emc2305_set_tz(dev);

I also noticed a few other issues in this file that are not introduced by
this patch, but might be worth addressing.

Is there missing locking between hwmon sysfs operations and thermal cooling
callbacks? Both emc2305_write() and emc2305_set_cur_state() can concurrently
modify shared state and issue I2C writes:

drivers/hwmon/emc2305.c:__emc2305_set_cur_state() {
    ...
	data->cdev_data[cdev_idx].cur_state = state;
	if (data->pwm_separate) {
		ret = i2c_smbus_write_byte_data(client, EMC2305_REG_FAN_DRIVE(cdev_idx), val);
    ...
}

Does the absence of a mutex here create a data race that can corrupt the
driver's internal state?

Additionally, is there a potential division by zero in emc2305_show_fan()?

drivers/hwmon/emc2305.c:emc2305_show_fan() {
    ...
	ret = i2c_smbus_read_word_swapped(client, EMC2305_REG_FAN_TACH(channel));
	if (ret <= 0)
		return ret;

	ret = ret >> EMC2305_TACH_REGS_UNUSE_BITS;
	ret = EMC2305_RPM_FACTOR / ret;
    ...
}

If the hardware returns a value between 1 and 7, shifting right by 3 makes
ret = 0. Doesn't the next line then divide by zero and trigger a panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260429065955.2113012-1-florin.leotescu@oss.nxp.com?part=1