Re: [PATCH] pwm: xilinx: Fix overflow issue in 32-bit width PWM mode.
From: Michal Simek
Date: Thu Dec 15 2022 - 08:59:15 EST
On 12/15/22 14:43, Kenneth Sloat wrote:
Hi Michal thanks for your reply.
On 12/12/22 14:59, Kenneth Sloat wrote:
This timer HW supports 8, 16 and 32-bit timer widths. This
driver uses a u32 to store the max value of the timer.
Because addition is done to this max value, when operating
in 32-bit mode, this will result in overflow that makes it
impossible to set the timer period and thus the PWM itself.
To fix this, simply make max a u64. This was tested on a
Zynq UltraScale+.
Can you please be more accurate where that overflow is happening.
I see that value is set only at probe like
priv->max = BIT_ULL(width) - 1;
No doubt that there are calculation based on u64 types.
It actually does not happen in probe but when applying the PWM settings, here:
period_cycles = min_t(u64, period_cycles, priv->max + 2);
ok. It means (u64)priv->max + 2
will solve the problem too.
if (period_cycles < 2)
return -ERANGE;
If the timer is 32 bit, priv->max + 2 will roll over to 1, and thus will always be rejected as out of range. So, likely at minimum, a cast on priv->max would be needed here first.
duty_cycles would also have the same issue:
duty_cycles = min_t(u64, duty_cycles, priv->max + 2);
and here as well.
Signed-off-by: Ken Sloat <ksloat@xxxxxxxxxxxxxxxx>
---
include/clocksource/timer-xilinx.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/clocksource/timer-xilinx.h b/include/clocksource/timer-xilinx.h
index c0f56fe6d22a..d116f18de899 100644
--- a/include/clocksource/timer-xilinx.h
+++ b/include/clocksource/timer-xilinx.h
@@ -41,7 +41,7 @@ struct regmap;
struct xilinx_timer_priv {
struct regmap *map;
struct clk *clk;
- u32 max;
+ u64 max;
};
/**
--
2.17.1
Thanks,
Michal
Are you are good with the code change as is? If so, what do you propose? Should I amend the commit message with more details about where the overflow is occurring?
I would update commit message with both cases with simply saying that one way is
to recast priv->max calculation because type is taken from priv->max which is
u32 and one way to fix it is to recast it or change the type.
And that you are using second approach because it is more cleaner.
Thanks,
Michal