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

[Tomasz Figa]

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.

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.

Best regards,
Tomasz