Hi Michael,
FYI, the error/warning still remains.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: 8404c9fbc84b741f66cff7d4934a25dd2c344452
commit: 6a82582d9fa438045191074856f47165334f2777 HID: ft260: add usb hid to i2c host bridge driver
date: 7 weeks ago
config: arm64-randconfig-r023-20210505 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 8f5a2a5836cc8e4c1def2bdeb022e7b496623439)
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
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6a82582d9fa438045191074856f47165334f2777
git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git fetch --no-tags linus master
git checkout 6a82582d9fa438045191074856f47165334f2777
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/hid/hid-ft260.c:515:59: warning: variable 'len' is uninitialized when used here [-Wuninitialized]
hid_err(hdev, "%s: unsupported wr len: %d\n", __func__, len);
^~~
include/linux/hid.h:1190:30: note: expanded from macro 'hid_err'
dev_err(&(hid)->dev, fmt, ##__VA_ARGS__)
^~~~~~~~~~~
include/linux/dev_printk.h:112:32: note: expanded from macro 'dev_err'
_dev_err(dev, dev_fmt(fmt), ##__VA_ARGS__)
^~~~~~~~~~~
drivers/hid/hid-ft260.c:507:9: note: initialize the variable 'len' to silence this warning
int len, ret;
^
= 0
>> drivers/hid/hid-ft260.c:794:44: warning: format specifies type 'short' but the argument has type 'int' [-Wformat]
return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
~~~ ^~~~~~~~~~~~~~~~~~~
%i
include/linux/byteorder/generic.h:91:21: note: expanded from macro 'le16_to_cpu'
#define le16_to_cpu __le16_to_cpu
^
include/uapi/linux/byteorder/big_endian.h:36:26: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/uapi/linux/swab.h:105:2: note: expanded from macro '__swab16'
(__builtin_constant_p((__u16)(x)) ? \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
vim +794 drivers/hid/hid-ft260.c
784
785 static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
786 u16 *field, u8 *buf)
787 {
788 int ret;
789
790 ret = ft260_hid_feature_report_get(hdev, id, cfg, len);
791 if (ret != len && ret >= 0)
792 return -EIO;
793
> 794 return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
795 }
796
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Thu, May 06, 2021 at 06:47:46AM +0800, kernel test robot wrote:
>
> drivers/hid/hid-ft260.c:515:59: warning: variable 'len' is uninitialized when used here [-Wuninitialized]
> hid_err(hdev, "%s: unsupported wr len: %d\n", __func__, len);
> ^~~
> include/linux/hid.h:1190:30: note: expanded from macro 'hid_err'
> dev_err(&(hid)->dev, fmt, ##__VA_ARGS__)
> ^~~~~~~~~~~
> include/linux/dev_printk.h:112:32: note: expanded from macro 'dev_err'
> _dev_err(dev, dev_fmt(fmt), ##__VA_ARGS__)
> ^~~~~~~~~~~
> drivers/hid/hid-ft260.c:507:9: note: initialize the variable 'len' to silence this warning
> int len, ret;
> ^
> = 0
This warning has already been found and fixed by Dan Carpenter in
the "HID: ft260: fix an error message in ft260_i2c_write_read()" commit
on March 18, 2021.
On 5/6/2021 7:55 PM, Michael Zaidman wrote:
> On Thu, May 06, 2021 at 06:47:46AM +0800, kernel test robot wrote:
>>
>> drivers/hid/hid-ft260.c:515:59: warning: variable 'len' is uninitialized when used here [-Wuninitialized]
>> hid_err(hdev, "%s: unsupported wr len: %d\n", __func__, len);
>> ^~~
>> include/linux/hid.h:1190:30: note: expanded from macro 'hid_err'
>> dev_err(&(hid)->dev, fmt, ##__VA_ARGS__)
>> ^~~~~~~~~~~
>> include/linux/dev_printk.h:112:32: note: expanded from macro 'dev_err'
>> _dev_err(dev, dev_fmt(fmt), ##__VA_ARGS__)
>> ^~~~~~~~~~~
>> drivers/hid/hid-ft260.c:507:9: note: initialize the variable 'len' to silence this warning
>> int len, ret;
>> ^
>> = 0
>
> This warning has already been found and fixed by Dan Carpenter in
> the "HID: ft260: fix an error message in ft260_i2c_write_read()" commit
> on March 18, 2021.
Hi Michael,
This report is about the below warning prefixed by '>>', it's still in
mainline:
>> drivers/hid/hid-ft260.c:794:44: warning: format specifies type
'short' but the argument has type 'int' [-Wformat]
return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
~~~ ^~~~~~~~~~~~~~~~~~~
%i
Best Regards,
Rong Chen
On Fri, May 07, 2021 at 06:00:31PM +0800, Chen, Rong A wrote:
>
>
> On 5/6/2021 7:55 PM, Michael Zaidman wrote:
> > On Thu, May 06, 2021 at 06:47:46AM +0800, kernel test robot wrote:
> > >
> > > drivers/hid/hid-ft260.c:515:59: warning: variable 'len' is uninitialized when used here [-Wuninitialized]
> > > hid_err(hdev, "%s: unsupported wr len: %d\n", __func__, len);
> > > ^~~
> > > include/linux/hid.h:1190:30: note: expanded from macro 'hid_err'
> > > dev_err(&(hid)->dev, fmt, ##__VA_ARGS__)
> > > ^~~~~~~~~~~
> > > include/linux/dev_printk.h:112:32: note: expanded from macro 'dev_err'
> > > _dev_err(dev, dev_fmt(fmt), ##__VA_ARGS__)
> > > ^~~~~~~~~~~
> > > drivers/hid/hid-ft260.c:507:9: note: initialize the variable 'len' to silence this warning
> > > int len, ret;
> > > ^
> > > = 0
> >
> > This warning has already been found and fixed by Dan Carpenter in
> > the "HID: ft260: fix an error message in ft260_i2c_write_read()" commit
> > on March 18, 2021.
>
> Hi Michael,
>
> This report is about the below warning prefixed by '>>', it's still in
> mainline:
>
> >> drivers/hid/hid-ft260.c:794:44: warning: format specifies type 'short'
> but the argument has type 'int' [-Wformat]
> return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
> ~~~ ^~~~~~~~~~~~~~~~~~~
> %i
>
> Best Regards,
> Rong Chen
Hi Rong,
Sure, but since the report states - "2 warnings generated", I addressed
the first one for which we already have a commit with the fix.
I tried to reproduce the second warning you referred to in this e-mail,
but make.cross script failed since the "clang-latest" directory does not
exist under the "0day". As a workaround, I modified the make command
generated by the script replacing all "clang-latest" occurrences with
"clang" and run it manually.
Another workaround could be to use the symbolic link as reported in the
https://groups.google.com/g/clang-built-linux/c/qCjZblHi8lY/m/eBEEl2JxBgAJ
on March 27, 2021.
Anyway, I reproduced this warning and am going to submit a fix shortly.
Thanks,
Michael
Fixes: 6a82582d9fa4 ("HID: ft260: add usb hid to i2c host bridge driver")
Fix warning reported by static analysis when built with W=1 for arm64 by
clang version 13.0.0
>> drivers/hid/hid-ft260.c:794:44: warning: format specifies type 'short' but
the argument has type 'int' [-Wformat]
return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
~~~ ^~~~~~~~~~~~~~~~~~~
%i
include/linux/byteorder/generic.h:91:21: note: expanded from
macro 'le16_to_cpu'
#define le16_to_cpu __le16_to_cpu
^
include/uapi/linux/byteorder/big_endian.h:36:26: note: expanded from
macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/uapi/linux/swab.h:105:2: note: expanded from macro '__swab16'
(__builtin_constant_p((__u16)(x)) ? \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Signed-off-by: Michael Zaidman <[email protected]>
Reported-by: kernel test robot <[email protected]>
---
drivers/hid/hid-ft260.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 047aa85a7c83..38794a29599c 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -791,7 +791,7 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
if (ret != len && ret >= 0)
return -EIO;
- return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
+ return scnprintf(buf, PAGE_SIZE, "%d\n", le16_to_cpu(*field));
}
#define FT260_ATTR_SHOW(name, reptype, id, type, func) \
--
2.25.1
On Sun, 2021-05-09 at 22:32 +0300, Michael Zaidman wrote:
> Fixes: 6a82582d9fa4 ("HID: ft260: add usb hid to i2c host bridge driver")
>
> Fix warning reported by static analysis when built with W=1 for arm64 by
> clang version 13.0.0
>
> > > drivers/hid/hid-ft260.c:794:44: warning: format specifies type 'short' but
> ???the argument has type 'int' [-Wformat]
> ???????????return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
> ?????????????????????????????????????????????~~~ ^~~~~~~~~~~~~~~~~~~
> ?????????????????????????????????????????????%i
> ???include/linux/byteorder/generic.h:91:21: note: expanded from
> ????????????????????????????????????????????macro 'le16_to_cpu'
> ???#define le16_to_cpu __le16_to_cpu
> ???????????????????????^
> ???include/uapi/linux/byteorder/big_endian.h:36:26: note: expanded from
> ????????????????????????????????????????????????????macro '__le16_to_cpu'
> ???#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
> ????????????????????????????^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ???include/uapi/linux/swab.h:105:2: note: expanded from macro '__swab16'
> ???????????(__builtin_constant_p((__u16)(x)) ? \
> ???????????^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Signed-off-by: Michael Zaidman <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> ---
> ?drivers/hid/hid-ft260.c | 2 +-
> ?1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 047aa85a7c83..38794a29599c 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -791,7 +791,7 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
> ? if (ret != len && ret >= 0)
> ? return -EIO;
> ?
>
> - return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
> + return scnprintf(buf, PAGE_SIZE, "%d\n", le16_to_cpu(*field));
> ?}
There are 2 of these so I wonder about the static analysis.
It's probably better to use sysfs_emit as well.
---
drivers/hid/hid-ft260.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 7a9ba984a75a..475641682fff 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -783,7 +783,7 @@ static int ft260_byte_show(struct hid_device *hdev, int id, u8 *cfg, int len,
if (ret != len && ret >= 0)
return -EIO;
- return scnprintf(buf, PAGE_SIZE, "%hi\n", *field);
+ return sysfs_emit(buf, "%d\n", *field);
}
static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
@@ -795,7 +795,7 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
if (ret != len && ret >= 0)
return -EIO;
- return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
+ return sysfs_emit(buf, "%d\n", le16_to_cpu(*field));
}
#define FT260_ATTR_SHOW(name, reptype, id, type, func) \
On Sun, May 09, 2021 at 01:39:29PM -0700, Joe Perches wrote:
> On Sun, 2021-05-09 at 22:32 +0300, Michael Zaidman wrote:
> > Fixes: 6a82582d9fa4 ("HID: ft260: add usb hid to i2c host bridge driver")
> >
> > Fix warning reported by static analysis when built with W=1 for arm64 by
> > clang version 13.0.0
> >
> > > > drivers/hid/hid-ft260.c:794:44: warning: format specifies type 'short' but
> > ???the argument has type 'int' [-Wformat]
> > ???????????return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
> > ?????????????????????????????????????????????~~~ ^~~~~~~~~~~~~~~~~~~
> > ?????????????????????????????????????????????%i
> > ???include/linux/byteorder/generic.h:91:21: note: expanded from
> > ????????????????????????????????????????????macro 'le16_to_cpu'
> > ???#define le16_to_cpu __le16_to_cpu
> > ???????????????????????^
> > ???include/uapi/linux/byteorder/big_endian.h:36:26: note: expanded from
> > ????????????????????????????????????????????????????macro '__le16_to_cpu'
> > ???#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
> > ????????????????????????????^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ???include/uapi/linux/swab.h:105:2: note: expanded from macro '__swab16'
> > ???????????(__builtin_constant_p((__u16)(x)) ? \
> > ???????????^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Signed-off-by: Michael Zaidman <[email protected]>
> > Reported-by: kernel test robot <[email protected]>
> > ---
> > ?drivers/hid/hid-ft260.c | 2 +-
> > ?1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> > index 047aa85a7c83..38794a29599c 100644
> > --- a/drivers/hid/hid-ft260.c
> > +++ b/drivers/hid/hid-ft260.c
> > @@ -791,7 +791,7 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
> > ? if (ret != len && ret >= 0)
> > ? return -EIO;
> > ?
> >
> > - return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
> > + return scnprintf(buf, PAGE_SIZE, "%d\n", le16_to_cpu(*field));
> > ?}
>
> There are 2 of these so I wonder about the static analysis.
There is nothing wrong with the static analysis. The first scnprintf format
type is perfectly valid as far as its size is greater than the size of the
data pointed by the *field pointer, which is a one byte size in our case.
The static analysis warned about the second scnprintf case, where the format
type was shorter than the integer returned by the __builtin_constant_p.
This warning can be considered as a false positive since the le16_to_cpu is
all about the 16 bits numbers, but to silence it, I submitted the above fix.
> It's probably better to use sysfs_emit as well.
The sysfs_emit was introduced in the 5.10 kernel:
2efc459d06f16 (Joe Perches 2020-09-16 13:40:38 -0700 335) int sysfs_emit(...)
But, the hid-ft260 driver will be used mostly with older kernels, at least,
for the next couple of years. Since older kernel versions do not have this API,
it will require patching the driver or kernel that I would like to avoid.
Nevertheless, we can reconsider the sysfs_emit usage in this driver in the
future, upon wider 5.10+ kernels' adoption.
> ---
> drivers/hid/hid-ft260.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> index 7a9ba984a75a..475641682fff 100644
> --- a/drivers/hid/hid-ft260.c
> +++ b/drivers/hid/hid-ft260.c
> @@ -783,7 +783,7 @@ static int ft260_byte_show(struct hid_device *hdev, int id, u8 *cfg, int len,
> if (ret != len && ret >= 0)
> return -EIO;
>
> - return scnprintf(buf, PAGE_SIZE, "%hi\n", *field);
> + return sysfs_emit(buf, "%d\n", *field);
> }
>
> static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
> @@ -795,7 +795,7 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
> if (ret != len && ret >= 0)
> return -EIO;
>
> - return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
> + return sysfs_emit(buf, "%d\n", le16_to_cpu(*field));
> }
>
> #define FT260_ATTR_SHOW(name, reptype, id, type, func) \
>
On Mon, 2021-05-10 at 12:17 +0300, Michael Zaidman wrote:
> On Sun, May 09, 2021 at 01:39:29PM -0700, Joe Perches wrote:
> > On Sun, 2021-05-09 at 22:32 +0300, Michael Zaidman wrote:
> > > Fixes: 6a82582d9fa4 ("HID: ft260: add usb hid to i2c host bridge driver")
> > >
> > > Fix warning reported by static analysis when built with W=1 for arm64 by
> > > clang version 13.0.0
> > >
> > > > > drivers/hid/hid-ft260.c:794:44: warning: format specifies type 'short' but
> > > ???the argument has type 'int' [-Wformat]
> > > ???????????return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
> > > ?????????????????????????????????????????????~~~ ^~~~~~~~~~~~~~~~~~~
> > > ?????????????????????????????????????????????%i
> > > ???include/linux/byteorder/generic.h:91:21: note: expanded from
> > > ????????????????????????????????????????????macro 'le16_to_cpu'
> > > ???#define le16_to_cpu __le16_to_cpu
> > > ???????????????????????^
> > > ???include/uapi/linux/byteorder/big_endian.h:36:26: note: expanded from
> > > ????????????????????????????????????????????????????macro '__le16_to_cpu'
> > > ???#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
> > > ????????????????????????????^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ???include/uapi/linux/swab.h:105:2: note: expanded from macro '__swab16'
> > > ???????????(__builtin_constant_p((__u16)(x)) ? \
> > > ???????????^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > Signed-off-by: Michael Zaidman <[email protected]>
> > > Reported-by: kernel test robot <[email protected]>
> > > ---
> > > ?drivers/hid/hid-ft260.c | 2 +-
> > > ?1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> > > index 047aa85a7c83..38794a29599c 100644
> > > --- a/drivers/hid/hid-ft260.c
> > > +++ b/drivers/hid/hid-ft260.c
> > > @@ -791,7 +791,7 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
> > > ? if (ret != len && ret >= 0)
> > > ? return -EIO;
> > > ?
> > >
> > > - return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
> > > + return scnprintf(buf, PAGE_SIZE, "%d\n", le16_to_cpu(*field));
> > > ?}
> >
> > There are 2 of these so I wonder about the static analysis.
>
> There is nothing wrong with the static analysis. The first scnprintf format
> type is perfectly valid as far as its size is greater than the size of the
> data pointed by the *field pointer, which is a one byte size in our case.
> The static analysis warned about the second scnprintf case, where the format
> type was shorter than the integer returned by the __builtin_constant_p.
> This warning can be considered as a false positive since the le16_to_cpu is
> all about the 16 bits numbers, but to silence it, I submitted the above fix.
$ git grep __arch_swab16 arch/arm*/
arch/arm/include/asm/swab.h:#define __arch_swab16(x) ((__u16)__arch_swahb32(x))
otherwise:
static inline __attribute_const__ __u16 __fswab16(__u16 val)
{
#if defined (__arch_swab16)
return __arch_swab16(val);
#else
return ___constant_swab16(val);
#endif
}
#define ___constant_swab16(x) ((__u16)( \
(((__u16)(x) & (__u16)0x00ffU) << 8) | \
(((__u16)(x) & (__u16)0xff00U) >> 8)))
/**
* __swab16 - return a byteswapped 16-bit value
* @x: value to byteswap
*/
#ifdef __HAVE_BUILTIN_BSWAP16__
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
#else
#define __swab16(x) \
(__builtin_constant_p((__u16)(x)) ? \
___constant_swab16(x) : \
__fswab16(x))
#endif
Under what condition does the ?: return an int sized value
rather than a u16 sized value? I fail to see a path where
the compiler should promote the returned value to int _before_
the promotion done for the varargs use.
If it's for the varargs use, then both instances are promoted.
> > It's probably better to use sysfs_emit as well.
>
> The sysfs_emit was introduced in the 5.10 kernel:
> 2efc459d06f16 (Joe Perches 2020-09-16 13:40:38 -0700 335) int sysfs_emit(...)
>
> But, the hid-ft260 driver will be used mostly with older kernels, at least,
> for the next couple of years. Since older kernel versions do not have this API,
> it will require patching the driver or kernel that I would like to avoid.
> Nevertheless, we can reconsider the sysfs_emit usage in this driver in the
> future, upon wider 5.10+ kernels' adoption.
If this is only for older kernels, then it's not really useful
upstream IMO.
any sprintf style use of %h or %hh for a sub int sized value isn't
particularly useful as integer promotion is done on the value so it
should use %d (or %i, but %i is atypical) anyway.
https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/
$ git grep '%d\b' | wc -l
109922
$ git grep '%i\b' | wc -l
3508
On Mon, May 10, 2021 at 02:52:14AM -0700, Joe Perches wrote:
> On Mon, 2021-05-10 at 12:17 +0300, Michael Zaidman wrote:
> > On Sun, May 09, 2021 at 01:39:29PM -0700, Joe Perches wrote:
> > > On Sun, 2021-05-09 at 22:32 +0300, Michael Zaidman wrote:
> > > > Fixes: 6a82582d9fa4 ("HID: ft260: add usb hid to i2c host bridge driver")
> > > >
> > > > Fix warning reported by static analysis when built with W=1 for arm64 by
> > > > clang version 13.0.0
> > > >
> > > > > > drivers/hid/hid-ft260.c:794:44: warning: format specifies type 'short' but
> > > > ???the argument has type 'int' [-Wformat]
> > > > ???????????return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
> > > > ?????????????????????????????????????????????~~~ ^~~~~~~~~~~~~~~~~~~
> > > > ?????????????????????????????????????????????%i
> > > > ???include/linux/byteorder/generic.h:91:21: note: expanded from
> > > > ????????????????????????????????????????????macro 'le16_to_cpu'
> > > > ???#define le16_to_cpu __le16_to_cpu
> > > > ???????????????????????^
> > > > ???include/uapi/linux/byteorder/big_endian.h:36:26: note: expanded from
> > > > ????????????????????????????????????????????????????macro '__le16_to_cpu'
> > > > ???#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
> > > > ????????????????????????????^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > ???include/uapi/linux/swab.h:105:2: note: expanded from macro '__swab16'
> > > > ???????????(__builtin_constant_p((__u16)(x)) ? \
> > > > ???????????^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >
> > > > Signed-off-by: Michael Zaidman <[email protected]>
> > > > Reported-by: kernel test robot <[email protected]>
> > > > ---
> > > > ?drivers/hid/hid-ft260.c | 2 +-
> > > > ?1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> > > > index 047aa85a7c83..38794a29599c 100644
> > > > --- a/drivers/hid/hid-ft260.c
> > > > +++ b/drivers/hid/hid-ft260.c
> > > > @@ -791,7 +791,7 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
> > > > ? if (ret != len && ret >= 0)
> > > > ? return -EIO;
> > > > ?
> > > >
> > > > - return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
> > > > + return scnprintf(buf, PAGE_SIZE, "%d\n", le16_to_cpu(*field));
> > > > ?}
> > >
> > > There are 2 of these so I wonder about the static analysis.
> >
> > There is nothing wrong with the static analysis. The first scnprintf format
> > type is perfectly valid as far as its size is greater than the size of the
> > data pointed by the *field pointer, which is a one byte size in our case.
> > The static analysis warned about the second scnprintf case, where the format
> > type was shorter than the integer returned by the __builtin_constant_p.
> > This warning can be considered as a false positive since the le16_to_cpu is
> > all about the 16 bits numbers, but to silence it, I submitted the above fix.
>
> $ git grep __arch_swab16 arch/arm*/
> arch/arm/include/asm/swab.h:#define __arch_swab16(x) ((__u16)__arch_swahb32(x))
>
> otherwise:
>
> static inline __attribute_const__ __u16 __fswab16(__u16 val)
> {
> #if defined (__arch_swab16)
> return __arch_swab16(val);
> #else
> return ___constant_swab16(val);
> #endif
> }
>
> #define ___constant_swab16(x) ((__u16)( \
> (((__u16)(x) & (__u16)0x00ffU) << 8) | \
> (((__u16)(x) & (__u16)0xff00U) >> 8)))
>
> /**
> * __swab16 - return a byteswapped 16-bit value
> * @x: value to byteswap
> */
> #ifdef __HAVE_BUILTIN_BSWAP16__
> #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
> #else
> #define __swab16(x) \
> (__builtin_constant_p((__u16)(x)) ? \
> ___constant_swab16(x) : \
> __fswab16(x))
> #endif
>
> Under what condition does the ?: return an int sized value
> rather than a u16 sized value? I fail to see a path where
> the compiler should promote the returned value to int _before_
> the promotion done for the varargs use.
>
> If it's for the varargs use, then both instances are promoted.
>
Ternary type promotion is a horrible thing.
foo = a ? b : c;
Everything is type promoted which ever of a, b or c has the most
positive bits. Or if none of them have 31 positive bits it's
type promoted to int.
I sent a series of patches earlier where one the a, b, or c was
a negative error code and another was a unsigned int. And foo
was a ssize_t. Because you end up type promoting the -ENOMEM
to something close to UINT_MAX and then it doesn't sign extend
so the ssize_t value is not negative.
regards,
dan carpenter
On Mon, May 10, 2021 at 01:15:51PM +0300, Dan Carpenter wrote:
> On Mon, May 10, 2021 at 02:52:14AM -0700, Joe Perches wrote:
> > On Mon, 2021-05-10 at 12:17 +0300, Michael Zaidman wrote:
> > > On Sun, May 09, 2021 at 01:39:29PM -0700, Joe Perches wrote:
> > > > On Sun, 2021-05-09 at 22:32 +0300, Michael Zaidman wrote:
> > > > > Fixes: 6a82582d9fa4 ("HID: ft260: add usb hid to i2c host bridge driver")
> > > > >
> > > > > Fix warning reported by static analysis when built with W=1 for arm64 by
> > > > > clang version 13.0.0
> > > > >
> > > > > > > drivers/hid/hid-ft260.c:794:44: warning: format specifies type 'short' but
> > > > > ???the argument has type 'int' [-Wformat]
> > > > > ???????????return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
> > > > > ?????????????????????????????????????????????~~~ ^~~~~~~~~~~~~~~~~~~
> > > > > ?????????????????????????????????????????????%i
> > > > > ???include/linux/byteorder/generic.h:91:21: note: expanded from
> > > > > ????????????????????????????????????????????macro 'le16_to_cpu'
> > > > > ???#define le16_to_cpu __le16_to_cpu
> > > > > ???????????????????????^
> > > > > ???include/uapi/linux/byteorder/big_endian.h:36:26: note: expanded from
> > > > > ????????????????????????????????????????????????????macro '__le16_to_cpu'
> > > > > ???#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
> > > > > ????????????????????????????^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > ???include/uapi/linux/swab.h:105:2: note: expanded from macro '__swab16'
> > > > > ???????????(__builtin_constant_p((__u16)(x)) ? \
> > > > > ???????????^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > >
> > > > > Signed-off-by: Michael Zaidman <[email protected]>
> > > > > Reported-by: kernel test robot <[email protected]>
> > > > > ---
> > > > > ?drivers/hid/hid-ft260.c | 2 +-
> > > > > ?1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> > > > > index 047aa85a7c83..38794a29599c 100644
> > > > > --- a/drivers/hid/hid-ft260.c
> > > > > +++ b/drivers/hid/hid-ft260.c
> > > > > @@ -791,7 +791,7 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
> > > > > ? if (ret != len && ret >= 0)
> > > > > ? return -EIO;
> > > > > ?
> > > > >
> > > > > - return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
> > > > > + return scnprintf(buf, PAGE_SIZE, "%d\n", le16_to_cpu(*field));
> > > > > ?}
> > > >
> > > > There are 2 of these so I wonder about the static analysis.
> > >
> > > There is nothing wrong with the static analysis. The first scnprintf format
> > > type is perfectly valid as far as its size is greater than the size of the
> > > data pointed by the *field pointer, which is a one byte size in our case.
> > > The static analysis warned about the second scnprintf case, where the format
> > > type was shorter than the integer returned by the __builtin_constant_p.
> > > This warning can be considered as a false positive since the le16_to_cpu is
> > > all about the 16 bits numbers, but to silence it, I submitted the above fix.
> >
> > $ git grep __arch_swab16 arch/arm*/
> > arch/arm/include/asm/swab.h:#define __arch_swab16(x) ((__u16)__arch_swahb32(x))
> >
> > otherwise:
> >
> > static inline __attribute_const__ __u16 __fswab16(__u16 val)
> > {
> > #if defined (__arch_swab16)
> > return __arch_swab16(val);
> > #else
> > return ___constant_swab16(val);
> > #endif
> > }
> >
> > #define ___constant_swab16(x) ((__u16)( \
> > (((__u16)(x) & (__u16)0x00ffU) << 8) | \
> > (((__u16)(x) & (__u16)0xff00U) >> 8)))
> >
> > /**
> > * __swab16 - return a byteswapped 16-bit value
> > * @x: value to byteswap
> > */
> > #ifdef __HAVE_BUILTIN_BSWAP16__
> > #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
> > #else
> > #define __swab16(x) \
> > (__builtin_constant_p((__u16)(x)) ? \
> > ___constant_swab16(x) : \
> > __fswab16(x))
> > #endif
> >
> > Under what condition does the ?: return an int sized value
> > rather than a u16 sized value? I fail to see a path where
> > the compiler should promote the returned value to int _before_
> > the promotion done for the varargs use.
> >
> > If it's for the varargs use, then both instances are promoted.
> >
>
> Ternary type promotion is a horrible thing.
>
> foo = a ? b : c;
>
> Everything is type promoted which ever of a, b or c has the most
> positive bits. Or if none of them have 31 positive bits it's
> type promoted to int.
Oops. Sorry, I'm not thinking straight. "a" doesn't affect the type
promotion, but it would if you had:
foo = a ?: c;
regards,
dan carpenter
On Mon, May 10, 2021 at 02:52:14AM -0700, Joe Perches wrote:
> On Mon, 2021-05-10 at 12:17 +0300, Michael Zaidman wrote:
> > On Sun, May 09, 2021 at 01:39:29PM -0700, Joe Perches wrote:
> > > On Sun, 2021-05-09 at 22:32 +0300, Michael Zaidman wrote:
> > > > Fixes: 6a82582d9fa4 ("HID: ft260: add usb hid to i2c host bridge driver")
> > > >
> > > > Fix warning reported by static analysis when built with W=1 for arm64 by
> > > > clang version 13.0.0
> > > >
> > > > > > drivers/hid/hid-ft260.c:794:44: warning: format specifies type 'short' but
> > > > ???the argument has type 'int' [-Wformat]
> > > > ???????????return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
> > > > ?????????????????????????????????????????????~~~ ^~~~~~~~~~~~~~~~~~~
> > > > ?????????????????????????????????????????????%i
> > > > ???include/linux/byteorder/generic.h:91:21: note: expanded from
> > > > ????????????????????????????????????????????macro 'le16_to_cpu'
> > > > ???#define le16_to_cpu __le16_to_cpu
> > > > ???????????????????????^
> > > > ???include/uapi/linux/byteorder/big_endian.h:36:26: note: expanded from
> > > > ????????????????????????????????????????????????????macro '__le16_to_cpu'
> > > > ???#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
> > > > ????????????????????????????^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > ???include/uapi/linux/swab.h:105:2: note: expanded from macro '__swab16'
> > > > ???????????(__builtin_constant_p((__u16)(x)) ? \
> > > > ???????????^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >
> > > > Signed-off-by: Michael Zaidman <[email protected]>
> > > > Reported-by: kernel test robot <[email protected]>
> > > > ---
> > > > ?drivers/hid/hid-ft260.c | 2 +-
> > > > ?1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
> > > > index 047aa85a7c83..38794a29599c 100644
> > > > --- a/drivers/hid/hid-ft260.c
> > > > +++ b/drivers/hid/hid-ft260.c
> > > > @@ -791,7 +791,7 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
> > > > ? if (ret != len && ret >= 0)
> > > > ? return -EIO;
> > > > ?
> > > >
> > > > - return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
> > > > + return scnprintf(buf, PAGE_SIZE, "%d\n", le16_to_cpu(*field));
> > > > ?}
> > >
> > > There are 2 of these so I wonder about the static analysis.
> >
> > There is nothing wrong with the static analysis. The first scnprintf format
> > type is perfectly valid as far as its size is greater than the size of the
> > data pointed by the *field pointer, which is a one byte size in our case.
> > The static analysis warned about the second scnprintf case, where the format
> > type was shorter than the integer returned by the __builtin_constant_p.
> > This warning can be considered as a false positive since the le16_to_cpu is
> > all about the 16 bits numbers, but to silence it, I submitted the above fix.
>
> $ git grep __arch_swab16 arch/arm*/
> arch/arm/include/asm/swab.h:#define __arch_swab16(x) ((__u16)__arch_swahb32(x))
>
> otherwise:
>
> static inline __attribute_const__ __u16 __fswab16(__u16 val)
> {
> #if defined (__arch_swab16)
> return __arch_swab16(val);
> #else
> return ___constant_swab16(val);
> #endif
> }
>
> #define ___constant_swab16(x) ((__u16)( \
> (((__u16)(x) & (__u16)0x00ffU) << 8) | \
> (((__u16)(x) & (__u16)0xff00U) >> 8)))
>
> /**
> * __swab16 - return a byteswapped 16-bit value
> * @x: value to byteswap
> */
> #ifdef __HAVE_BUILTIN_BSWAP16__
> #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
> #else
> #define __swab16(x) \
> (__builtin_constant_p((__u16)(x)) ? \
> ___constant_swab16(x) : \
> __fswab16(x))
> #endif
>
> Under what condition does the ?: return an int sized value
> rather than a u16 sized value? I fail to see a path where
> the compiler should promote the returned value to int _before_
> the promotion done for the varargs use.
Oh, I see your point. Might it be that the static analysis misinterpreted
the __builtin_constant_p function which has a `int __builtin_constant_p (exp)`
prototype according to the GCC and clang built-in functions description?
>
> If it's for the varargs use, then both instances are promoted.
>
> > > It's probably better to use sysfs_emit as well.
> >
> > The sysfs_emit was introduced in the 5.10 kernel:
> > 2efc459d06f16 (Joe Perches 2020-09-16 13:40:38 -0700 335) int sysfs_emit(...)
> >
> > But, the hid-ft260 driver will be used mostly with older kernels, at least,
> > for the next couple of years. Since older kernel versions do not have this API,
> > it will require patching the driver or kernel that I would like to avoid.
> > Nevertheless, we can reconsider the sysfs_emit usage in this driver in the
> > future, upon wider 5.10+ kernels' adoption.
>
> If this is only for older kernels, then it's not really useful
> upstream IMO.
Under "mostly", I meant that the majority of the kernels used in the existing and
currently developing electronic appliances (not necessarily computers) are older
than the 5.10 version at the moment, and this driver should be usable also by them.
The scnprintf enables the hid-ft260 driver reuse by virtually any kernel version.
$ git grep scnprintf | wc -l
6121
>
> any sprintf style use of %h or %hh for a sub int sized value isn't
> particularly useful as integer promotion is done on the value so it
> should use %d (or %i, but %i is atypical) anyway.
>
> https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/
Thanks for sharing this info. I will replace the %hi with %d as you
suggested.
>
> $ git grep '%d\b' | wc -l
> 109922
> $ git grep '%i\b' | wc -l
> 3508
>
>
Fixes: 6a82582d9fa4 ("HID: ft260: add usb hid to i2c host bridge driver")
Fix warning reported by static analysis when built with W=1 for arm64 by
clang version 13.0.0
>> drivers/hid/hid-ft260.c:794:44: warning: format specifies type 'short' but
the argument has type 'int' [-Wformat]
return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
~~~ ^~~~~~~~~~~~~~~~~~~
%i
include/linux/byteorder/generic.h:91:21: note: expanded from
macro 'le16_to_cpu'
#define le16_to_cpu __le16_to_cpu
^
include/uapi/linux/byteorder/big_endian.h:36:26: note: expanded from
macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/uapi/linux/swab.h:105:2: note: expanded from macro '__swab16'
(__builtin_constant_p((__u16)(x)) ? \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Any sprintf style use of %h or %hi for a sub-int sized value isn't useful
since integer promotion is done on the value anyway. So, use %d instead.
https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/
Signed-off-by: Michael Zaidman <[email protected]>
Suggested-by: Joe Perches <[email protected]>
Reported-by: kernel test robot <[email protected]>
---
drivers/hid/hid-ft260.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 047aa85a7c83..38794a29599c 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -791,7 +791,7 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
if (ret != len && ret >= 0)
return -EIO;
- return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
+ return scnprintf(buf, PAGE_SIZE, "%d\n", le16_to_cpu(*field));
}
#define FT260_ATTR_SHOW(name, reptype, id, type, func) \
--
2.25.1
Fixes: 6a82582d9fa4 ("HID: ft260: add usb hid to i2c host bridge driver")
Fix warning reported by static analysis when built with W=1 for arm64 by
clang version 13.0.0
>> drivers/hid/hid-ft260.c:794:44: warning: format specifies type 'short' but
the argument has type 'int' [-Wformat]
return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
~~~ ^~~~~~~~~~~~~~~~~~~
%i
include/linux/byteorder/generic.h:91:21: note: expanded from
macro 'le16_to_cpu'
#define le16_to_cpu __le16_to_cpu
^
include/uapi/linux/byteorder/big_endian.h:36:26: note: expanded from
macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/uapi/linux/swab.h:105:2: note: expanded from macro '__swab16'
(__builtin_constant_p((__u16)(x)) ? \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Any sprintf style use of %h or %hi for a sub-int sized value isn't useful
since integer promotion is done on the value anyway. So, use %d instead.
https://lore.kernel.org/lkml/CAHk-=wgoxnmsj8GEVFJSvTwdnWm8wVJthefNk2n6+4TC=20e0Q@mail.gmail.com/
Signed-off-by: Michael Zaidman <[email protected]>
Suggested-by: Joe Perches <[email protected]>
Reported-by: kernel test robot <[email protected]>
---
drivers/hid/hid-ft260.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
index 047aa85a7c83..ff2a49b5cac5 100644
--- a/drivers/hid/hid-ft260.c
+++ b/drivers/hid/hid-ft260.c
@@ -779,7 +779,7 @@ static int ft260_byte_show(struct hid_device *hdev, int id, u8 *cfg, int len,
if (ret != len && ret >= 0)
return -EIO;
- return scnprintf(buf, PAGE_SIZE, "%hi\n", *field);
+ return scnprintf(buf, PAGE_SIZE, "%d\n", *field);
}
static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
@@ -791,7 +791,7 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
if (ret != len && ret >= 0)
return -EIO;
- return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
+ return scnprintf(buf, PAGE_SIZE, "%d\n", le16_to_cpu(*field));
}
#define FT260_ATTR_SHOW(name, reptype, id, type, func) \
--
2.25.1
On Mon, 2021-05-10 at 19:30 +0300, Michael Zaidman wrote:
> Fixes: 6a82582d9fa4 ("HID: ft260: add usb hid to i2c host bridge driver")
>
> Fix warning reported by static analysis when built with W=1 for arm64 by
> clang version 13.0.0
>
> > > drivers/hid/hid-ft260.c:794:44: warning: format specifies type 'short' but
> ???the argument has type 'int' [-Wformat]
[]
> diff --git a/drivers/hid/hid-ft260.c b/drivers/hid/hid-ft260.c
[]
> @@ -791,7 +791,7 @@ static int ft260_word_show(struct hid_device *hdev, int id, u8 *cfg, int len,
> ? if (ret != len && ret >= 0)
> ? return -EIO;
> ?
>
> - return scnprintf(buf, PAGE_SIZE, "%hi\n", le16_to_cpu(*field));
> + return scnprintf(buf, PAGE_SIZE, "%d\n", le16_to_cpu(*field));
> ?}
This is likely a clang defect and not an actual problem.
If you are going to convert one of these, convert both.