From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753258AbcHPIZq (ORCPT ); Tue, 16 Aug 2016 04:25:46 -0400 Received: from mailout2.samsung.com ([203.254.224.25]:45873 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751057AbcHPIZm (ORCPT ); Tue, 16 Aug 2016 04:25:42 -0400 MIME-version: 1.0 Content-type: text/plain; charset=UTF-8 X-AuditID: cbfee68d-f79286d000007a9a-a2-57b2cde72518 Content-transfer-encoding: 8BIT Message-id: <57B2CDF0.8020809@samsung.com> Date: Tue, 16 Aug 2016 17:25:20 +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: 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, jy0922.shim@samsung.com, Tomasz Figa 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> In-reply-to: <329cb013-4832-c443-d593-327531db7b40@samsung.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrNIsWRmVeSWpSXmKPExsWyRsSkQPf52U3hBkvPc1q8uHeRxeL1C0OL TY+vsVpc3jWHzeLu3VWMFjPO72Oy+LlrHovFql1/GB04PHbOusvusXlJvUffllWMHp83yQWw RHHZpKTmZJalFunbJXBlPLg4ibFgvmTFjsnXGRsYN4l0MXJySAiYSHT8/8cKYYtJXLi3nq2L kYtDSGAFo8Tf7z+ZYIpOX77AApFYyiix+OxfsA5eAUGJH5PvASU4OJgF5CWOXMoGCTMLqEtM mreIGcQWEnjAKLF9YQpEuZbEqk17wWayCKhKXJ/RyQhiswnoSOxf8psVol5B4srEY+wgI0UF wiR2bk4HCYsIGEoc3L2dCeQEZoFHjBIPN1wBmy8sEC1x5dd8qKPPMUos/beOGaSZU8Be4ugf I5C4hMAjdonGj3/ZIRYLSHybfAjsZgkBWYlNB5ghfpSUOLjiBssERvFZSD6bhfDZLCSfLWBk XsUomlqQXFCclF5kqFecmFtcmpeul5yfu4kRGIen/z3r3cF4+4D1IUYBDkYlHt4XmzaFC7Em lhVX5h5iNAU6YiKzlGhyPjDa80riDY3NjCxMTUyNjcwtzZTEeRWlfgYLCaQnlqRmp6YWpBbF F5XmpBYfYmTi4JRqYPSccs4xTe3o8U08bC4ufBN+zli/e1ZoqeVrprJJ+bpHFDwmHTAQ9Xw+ qXul1PFTHQwTFkdqrH68Ta6vPPOASG795Lk6flcatW9zKvzfUGdatPai8FfZSZqP7nKJvEuM 7quTMPD39y6snTul9ipjxMLV30OzNVM/BOpUT1sb+Lhl8YJvIo7XJiqxFGckGmoxFxUnAgA/ Wt9pvgIAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrBIsWRmVeSWpSXmKPExsVy+t9jAd1nZzeFG0x6ZGPx4t5FFovXLwwt Nj2+xmpxedccNou7d1cxWsw4v4/J4ueueSwWq3b9YXTg8Ng56y67x+Yl9R59W1YxenzeJBfA EtXAaJORmpiSWqSQmpecn5KZl26r5B0c7xxvamZgqGtoaWGupJCXmJtqq+TiE6DrlpkDdIWS QlliTilQKCCxuFhJ3w7ThNAQN10LmMYIXd+QILgeIwM0kLCGMePBxUmMBfMlK3ZMvs7YwLhJ pIuRk0NCwETi9OULLBC2mMSFe+vZuhi5OIQEljJKLD77lxUkwSsgKPFj8j2gIg4OZgF5iSOX skHCzALqEpPmLWIGsYUEHjBKbF+YAlGuJbFq014mEJtFQFXi+oxORhCbTUBHYv+S36wQ9QoS VyYeYwcZKSoQJrFzczpIWETAUOLg7u1MICcwCzxilHi44QrYfGGBaIkrv+ZD3XaOUWLpv3XM IM2cAvYSR/8YTWAUnIXk0lkIl85CcukCRuZVjBKpBckFxUnpuYZ5qeV6xYm5xaV56XrJ+bmb GMGx/kxqB+PBXe6HGAU4GJV4eE8wbAoXYk0sK67MPcQowcGsJMLrewooxJuSWFmVWpQfX1Sa k1p8iNEU6NeJzFKiyfnANJRXEm9obGJmZGlkbmhhZGyuJM77+P+6MCGB9MSS1OzU1ILUIpg+ Jg5OqQZGyy9+G7las943rL65ZufEy0efTgvpenkm8Z+bSEzftXI1Pw1WHkHDHIPgqJXKEd+j FkStuTefcX/5n5ttL3Z7Ocekfd6gFlV8V9b54kbTS6JndL92mCzz7Xlg728lw2r6Xqp0m37h uuM+zp3Neo/bpk772fApeZ6Jo/C8QJ8Yk5cz9Ll/zTmixFKckWioxVxUnAgAJihh8AsDAAA= 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 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. 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 --