From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753058AbcHPHhN (ORCPT ); Tue, 16 Aug 2016 03:37:13 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:16953 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751226AbcHPHhL (ORCPT ); Tue, 16 Aug 2016 03:37:11 -0400 X-AuditID: cbfec7f5-f792a6d000001302-ae-57b2c2a4bfb3 Subject: Re: [PATCH] pwm: samsung: fix to use lowest div for large enough modulation bits To: Seung-Woo Kim , linux-samsung-soc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, thierry.reding@gmail.com, linux-pwm@vger.kernel.org References: <1470133006-4272-1-git-send-email-sw0312.kim@samsung.com> Cc: jy0922.shim@samsung.com, Tomasz Figa From: Krzysztof Kozlowski Message-id: <329cb013-4832-c443-d593-327531db7b40@samsung.com> Date: Tue, 16 Aug 2016 09:37:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-version: 1.0 In-reply-to: <1470133006-4272-1-git-send-email-sw0312.kim@samsung.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrLLMWRmVeSWpSXmKPExsVy+t/xK7pLDm0KN3j1k9Xixb2LLBavXxha bHp8jdXi8q45bBZ3765itJhxfh+TxYzJL9ksfu6ax2KxatcfRgdOj52z7rJ7bF5S79G3ZRWj x+dNcgEsUVw2Kak5mWWpRfp2CVwZaxc+Yiq4KlyxdvsplgbGNoEuRk4OCQETiY9trewQtpjE hXvr2boYuTiEBJYyStz6uYMFwnnGKHHlxRewKmGBaIkrv+aDVYkInGKUeHp8BTNIQkjAVeLZ 7dWsIDazgIPEqemnwOJsAsYSm5cvYQOxeQXsJFr/bWbqYuTgYBFQleh+FgASFhWIkLi16iMj RImgxI/J91hAbE4BN4lHK/6zgpQzC+hJ3L+oBTFdXmLzmrfMExgFZiHpmIVQNQtJ1QJG5lWM oqmlyQXFSem5RnrFibnFpXnpesn5uZsYIYH+dQfj0mNWhxgFOBiVeHhPMGwKF2JNLCuuzD3E KMHBrCTC+/gAUIg3JbGyKrUoP76oNCe1+BCjNAeLkjjvzF3vQ4QE0hNLUrNTUwtSi2CyTByc Ug2M3nZnzBu391/4sezhZ9aUR7YnH+Zecps3db3YNLkvErbc1zLe77mp6H3KYmbFjh39viU7 9m/VN72lrB2z1FI8Nj8m7bKHybS5mzfaTOzqiJNWq6tyWtu4dFZxjslDueOaYhfNbWdniq4q FK/JXZ26WVz8ygrFwg/6U4VlqtaU7Jc2sX2ibJimxFKckWioxVxUnAgArejNx3ACAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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' > [...] > [] (ubsan_epilogue) from [] (__ubsan_handle_shift_out_of_bounds+0xd8/0x120) > [] (__ubsan_handle_shift_out_of_bounds) from [] (pwm_samsung_config+0x508/0x6a4) > [] (pwm_samsung_config) from [] (pwm_apply_state+0x174/0x40c) > [] (pwm_apply_state) from [] (pwm_fan_probe+0xc8/0x488) > [] (pwm_fan_probe) from [] (platform_drv_probe+0x70/0x150) > [...] > > Signed-off-by: Seung-Woo Kim > --- > 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