Messages in this thread |  | | From | Kenneth Sloat <> | Subject | Re: [PATCH] pwm: xilinx: Fix overflow issue in 32-bit width PWM mode. | Date | Thu, 15 Dec 2022 14:51:37 +0000 |
| |
Hi Michal,
> 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. > That is correct
>>>> >>>> Signed-off-by: Ken Sloat <ksloat@designlinxhs.com> >>>> --- >>>> 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
Agreed that changing the type of max is much cleaner and would also avoid any other potential similar math errors in the future. I will update the patch with these details and re-submit. Thanks for your feedback!
Sincerely, Ken Sloat
|  |