When compiling with -Wformat, clang warns:
./include/linux/usb/wusb.h:245:5: warning: format specifies type
'unsigned short' but the argument has type 'u8' (aka 'unsigned char')
[-Wformat]
ckhdid->data[0], ckhdid->data[1],
^~~~~~~~~~~~~~~
ckhdid->data is unconditionally defined as `u8 data[16]`, so this patch
updates the format characters to the correct one for unsigned char types.
Link: https://github.com/ClangBuiltLinux/linux/issues/378
Signed-off-by: Louis Taylor <[email protected]>
---
include/linux/usb/wusb.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/usb/wusb.h b/include/linux/usb/wusb.h
index 9e4a3213f2c2..0a3cdf10972d 100644
--- a/include/linux/usb/wusb.h
+++ b/include/linux/usb/wusb.h
@@ -240,8 +240,8 @@ static inline size_t ckhdid_printf(char *pr_ckhdid, size_t size,
const struct wusb_ckhdid *ckhdid)
{
return scnprintf(pr_ckhdid, size,
- "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx "
- "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx",
+ "%02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu "
+ "%02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu",
ckhdid->data[0], ckhdid->data[1],
ckhdid->data[2], ckhdid->data[3],
ckhdid->data[4], ckhdid->data[5],
--
2.20.1
When compiling with -Wformat, clang warns:
./include/linux/usb/wusb.h:245:5: warning: format specifies type
'unsigned short' but the argument has type 'u8' (aka 'unsigned char')
[-Wformat]
ckhdid->data[0], ckhdid->data[1],
^~~~~~~~~~~~~~~
ckhdid->data is unconditionally defined as `u8 data[16]`, so this patch
updates the format characters to the correct one for unsigned char types.
Link: https://github.com/ClangBuiltLinux/linux/issues/378
Signed-off-by: Louis Taylor <[email protected]>
---
include/linux/usb/wusb.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/usb/wusb.h b/include/linux/usb/wusb.h
index 9e4a3213f2c2..625366d3499e 100644
--- a/include/linux/usb/wusb.h
+++ b/include/linux/usb/wusb.h
@@ -240,8 +240,8 @@ static inline size_t ckhdid_printf(char *pr_ckhdid, size_t size,
const struct wusb_ckhdid *ckhdid)
{
return scnprintf(pr_ckhdid, size,
- "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx "
- "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx",
+ "%02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx "
+ "%02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx",
ckhdid->data[0], ckhdid->data[1],
ckhdid->data[2], ckhdid->data[3],
ckhdid->data[4], ckhdid->data[5],
--
2.20.1
On Thu, Feb 28, 2019 at 11:52:56AM +0000, Louis Taylor wrote:
> When compiling with -Wformat, clang warns:
>
> ./include/linux/usb/wusb.h:245:5: warning: format specifies type
> 'unsigned short' but the argument has type 'u8' (aka 'unsigned char')
> [-Wformat]
> ckhdid->data[0], ckhdid->data[1],
> ^~~~~~~~~~~~~~~
>
> ckhdid->data is unconditionally defined as `u8 data[16]`, so this patch
> updates the format characters to the correct one for unsigned char types.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Signed-off-by: Louis Taylor <[email protected]>
> ---
> include/linux/usb/wusb.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/usb/wusb.h b/include/linux/usb/wusb.h
> index 9e4a3213f2c2..0a3cdf10972d 100644
> --- a/include/linux/usb/wusb.h
> +++ b/include/linux/usb/wusb.h
> @@ -240,8 +240,8 @@ static inline size_t ckhdid_printf(char *pr_ckhdid, size_t size,
> const struct wusb_ckhdid *ckhdid)
> {
> return scnprintf(pr_ckhdid, size,
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx "
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx",
> + "%02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu "
> + "%02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu",
Why did you switch from hex to decimal in the output format? That feels
very wrong to me.
greg k-h
On Thu, 2019-02-28 at 12:24 +0000, Louis Taylor wrote:
> When compiling with -Wformat, clang warns:
> ./include/linux/usb/wusb.h:245:5: warning: format specifies type
> 'unsigned short' but the argument has type 'u8' (aka 'unsigned char')
> [-Wformat]
> ckhdid->data[0], ckhdid->data[1],
> ^~~~~~~~~~~~~~~
I think the message is somewhat misguided as all the
vararg arguments have implicit integer promotions.
> ckhdid->data is unconditionally defined as `u8 data[16]`, so this patch
> updates the format characters to the correct one for unsigned char types.
[]
> diff --git a/include/linux/usb/wusb.h b/include/linux/usb/wusb.h
[]
> @@ -240,8 +240,8 @@ static inline size_t ckhdid_printf(char *pr_ckhdid, size_t size,
> const struct wusb_ckhdid *ckhdid)
> {
> return scnprintf(pr_ckhdid, size,
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx "
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx",
> + "%02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx "
> + "%02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx",
> ckhdid->data[0], ckhdid->data[1],
> ckhdid->data[2], ckhdid->data[3],
> ckhdid->data[4], ckhdid->data[5],
Better to use the vsprintf %ph extension insead.
---
include/linux/usb/wusb.h | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/include/linux/usb/wusb.h b/include/linux/usb/wusb.h
index 9e4a3213f2c2..8c39ddf62951 100644
--- a/include/linux/usb/wusb.h
+++ b/include/linux/usb/wusb.h
@@ -239,17 +239,7 @@ enum {
static inline size_t ckhdid_printf(char *pr_ckhdid, size_t size,
const struct wusb_ckhdid *ckhdid)
{
- return scnprintf(pr_ckhdid, size,
- "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx "
- "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx",
- ckhdid->data[0], ckhdid->data[1],
- ckhdid->data[2], ckhdid->data[3],
- ckhdid->data[4], ckhdid->data[5],
- ckhdid->data[6], ckhdid->data[7],
- ckhdid->data[8], ckhdid->data[9],
- ckhdid->data[10], ckhdid->data[11],
- ckhdid->data[12], ckhdid->data[13],
- ckhdid->data[14], ckhdid->data[15]);
+ return scnprintf(pr_ckhdid, size, "%16ph", ckhdid->data);
}
/*
Thanks for the patch and comments.
On Thu, Feb 28, 2019 at 1:53 PM Joe Perches <[email protected]> wrote:
>
> On Thu, 2019-02-28 at 12:24 +0000, Louis Taylor wrote:
> > When compiling with -Wformat, clang warns:
> > ./include/linux/usb/wusb.h:245:5: warning: format specifies type
> > 'unsigned short' but the argument has type 'u8' (aka 'unsigned char')
> > [-Wformat]
> > ckhdid->data[0], ckhdid->data[1],
> > ^~~~~~~~~~~~~~~
>
> I think the message is somewhat misguided as all the
> vararg arguments have implicit integer promotions.
That's a fair point, but Clang checks the arguments against their
format specifier before they're promoted when using -Wformat. When
considering integer promotions it's difficult to say if this is
"wrong", but since 'unsigned char' corresponds to the "hh" length
specifier I don't think this is misguided. Otherwise, why use the "h"
length specifier at all?
>
> > ckhdid->data is unconditionally defined as `u8 data[16]`, so this patch
> > updates the format characters to the correct one for unsigned char types.
> []
> > diff --git a/include/linux/usb/wusb.h b/include/linux/usb/wusb.h
> []
> > @@ -240,8 +240,8 @@ static inline size_t ckhdid_printf(char *pr_ckhdid, size_t size,
> > const struct wusb_ckhdid *ckhdid)
> > {
> > return scnprintf(pr_ckhdid, size,
> > - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx "
> > - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx",
> > + "%02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx "
> > + "%02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx",
> > ckhdid->data[0], ckhdid->data[1],
> > ckhdid->data[2], ckhdid->data[3],
> > ckhdid->data[4], ckhdid->data[5],
>
> Better to use the vsprintf %ph extension insead.
Agreed, so I guess my previous comment is irrelevant in this scenario.
> ---
> include/linux/usb/wusb.h | 12 +-----------
> 1 file changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/include/linux/usb/wusb.h b/include/linux/usb/wusb.h
> index 9e4a3213f2c2..8c39ddf62951 100644
> --- a/include/linux/usb/wusb.h
> +++ b/include/linux/usb/wusb.h
> @@ -239,17 +239,7 @@ enum {
> static inline size_t ckhdid_printf(char *pr_ckhdid, size_t size,
> const struct wusb_ckhdid *ckhdid)
> {
> - return scnprintf(pr_ckhdid, size,
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx "
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx",
> - ckhdid->data[0], ckhdid->data[1],
> - ckhdid->data[2], ckhdid->data[3],
> - ckhdid->data[4], ckhdid->data[5],
> - ckhdid->data[6], ckhdid->data[7],
> - ckhdid->data[8], ckhdid->data[9],
> - ckhdid->data[10], ckhdid->data[11],
> - ckhdid->data[12], ckhdid->data[13],
> - ckhdid->data[14], ckhdid->data[15]);
> + return scnprintf(pr_ckhdid, size, "%16ph", ckhdid->data);
> }
>
> /*
>
>
On Thu, Feb 28, 2019 at 4:28 AM Louis Taylor <[email protected]> wrote:
>
> When compiling with -Wformat, clang warns:
>
> ./include/linux/usb/wusb.h:245:5: warning: format specifies type
> 'unsigned short' but the argument has type 'u8' (aka 'unsigned char')
We should probably update Documentation/core-api/printk-formats.rst to
list [u|s][8|16] printk formats so that I don't have to go read
lib/vsprintf.c#format_decode(). (TODO in a separate patch)
> [-Wformat]
> ckhdid->data[0], ckhdid->data[1],
> ^~~~~~~~~~~~~~~
>
> ckhdid->data is unconditionally defined as `u8 data[16]`, so this patch
> updates the format characters to the correct one for unsigned char types.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Signed-off-by: Louis Taylor <[email protected]>
> ---
> include/linux/usb/wusb.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/usb/wusb.h b/include/linux/usb/wusb.h
> index 9e4a3213f2c2..625366d3499e 100644
> --- a/include/linux/usb/wusb.h
> +++ b/include/linux/usb/wusb.h
> @@ -240,8 +240,8 @@ static inline size_t ckhdid_printf(char *pr_ckhdid, size_t size,
> const struct wusb_ckhdid *ckhdid)
> {
> return scnprintf(pr_ckhdid, size,
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx "
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx",
> + "%02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx "
> + "%02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx %02hhx",
Looks like lib/vsprintf.c#format_decode() accepts either %hh or %H for
lone unsigned bytes, IIUC.
Thanks for the patch and for following up on the feedback.
Reviewed-by: Nick Desaulniers <[email protected]>
I wonder if __attribute__((packed)) is needed on the definition of
`struct wusb_ckhdid`? hmm...
via Stephen: https://godbolt.org/z/vs5JNo
Looks like alignof(struct wusb_ckhdid) is 1. And struct wusb_ckhdid
was introduced as is in commit c7f736484f8e ("wusb: add the Wireless
USB include files.") so it's not like it ever had additional members
that would disturb the alignment. `__aligned(__alignof__(u8))` might
be clearer than `__attribute__((packed))`, but even then, I don't
think anything is necessary since the alignof should always be 1?
> ckhdid->data[0], ckhdid->data[1],
> ckhdid->data[2], ckhdid->data[3],
> ckhdid->data[4], ckhdid->data[5],
> --
> 2.20.1
>
--
Thanks,
~Nick Desaulniers
On Thu, 2019-02-28 at 14:23 -0800, Jon Flatley wrote:
> Thanks for the patch and comments.
>
> On Thu, Feb 28, 2019 at 1:53 PM Joe Perches <[email protected]> wrote:
> > On Thu, 2019-02-28 at 12:24 +0000, Louis Taylor wrote:
> > > When compiling with -Wformat, clang warns:
> > > ./include/linux/usb/wusb.h:245:5: warning: format specifies type
> > > 'unsigned short' but the argument has type 'u8' (aka 'unsigned char')
> > > [-Wformat]
> > > ckhdid->data[0], ckhdid->data[1],
> > > ^~~~~~~~~~~~~~~
> >
> > I think the message is somewhat misguided as all the
> > vararg arguments have implicit integer promotions.
>
> That's a fair point, but Clang checks the arguments against their
> format specifier before they're promoted when using -Wformat.
Perhaps clang could be a bit more verbose if
checking signed types emitted as unsigned or
unsigned types emitted as signed instead.
> When
> considering integer promotions it's difficult to say if this is
> "wrong",
I didn't write "wrong", I wrote misguided.
> but since 'unsigned char' corresponds to the "hh" length
> specifier I don't think this is misguided. Otherwise, why use the "h"
> length specifier at all?
e.g.: signed char as %x needs %hhx
cheers, Joe
On Thu, Feb 28, 2019 at 3:05 PM Joe Perches <[email protected]> wrote:
>
> On Thu, 2019-02-28 at 14:23 -0800, Jon Flatley wrote:
> > Thanks for the patch and comments.
> >
> > On Thu, Feb 28, 2019 at 1:53 PM Joe Perches <[email protected]> wrote:
> > > On Thu, 2019-02-28 at 12:24 +0000, Louis Taylor wrote:
> > > > When compiling with -Wformat, clang warns:
> > > > ./include/linux/usb/wusb.h:245:5: warning: format specifies type
> > > > 'unsigned short' but the argument has type 'u8' (aka 'unsigned char')
> > > > [-Wformat]
> > > > ckhdid->data[0], ckhdid->data[1],
> > > > ^~~~~~~~~~~~~~~
> > >
> > > I think the message is somewhat misguided as all the
> > > vararg arguments have implicit integer promotions.
> >
> > That's a fair point, but Clang checks the arguments against their
> > format specifier before they're promoted when using -Wformat.
>
> Perhaps clang could be a bit more verbose if
> checking signed types emitted as unsigned or
> unsigned types emitted as signed instead.
It is a little strange that clang warns when the length specifier
doesn't match but not when an unsigned specifier is used for a signed
value and vice versa.
>
> > When
> > considering integer promotions it's difficult to say if this is
> > "wrong",
>
> I didn't write "wrong", I wrote misguided.
Apologies for my poor wording. I meant "wrong" in the sense that it's
unclear if an improper length specifier is deserving of a warning.
After all GCC doesn't warn for incorrect length specifiers, and as you
pointed out Clang doesn't pay attention to if the specifier expects a
signed or unsigned value.
Cheers,
Jon
This static inline is unnecessary and can be removed
by using the vsprintf %ph extension.
This reduces overall object size by more than 2K.
Signed-off-by: Joe Perches <[email protected]>
---
drivers/usb/wusbcore/cbaf.c | 15 ++++-----------
drivers/usb/wusbcore/dev-sysfs.c | 5 ++---
drivers/usb/wusbcore/devconnect.c | 2 +-
drivers/usb/wusbcore/wusbhc.c | 6 +-----
include/linux/usb/wusb.h | 16 ----------------
5 files changed, 8 insertions(+), 36 deletions(-)
diff --git a/drivers/usb/wusbcore/cbaf.c b/drivers/usb/wusbcore/cbaf.c
index 222228c5c1e1..af77064c7456 100644
--- a/drivers/usb/wusbcore/cbaf.c
+++ b/drivers/usb/wusbcore/cbaf.c
@@ -302,10 +302,8 @@ static ssize_t cbaf_wusb_chid_show(struct device *dev,
{
struct usb_interface *iface = to_usb_interface(dev);
struct cbaf *cbaf = usb_get_intfdata(iface);
- char pr_chid[WUSB_CKHDID_STRSIZE];
- ckhdid_printf(pr_chid, sizeof(pr_chid), &cbaf->chid);
- return scnprintf(buf, PAGE_SIZE, "%s\n", pr_chid);
+ return sprintf(buf, "%16ph\n", cbaf->chid.data);
}
static ssize_t cbaf_wusb_chid_store(struct device *dev,
@@ -415,10 +413,8 @@ static ssize_t cbaf_wusb_cdid_show(struct device *dev,
{
struct usb_interface *iface = to_usb_interface(dev);
struct cbaf *cbaf = usb_get_intfdata(iface);
- char pr_cdid[WUSB_CKHDID_STRSIZE];
- ckhdid_printf(pr_cdid, sizeof(pr_cdid), &cbaf->cdid);
- return scnprintf(buf, PAGE_SIZE, "%s\n", pr_cdid);
+ return sprintf(buf, "%16ph\n", cbaf->cdid.data);
}
static ssize_t cbaf_wusb_cdid_store(struct device *dev,
@@ -503,7 +499,6 @@ static int cbaf_cc_upload(struct cbaf *cbaf)
int result;
struct device *dev = &cbaf->usb_iface->dev;
struct wusb_cbaf_cc_data *ccd;
- char pr_cdid[WUSB_CKHDID_STRSIZE];
ccd = cbaf->buffer;
*ccd = cbaf_cc_data_defaults;
@@ -513,10 +508,8 @@ static int cbaf_cc_upload(struct cbaf *cbaf)
ccd->BandGroups = cpu_to_le16(cbaf->host_band_groups);
dev_dbg(dev, "Trying to upload CC:\n");
- ckhdid_printf(pr_cdid, sizeof(pr_cdid), &ccd->CHID);
- dev_dbg(dev, " CHID %s\n", pr_cdid);
- ckhdid_printf(pr_cdid, sizeof(pr_cdid), &ccd->CDID);
- dev_dbg(dev, " CDID %s\n", pr_cdid);
+ dev_dbg(dev, " CHID %16ph\n", ccd->CHID.data);
+ dev_dbg(dev, " CDID %16ph\n", ccd->CDID.data);
dev_dbg(dev, " Bandgroups 0x%04x\n", cbaf->host_band_groups);
result = usb_control_msg(
diff --git a/drivers/usb/wusbcore/dev-sysfs.c b/drivers/usb/wusbcore/dev-sysfs.c
index 85a1acf3a729..67b0a4c412b2 100644
--- a/drivers/usb/wusbcore/dev-sysfs.c
+++ b/drivers/usb/wusbcore/dev-sysfs.c
@@ -50,10 +50,9 @@ static ssize_t wusb_cdid_show(struct device *dev,
wusb_dev = wusb_dev_get_by_usb_dev(to_usb_device(dev));
if (wusb_dev == NULL)
return -ENODEV;
- result = ckhdid_printf(buf, PAGE_SIZE, &wusb_dev->cdid);
- strcat(buf, "\n");
+ result = sprintf(buf, "%16ph\n", wusb_dev->cdid.data);
wusb_dev_put(wusb_dev);
- return result + 1;
+ return result;
}
static DEVICE_ATTR_RO(wusb_cdid);
diff --git a/drivers/usb/wusbcore/devconnect.c b/drivers/usb/wusbcore/devconnect.c
index fcb06aef2675..a93837d57d53 100644
--- a/drivers/usb/wusbcore/devconnect.c
+++ b/drivers/usb/wusbcore/devconnect.c
@@ -532,7 +532,7 @@ static void wusbhc_handle_dn_connect(struct wusbhc *wusbhc,
}
dnc = container_of(dn_hdr, struct wusb_dn_connect, hdr);
- ckhdid_printf(pr_cdid, sizeof(pr_cdid), &dnc->CDID);
+ sprintf(pr_cdid, "%16ph", dnc->CDID.data);
dev_info(dev, "DN CONNECT: device %s @ %x (%s) wants to %s\n",
pr_cdid,
wusb_dn_connect_prev_dev_addr(dnc),
diff --git a/drivers/usb/wusbcore/wusbhc.c b/drivers/usb/wusbcore/wusbhc.c
index e5ba6140c1ba..d0b404d258e8 100644
--- a/drivers/usb/wusbcore/wusbhc.c
+++ b/drivers/usb/wusbcore/wusbhc.c
@@ -80,17 +80,13 @@ static ssize_t wusb_chid_show(struct device *dev,
{
struct wusbhc *wusbhc = usbhc_dev_to_wusbhc(dev);
const struct wusb_ckhdid *chid;
- ssize_t result = 0;
if (wusbhc->wuie_host_info != NULL)
chid = &wusbhc->wuie_host_info->CHID;
else
chid = &wusb_ckhdid_zero;
- result += ckhdid_printf(buf, PAGE_SIZE, chid);
- result += sprintf(buf + result, "\n");
-
- return result;
+ return sprintf(buf, "%16ph\n", chid->data);
}
/*
diff --git a/include/linux/usb/wusb.h b/include/linux/usb/wusb.h
index 9e4a3213f2c2..65adee629106 100644
--- a/include/linux/usb/wusb.h
+++ b/include/linux/usb/wusb.h
@@ -236,22 +236,6 @@ enum {
WUSB_TRUST_TIMEOUT_MS = 4000, /* [WUSB] section 4.15.1 */
};
-static inline size_t ckhdid_printf(char *pr_ckhdid, size_t size,
- const struct wusb_ckhdid *ckhdid)
-{
- return scnprintf(pr_ckhdid, size,
- "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx "
- "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx",
- ckhdid->data[0], ckhdid->data[1],
- ckhdid->data[2], ckhdid->data[3],
- ckhdid->data[4], ckhdid->data[5],
- ckhdid->data[6], ckhdid->data[7],
- ckhdid->data[8], ckhdid->data[9],
- ckhdid->data[10], ckhdid->data[11],
- ckhdid->data[12], ckhdid->data[13],
- ckhdid->data[14], ckhdid->data[15]);
-}
-
/*
* WUSB Crypto stuff (WUSB1.0[6])
*/
On Thu, Feb 28, 2019 at 08:38:16PM -0800, Joe Perches wrote:
> This static inline is unnecessary and can be removed
> by using the vsprintf %ph extension.
>
> This reduces overall object size by more than 2K.
>
> Signed-off-by: Joe Perches <[email protected]>
> ---
> drivers/usb/wusbcore/cbaf.c | 15 ++++-----------
> drivers/usb/wusbcore/dev-sysfs.c | 5 ++---
> drivers/usb/wusbcore/devconnect.c | 2 +-
> drivers/usb/wusbcore/wusbhc.c | 6 +-----
> include/linux/usb/wusb.h | 16 ----------------
> 5 files changed, 8 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/usb/wusbcore/cbaf.c b/drivers/usb/wusbcore/cbaf.c
> index 222228c5c1e1..af77064c7456 100644
> --- a/drivers/usb/wusbcore/cbaf.c
> +++ b/drivers/usb/wusbcore/cbaf.c
> @@ -302,10 +302,8 @@ static ssize_t cbaf_wusb_chid_show(struct device *dev,
> {
> struct usb_interface *iface = to_usb_interface(dev);
> struct cbaf *cbaf = usb_get_intfdata(iface);
> - char pr_chid[WUSB_CKHDID_STRSIZE];
>
> - ckhdid_printf(pr_chid, sizeof(pr_chid), &cbaf->chid);
> - return scnprintf(buf, PAGE_SIZE, "%s\n", pr_chid);
> + return sprintf(buf, "%16ph\n", cbaf->chid.data);
> }
>
> static ssize_t cbaf_wusb_chid_store(struct device *dev,
> @@ -415,10 +413,8 @@ static ssize_t cbaf_wusb_cdid_show(struct device *dev,
> {
> struct usb_interface *iface = to_usb_interface(dev);
> struct cbaf *cbaf = usb_get_intfdata(iface);
> - char pr_cdid[WUSB_CKHDID_STRSIZE];
>
> - ckhdid_printf(pr_cdid, sizeof(pr_cdid), &cbaf->cdid);
> - return scnprintf(buf, PAGE_SIZE, "%s\n", pr_cdid);
> + return sprintf(buf, "%16ph\n", cbaf->cdid.data);
> }
>
> static ssize_t cbaf_wusb_cdid_store(struct device *dev,
> @@ -503,7 +499,6 @@ static int cbaf_cc_upload(struct cbaf *cbaf)
> int result;
> struct device *dev = &cbaf->usb_iface->dev;
> struct wusb_cbaf_cc_data *ccd;
> - char pr_cdid[WUSB_CKHDID_STRSIZE];
>
> ccd = cbaf->buffer;
> *ccd = cbaf_cc_data_defaults;
> @@ -513,10 +508,8 @@ static int cbaf_cc_upload(struct cbaf *cbaf)
> ccd->BandGroups = cpu_to_le16(cbaf->host_band_groups);
>
> dev_dbg(dev, "Trying to upload CC:\n");
> - ckhdid_printf(pr_cdid, sizeof(pr_cdid), &ccd->CHID);
> - dev_dbg(dev, " CHID %s\n", pr_cdid);
> - ckhdid_printf(pr_cdid, sizeof(pr_cdid), &ccd->CDID);
> - dev_dbg(dev, " CDID %s\n", pr_cdid);
> + dev_dbg(dev, " CHID %16ph\n", ccd->CHID.data);
> + dev_dbg(dev, " CDID %16ph\n", ccd->CDID.data);
> dev_dbg(dev, " Bandgroups 0x%04x\n", cbaf->host_band_groups);
>
> result = usb_control_msg(
> diff --git a/drivers/usb/wusbcore/dev-sysfs.c b/drivers/usb/wusbcore/dev-sysfs.c
> index 85a1acf3a729..67b0a4c412b2 100644
> --- a/drivers/usb/wusbcore/dev-sysfs.c
> +++ b/drivers/usb/wusbcore/dev-sysfs.c
> @@ -50,10 +50,9 @@ static ssize_t wusb_cdid_show(struct device *dev,
> wusb_dev = wusb_dev_get_by_usb_dev(to_usb_device(dev));
> if (wusb_dev == NULL)
> return -ENODEV;
> - result = ckhdid_printf(buf, PAGE_SIZE, &wusb_dev->cdid);
> - strcat(buf, "\n");
> + result = sprintf(buf, "%16ph\n", wusb_dev->cdid.data);
> wusb_dev_put(wusb_dev);
> - return result + 1;
> + return result;
> }
> static DEVICE_ATTR_RO(wusb_cdid);
>
> diff --git a/drivers/usb/wusbcore/devconnect.c b/drivers/usb/wusbcore/devconnect.c
> index fcb06aef2675..a93837d57d53 100644
> --- a/drivers/usb/wusbcore/devconnect.c
> +++ b/drivers/usb/wusbcore/devconnect.c
> @@ -532,7 +532,7 @@ static void wusbhc_handle_dn_connect(struct wusbhc *wusbhc,
> }
>
> dnc = container_of(dn_hdr, struct wusb_dn_connect, hdr);
> - ckhdid_printf(pr_cdid, sizeof(pr_cdid), &dnc->CDID);
> + sprintf(pr_cdid, "%16ph", dnc->CDID.data);
> dev_info(dev, "DN CONNECT: device %s @ %x (%s) wants to %s\n",
> pr_cdid,
> wusb_dn_connect_prev_dev_addr(dnc),
> diff --git a/drivers/usb/wusbcore/wusbhc.c b/drivers/usb/wusbcore/wusbhc.c
> index e5ba6140c1ba..d0b404d258e8 100644
> --- a/drivers/usb/wusbcore/wusbhc.c
> +++ b/drivers/usb/wusbcore/wusbhc.c
> @@ -80,17 +80,13 @@ static ssize_t wusb_chid_show(struct device *dev,
> {
> struct wusbhc *wusbhc = usbhc_dev_to_wusbhc(dev);
> const struct wusb_ckhdid *chid;
> - ssize_t result = 0;
>
> if (wusbhc->wuie_host_info != NULL)
> chid = &wusbhc->wuie_host_info->CHID;
> else
> chid = &wusb_ckhdid_zero;
>
> - result += ckhdid_printf(buf, PAGE_SIZE, chid);
> - result += sprintf(buf + result, "\n");
> -
> - return result;
> + return sprintf(buf, "%16ph\n", chid->data);
> }
>
> /*
> diff --git a/include/linux/usb/wusb.h b/include/linux/usb/wusb.h
> index 9e4a3213f2c2..65adee629106 100644
> --- a/include/linux/usb/wusb.h
> +++ b/include/linux/usb/wusb.h
> @@ -236,22 +236,6 @@ enum {
> WUSB_TRUST_TIMEOUT_MS = 4000, /* [WUSB] section 4.15.1 */
> };
>
> -static inline size_t ckhdid_printf(char *pr_ckhdid, size_t size,
> - const struct wusb_ckhdid *ckhdid)
> -{
> - return scnprintf(pr_ckhdid, size,
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx "
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx",
> - ckhdid->data[0], ckhdid->data[1],
> - ckhdid->data[2], ckhdid->data[3],
> - ckhdid->data[4], ckhdid->data[5],
> - ckhdid->data[6], ckhdid->data[7],
> - ckhdid->data[8], ckhdid->data[9],
> - ckhdid->data[10], ckhdid->data[11],
> - ckhdid->data[12], ckhdid->data[13],
> - ckhdid->data[14], ckhdid->data[15]);
> -}
This removes the original clang warning and seems like a nice cleanup.
Reviewed-by: Louis Taylor <[email protected]>
Cheers,
Louis
On Thu, Feb 28, 2019 at 8:38 PM Joe Perches <[email protected]> wrote:
>
> This static inline is unnecessary and can be removed
> by using the vsprintf %ph extension.
>
> This reduces overall object size by more than 2K.
>
> Signed-off-by: Joe Perches <[email protected]>
Deserves a reported by tag from (https://lkml.org/lkml/2019/2/28/1138)
Reported-by: Louis Taylor <[email protected]>
> ---
> drivers/usb/wusbcore/cbaf.c | 15 ++++-----------
> drivers/usb/wusbcore/dev-sysfs.c | 5 ++---
> drivers/usb/wusbcore/devconnect.c | 2 +-
> drivers/usb/wusbcore/wusbhc.c | 6 +-----
> include/linux/usb/wusb.h | 16 ----------------
> 5 files changed, 8 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/usb/wusbcore/cbaf.c b/drivers/usb/wusbcore/cbaf.c
> index 222228c5c1e1..af77064c7456 100644
> --- a/drivers/usb/wusbcore/cbaf.c
> +++ b/drivers/usb/wusbcore/cbaf.c
> @@ -302,10 +302,8 @@ static ssize_t cbaf_wusb_chid_show(struct device *dev,
> {
> struct usb_interface *iface = to_usb_interface(dev);
> struct cbaf *cbaf = usb_get_intfdata(iface);
> - char pr_chid[WUSB_CKHDID_STRSIZE];
>
> - ckhdid_printf(pr_chid, sizeof(pr_chid), &cbaf->chid);
> - return scnprintf(buf, PAGE_SIZE, "%s\n", pr_chid);
> + return sprintf(buf, "%16ph\n", cbaf->chid.data);
> }
>
> static ssize_t cbaf_wusb_chid_store(struct device *dev,
> @@ -415,10 +413,8 @@ static ssize_t cbaf_wusb_cdid_show(struct device *dev,
> {
> struct usb_interface *iface = to_usb_interface(dev);
> struct cbaf *cbaf = usb_get_intfdata(iface);
> - char pr_cdid[WUSB_CKHDID_STRSIZE];
>
> - ckhdid_printf(pr_cdid, sizeof(pr_cdid), &cbaf->cdid);
> - return scnprintf(buf, PAGE_SIZE, "%s\n", pr_cdid);
> + return sprintf(buf, "%16ph\n", cbaf->cdid.data);
> }
Great simplification overall. If you wanted to push further,
cbaf_wusb_c{h|d}id_show() are basically (almost) the same function
now. They probably could share more code. Everything else looks
good, thanks for the patch.
Reviewed-by: Nick Desaulniers <[email protected]>
>
> static ssize_t cbaf_wusb_cdid_store(struct device *dev,
> @@ -503,7 +499,6 @@ static int cbaf_cc_upload(struct cbaf *cbaf)
> int result;
> struct device *dev = &cbaf->usb_iface->dev;
> struct wusb_cbaf_cc_data *ccd;
> - char pr_cdid[WUSB_CKHDID_STRSIZE];
>
> ccd = cbaf->buffer;
> *ccd = cbaf_cc_data_defaults;
> @@ -513,10 +508,8 @@ static int cbaf_cc_upload(struct cbaf *cbaf)
> ccd->BandGroups = cpu_to_le16(cbaf->host_band_groups);
>
> dev_dbg(dev, "Trying to upload CC:\n");
> - ckhdid_printf(pr_cdid, sizeof(pr_cdid), &ccd->CHID);
> - dev_dbg(dev, " CHID %s\n", pr_cdid);
> - ckhdid_printf(pr_cdid, sizeof(pr_cdid), &ccd->CDID);
> - dev_dbg(dev, " CDID %s\n", pr_cdid);
> + dev_dbg(dev, " CHID %16ph\n", ccd->CHID.data);
> + dev_dbg(dev, " CDID %16ph\n", ccd->CDID.data);
> dev_dbg(dev, " Bandgroups 0x%04x\n", cbaf->host_band_groups);
>
> result = usb_control_msg(
> diff --git a/drivers/usb/wusbcore/dev-sysfs.c b/drivers/usb/wusbcore/dev-sysfs.c
> index 85a1acf3a729..67b0a4c412b2 100644
> --- a/drivers/usb/wusbcore/dev-sysfs.c
> +++ b/drivers/usb/wusbcore/dev-sysfs.c
> @@ -50,10 +50,9 @@ static ssize_t wusb_cdid_show(struct device *dev,
> wusb_dev = wusb_dev_get_by_usb_dev(to_usb_device(dev));
> if (wusb_dev == NULL)
> return -ENODEV;
> - result = ckhdid_printf(buf, PAGE_SIZE, &wusb_dev->cdid);
> - strcat(buf, "\n");
> + result = sprintf(buf, "%16ph\n", wusb_dev->cdid.data);
> wusb_dev_put(wusb_dev);
> - return result + 1;
> + return result;
> }
> static DEVICE_ATTR_RO(wusb_cdid);
>
> diff --git a/drivers/usb/wusbcore/devconnect.c b/drivers/usb/wusbcore/devconnect.c
> index fcb06aef2675..a93837d57d53 100644
> --- a/drivers/usb/wusbcore/devconnect.c
> +++ b/drivers/usb/wusbcore/devconnect.c
> @@ -532,7 +532,7 @@ static void wusbhc_handle_dn_connect(struct wusbhc *wusbhc,
> }
>
> dnc = container_of(dn_hdr, struct wusb_dn_connect, hdr);
> - ckhdid_printf(pr_cdid, sizeof(pr_cdid), &dnc->CDID);
> + sprintf(pr_cdid, "%16ph", dnc->CDID.data);
> dev_info(dev, "DN CONNECT: device %s @ %x (%s) wants to %s\n",
> pr_cdid,
> wusb_dn_connect_prev_dev_addr(dnc),
> diff --git a/drivers/usb/wusbcore/wusbhc.c b/drivers/usb/wusbcore/wusbhc.c
> index e5ba6140c1ba..d0b404d258e8 100644
> --- a/drivers/usb/wusbcore/wusbhc.c
> +++ b/drivers/usb/wusbcore/wusbhc.c
> @@ -80,17 +80,13 @@ static ssize_t wusb_chid_show(struct device *dev,
> {
> struct wusbhc *wusbhc = usbhc_dev_to_wusbhc(dev);
> const struct wusb_ckhdid *chid;
> - ssize_t result = 0;
>
> if (wusbhc->wuie_host_info != NULL)
> chid = &wusbhc->wuie_host_info->CHID;
> else
> chid = &wusb_ckhdid_zero;
>
> - result += ckhdid_printf(buf, PAGE_SIZE, chid);
> - result += sprintf(buf + result, "\n");
> -
> - return result;
> + return sprintf(buf, "%16ph\n", chid->data);
> }
>
> /*
> diff --git a/include/linux/usb/wusb.h b/include/linux/usb/wusb.h
> index 9e4a3213f2c2..65adee629106 100644
> --- a/include/linux/usb/wusb.h
> +++ b/include/linux/usb/wusb.h
> @@ -236,22 +236,6 @@ enum {
> WUSB_TRUST_TIMEOUT_MS = 4000, /* [WUSB] section 4.15.1 */
> };
>
> -static inline size_t ckhdid_printf(char *pr_ckhdid, size_t size,
> - const struct wusb_ckhdid *ckhdid)
> -{
> - return scnprintf(pr_ckhdid, size,
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx "
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx",
> - ckhdid->data[0], ckhdid->data[1],
> - ckhdid->data[2], ckhdid->data[3],
> - ckhdid->data[4], ckhdid->data[5],
> - ckhdid->data[6], ckhdid->data[7],
> - ckhdid->data[8], ckhdid->data[9],
> - ckhdid->data[10], ckhdid->data[11],
> - ckhdid->data[12], ckhdid->data[13],
> - ckhdid->data[14], ckhdid->data[15]);
> -}
> -
> /*
> * WUSB Crypto stuff (WUSB1.0[6])
> */
>
>
--
Thanks,
~Nick Desaulniers
On Fri, Mar 01, 2019 at 10:18:32AM -0800, Nick Desaulniers wrote:
> On Thu, Feb 28, 2019 at 8:38 PM Joe Perches <[email protected]> wrote:
> >
> > This static inline is unnecessary and can be removed
> > by using the vsprintf %ph extension.
> >
> > This reduces overall object size by more than 2K.
> >
> > Signed-off-by: Joe Perches <[email protected]>
>
> Deserves a reported by tag from (https://lkml.org/lkml/2019/2/28/1138)
> Reported-by: Louis Taylor <[email protected]>
Good idea, will go add that...
Hallo
Am 28.02.19 um 12:52 schrieb Louis Taylor:
> When compiling with -Wformat, clang warns:
>
> ./include/linux/usb/wusb.h:245:5: warning: format specifies type
> 'unsigned short' but the argument has type 'u8' (aka 'unsigned char')
> [-Wformat]
> ckhdid->data[0], ckhdid->data[1],
> ^~~~~~~~~~~~~~~
>
> ckhdid->data is unconditionally defined as `u8 data[16]`, so this patch
> updates the format characters to the correct one for unsigned char types.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Signed-off-by: Louis Taylor <[email protected]>
> ---
> include/linux/usb/wusb.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/usb/wusb.h b/include/linux/usb/wusb.h
> index 9e4a3213f2c2..0a3cdf10972d 100644
> --- a/include/linux/usb/wusb.h
> +++ b/include/linux/usb/wusb.h
> @@ -240,8 +240,8 @@ static inline size_t ckhdid_printf(char *pr_ckhdid, size_t size,
> const struct wusb_ckhdid *ckhdid)
> {
> return scnprintf(pr_ckhdid, size,
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx "
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx",
> + "%02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu "
> + "%02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu",
> ckhdid->data[0], ckhdid->data[1],
> ckhdid->data[2], ckhdid->data[3],
> ckhdid->data[4], ckhdid->data[5],
Test
Am 28.02.19 um 12:52 schrieb Louis Taylor:
> When compiling with -Wformat, clang warns:
>
> ./include/linux/usb/wusb.h:245:5: warning: format specifies type
> 'unsigned short' but the argument has type 'u8' (aka 'unsigned char')
> [-Wformat]
> ckhdid->data[0], ckhdid->data[1],
> ^~~~~~~~~~~~~~~
>
> ckhdid->data is unconditionally defined as `u8 data[16]`, so this patch
> updates the format characters to the correct one for unsigned char types.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/378
> Signed-off-by: Louis Taylor <[email protected]>
> ---
> include/linux/usb/wusb.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/usb/wusb.h b/include/linux/usb/wusb.h
> index 9e4a3213f2c2..0a3cdf10972d 100644
> --- a/include/linux/usb/wusb.h
> +++ b/include/linux/usb/wusb.h
> @@ -240,8 +240,8 @@ static inline size_t ckhdid_printf(char *pr_ckhdid, size_t size,
> const struct wusb_ckhdid *ckhdid)
> {
> return scnprintf(pr_ckhdid, size,
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx "
> - "%02hx %02hx %02hx %02hx %02hx %02hx %02hx %02hx",
> + "%02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu "
> + "%02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu %02hhu",
> ckhdid->data[0], ckhdid->data[1],
> ckhdid->data[2], ckhdid->data[3],
> ckhdid->data[4], ckhdid->data[5],