2016-08-16 18:10 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>: > 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) { 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. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |