Re: [PATCH v7 3/3] hwmon: emc2305: Support configurable fan PWM at shutdown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The emc2305_shutdown() callback sets the hardware to a safe shutdown state but does not prevent concurrent or subsequent modifications from the thermal framework or sysfs, and violates hwmon locking guidelines.
--

commit 13f5fb0080b01e93b042a4e45c254e10cd715c59
Author: Florin Leotescu <florin.leotescu@nxp.com>

hwmon: emc2305: Support configurable fan PWM at shutdown

This commit adds support for an optional "fan-shutdown-percent" device tree
property to configure the safe shutdown PWM duty cycle per fan output. It
introduces a shutdown callback to apply these configured states during
system handoff.

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

[ ... ]

> @@ -727,6 +744,23 @@ static int emc2305_probe(struct i2c_client *client)
>  	return 0;
>  }
>  
> +static void emc2305_shutdown(struct i2c_client *client)
> +{
> +	int i;
> +	int ret;
> +	struct emc2305_data *data = i2c_get_clientdata(client);
> +
> +	for (i = 0; i < data->pwm_num; i++) {
> +		if (data->pwm_shutdown[i] != EMC2305_PWM_SHUTDOWN_UNSET) {
> +			ret = i2c_smbus_write_byte_data(client, EMC2305_REG_FAN_DRIVE(i),
> +							data->pwm_shutdown[i]);

Does this hardware access need to be serialized? The hwmon subsystem
guidelines explicitly state that drivers must use hwmon_lock() and
hwmon_unlock() to serialize hardware operations for attributes registered
outside the hwmon core.

Also, can concurrent or subsequent updates overwrite this safe shutdown state?

During system shutdown, kernel workqueues remain active and can continue to
execute thermal zone polling. Because the driver does not track whether it is
shutting down, the thermal framework's set_cur_state callback could execute
and overwrite the safe shutdown PWM state with a temperature-based value.

Would it make sense to introduce a boolean flag (e.g., shutting_down)
protected by hwmon_lock() here, and check this flag in the thermal and sysfs
write paths to abort further updates?

> +			if (ret < 0)
> +				dev_warn(&client->dev,
> +					 "Failed to set shutdown PWM for ch %d\n", i);
> +		}
> +	}
> +}

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