| Message ID | 20210509193213.5974-1-michael.zaidman@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: ft260: fix format type warning in ft260_word_show() | expand |
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) \
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) \ >
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
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
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
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 > >
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) \
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(-)