Re: [PATCH] pwm: samsung: fix to use lowest div for large enough modulation bits

[Seung-Woo Kim]

Hello Krzysztof,

On 2016ë 08ì 16ì 18:10, Krzysztof Kozlowski wrote:
> On 08/16/2016 11:00 AM, Tomasz Figa wrote:
>> 2016-08-16 17:25 GMT+09:00 Seung-Woo Kim <sw0312.kim@xxxxxxxxxxx>:
>>> Hi Krzysztof,
>>>
>>> On 2016ë 08ì 16ì 16:37, Krzysztof Kozlowski wrote:
>>>> On 08/02/2016 12:16 PM, Seung-Woo Kim wrote:
>>>>> >From pwm_samsung_calc_tin(), there is routine to find the lowest
>>>>> divider possible to generate lower frequency than requested one.
>>>>> But it is always possible to generate requested frequency with
>>>>> large enough modulation bits, so this patch fixes to use lowest
>>>>> div for the case. This patch removes following UBSAN warning:
>>>>>
>>>>>    UBSAN: Undefined behaviour in drivers/pwm/pwm-samsung.c:197:13
>>>>>    shift exponent 32 is too large for 32-bit type 'long unsigned int'
>>>>>    [...]
>>>>>    [<c0670248>] (ubsan_epilogue) from [<c06707b4>] (__ubsan_handle_shift_out_of_bounds+0xd8/0x120)
>>>>>    [<c06707b4>] (__ubsan_handle_shift_out_of_bounds) from [<c0694b28>] (pwm_samsung_config+0x508/0x6a4)
>>>>>    [<c0694b28>] (pwm_samsung_config) from [<c069286c>] (pwm_apply_state+0x174/0x40c)
>>>>>    [<c069286c>] (pwm_apply_state) from [<c0b2e070>] (pwm_fan_probe+0xc8/0x488)
>>>>>    [<c0b2e070>] (pwm_fan_probe) from [<c07ba8b0>] (platform_drv_probe+0x70/0x150)
>>>>>    [...]
>>>>>
>>>>> Signed-off-by: Seung-Woo Kim <sw0312.kim@xxxxxxxxxxx>
>>>>> ---
>>>>> The UBSAN warning from ARM is reported with the patch in following link:
>>>>> https://patchwork.kernel.org/patch/9189575/
>>>>> ---
>>>>>  drivers/pwm/pwm-samsung.c |   10 +++++++---
>>>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
>>>>> index ada2d32..ff0def6 100644
>>>>> --- a/drivers/pwm/pwm-samsung.c
>>>>> +++ b/drivers/pwm/pwm-samsung.c
>>>>> @@ -193,9 +193,13 @@ static unsigned long pwm_samsung_calc_tin(struct samsung_pwm_chip *chip,
>>>>>       * divider settings and choose the lowest divisor that can generate
>>>>>       * frequencies lower than requested.
>>>>>       */
>>>>> -    for (div = variant->div_base; div < 4; ++div)
>>>>> -            if ((rate >> (variant->bits + div)) < freq)
>>>>> -                    break;
>>>>> +    if (fls(rate) <= variant->bits) {
>>>>> +            div = variant->div_base;
>>>>> +    } else {
>>>>> +            for (div = variant->div_base; div < 4; ++div)
>>>>> +                    if ((rate >> (variant->bits + div)) < freq)
>>>>> +                            break;
>>>>> +    }
>>>>
>>>> I have trouble with understanding the idea behind initial code from
>>>> Tomasz (commit 11ad39ede24ee). The variant->bits for all SoC except
>>>> S3C24xx is 32. This means the shift:
>>>>       if ((rate >> (variant->bits + div)) < freq)
>>>> will be always by 32 or more... In practice this will choose always a
>>>> "div" of 0 because in first iteration of this loop, the shift will be by 32.
>>>
>>> I also confused that part, but I figured out that the bit is used to
>>> consider modulation bit to generate pwm signal from the input clock.
>>>
>>> Only the old s3c2440 has 16 bit modulation timer for pwm, and all later
>>> soc has 32 bit modulation timer. So 32 bit timer cases, with the lowest
>>> div, it can generate all frequencies which can be assigned with 32bit
>>> variable.
>>> But I uses fls() to consider 64bit case also even though there is no
>>> really that kind of clock.
>>
>> The code may look complicated (in fact I had to think a bit to recall
>> what exactly it was supposed to do), but I'm not sure how it could be
>> simplified. It's generally intended to handle variant->bits < 32 cases
>> only and is effectively a no-op when variant->bits >= 32.
> 
> Right, a comment for this behavior would be very useful. No need to
> waste time for re-thinking it later.
> 
>> I would suggest just making rate an u64 and be done with the warning.
>> IMHO adding this kind of special cases only complicates the (already
>> complicated) code unnecessarily.
> 
> u64 could solve the warning but then one would have to figure out
> whether the casts are safe or not. Unsigned long is assigned to rate and
> then returned.
> 
> How about specific check (+comment) like:
> if (variant->bits < 32) {
> 	/* Only for s3c24xx */
> 	// the for loop as it was
> } else {
> 	/* For other variants just choose lowest divider always */
> 	div = variant->div_base;
> }
> 
> For me this is quite obvious and error-prone (explicit check for value
> to be used in shift).

Actually above was my internal first version and it also removes UBSAN
warning. I will send as you suggested with Tomasz's comment.

Thanks,
- Seung-Woo Kim

> 
> Best regards,
> Krzysztof
> 
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--