Re: [PATCH] dt-bindings: hwmon: microchip,emc2305: add vcc-supply and #cooling-cells

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The commit message makes materially false claims about the driver's device tree parsing logic to justify leaving the binding examples unchanged.

Pre-existing issues:
- [High] The driver fails to use `hwmon_lock()` or any other locking to serialize accesses between hwmon sysfs and thermal subsystem operations, leading to race conditions and potential I2C corruption.
--

--- Patch [1]: [PATCH] dt-bindings: hwmon: microchip,emc2305: add vcc-supply and #cooling-cells ---
commit 2e59e1a6682ca5d565101743c080415ed986b896
Author: Vincent Jardin <vjardin@free.fr>

dt-bindings: hwmon: microchip,emc2305: add vcc-supply and #cooling-cells

This commit adds the missing vcc-supply and #cooling-cells properties to the
microchip,emc2305 device tree binding to resolve schema validation errors.
It places #cooling-cells at the controller node level.

> The #cooling-cells property belongs to the controller node, which is
> where the driver actually registers.
>
> Two cells is what that registration needs: a cooling-maps entry
> references the fan as <&emc2305 min max>.
>
> Two cells also documents what the driver supports today. The
> pwm-separate path registers one cooling device per PWM against the same
> node, and they all get cdev_id 0, so no cell count can tell them apart.
> That is a driver issue, noted here but not addressed.
>
> The examples in this binding put that property on the fan@N subnodes
> instead, which does not match what the driver does: they are left alone
> here rather than changed blind.

[Severity: Medium]
Is this description accurate regarding where the driver registers?

Looking at emc2305_probe() in drivers/hwmon/emc2305.c, when pwm_childs > 0,
the driver iterates through the subnodes and passes the child subnode directly
to devm_thermal_of_child_cooling_device_register():

drivers/hwmon/emc2305.c:emc2305_probe() {
    ...
    ret = emc2305_set_single_tz(dev, child, ch + 1);
    ...
}

It appears the examples in the binding are actually correct in placing
the #cooling-cells property on the fan@N subnodes when subnodes are present.

Could the commit message be updated to reflect this?

[Severity: High]
This is a pre-existing issue, but does the driver lack locking between
thermal subsystem and sysfs operations?

Looking at drivers/hwmon/emc2305.c, the thermal subsystem callback reads and
modifies shared state and performs I2C operations without acquiring any locks:

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);
    ...
}

Concurrently, the hwmon sysfs callback modifies the hwmon state and triggers
I2C updates:

drivers/hwmon/emc2305.c:emc2305_write() {
    ...
    data->cdev_data[cdev_idx].last_hwmon_state =
        EMC2305_PWM_DUTY2STATE(val, data->max_state,
                               EMC2305_FAN_MAX);
    ...
}

Because the driver uses devm_thermal_of_child_cooling_device_register() for
its thermal device, the hwmon core lock is not automatically held. The hwmon
subsystem API requires drivers to implement their own locking (like hwmon_lock())
for manually registered attributes.

Can this lead to race conditions accessing shared state and potential
interleaving of I2C operations to the same chip?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-for-upstream-dt-emc2305-vcc-cooling-v1-1-cd3b53dfb5d4@free.fr?part=1