Re: [PATCH v2 2/2] hwmon: (starfive-fan-tach) Add fan tach driver for StarFive JHB100

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Alarm attributes violate the hwmon ABI (sticky semantics) and hold a global mutex across a 200ms sleep, forcing strict serialization and massive latency.
--

--- Patch [2]: [PATCH v2 2/2] hwmon: (starfive-fan-tach) Add fan tach driver for StarFive JHB100 ---
commit f832cfa14cd6c4c786efa0a34afc865eca6dcf71
Author: Changhuang Liang <[email protected]>
Subject: hwmon: (starfive-fan-tach) Add fan tach driver for StarFive JHB100

This commit adds a fan tachometer driver for the StarFive JHB100 SoC. It
supports up to 16 independent fan tachometer inputs and reports fan speed,
stall, and low-speed alarms through the hwmon sysfs interface.

> diff --git a/drivers/hwmon/starfive-fan-tach.c b/drivers/hwmon/starfive-fan-tach.c
> new file mode 100644
> index 0000000000000..1e6399db424dc
> --- /dev/null
> +++ b/drivers/hwmon/starfive-fan-tach.c

[ ... ]

> +static int starfive_fan_tach_hwmon_read(struct device *dev,
> +					enum hwmon_sensor_types type, u32 attr,
> +					int channel, long *val)
> +{
> +	struct starfive_fan_tach_data *priv = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	switch (attr) {
> +	case hwmon_fan_fault:
> +		scoped_guard(mutex, &priv->lock) {
> +			scoped_guard(spinlock_irqsave, &priv->irq_lock) {
> +				writel(STARFIVE_FAN_TACH_STALL_INT(channel),
> +				       priv->regs + STARFIVE_FAN_TACH_STATUS);
> +				/* clear fan_stall first */
> +				priv->fan_stall[channel] = 0;
> +				reinit_completion(&priv->comp_stall[channel]);
> +				priv->armed_stall |= BIT(channel);
> +			}
> +
> +			starfive_fan_tach_ch_stall_unmask(priv, channel, true);
> +
> +			/* Waiting for hardware to measure */
> +			wait_for_completion_timeout(&priv->comp_stall[channel],
> +						    2 * STARFIVE_FAN_TACH_TIMEOUT_JIFFIES);

[Severity: Medium]
Does reading the fault attribute violate the hwmon ABI by blocking to poll the
hardware instead of returning instantaneous sticky status?

Also, because wait_for_completion_timeout() is called inside
scoped_guard(mutex, &priv->lock), doesn't this hold the global mutex across a
200ms sleep?

If a user tool (like sensors) sequentially reads the alarms for all 16 channels,
could this stall the reader for over 6 seconds and block all other concurrent
reads/writes to this device?

[ ... ]

> +	case hwmon_fan_min_alarm:
> +		scoped_guard(mutex, &priv->lock) {
> +			scoped_guard(spinlock_irqsave, &priv->irq_lock) {
> +				writel(STARFIVE_FAN_TACH_SLOW_INT(channel),
> +				       priv->regs + STARFIVE_FAN_TACH_STATUS);
> +				/* clear fan_slow first */
> +				priv->fan_slow[channel] = 0;
> +				reinit_completion(&priv->comp_slow[channel]);
> +				priv->armed_slow |= BIT(channel);
> +			}
> +
> +			starfive_fan_tach_ch_slow_unmask(priv, channel, true);
> +
> +			/* Waiting for hardware to measure */
> +			wait_for_completion_timeout(&priv->comp_slow[channel],
> +						    2 * STARFIVE_FAN_TACH_TIMEOUT_JIFFIES);

[Severity: Medium]
Similar to the fault attribute above, does reading the min_alarm attribute
violate the hwmon ABI by blocking instead of returning a sticky status?

Does holding the global mutex during this sleep similarly risk blocking
concurrent device accesses and causing long stalls when reading multiple
channels?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2