From: Nathan Chancellor <nathan@kernel.org> To: Baruch Siach <baruch@tkos.co.il> Cc: "kernel test robot" <lkp@intel.com>, "Thierry Reding" <thierry.reding@gmail.com>, "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>, "Andy Gross" <agross@kernel.org>, "Bjorn Andersson" <bjorn.andersson@linaro.org>, llvm@lists.linux.dev, kbuild-all@lists.01.org, "Balaji Prakash J" <bjagadee@codeaurora.org>, "Rob Herring" <robh+dt@kernel.org>, "Robert Marko" <robert.marko@sartura.hr>, "Kathiravan T" <kathirav@codeaurora.org>, linux-pwm@vger.kernel.org Subject: Re: [PATCH 1/3] pwm: driver for qualcomm ipq6018 pwm block Date: Tue, 8 Feb 2022 11:47:57 -0700 [thread overview] Message-ID: <YgK63cI177ZeF5v1@dev-arch.archlinux-ax161> (raw) In-Reply-To: <87ee4ddejv.fsf@tarshish> Hi Baruch, On Tue, Feb 08, 2022 at 08:51:40AM +0200, Baruch Siach wrote: > Hi test robot, > > Thanks for testing and reporting. > > On Tue, Feb 08 2022, kernel test robot wrote: > > [snip] > > >>> drivers/pwm/pwm-ipq.c:122:11: warning: result of comparison of constant 16000000000 with expression of type 'unsigned long' is always false [-Wtautological-constant-out-of-range-compare] > > if (rate > 16ULL * GIGA) > > ~~~~ ^ ~~~~~~~~~~~~ > > 1 warning generated. > > This clang warning is only enabled with W=1 (see commit > afe956c577b). Not sure how to avoid it. > > Is there a way to express this condition without making clang warn on > platforms where ULONG_MAX == 2^32? Maybe cast to unsigned long long? Or > should we just ignore this W=1 warning? As far as I am aware, casting to unsigned long long would be an appropriate way to fix this warning, as has been done in the following patches in mainline: c9ae8eed4463 ("media: omap3isp: avoid warnings at IS_OUT_OF_BOUNDS()") 4853396f03c3 ("memstick: avoid out-of-range warning") 7ff4034e910f ("staging: vc04_services: shut up out-of-range warning") a2fa9e57a68c ("ARM: mvebu: avoid clang -Wtautological-constant warning") The below diff fixes the warning for me with ARCH=hexagon allyesconfig: diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c index 994027290bcb..7ea29468e76e 100644 --- a/drivers/pwm/pwm-ipq.c +++ b/drivers/pwm/pwm-ipq.c @@ -119,7 +119,7 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, * period_ns is 1G or less. As long as rate is less than 16 GHz, * period_rate does not overflow. Make that explicit. */ - if (rate > 16ULL * GIGA) + if ((unsigned long long)rate > 16ULL * GIGA) return -EINVAL; period_rate = period_ns * rate; best_pre_div = IPQ_PWM_MAX_DIV; Alternatively, you could widen rate to unsigned long long / u64 but I don't know what kind of implications that has in this function but it has been done in other places: 95c58291ee70 ("drm/msm/submit: fix overflow check on 64-bit architectures") cfd6fb45cfaf ("crypto: ccree - avoid out-of-range warnings from clang") 335aea75b0d9 ("drm/amdgpu: fix warning for overflow check") 844b85dda2f5 ("ARM: keystone: fix integer overflow warning") While the warning is currently under W=1, I think it is one that we would like to turn on at some point so fixing instances as they come up helps us get closer to that goal. Cheers, Nathan > > vim +122 drivers/pwm/pwm-ipq.c > > > > 99 > > 100 static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > 101 const struct pwm_state *state) > > 102 { > > 103 struct ipq_pwm_chip *ipq_chip = ipq_pwm_from_chip(chip); > > 104 unsigned int pre_div, pwm_div, best_pre_div, best_pwm_div; > > 105 unsigned long rate = clk_get_rate(ipq_chip->clk); > > 106 u64 period_ns, duty_ns, period_rate; > > 107 u64 min_diff; > > 108 > > 109 if (state->polarity != PWM_POLARITY_NORMAL) > > 110 return -EINVAL; > > 111 > > 112 if (state->period < DIV64_U64_ROUND_UP(NSEC_PER_SEC, rate)) > > 113 return -ERANGE; > > 114 > > 115 period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS); > > 116 duty_ns = min(state->duty_cycle, period_ns); > > 117 > > 118 /* > > 119 * period_ns is 1G or less. As long as rate is less than 16 GHz, > > 120 * period_rate does not overflow. Make that explicit. > > 121 */ > > > 122 if (rate > 16ULL * GIGA) > > 123 return -EINVAL; > > 124 period_rate = period_ns * rate; > > 125 best_pre_div = IPQ_PWM_MAX_DIV; > > 126 best_pwm_div = IPQ_PWM_MAX_DIV; > > 127 /* > > 128 * We don't need to consider pre_div values smaller than > > 129 * > > 130 * period_rate > > 131 * pre_div_min := ------------------------------------ > > 132 * NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1) > > 133 * > > 134 * because pre_div = pre_div_min results in a better > > 135 * approximation. > > 136 */ > > 137 pre_div = div64_u64(period_rate, > > 138 (u64)NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1)); > > 139 min_diff = period_rate; > > 140 > > 141 for (; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) { > > 142 u64 remainder; > > 143 > > 144 pwm_div = div64_u64_rem(period_rate, > > 145 (u64)NSEC_PER_SEC * (pre_div + 1), &remainder); > > 146 /* pwm_div is unsigned; the check below catches underflow */ > > 147 pwm_div--; > > 148 > > 149 /* > > 150 * Swapping values for pre_div and pwm_div produces the same > > 151 * period length. So we can skip all settings with pre_div > > > 152 * pwm_div which results in bigger constraints for selecting > > 153 * the duty_cycle than with the two values swapped. > > 154 */ > > 155 if (pre_div > pwm_div) > > 156 break; > > 157 > > 158 /* > > 159 * Make sure we can do 100% duty cycle where > > 160 * hi_dur == pwm_div + 1 > > 161 */ > > 162 if (pwm_div > IPQ_PWM_MAX_DIV - 1) > > 163 continue; > > 164 > > 165 if (remainder < min_diff) { > > 166 best_pre_div = pre_div; > > 167 best_pwm_div = pwm_div; > > 168 min_diff = remainder; > > 169 > > 170 if (min_diff == 0) /* bingo */ > > 171 break; > > 172 } > > 173 } > > 174 > > 175 /* config divider values for the closest possible frequency */ > > 176 config_div_and_duty(pwm, best_pre_div, best_pwm_div, > > 177 rate, duty_ns, state->enabled); > > 178 > > 179 return 0; > > 180 } > > 181 > > > > --- > > 0-DAY CI Kernel Test Service, Intel Corporation > > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org > > > -- > ~. .~ Tk Open Systems > =}------------------------------------------------ooO--U--Ooo------------{= > - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il - >
WARNING: multiple messages have this Message-ID (diff)
From: Nathan Chancellor <nathan@kernel.org> To: kbuild-all@lists.01.org Subject: Re: [PATCH 1/3] pwm: driver for qualcomm ipq6018 pwm block Date: Tue, 08 Feb 2022 11:47:57 -0700 [thread overview] Message-ID: <YgK63cI177ZeF5v1@dev-arch.archlinux-ax161> (raw) In-Reply-To: <87ee4ddejv.fsf@tarshish> [-- Attachment #1: Type: text/plain, Size: 6213 bytes --] Hi Baruch, On Tue, Feb 08, 2022 at 08:51:40AM +0200, Baruch Siach wrote: > Hi test robot, > > Thanks for testing and reporting. > > On Tue, Feb 08 2022, kernel test robot wrote: > > [snip] > > >>> drivers/pwm/pwm-ipq.c:122:11: warning: result of comparison of constant 16000000000 with expression of type 'unsigned long' is always false [-Wtautological-constant-out-of-range-compare] > > if (rate > 16ULL * GIGA) > > ~~~~ ^ ~~~~~~~~~~~~ > > 1 warning generated. > > This clang warning is only enabled with W=1 (see commit > afe956c577b). Not sure how to avoid it. > > Is there a way to express this condition without making clang warn on > platforms where ULONG_MAX == 2^32? Maybe cast to unsigned long long? Or > should we just ignore this W=1 warning? As far as I am aware, casting to unsigned long long would be an appropriate way to fix this warning, as has been done in the following patches in mainline: c9ae8eed4463 ("media: omap3isp: avoid warnings at IS_OUT_OF_BOUNDS()") 4853396f03c3 ("memstick: avoid out-of-range warning") 7ff4034e910f ("staging: vc04_services: shut up out-of-range warning") a2fa9e57a68c ("ARM: mvebu: avoid clang -Wtautological-constant warning") The below diff fixes the warning for me with ARCH=hexagon allyesconfig: diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c index 994027290bcb..7ea29468e76e 100644 --- a/drivers/pwm/pwm-ipq.c +++ b/drivers/pwm/pwm-ipq.c @@ -119,7 +119,7 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, * period_ns is 1G or less. As long as rate is less than 16 GHz, * period_rate does not overflow. Make that explicit. */ - if (rate > 16ULL * GIGA) + if ((unsigned long long)rate > 16ULL * GIGA) return -EINVAL; period_rate = period_ns * rate; best_pre_div = IPQ_PWM_MAX_DIV; Alternatively, you could widen rate to unsigned long long / u64 but I don't know what kind of implications that has in this function but it has been done in other places: 95c58291ee70 ("drm/msm/submit: fix overflow check on 64-bit architectures") cfd6fb45cfaf ("crypto: ccree - avoid out-of-range warnings from clang") 335aea75b0d9 ("drm/amdgpu: fix warning for overflow check") 844b85dda2f5 ("ARM: keystone: fix integer overflow warning") While the warning is currently under W=1, I think it is one that we would like to turn on at some point so fixing instances as they come up helps us get closer to that goal. Cheers, Nathan > > vim +122 drivers/pwm/pwm-ipq.c > > > > 99 > > 100 static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > 101 const struct pwm_state *state) > > 102 { > > 103 struct ipq_pwm_chip *ipq_chip = ipq_pwm_from_chip(chip); > > 104 unsigned int pre_div, pwm_div, best_pre_div, best_pwm_div; > > 105 unsigned long rate = clk_get_rate(ipq_chip->clk); > > 106 u64 period_ns, duty_ns, period_rate; > > 107 u64 min_diff; > > 108 > > 109 if (state->polarity != PWM_POLARITY_NORMAL) > > 110 return -EINVAL; > > 111 > > 112 if (state->period < DIV64_U64_ROUND_UP(NSEC_PER_SEC, rate)) > > 113 return -ERANGE; > > 114 > > 115 period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS); > > 116 duty_ns = min(state->duty_cycle, period_ns); > > 117 > > 118 /* > > 119 * period_ns is 1G or less. As long as rate is less than 16 GHz, > > 120 * period_rate does not overflow. Make that explicit. > > 121 */ > > > 122 if (rate > 16ULL * GIGA) > > 123 return -EINVAL; > > 124 period_rate = period_ns * rate; > > 125 best_pre_div = IPQ_PWM_MAX_DIV; > > 126 best_pwm_div = IPQ_PWM_MAX_DIV; > > 127 /* > > 128 * We don't need to consider pre_div values smaller than > > 129 * > > 130 * period_rate > > 131 * pre_div_min := ------------------------------------ > > 132 * NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1) > > 133 * > > 134 * because pre_div = pre_div_min results in a better > > 135 * approximation. > > 136 */ > > 137 pre_div = div64_u64(period_rate, > > 138 (u64)NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1)); > > 139 min_diff = period_rate; > > 140 > > 141 for (; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) { > > 142 u64 remainder; > > 143 > > 144 pwm_div = div64_u64_rem(period_rate, > > 145 (u64)NSEC_PER_SEC * (pre_div + 1), &remainder); > > 146 /* pwm_div is unsigned; the check below catches underflow */ > > 147 pwm_div--; > > 148 > > 149 /* > > 150 * Swapping values for pre_div and pwm_div produces the same > > 151 * period length. So we can skip all settings with pre_div > > > 152 * pwm_div which results in bigger constraints for selecting > > 153 * the duty_cycle than with the two values swapped. > > 154 */ > > 155 if (pre_div > pwm_div) > > 156 break; > > 157 > > 158 /* > > 159 * Make sure we can do 100% duty cycle where > > 160 * hi_dur == pwm_div + 1 > > 161 */ > > 162 if (pwm_div > IPQ_PWM_MAX_DIV - 1) > > 163 continue; > > 164 > > 165 if (remainder < min_diff) { > > 166 best_pre_div = pre_div; > > 167 best_pwm_div = pwm_div; > > 168 min_diff = remainder; > > 169 > > 170 if (min_diff == 0) /* bingo */ > > 171 break; > > 172 } > > 173 } > > 174 > > 175 /* config divider values for the closest possible frequency */ > > 176 config_div_and_duty(pwm, best_pre_div, best_pwm_div, > > 177 rate, duty_ns, state->enabled); > > 178 > > 179 return 0; > > 180 } > > 181 > > > > --- > > 0-DAY CI Kernel Test Service, Intel Corporation > > https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org > > > -- > ~. .~ Tk Open Systems > =}------------------------------------------------ooO--U--Ooo------------{= > - baruch(a)tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il - >
next prev parent reply other threads:[~2022-02-08 18:48 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-02-07 9:30 [PATCH 1/3] pwm: driver for qualcomm ipq6018 pwm block Baruch Siach 2022-02-07 9:30 ` Baruch Siach 2022-02-07 9:30 ` [PATCH 2/3] dt-bindings: pwm: add IPQ6018 binding Baruch Siach 2022-02-07 9:30 ` Baruch Siach 2022-02-24 19:48 ` Bjorn Andersson 2022-02-24 19:48 ` Bjorn Andersson 2022-02-07 9:30 ` [PATCH 3/3] arm64: dts: ipq6018: add pwm node Baruch Siach 2022-02-07 9:30 ` Baruch Siach 2022-02-07 20:22 ` [PATCH 1/3] pwm: driver for qualcomm ipq6018 pwm block kernel test robot 2022-02-07 20:22 ` kernel test robot 2022-02-08 6:51 ` Baruch Siach 2022-02-08 6:51 ` Baruch Siach 2022-02-08 18:47 ` Nathan Chancellor [this message] 2022-02-08 18:47 ` Nathan Chancellor 2023-09-15 6:25 ` Devi Priya 2023-09-15 6:25 ` Devi Priya 2023-09-15 6:36 ` Baruch Siach 2023-09-15 6:36 ` Baruch Siach 2023-09-20 4:58 ` Devi Priya 2023-09-20 4:58 ` Devi Priya 2023-09-22 6:00 ` Devi Priya 2023-09-22 6:00 ` Devi Priya 2023-09-22 8:35 ` Baruch Siach 2023-09-22 8:35 ` Baruch Siach 2023-09-22 10:56 ` Devi Priya 2023-09-22 10:56 ` Devi Priya -- strict thread matches above, loose matches on Subject: below -- 2021-05-19 7:48 Baruch Siach 2021-05-19 7:48 ` Baruch Siach 2021-05-22 21:35 ` Uwe Kleine-König 2021-05-22 21:35 ` Uwe Kleine-König 2021-05-23 15:54 ` Baruch Siach 2021-05-23 15:54 ` Baruch Siach 2021-05-22 21:37 ` Uwe Kleine-König 2021-05-22 21:37 ` Uwe Kleine-König
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YgK63cI177ZeF5v1@dev-arch.archlinux-ax161 \ --to=nathan@kernel.org \ --cc=agross@kernel.org \ --cc=baruch@tkos.co.il \ --cc=bjagadee@codeaurora.org \ --cc=bjorn.andersson@linaro.org \ --cc=kathirav@codeaurora.org \ --cc=kbuild-all@lists.01.org \ --cc=linux-pwm@vger.kernel.org \ --cc=lkp@intel.com \ --cc=llvm@lists.linux.dev \ --cc=robert.marko@sartura.hr \ --cc=robh+dt@kernel.org \ --cc=thierry.reding@gmail.com \ --cc=u.kleine-koenig@pengutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.