Re: [PATCH v5 2/2] hwmon: (starfive-pwm-fan) Add PWM fan driver for StarFive JHB100

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hwmon: starfive: fan tachometer register state is lost on system suspend
- [Medium] hwmon: starfive: aborted teardown loop in pwm_disable_all leaks hardware state
--

commit ba72917646ababbe1ce560dd96b1e73eb3c6b2f3
Author: Changhuang Liang <changhuang.liang@starfivetech.com>
Subject: hwmon: (starfive-pwm-fan) Add PWM fan driver for StarFive JHB100

This commit adds a new PWM fan controller driver for the StarFive JHB100 SoC.
It allows configuring fan speeds using up to 8 external PWM channels and
provides RPM readings and alarms via a 16-channel hardware tachometer block.

> diff --git a/drivers/hwmon/starfive-pwm-fan.c b/drivers/hwmon/starfive-pwm-fan.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/hwmon/starfive-pwm-fan.c

[ ... ]

> +static int starfive_pwm_fan_pwm_disable_all(struct starfive_pwm_fan_data *priv)
> +{
> +	struct pwm_state state;
> +	int i, ret;
> +
> +	guard(mutex)(&priv->pwm_lock);
> +
> +	for (i = 0; i < STARFIVE_FAN_MAX; i++) {
> +		struct starfive_pwm_fan *fan = &priv->fans[i];
> +
> +		if (!fan->pwm || !fan->pwm_state.enabled)
> +			continue;
> +
> +		state = fan->pwm_state;
> +		state.duty_cycle = 0;
> +		state.enabled = false;
> +
> +		ret = pwm_apply_might_sleep(fan->pwm, &state);
> +		if (ret)
> +			return ret;

[Severity: Medium]
If pwm_apply_might_sleep() fails for a fan channel (for example, due to an
underlying PWM controller communication error), this early return abruptly
breaks the teardown loop.

Could this leave active PWM channels and fans running during driver removal
or system suspend? Because a failure in one PWM channel shouldn't necessarily
prevent the teardown of the remaining independent channels, would it be safer
to log the error and continue disabling the others?

> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int starfive_pwm_fan_resume(struct device *dev)
> +{
> +	struct starfive_pwm_fan_data *priv = dev_get_drvdata(dev);
> +	int i, ret;
> +
> +	guard(mutex)(&priv->pwm_lock);
> +
> +	for (i = 0; i < STARFIVE_FAN_MAX; i++) {
> +		struct starfive_pwm_fan *fan = &priv->fans[i];
> +
> +		if (!fan->pwm)
> +			continue;
> +
> +		ret = pwm_apply_might_sleep(fan->pwm, &fan->pwm_state);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
This resume handler restores the external PWM state, but does it miss
restoring the internal tachometer registers (measure time, channel enables,
and sysfs thresholds)?

If the peripheral's power domain is disabled during sleep, the hardware
configuration will reset to power-on defaults. Could this cause fan
monitoring and alarms to silently fail after a system resume? It appears
the driver does not cache the user-configured threshold values written via
sysfs, making it impossible to restore them upon resume.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923130624.239499-1-changhuang.liang@starfivetech.com?part=2