Re: [PATCH 1/1] ARM: dts: aspeed: g6: Add AST2600 pwm tacho controller

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The driver's RPM calculation function does not guard against a zero tachometer value, potentially causing a kernel panic via divide-by-zero.
- [High] The driver incorrectly shares a single `tach_divisor` variable across all 16 independent fan channels, leading to incorrect RPM calculations.
- [High] If any child fan node fails to parse, the driver silently aborts initialization but returns success from probe, skipping hwmon registration entirely.
--

commit 557da8ba2550852a62a5729f5acc74635465209e
Author: Grégoire Layet <gregoire.layet@9elements.com>

ARM: dts: aspeed: g6: Add AST2600 pwm tacho controller

This patch adds the device tree node for the AST2600 PWM tachometer
controller, placed according to bus address ordering. It enables the use of
the existing ast2600-pwm-tach driver.

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> index 189bc3bbb47c9..818d486b94ac7 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> @@ -102,6 +102,15 @@ ahbc: bus@1e600000 {
>  			reg = <0x1e600000 0x100>;
>  		};
>  
> +		pwm_tach: pwm-tach-controller@1e610000 {
> +			compatible = "aspeed,ast2600-pwm-tach";

[Severity: Critical]
This isn't a bug introduced by this patch, but since this newly added node
enables the use of the ast2600-pwm-tach driver, could the driver trigger a
divide-by-zero panic?

In drivers/hwmon/aspeed-g6-pwm-tach.c:aspeed_get_fan_tach_ch_rpm(), the
hardware status register is read and its raw value is passed to the RPM
calculation function without verifying it is non-zero:

    val = FIELD_GET(TACH_ASPEED_VALUE_MASK, val);
    return aspeed_tach_val_to_rpm(priv, val);

If the hardware reports 0, tach_div evaluates to 0 in
aspeed_tach_val_to_rpm():

    tach_div = tach_val * priv->tach_divisor * DEFAULT_FAN_PULSE_PR;
    ...
    rpm = (u64)priv->clk_rate * 60;
    do_div(rpm, tach_div);

Since this evaluates to 0 and is directly passed to do_div() without any
checks, will this result in a kernel panic?

[Severity: High]
This is another pre-existing issue in the driver enabled by this node, but
does the driver incorrectly share a single tach_divisor variable across all 16
independent fan channels?

In drivers/hwmon/aspeed-g6-pwm-tach.c:aspeed_tach_hwmon_write(), updating a
channel's divisor blindly overwrites the global state variable:

    priv->tach_divisor = val;

When calculating RPM for any channel in aspeed_tach_val_to_rpm(), it uses this
global divisor instead of retrieving the per-channel hardware configuration:

    tach_div = tach_val * priv->tach_divisor * DEFAULT_FAN_PULSE_PR;

Will this corrupt the RPM calculations for all other fan channels if a new
divisor is written to a specific channel?

[Severity: High]
This is also a pre-existing issue, but does the probe function silently abort
initialization on child node parsing failure?

In drivers/hwmon/aspeed-g6-pwm-tach.c:aspeed_pwm_tach_probe():

    if (ret) {
        dev_warn(dev, "Failed to create fan %d", ret);
        return 0;
    }

This early return with a 0 (success) code inappropriately exits the probe
function, completely skipping crucial device registration steps like
devm_hwmon_device_register_with_info().

Should the driver either propagate the error or continue to skip the malformed
node instead of aborting hwmon registration entirely?

> +			reg = <0x1e610000 0x100>;
> +			clocks = <&syscon ASPEED_CLK_AHB>;
> +			resets = <&syscon ASPEED_RESET_PWM>;
> +			#pwm-cells = <3>;
> +			status = "disabled";
> +		};
> +
>  		fmc: spi@1e620000 {
>  			reg = <0x1e620000 0xc4>, <0x20000000 0x10000000>;
>  			#address-cells = <1>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612072341.278591-1-gregoire.layet@9elements.com?part=1