[usb:usb-testing 17/35] drivers/usb/storage/usb.c:141:21: error: incompatible pointer types passing 'unsigned int *' to parameter of type 'uint64_t *' (aka 'unsigned long long *')

[usb:usb-testing 17/35] drivers/usb/storage/usb.c:141:21: error: incompatible pointer types passing 'unsigned int *' to parameter of type 'uint64_t *' (aka 'unsigned long long *')

[kernel test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
head:   ac6a205c5bef39d65ecd9f5dd2c1d75652c35405
commit: 70d66b8e47e6fa031f541291e9dd3d7f0c44b41e [17/35] usb-storage: Optimize scan delay more precisely
config: arm-defconfig (https://download.01.org/0day-ci/archive/20240501/202405011431.PvaZHpRK-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240501/202405011431.PvaZHpRK-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405011431.PvaZHpRK-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/usb/storage/usb.c:141:21: warning: comparison of distinct pointer types ('typeof ((delay_ms)) *' (aka 'unsigned int *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]
           unsigned int rem = do_div(delay_ms, int_pow(10, ndecimals));
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
           (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
                  ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~
>> drivers/usb/storage/usb.c:141:21: error: incompatible pointer types passing 'unsigned int *' to parameter of type 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types]
           unsigned int rem = do_div(delay_ms, int_pow(10, ndecimals));
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:238:22: note: expanded from macro 'do_div'
                   __rem = __div64_32(&(n), __base);       \
                                      ^~~~
   arch/arm/include/asm/div64.h:24:45: note: passing argument to parameter 'n' here
   static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
                                               ^
>> drivers/usb/storage/usb.c:141:21: warning: shift count >= width of type [-Wshift-count-overflow]
           unsigned int rem = do_div(delay_ms, int_pow(10, ndecimals));
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:234:25: note: expanded from macro 'do_div'
           } else if (likely(((n) >> 32) == 0)) {          \
                                  ^  ~~
   include/linux/compiler.h:76:40: note: expanded from macro 'likely'
   # define likely(x)      __builtin_expect(!!(x), 1)
                                               ^
   2 warnings and 1 error generated.


vim +141 drivers/usb/storage/usb.c

   123	
   124	/**
   125	 * fixed_point_uint_to_str - format a fixed-point decimal value into a string
   126	 * @val: The integer value to format, scaled by 10^(@ndecimals).
   127	 * @ndecimals: Number of decimal places in the fixed-point value.
   128	 * @str: Where to store the formatted string.
   129	 * @size: The size of buffer for @str.
   130	 *
   131	 * Format a fixed-point decimal value in @val scaled by 10^(@ndecimals)
   132	 * into a string in @str where to store the formatted string.
   133	 * The string trailing fractional part '0' is trimmed.
   134	 *
   135	 * Returns the number of characters written into @str.
   136	 */
   137	static int fixed_point_uint_to_str(unsigned int val, int ndecimals,
   138					   char *str, int size)
   139	{
   140		unsigned int delay_ms = val;
 > 141		unsigned int rem = do_div(delay_ms, int_pow(10, ndecimals));
   142		int len;
   143		char buf[16];
   144	
   145		len = scnprintf(buf, sizeof(buf), "%d", delay_ms);
   146		if (rem) {
   147			char format[8];
   148	
   149			snprintf(format, sizeof(format) - 1, ".%%0%dd", ndecimals);
   150			len += scnprintf(buf + len, sizeof(buf) - len, format, rem);
   151			while (buf[--len] == '0')
   152				buf[len] = '\0';
   153		}
   154		return scnprintf(str, size, "%s\n", buf);
   155	}
   156	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [usb:usb-testing 17/35] drivers/usb/storage/usb.c:141:21: error: incompatible pointer types passing 'unsigned int *' to parameter of type 'uint64_t *' (aka 'unsigned long long *')

[Greg Kroah-Hartman]

On Wed, May 01, 2024 at 02:32:56PM +0800, kernel test robot wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> head:   ac6a205c5bef39d65ecd9f5dd2c1d75652c35405
> commit: 70d66b8e47e6fa031f541291e9dd3d7f0c44b41e [17/35] usb-storage: Optimize scan delay more precisely

I'm going to drop this commit from my tree now.

But really, it feels odd to have this parsing logic in a random driver
for something like handling floating point numbers "properly" from
userspace.  Why isn't this in the core kernel somewhere?  And, as that
was a rhetorical question, the answer is we probably shouldn't be
handling floating point number parsing in the kernel...

I understand the quest for handling error time outs "faster" when you
assume that you know that the error will never happen, but what's the
odds that it will end up happening...

there has to be a better way to do this.

thanks,

greg k-h

Re: [usb:usb-testing 17/35] drivers/usb/storage/usb.c:141:21: error: incompatible pointer types passing 'unsigned int *' to parameter of type 'uint64_t *' (aka 'unsigned long long *')

[Alan Stern]

On Wed, May 01, 2024 at 08:50:36AM +0200, Greg Kroah-Hartman wrote:
> On Wed, May 01, 2024 at 02:32:56PM +0800, kernel test robot wrote:
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> > head:   ac6a205c5bef39d65ecd9f5dd2c1d75652c35405
> > commit: 70d66b8e47e6fa031f541291e9dd3d7f0c44b41e [17/35] usb-storage: Optimize scan delay more precisely
> 
> I'm going to drop this commit from my tree now.
> 
> But really, it feels odd to have this parsing logic in a random driver
> for something like handling floating point numbers "properly" from
> userspace.  Why isn't this in the core kernel somewhere?  And, as that
> was a rhetorical question, the answer is we probably shouldn't be
> handling floating point number parsing in the kernel...

Strictly speaking, this is fixed point rather than floating point.  And 
the reason the commit didn't put the parsing code in the core kernel is 
because no other places seem to need it.  Still, I get your objection.

> I understand the quest for handling error time outs "faster" when you
> assume that you know that the error will never happen, but what's the
> odds that it will end up happening...
> 
> there has to be a better way to do this.

Norihiko, another possibility is to use ordinary integers but have 
users add an "m" or "ms" suffix when they want the value to be 
interpreted in milliseconds.  So "5" means 5 seconds, but "500ms" means 
500 milliseconds.

Alan Stern

RE: [usb:usb-testing 17/35] drivers/usb/storage/usb.c:141:21: error: incompatible pointer types passing 'unsigned int *' to parameter of type 'uint64_t *' (aka 'unsigned long long *')

[Norihiko Hama]

On Wed, May 011, 2024 at 11:03 PM +0900, Alan Stern wrote:
> On Wed, May 01, 2024 at 08:50:36AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, May 01, 2024 at 02:32:56PM +0800, kernel test robot wrote:
> > > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> usb-testing
> > > head:   ac6a205c5bef39d65ecd9f5dd2c1d75652c35405
> > > commit: 70d66b8e47e6fa031f541291e9dd3d7f0c44b41e [17/35]
> > > usb-storage: Optimize scan delay more precisely
> >
> Norihiko, another possibility is to use ordinary integers but have users add an
> "m" or "ms" suffix when they want the value to be interpreted in milliseconds.
> So "5" means 5 seconds, but "500ms" means
> 500 milliseconds.

Greg, I'm sorry for my problem.
Alan, thank you for your proposal.
I'll change the code based on it although I'm not sure if it's acceptable or not.
I'll use only suffix as "ms" so that it could be also shown as 'delay_use' with unique suffix.