Re: [PATCH v1 2/2] hwmon: Add fan tach driver for StarFive

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Sysfs read operations for fault and alarm attributes block for 1 second during normal operation.
- [High] Interrupt handler is registered before the device clock is enabled, risking fatal bus faults.
- [High] Probe returns success (0) on failure paths, bypassing hwmon registration but leaving resources active.
- [High] Copy-paste error in `hwmon_fan_min_alarm` waits on the wrong completion structure, guaranteeing a timeout.
- [High] Race condition when enabling interrupts causes missed fault/alarm events.
- [High] IRQ handler processes uninitialized completions for unused channels, leading to a kernel panic.
- [High] Unvalidated device tree properties lead to out-of-bounds array access.
- [Medium] Hardware read errors are leaked to user space as valid sensor values.
- [Medium] Integer overflow in RPM conversion calculations yields incorrect readings.
--

commit 48298341a9b8797b12cfca7ded254542d53095fd
Author: Changhuang Liang <changhuang.liang@starfivetech.com>
Subject: hwmon: Add fan tach driver for StarFive

This commit adds the fan tach driver for the StarFive JHB100 SoC.

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

[ ... ]

> +static int starfive_fan_tach_rpm_to_val(struct starfive_fan_tach_data *priv, u32 rpm)
> +{
> +	u64 tach_val;
> +
> +	rpm *= STARFIVE_FAN_DEFAULT_PULSE_PR;

Does this multiplication overflow a 32-bit integer before assignment? Casting
rpm to u64 before multiplying would prevent truncation.

> +	tach_val = rpm / STARFIVE_FAN_DEFAULT_RPM_PAUSE_TIME;
> +
> +	return (int)tach_val;
> +}
> +
> +static int starfive_fan_tach_val_to_rpm(struct starfive_fan_tach_data *priv, u32 tach_val)
> +{
> +	u64 rpm;
> +
> +	rpm = tach_val * STARFIVE_FAN_DEFAULT_RPM_PAUSE_TIME;

Can this code overflow the 32-bit integer when multiplying tach_val by 120?
Since tach_val can be up to 31 bits, casting to u64 beforehand would
ensure correct calculations.

> +
> +	do_div(rpm, STARFIVE_FAN_DEFAULT_PULSE_PR);
> +
> +	return (int)rpm;
> +}

[ ... ]

> +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);
> +
> +	switch (attr) {
> +	case hwmon_fan_fault:
> +		starfive_fan_tach_ch_stall_unmask(priv, channel, true);
> +		/* clear fan_stall first */
> +		priv->fan_stall[channel] = 0;
> +		reinit_completion(&priv->comp_stall[channel]);

Is there a race condition here that can cause a missed fault event? Unmasking
the hardware interrupt before clearing priv->fan_stall[] and calling
reinit_completion() might allow the interrupt to fire instantly. 

If the ISR sets the flag and completes the completion, the read thread will 
subsequently overwrite the flag and reinitialize the completion, erasing the
event and causing a timeout.

> +		wait_for_completion_timeout(&priv->comp_stall[channel],
> +					    2 * STARFIVE_FAN_TACH_TIMEOUT_JIFFIES);

If the fan is operating normally, will this read operation unconditionally block
for the entire 1-second timeout duration? Since the fault interrupt will never
fire during normal operation, and the hwmon core serializes sysfs accesses
with a device-wide lock, this might cause user-space tools to hang.

> +		*val = priv->fan_stall[channel];
> +		starfive_fan_tach_ch_stall_unmask(priv, channel, false);
> +		break;
> +	case hwmon_fan_input:
> +		*val = starfive_fan_tach_get_rpm(priv, channel);

Does this code leak hardware read errors to user space as valid sensor values?
If starfive_fan_tach_get_rpm() returns a negative error code like -ETIMEDOUT,
it is directly assigned to *val while returning 0 for success.

> +		break;
> +	case hwmon_fan_min:
> +		*val = starfive_fan_tach_get_rpm_threshold(priv, channel);
> +		break;
> +	case hwmon_fan_min_alarm:
> +		starfive_fan_tach_ch_slow_unmask(priv, channel, true);
> +		/* clear fan_slow first */
> +		priv->fan_slow[channel] = 0;
> +		reinit_completion(&priv->comp_slow[channel]);
> +		wait_for_completion_timeout(&priv->comp_stall[channel],
> +					    2 * STARFIVE_FAN_TACH_TIMEOUT_JIFFIES);

Should this wait on priv->comp_slow[channel] instead? Waiting on comp_stall
after reinitializing comp_slow guarantees a timeout, as a valid slow alarm
interrupt will never wake up the stall completion.

> +		*val = priv->fan_slow[channel];
> +		starfive_fan_tach_ch_slow_unmask(priv, channel, false);
> +		break;

[ ... ]

> +static void starfive_fan_tach_present(struct starfive_fan_tach_data *priv, u8 *tach_ch,
> +				      int count)
> +{
> +	u8 ch, index;
> +
> +	for (index = 0; index < count; index++) {
> +		ch = tach_ch[index];
> +		priv->tach_present[ch] = true;

Are we checking for out-of-bounds array access here? Since the arrays
have a fixed size of 16 (STARFIVE_FAN_TACH_CH), a device tree providing
a channel number >= 16 will write outside the bounds of tach_present,
comp_stall, and comp_slow.

[ ... ]

> +static irqreturn_t starfive_fan_tach_irq(int irq, void *dev_id)
> +{
> +	struct starfive_fan_tach_data *priv = dev_id;
> +	u32 fan_status;
> +	int i;
> +
> +	fan_status = readl(priv->regs + STARFIVE_FAN_TACH_STATUS);
> +
> +	for (i = 0; i < STARFIVE_FAN_TACH_CH; i++) {
> +		if (fan_status & STARFIVE_FAN_TACH_STALL_INT(i)) {
> +			priv->fan_stall[i] = 1;
> +			complete(&priv->comp_stall[i]);

Can this code process uninitialized completions for unused channels? If the
hardware asserts a bit for an unconfigured channel, calling complete() on an
uninitialized waitqueue lock can result in a kernel panic. Verifying
priv->tach_present[i] before processing would prevent this.

> +		}
> +
> +		if (fan_status & STARFIVE_FAN_TACH_SLOW_INT(i)) {
> +			priv->fan_slow[i] = 1;
> +			complete(&priv->comp_slow[i]);
> +		}
> +	}

[ ... ]

> +static int starfive_fan_tach_probe(struct platform_device *pdev)
> +{
> +	struct device *hwmon_dev, *dev = &pdev->dev;
> +	struct starfive_fan_tach_data *priv;
> +	struct reset_control *rst;
> +	struct clk *clk;
> +	int irq;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +
> +	priv->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->regs))
> +		return dev_err_probe(dev, PTR_ERR(priv->regs),
> +				     "Unable to map IO resources\n");
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return dev_err_probe(dev, irq, "Failed to get IRQ\n");
> +
> +	ret = devm_request_irq(dev, irq, starfive_fan_tach_irq, 0, pdev->name,
> +			       (void *)priv);

Does requesting the IRQ here before the clock is enabled and the reset is
deasserted risk fatal bus faults? If an interrupt is pending or fires
spuriously, starfive_fan_tach_irq() will execute and attempt to read the
peripheral's status register without an enabled clock.

> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to register interrupt handler\n");
> +
> +	rst = devm_reset_control_get_exclusive(dev, NULL);
> +	if (IS_ERR(rst))
> +		return dev_err_probe(dev, PTR_ERR(rst),
> +				     "Unable to get fan tach's reset\n");
> +
> +	clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk),
> +				     "Unable to get & enable fan tach's clock\n");
> +
> +	priv->clk_rate = clk_get_rate(clk);
> +	if (priv->clk_rate <= 0)
> +		return dev_err_probe(dev, priv->clk_rate,
> +				     "Unable to get clock's rate\n");

Is it intentional to return 0 on this error path? Passing 0 to dev_err_probe()
and returning 0 bypasses hwmon registration while leaving the device entirely
non-functional and leaking the enabled clock and active IRQ.

> +
> +	reset_control_deassert(rst);
> +	ret = devm_add_action_or_reset(dev, starfive_fan_tach_reset_control_assert, rst);
> +	if (ret)
> +		return ret;
> +
> +	writel(STARFIVE_FAN_TACH_STALL_INT_MASK | STARFIVE_FAN_TACH_SLOW_INT_MASK,
> +	       priv->regs + STARFIVE_FAN_TACH_STATUS);
> +	writel(priv->clk_rate / STARFIVE_FAN_DEFAULT_MEASURE_RATIO,
> +	       priv->regs + STARFIVE_FAN_TACH_MEASURE_TIME);
> +
> +	for_each_child_of_node_scoped(dev->of_node, child) {
> +		ret = starfive_fan_tach_create_fan(dev, child, priv);
> +		if (ret < 0) {
> +			dev_warn(dev, "Failed to create fan %d", ret);
> +			return 0;

Could returning 0 here mistakenly signal success during a probe failure? This
would also skip devm_hwmon_device_register_with_info() and leave resources
active but non-functional.

> +		}
> +	}

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