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

[Guenter Roeck <linux@roeck-us.net>]

On 8/23/26 20:27, sashiko-bot@kernel.org wrote:
> 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?
> 
Please either implement as suggested or explain why it is not needed.
In that case, please add a comment into the code explaining the reason.

Thanks,
Guenter