Re: [PATCH] hwmon: aspeed-pwm-tacho: propagate reset deassert errors

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent writes from sysfs and the thermal subsystem can cause data races and corrupted hardware state.
- [High] Potential division by zero in `aspeed_get_fan_tach_ch_rpm()` if clock frequency is low or missing.
- [High] The loop counter in `aspeed_create_fan_tach_channel` is an 8-bit integer, which can overflow and cause an infinite loop.
- [High] Probe deferral is broken because the driver incorrectly masks `-EPROBE_DEFER` when fetching the clock.
--

commit 23ab079c9c9897273758de82bd4212ec4f964294
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
Subject: hwmon: aspeed-pwm-tacho: propagate reset deassert errors

aspeed_pwm_tacho_probe() installs its reset cleanup action and configures the
controller after an unchecked reset deassertion. Stop probing when the reset
controller rejects the transition, before the hwmon device becomes visible.

> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
> index 1c5945d4ba377..bfce589c3fb1f 100644
> --- a/drivers/hwmon/aspeed-pwm-tacho.c
> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
> @@ -934,7 +934,9 @@ static int aspeed_pwm_tacho_probe(struct platform_device *pdev)
>  			"missing or invalid reset controller device tree entry");
>  		return PTR_ERR(priv->rst);
>  	}
> -	reset_control_deassert(priv->rst);
> +	ret = reset_control_deassert(priv->rst);
> +	if (ret)
> +		return ret;
>  
>  	ret = devm_add_action_or_reset(dev, aspeed_pwm_tacho_remove, priv);
>  	if (ret)

[Severity: High]
This is a pre-existing issue, but is probe deferral broken here? Shortly
after this reset deassert, the driver fetches a clock:

	clk = devm_clk_get(dev, NULL);
	if (IS_ERR(clk))
		return -ENODEV;

If devm_clk_get() returns -EPROBE_DEFER, returning -ENODEV will permanently
fail the probe instead of allowing it to be retried later.

[Severity: High]
This is a pre-existing issue, but can concurrent writes corrupt hardware state?
The driver registers with devm_hwmon_device_register_with_groups() without
using hwmon_lock() to serialize operations. A user writing to the PWM sysfs
attribute calls:

pwm_store() {
	...
	priv->pwm_port_fan_ctrl[index] = fan_ctrl;
	aspeed_set_pwm_port_fan_ctrl(priv, index, fan_ctrl);
	...
}

While the thermal subsystem can simultaneously update the cooling state:

aspeed_pwm_cz_set_cur_state() {
	...
	cdev->priv->pwm_port_fan_ctrl[cdev->pwm_port] =
					cdev->cooling_levels[cdev->cur_state];
	aspeed_set_pwm_port_fan_ctrl(cdev->priv, cdev->pwm_port,
				     cdev->cooling_levels[cdev->cur_state]);
	...
}

Will interleaving these multi-step non-atomic regmap writes in
aspeed_set_pwm_port_fan_ctrl() cause inconsistent hardware behavior?

[Severity: High]
This is a pre-existing issue, but could this result in a division by zero
panic? When initializing, priv->clk_freq is populated from clk_get_rate(clk).
If the rate evaluates to a value smaller than the divisor product, this
division truncates to 0:

aspeed_get_fan_tach_ch_measure_period() {
	...
	return clk / (clk_unit * div_h * div_l * tacho_div * tacho_unit);
}

The result is then used in a subsequent division:

aspeed_get_fan_tach_ch_rpm() {
	...
	msec = (1000 / aspeed_get_fan_tach_ch_measure_period(priv, type));
	...
}

Would a low or misconfigured clock frequency allow an unprivileged local user
reading the fanX_input sysfs file to crash the system?

[Severity: High]
This is a pre-existing issue, but is there a potential infinite loop here?

aspeed_create_fan_tach_channel() {
	...
	u8 val, index;

	for (val = 0; val < count; val++) {
	...
}

If a malformed device tree specifies 256 or more elements for count, will
the 8-bit loop counter val overflow and remain less than count, causing
the kernel to hang permanently during boot?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830125044.97718-1-pengpeng@iscas.ac.cn?part=1