From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753412AbcHPJyv (ORCPT ); Tue, 16 Aug 2016 05:54:51 -0400 Received: from mailout2.samsung.com ([203.254.224.25]:45466 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751562AbcHPJyt (ORCPT ); Tue, 16 Aug 2016 05:54:49 -0400 MIME-version: 1.0 Content-type: text/plain; charset=UTF-8 X-AuditID: cbfee68e-f79cb6d000006cfe-50-57b2e2e6fb32 Content-transfer-encoding: 8BIT Message-id: <57B2E2F1.1020205@samsung.com> Date: Tue, 16 Aug 2016 18:54:57 +0900 From: Seung-Woo Kim Reply-to: sw0312.kim@samsung.com User-Agent: Mozilla/5.0 (X11; Linux i686; rv:16.0) Gecko/20121011 Thunderbird/16.0.1 To: Krzysztof Kozlowski Cc: Tomasz Figa , "linux-samsung-soc@vger.kernel.org" , linux-arm-kernel , linux-kernel , Thierry Reding , linux-pwm@vger.kernel.org, Joonyoung Shim Subject: Re: [PATCH] pwm: samsung: fix to use lowest div for large enough modulation bits References: <1470133006-4272-1-git-send-email-sw0312.kim@samsung.com> <329cb013-4832-c443-d593-327531db7b40@samsung.com> <57B2CDF0.8020809@samsung.com> <369cde8b-f761-4a0b-50b9-a3a01ba0b75d@samsung.com> In-reply-to: <369cde8b-f761-4a0b-50b9-a3a01ba0b75d@samsung.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrLIsWRmVeSWpSXmKPExsWyRsSkQPfZo03hBtc3slq8uHeRxeL1C0OL TY+vsVpc3jWHzeLu3VWMFjPO72Oy+LlrHovFql1/GB04PHbOusvusXlJvUffllWMHp83yQWw RHHZpKTmZJalFunbJXBlvPu4lL1gmlrFvR3zWRoYp8p1MXJySAiYSEz4P5EJwhaTuHBvPVsX IxeHkMAKRon2d31sMEWH1k1igUjMYpTYt/oTWAevgKDEj8n3gBIcHMwC8hJHLmWDhJkF1CUm zVvEDFH/gFHi/41TrBD1WhL79u1gBrFZBFQlnv3bDbaATUBHYv+S32A1QgIKElcmHmMHmSkq ECaxc3M6SFhEwFDi4O7tTCAzmQUuMEls3gVxnLBAtMSVX/Ohrn7NJDGnYQ3YAk4Be4ktp16C XS0h8Ihd4s3Wz2wQmwUkvk0+BHa1hICsxKYDzBBfSkocXHGDZQKj+Cwkv81C+G0Wkt8WMDKv YhRNLUguKE5KLzLSK07MLS7NS9dLzs/dxAiMxdP/nvXtYLx5wPoQowAHoxIPbwPPpnAh1sSy 4srcQ4ymQEdMZJYSTc4HRnxeSbyhsZmRhamJqbGRuaWZkjhvgtTPYCGB9MSS1OzU1ILUovii 0pzU4kOMTBycUg2MsUVJbidqbafMf+JsErdrxVYvHe9fy5bNu/d91vZ08RXFVy/e/5D5jO3b iRJNDwWV142ptX0htocfrFg1v0pUpVUqYcOHqxILbywI8Ch9/CGx5rKTW2ZW6WKOjDapW7x1 MktjXzVFsUsqBbNu2ljB3nl61bmyfYban/M5T0T6fZ36pLeaPeitEktxRqKhFnNRcSIAADsu fcACAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrFIsWRmVeSWpSXmKPExsVy+t9jQd2njzaFG+zaa23x4t5FFovXLwwt Nj2+xmpxedccNou7d1cxWsw4v4/J4ueueSwWq3b9YXTg8Ng56y67x+Yl9R59W1YxenzeJBfA EtXAaJORmpiSWqSQmpecn5KZl26r5B0c7xxvamZgqGtoaWGupJCXmJtqq+TiE6DrlpkDdIWS QlliTilQKCCxuFhJ3w7ThNAQN10LmMYIXd+QILgeIwM0kLCGMePdx6XsBdPUKu7tmM/SwDhV rouRk0NCwETi0LpJLBC2mMSFe+vZuhi5OIQEZjFK7Fv9iQkkwSsgKPFj8j2gIg4OZgF5iSOX skHCzALqEpPmLWKGqH/AKPH/xilWiHotiX37djCD2CwCqhLP/u1mA7HZBHQk9i/5DVYjJKAg cWXiMXaQmaICYRI7N6eDhEUEDCUO7t7OBDKTWeACk8TmXX1gvcIC0RJXfs2HOu41k8SchjVg CzgF7CW2nHrJMoFRcBaSW2ch3DoLya0LGJlXMUqkFiQXFCel5xrlpZbrFSfmFpfmpesl5+du YgTH+zPpHYyHd7kfYhTgYFTi4T3BsClciDWxrLgy9xCjBAezkgiv2l2gEG9KYmVValF+fFFp TmrxIUZToGcnMkuJJucDU1FeSbyhsYmZkaWRuaGFkbG5kjjv4//rwoQE0hNLUrNTUwtSi2D6 mDg4pRoYt5oWvr55O3uam3V+9IPGmieH2zgUDvn2LSsQLbFwm57kekzR+UPY32kXvD6d+cd1 VsfI7ThPPb+O2up1kvmvSg0Ldguf7mS8cGgClzPHvEOWJ43e37zAnbtV43Bc/P/rJ1bxPapt fMy5fcnlCYa6OabqbTvkb3HZ/K69rrV95RL3E18vSHxLV2Ipzkg01GIuKk4EAKdfnsENAwAA DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 : >>> 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' >>>>> [...] >>>>> [] (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. >>> >>> 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 --