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

[Seung-Woo Kim]

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.

Best Regards,
- Seung-Woo Kim

> 
> This looks weird and the fix does not really remove the weirdness out of it.
> 
> Best regards,
> Krzysztof
> 
> 
> 

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