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 -- 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
 |