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

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

[kernel test robot]

Hi Baruch,

I love your patch! Perhaps something to improve:

[auto build test WARNING on thierry-reding-pwm/for-next]
[also build test WARNING on robh/for-next v5.17-rc3 next-20220207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Baruch-Siach/pwm-driver-for-qualcomm-ipq6018-pwm-block/20220207-175605
base:   https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: hexagon-allyesconfig (https://download.01.org/0day-ci/archive/20220208/202202080410.R0qwqtXx-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0d8850ae2cae85d49bea6ae0799fa41c7202c05c)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/71e449eb6d19b141b4527caae529e16c52bcfeea
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Baruch-Siach/pwm-driver-for-qualcomm-ipq6018-pwm-block/20220207-175605
        git checkout 71e449eb6d19b141b4527caae529e16c52bcfeea
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/pwm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

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


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

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

[Baruch Siach]

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?

baruch

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

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

[Nathan Chancellor]

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