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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing explicit initialization of interrupt and channel enable registers during probe
--

--- Patch [2]: [PATCH v3 2/2] hwmon: (starfive-fan-tach) Add fan tach driver for StarFive JHB100 ---
commit ea266a681c03cda9faa04f08b171c20f1e003841
Author: Changhuang Liang <changhuang.liang@starfivetech.com>

hwmon: (starfive-fan-tach) Add fan tach driver for StarFive JHB100

This commit adds a new driver for the fan tachometer controller found on the
StarFive JHB100 SoC. It provides support for up to 16 independent fan
tachometer inputs and reports fan speed in RPM 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..7f56ecc87c1c8
> --- /dev/null
> +++ b/drivers/hwmon/starfive-fan-tach.c
[ ... ]
> +static int starfive_fan_tach_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = reset_control_deassert(priv->rst);
> +	if (ret) {
> +		clk_disable_unprepare(priv->clk);
> +		return dev_err_probe(dev, ret, "Failed to deassert reset\n");
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, starfive_fan_tach_disable, priv);
> +	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);

[Severity: Medium]
Could this code be susceptible to an interrupt storm if the bootloader leaves
the FAN-TACH controller enabled with unmasked interrupts?

Since the driver only deasserts the reset line during probe (and does not
assert it first), the hardware state might be retained from before the kernel
booted. Any read-modify-write operations on STARFIVE_FAN_TACH_INT_EN and
STARFIVE_FAN_TACH_CH_EN later in the driver would then preserve these stray
bits.

Once devm_request_irq() is called, any unexpected unmasked interrupts will
trigger. If the interrupt condition is continuous (e.g., a disconnected or
stalled fan), the ISR will clear the STATUS bit, but the hardware will
immediately re-assert it. Because the driver's ISR does not mask unexpected
interrupts in INT_EN (it only signals completions if armed_stall or
armed_slow are set), wouldn't this lead to an interrupt storm?

Should the interrupt and channel enable registers be explicitly initialized
to 0 before requesting the IRQ to ensure a known safe hardware state?

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