Re: [PATCH] pwm: xilinx: Fix overflow issue in 32-bit width PWM mode.

[Kenneth Sloat]

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@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

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