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

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

[Kenneth Sloat]

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+.

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

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

[Michal Simek]

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.


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

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

[Kenneth Sloat]

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);
	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);
>> 
>> 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?

Thanks

Sincerely,
Ken Sloat

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

[Michal Simek]

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

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