* Re: [PATCH 1/1] ARM: dts: aspeed: g6: Add AST2600 pwm tacho controller
2026-06-12 7:23 [PATCH 1/1] ARM: dts: aspeed: g6: Add AST2600 pwm tacho controller Grégoire Layet
@ 2026-06-12 7:32 ` Andrew Jeffery
2026-06-12 7:43 ` Grégoire Layet
2026-06-12 7:35 ` sashiko-bot
1 sibling, 1 reply; 4+ messages in thread
From: Andrew Jeffery @ 2026-06-12 7:32 UTC (permalink / raw)
To: Grégoire Layet, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley
Cc: devicetree, linux-arm-kernel, linux-aspeed, linux-kernel
Hi Grégoire,
On Fri, 2026-06-12 at 07:23 +0000, Grégoire Layet wrote:
> It use the existing ast2600-pwm-tach driver.
> Placed according to bus adresses ordering.
>
> Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
> ---
> arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> index 189bc3bbb47c..818d486b94ac 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";
> + 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>;
Thanks for the patch, however:
https://git.kernel.org/pub/scm/linux/kernel/git/bmc/linux.git/commit/?h=aspeed/arm/dt&id=6cf976b2728f2494215c51c7339dd50b154125ce
You can also find the commit in linux-next.git master and soc.git
soc/dt, queued for v7.2.
Andrew
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH 1/1] ARM: dts: aspeed: g6: Add AST2600 pwm tacho controller
2026-06-12 7:23 [PATCH 1/1] ARM: dts: aspeed: g6: Add AST2600 pwm tacho controller Grégoire Layet
2026-06-12 7:32 ` Andrew Jeffery
@ 2026-06-12 7:35 ` sashiko-bot
1 sibling, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-06-12 7:35 UTC (permalink / raw)
To: Grégoire Layet; +Cc: devicetree, robh, conor+dt
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
^ permalink raw reply [flat|nested] 4+ messages in thread