* [PATCH] HID: ft260: fix format type warning in ft260_word_show() [not found] <202105060637.LeEC6ztp-lkp@intel.com> @ 2021-05-09 19:32 ` Michael Zaidman 2021-05-09 20:39 ` Joe Perches 2021-05-10 16:30 ` [PATCH v2] " Michael Zaidman 2021-05-10 16:34 ` [PATCH v3] " Michael Zaidman 2 siblings, 1 reply; 10+ messages in thread From: Michael Zaidman @ 2021-05-09 19:32 UTC (permalink / raw) To: lkp Cc: kbuild-all, clang-built-linux, linux-kernel, jikos, dan.carpenter, linux-input, Michael Zaidman 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 <michael.zaidman@gmail.com> Reported-by: kernel test robot <lkp@intel.com> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] HID: ft260: fix format type warning in ft260_word_show() 2021-05-09 19:32 ` [PATCH] HID: ft260: fix format type warning in ft260_word_show() Michael Zaidman @ 2021-05-09 20:39 ` Joe Perches 2021-05-10 9:17 ` Michael Zaidman 0 siblings, 1 reply; 10+ messages in thread From: Joe Perches @ 2021-05-09 20:39 UTC (permalink / raw) To: Michael Zaidman, lkp Cc: kbuild-all, clang-built-linux, linux-kernel, jikos, dan.carpenter, linux-input 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 <michael.zaidman@gmail.com> > Reported-by: kernel test robot <lkp@intel.com> > --- > 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) \ ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] HID: ft260: fix format type warning in ft260_word_show() 2021-05-09 20:39 ` Joe Perches @ 2021-05-10 9:17 ` Michael Zaidman 2021-05-10 9:52 ` Joe Perches 0 siblings, 1 reply; 10+ messages in thread From: Michael Zaidman @ 2021-05-10 9:17 UTC (permalink / raw) To: Joe Perches Cc: lkp, kbuild-all, clang-built-linux, linux-kernel, jikos, dan.carpenter, linux-input, michael.zaidman 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 <michael.zaidman@gmail.com> > > Reported-by: kernel test robot <lkp@intel.com> > > --- > > 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) \ > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] HID: ft260: fix format type warning in ft260_word_show() 2021-05-10 9:17 ` Michael Zaidman @ 2021-05-10 9:52 ` Joe Perches 2021-05-10 10:15 ` Dan Carpenter 2021-05-10 12:51 ` Michael Zaidman 0 siblings, 2 replies; 10+ messages in thread From: Joe Perches @ 2021-05-10 9:52 UTC (permalink / raw) To: Michael Zaidman Cc: lkp, kbuild-all, clang-built-linux, linux-kernel, jikos, dan.carpenter, linux-input 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 <michael.zaidman@gmail.com> > > > Reported-by: kernel test robot <lkp@intel.com> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] HID: ft260: fix format type warning in ft260_word_show() 2021-05-10 9:52 ` Joe Perches @ 2021-05-10 10:15 ` Dan Carpenter 2021-05-10 10:22 ` Dan Carpenter 2021-05-10 12:51 ` Michael Zaidman 1 sibling, 1 reply; 10+ messages in thread From: Dan Carpenter @ 2021-05-10 10:15 UTC (permalink / raw) To: Joe Perches Cc: Michael Zaidman, lkp, kbuild-all, clang-built-linux, linux-kernel, jikos, linux-input 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 <michael.zaidman@gmail.com> > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > --- > > > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] HID: ft260: fix format type warning in ft260_word_show() 2021-05-10 10:15 ` Dan Carpenter @ 2021-05-10 10:22 ` Dan Carpenter 0 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2021-05-10 10:22 UTC (permalink / raw) To: Joe Perches Cc: Michael Zaidman, lkp, kbuild-all, clang-built-linux, linux-kernel, jikos, linux-input 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 <michael.zaidman@gmail.com> > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > > --- > > > > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] HID: ft260: fix format type warning in ft260_word_show() 2021-05-10 9:52 ` Joe Perches 2021-05-10 10:15 ` Dan Carpenter @ 2021-05-10 12:51 ` Michael Zaidman 1 sibling, 0 replies; 10+ messages in thread From: Michael Zaidman @ 2021-05-10 12:51 UTC (permalink / raw) To: Joe Perches Cc: lkp, kbuild-all, clang-built-linux, linux-kernel, jikos, dan.carpenter, linux-input, michael.zaidman 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 <michael.zaidman@gmail.com> > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > --- > > > > 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 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] HID: ft260: fix format type warning in ft260_word_show() [not found] <202105060637.LeEC6ztp-lkp@intel.com> 2021-05-09 19:32 ` [PATCH] HID: ft260: fix format type warning in ft260_word_show() Michael Zaidman @ 2021-05-10 16:30 ` Michael Zaidman 2021-05-10 16:41 ` Joe Perches 2021-05-10 16:34 ` [PATCH v3] " Michael Zaidman 2 siblings, 1 reply; 10+ messages in thread From: Michael Zaidman @ 2021-05-10 16:30 UTC (permalink / raw) To: lkp Cc: kbuild-all, clang-built-linux, linux-kernel, jikos, joe, dan.carpenter, linux-input, Michael Zaidman 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 <michael.zaidman@gmail.com> Suggested-by: Joe Perches <joe@perches.com> Reported-by: kernel test robot <lkp@intel.com> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] HID: ft260: fix format type warning in ft260_word_show() 2021-05-10 16:30 ` [PATCH v2] " Michael Zaidman @ 2021-05-10 16:41 ` Joe Perches 0 siblings, 0 replies; 10+ messages in thread From: Joe Perches @ 2021-05-10 16:41 UTC (permalink / raw) To: Michael Zaidman, lkp Cc: kbuild-all, clang-built-linux, linux-kernel, jikos, dan.carpenter, linux-input 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3] HID: ft260: fix format type warning in ft260_word_show() [not found] <202105060637.LeEC6ztp-lkp@intel.com> 2021-05-09 19:32 ` [PATCH] HID: ft260: fix format type warning in ft260_word_show() Michael Zaidman 2021-05-10 16:30 ` [PATCH v2] " Michael Zaidman @ 2021-05-10 16:34 ` Michael Zaidman 2 siblings, 0 replies; 10+ messages in thread From: Michael Zaidman @ 2021-05-10 16:34 UTC (permalink / raw) To: lkp Cc: kbuild-all, clang-built-linux, linux-kernel, jikos, joe, dan.carpenter, linux-input, Michael Zaidman 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 <michael.zaidman@gmail.com> Suggested-by: Joe Perches <joe@perches.com> Reported-by: kernel test robot <lkp@intel.com> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-05-10 16:42 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <202105060637.LeEC6ztp-lkp@intel.com>
2021-05-09 19:32 ` [PATCH] HID: ft260: fix format type warning in ft260_word_show() Michael Zaidman
2021-05-09 20:39 ` Joe Perches
2021-05-10 9:17 ` Michael Zaidman
2021-05-10 9:52 ` Joe Perches
2021-05-10 10:15 ` Dan Carpenter
2021-05-10 10:22 ` Dan Carpenter
2021-05-10 12:51 ` Michael Zaidman
2021-05-10 16:30 ` [PATCH v2] " Michael Zaidman
2021-05-10 16:41 ` Joe Perches
2021-05-10 16:34 ` [PATCH v3] " Michael Zaidman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).