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

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

[Seung-Woo Kim]

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@samsung.com>
---
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;
+	}
 
 	pwm_samsung_set_divisor(chip, chan, BIT(div));
 
-- 
1.7.9.5

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

[Joonyoung Shim]

Hi,

On 08/02/2016 07: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@samsung.com>
> ---
> 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;

It's reasonable to me not to check to decide div at above case.

> +	} else {
> +		for (div = variant->div_base; div < 4; ++div)
> +			if ((rate >> (variant->bits + div)) < freq)
> +				break;
> +	}
>  

Reviewed-by: Joonyoung Shim <jy0922.shim@samsung.com>

Thanks.

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

[Krzysztof Kozlowski]

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@samsung.com>
> ---
> 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.

This looks weird and the fix does not really remove the weirdness out of it.

Best regards,
Krzysztof

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@samsung.com>
>> ---
>> 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
--

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@samsung.com>:
> 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@samsung.com>
>>> ---
>>> 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

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

[Krzysztof Kozlowski]

On 08/16/2016 11:00 AM, Tomasz Figa wrote:
> 2016-08-16 17:25 GMT+09:00 Seung-Woo Kim <sw0312.kim@samsung.com>:
>> 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@samsung.com>
>>>> ---
>>>> 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).

Best regards,
Krzysztof

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

[Tomasz Figa]

2016-08-16 18:10 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> On 08/16/2016 11:00 AM, Tomasz Figa wrote:
>> 2016-08-16 17:25 GMT+09:00 Seung-Woo Kim <sw0312.kim@samsung.com>:
>>> 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@samsung.com>
>>>>> ---
>>>>> 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) {

Technically this still doesn't guarantee safe behavior, but in
practice (24xx has bits == 16) should be fine, assuming that it stops
UBSAN from grumbling.

>         /* Only for s3c24xx */
>         // the for loop as it was
> } else {
>         /* For other variants just choose lowest divider always */

/*
 * Other variants have enough counter bits to generate any requested rate,
 * so no need to check higher divisors.
 */

>         div = variant->div_base;
> }
>
> For me this is quite obvious and error-prone (explicit check for value
> to be used in shift).

I guess you meant error-proof. :) Sounds good to me.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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@samsung.com>:
>>> 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@samsung.com>
>>>>> ---
>>>>> 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
--

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

[Seung-Woo Kim]

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 except s3c24xx, 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)
   [...]

Cc: Tomasz Figa <tomasz.figa@gmail.com>
Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
---
Changes from v1
 * Remove too complex condition, instead, just checking simple variant->bits as Krzysztof's suggestion
 * Add comment from Tomasz

The UBSAN warning from ARM is reported with the patch in following link:
https://patchwork.kernel.org/patch/9189575/
---
 drivers/pwm/pwm-samsung.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
index ada2d32..f113cda 100644
--- a/drivers/pwm/pwm-samsung.c
+++ b/drivers/pwm/pwm-samsung.c
@@ -193,9 +193,18 @@ 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 (variant->bits < 32) {
+		/* Only for s3c24xx */
+		for (div = variant->div_base; div < 4; ++div)
+			if ((rate >> (variant->bits + div)) < freq)
+				break;
+	} else {
+		/*
+		 * Other variants have enough counter bits to generate any
+		 * requested rate, so no need to check higher divisors.
+		 */
+		div = variant->div_base;
+	}
 
 	pwm_samsung_set_divisor(chip, chan, BIT(div));
 
-- 
2.9.2

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

[Krzysztof Kozlowski]

On Tue, Aug 16, 2016 at 11:22:01PM +0900, 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 except s3c24xx, 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)
>    [...]
> 
> Cc: Tomasz Figa <tomasz.figa@gmail.com>
> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> ---
> Changes from v1
>  * Remove too complex condition, instead, just checking simple variant->bits as Krzysztof's suggestion
>  * Add comment from Tomasz
> 
> The UBSAN warning from ARM is reported with the patch in following link:
> https://patchwork.kernel.org/patch/9189575/
> ---
>  drivers/pwm/pwm-samsung.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)


Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]

On Tue, Aug 16, 2016 at 11:22:01PM +0900, 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 except s3c24xx, 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)
>    [...]
> 
> Cc: Tomasz Figa <tomasz.figa@gmail.com>
> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> ---
> Changes from v1
>  * Remove too complex condition, instead, just checking simple variant->bits as Krzysztof's suggestion
>  * Add comment from Tomasz
> 
> The UBSAN warning from ARM is reported with the patch in following link:
> https://patchwork.kernel.org/patch/9189575/
> ---
>  drivers/pwm/pwm-samsung.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]