Re: [PATCH 1/3] pwm: driver for qualcomm ipq6018 pwm block

[Nathan Chancellor <nathan@kernel.org>]

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