Re: [PATCH v2 1/7] hwmon: (axi-fan-control) Remove redundant dev_err_probe()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Widespread missing 64-bit casts and unsigned logic errors across sysfs and interrupt callbacks lead to arithmetic overflows on 32-bit architectures, emitting invalid hardware configurations and garbage telemetry data.
- [High] The `fan_fault` state is concurrently accessed by the interrupt handler thread and sysfs reads without locking, directly violating the hwmon subsystem synchronization guidelines.
- [High] The hardware is left active, out of reset, and with unmasked hardware interrupts upon probe failure or module unload.
--

--- Patch [1]: [PATCH v2 1/7] hwmon: (axi-fan-control) Remove redundant dev_err_probe() ---
commit df8b06ee0f4291234f7eb4185e5e9fddf05147c5
Author: Pan Chuang <panchuang@vivo.com>

hwmon: (axi-fan-control) Remove redundant dev_err_probe()

Since commit 55b48e23f5c4 ("genirq/devres: Add error handling in
devm_request_*_irq()"), devm_request_threaded_irq() automatically logs
detailed error messages on failure. Remove the now-redundant
driver-specific dev_err() calls.

> --- a/drivers/hwmon/axi-fan-control.c
> +++ b/drivers/hwmon/axi-fan-control.c
> @@ -508,8 +508,7 @@ static int axi_fan_control_probe(struct platform_device *pdev)
>  					IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
>  					NULL, ctl);
>  	if (ret)
> -		return dev_err_probe(&pdev->dev, ret,
> -				     "failed to request an irq\n");
> +		return ret;

[Severity: High]
This is a pre-existing issue, but if devm_request_threaded_irq() fails here,
does the hardware remain active and out of reset?

axi_fan_control_init() writes to ADI_REG_IRQ_MASK to enable interrupts and
ADI_REG_RSTN to bring the hardware out of reset. If the probe fails at this
point, there doesn't seem to be any cleanup to quiesce the hardware.

There also doesn't appear to be a .remove callback to handle module unload.

[Severity: High]
This isn't a bug introduced by this patch, but do these calculations overflow
on 32-bit architectures?

drivers/hwmon/axi-fan-control.c:axi_fan_control_get_fan_rpm() {
    ...
    return DIV_ROUND_CLOSEST(60 * ctl->clk_rate, ctl->ppr * tach);
    ...
}

For standard clock rates like 100MHz, 60 * ctl->clk_rate would evaluate to
6,000,000,000, which unconditionally overflows a 32-bit unsigned integer.

Similarly, in axi_fan_control_read_temp():

drivers/hwmon/axi-fan-control.c:axi_fan_control_read_temp() {
    ...
    *val = ((raw_temp * 501374) >> 16) - 273677;
    ...
}

For 16-bit ADC values, raw_temp * 501374 exceeds 32-bit signed limits.

Additionally, in axi_fan_control_show(), if the calculated value is negative,
does the subtraction of 280230 underflow into a large positive number since it
is stored in a u32?

[Severity: High]
This is a pre-existing issue, but is there a missing lock around the fan_fault
state?

drivers/hwmon/axi-fan-control.c:axi_fan_control_read_fan() {
    ...
    *val = ctl->fan_fault;
    /* clear it now */
    ctl->fan_fault = 0;
    ...
}

If the hardware triggers an ADI_IRQ_SRC_TACH_ERR interrupt right between the
read and the clear, will the interrupt handler's update be lost?

drivers/hwmon/axi-fan-control.c:axi_fan_control_irq_handler() {
    ...
    if (irq_pending & ADI_IRQ_SRC_TACH_ERR)
        ctl->fan_fault = 1;
    ...
}

It seems this state is accessed concurrently by the interrupt handler thread
and sysfs reads without locking, which might lead to dropped fault indicators.

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720063745.309331-1-panchuang@vivo.com?part=1