[PATCH 0/5] vsprintf and uses: Add upper case output to %*ph extension

[PATCH 0/5] vsprintf and uses: Add upper case output to %*ph extension

[Joe Perches]

Several sysfs uses that could use %*ph are upper case hex output.
Add a flag to the short hex formatting routine in vsprintf to support them.
Add documentation too.

Joe Perches (5):
  vsprintf/Documentation: Add X to %*ph extension to output upper case hex
  scsi: aacraid: Use vsprintf %phNX extension
  scsi: hpsa: Use vsprintf %phNX extension
  scsi: smartpqi: Use vsprintf %phNX extension
  staging: r8188eu: Use vsprintf extension %phCX to format a copy_to_user string

 Documentation/core-api/printk-formats.rst    |  6 +++
 drivers/scsi/aacraid/linit.c                 |  7 +---
 drivers/scsi/hpsa.c                          |  8 +---
 drivers/scsi/smartpqi/smartpqi_init.c        |  8 +---
 drivers/staging/r8188eu/os_dep/ioctl_linux.c |  9 ++---
 lib/vsprintf.c                               | 42 ++++++++++++--------
 6 files changed, 37 insertions(+), 43 deletions(-)

-- 
2.30.0

[PATCH 5/5] staging: r8188eu: Use vsprintf extension %phCX to format a copy_to_user string

[Joe Perches]

This reduces object size without changing the string content.

compiled x86-64 defconfig w/ r8188eu and gcc 10.3.0

$ size drivers/staging/r8188eu/os_dep/ioctl_linux.o*
   text	   data	    bss	    dec	    hex	filename
  72556	   1548	      0	  74104	  12178	drivers/staging/r8188eu/os_dep/ioctl_linux.o.new
  72758	   1548	      0	  74306	  12242	drivers/staging/r8188eu/os_dep/ioctl_linux.o.old

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/staging/r8188eu/os_dep/ioctl_linux.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
index ab4a9200f0791..048164659d872 100644
--- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
+++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
@@ -2907,10 +2907,8 @@ static int rtw_p2p_get_groupid(struct net_device *dev,
 	struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
 	struct wifidirect_info *pwdinfo = &padapter->wdinfo;
 
-	sprintf(extra, "\n%.2X:%.2X:%.2X:%.2X:%.2X:%.2X %s",
-		pwdinfo->groupid_info.go_device_addr[0], pwdinfo->groupid_info.go_device_addr[1],
-		pwdinfo->groupid_info.go_device_addr[2], pwdinfo->groupid_info.go_device_addr[3],
-		pwdinfo->groupid_info.go_device_addr[4], pwdinfo->groupid_info.go_device_addr[5],
+	sprintf(extra, "\n%pM %s",
+		pwdinfo->groupid_info.go_device_addr,
 		pwdinfo->groupid_info.ssid);
 	wrqu->data.length = strlen(extra);
 	return ret;
@@ -3075,8 +3073,7 @@ static int rtw_p2p_get_go_device_address(struct net_device *dev,
 	if (!blnMatch)
 		sprintf(go_devadd_str, "\n\ndev_add = NULL");
 	else
-		sprintf(go_devadd_str, "\ndev_add =%.2X:%.2X:%.2X:%.2X:%.2X:%.2X",
-			attr_content[0], attr_content[1], attr_content[2], attr_content[3], attr_content[4], attr_content[5]);
+		sprintf(go_devadd_str, "\ndev_add =%6phCX", attr_content);
 
 	if (copy_to_user(wrqu->data.pointer, go_devadd_str, 10 + 17))
 		return -EFAULT;
-- 
2.30.0

Re: [PATCH 0/5] vsprintf and uses: Add upper case output to %*ph extension

[Andy Shevchenko]

On Thu, Aug 26, 2021 at 11:43:00AM -0700, Joe Perches wrote:
> Several sysfs uses that could use %*ph are upper case hex output.
> Add a flag to the short hex formatting routine in vsprintf to support them.
> Add documentation too.

Thanks!

Unfortunately I have got only first patch and this cover letter. Can you,
please, Cc entire series?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/5] vsprintf and uses: Add upper case output to %*ph extension

[Joe Perches]

On Fri, 2021-08-27 at 10:51 +0300, Andy Shevchenko wrote:
> On Thu, Aug 26, 2021 at 11:43:00AM -0700, Joe Perches wrote:
> > Several sysfs uses that could use %*ph are upper case hex output.
> > Add a flag to the short hex formatting routine in vsprintf to support them.
> > Add documentation too.
> 
> Thanks!
> 
> Unfortunately I have got only first patch and this cover letter. Can you,
> please, Cc entire series?

It's on lore.

https://lore.kernel.org/lkml/cover.1630003183.git.joe@perches.com/T/#u
 

Re: [PATCH 5/5] staging: r8188eu: Use vsprintf extension %phCX to format a copy_to_user string

[Greg Kroah-Hartman]

On Thu, Aug 26, 2021 at 11:43:05AM -0700, Joe Perches wrote:
> This reduces object size without changing the string content.
> 
> compiled x86-64 defconfig w/ r8188eu and gcc 10.3.0
> 
> $ size drivers/staging/r8188eu/os_dep/ioctl_linux.o*
>    text	   data	    bss	    dec	    hex	filename
>   72556	   1548	      0	  74104	  12178	drivers/staging/r8188eu/os_dep/ioctl_linux.o.new
>   72758	   1548	      0	  74306	  12242	drivers/staging/r8188eu/os_dep/ioctl_linux.o.old
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  drivers/staging/r8188eu/os_dep/ioctl_linux.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> index ab4a9200f0791..048164659d872 100644
> --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> @@ -2907,10 +2907,8 @@ static int rtw_p2p_get_groupid(struct net_device *dev,
>  	struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
>  	struct wifidirect_info *pwdinfo = &padapter->wdinfo;
>  
> -	sprintf(extra, "\n%.2X:%.2X:%.2X:%.2X:%.2X:%.2X %s",
> -		pwdinfo->groupid_info.go_device_addr[0], pwdinfo->groupid_info.go_device_addr[1],
> -		pwdinfo->groupid_info.go_device_addr[2], pwdinfo->groupid_info.go_device_addr[3],
> -		pwdinfo->groupid_info.go_device_addr[4], pwdinfo->groupid_info.go_device_addr[5],
> +	sprintf(extra, "\n%pM %s",
> +		pwdinfo->groupid_info.go_device_addr,
>  		pwdinfo->groupid_info.ssid);

We can just use the lower-case one here, no need for a new modifier just
for something like this (i.e. log file output)

thanks,

greg k-h

Re: [PATCH 0/5] vsprintf and uses: Add upper case output to %*ph extension

[Andy Shevchenko]

On Fri, Aug 27, 2021 at 01:10:41AM -0700, Joe Perches wrote:
> On Fri, 2021-08-27 at 10:51 +0300, Andy Shevchenko wrote:
> > On Thu, Aug 26, 2021 at 11:43:00AM -0700, Joe Perches wrote:
> > > Several sysfs uses that could use %*ph are upper case hex output.
> > > Add a flag to the short hex formatting routine in vsprintf to support them.
> > > Add documentation too.
> >
> > Thanks!
> >
> > Unfortunately I have got only first patch and this cover letter. Can you,
> > please, Cc entire series?
> 
> It's on lore.
> 
> https://lore.kernel.org/lkml/cover.1630003183.git.joe@perches.com/T/#u

Thanks. So, you won't me to review them in a regular way :-)

TBH, I think those examples may pretty much be safe to use small
letters always.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/5] vsprintf and uses: Add upper case output to %*ph extension

[Greg KH]

On Fri, Aug 27, 2021 at 11:46:20AM +0300, Andy Shevchenko wrote:
> On Fri, Aug 27, 2021 at 01:10:41AM -0700, Joe Perches wrote:
> > On Fri, 2021-08-27 at 10:51 +0300, Andy Shevchenko wrote:
> > > On Thu, Aug 26, 2021 at 11:43:00AM -0700, Joe Perches wrote:
> > > > Several sysfs uses that could use %*ph are upper case hex output.
> > > > Add a flag to the short hex formatting routine in vsprintf to support them.
> > > > Add documentation too.
> > >
> > > Thanks!
> > >
> > > Unfortunately I have got only first patch and this cover letter. Can you,
> > > please, Cc entire series?
> > 
> > It's on lore.
> > 
> > https://lore.kernel.org/lkml/cover.1630003183.git.joe@perches.com/T/#u
> 
> Thanks. So, you won't me to review them in a regular way :-)
> 
> TBH, I think those examples may pretty much be safe to use small
> letters always.

I agree, let's just fix the users here to use small letters instead of
adding another modifier to the kernel.

thanks,

greg k-h

Re: [PATCH 5/5] staging: r8188eu: Use vsprintf extension %phCX to format a copy_to_user string

[Joe Perches]

On Fri, 2021-08-27 at 10:42 +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 26, 2021 at 11:43:05AM -0700, Joe Perches wrote:
> > This reduces object size without changing the string content.
> > 
> > compiled x86-64 defconfig w/ r8188eu and gcc 10.3.0
> > 
> > $ size drivers/staging/r8188eu/os_dep/ioctl_linux.o*
> >    text	   data	    bss	    dec	    hex	filename
> >   72556	   1548	      0	  74104	  12178	drivers/staging/r8188eu/os_dep/ioctl_linux.o.new
> >   72758	   1548	      0	  74306	  12242	drivers/staging/r8188eu/os_dep/ioctl_linux.o.old
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> > ---
> >  drivers/staging/r8188eu/os_dep/ioctl_linux.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > index ab4a9200f0791..048164659d872 100644
> > --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > @@ -2907,10 +2907,8 @@ static int rtw_p2p_get_groupid(struct net_device *dev,
> >  	struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> >  	struct wifidirect_info *pwdinfo = &padapter->wdinfo;
> >  
> > 
> > -	sprintf(extra, "\n%.2X:%.2X:%.2X:%.2X:%.2X:%.2X %s",
> > -		pwdinfo->groupid_info.go_device_addr[0], pwdinfo->groupid_info.go_device_addr[1],
> > -		pwdinfo->groupid_info.go_device_addr[2], pwdinfo->groupid_info.go_device_addr[3],
> > -		pwdinfo->groupid_info.go_device_addr[4], pwdinfo->groupid_info.go_device_addr[5],
> > +	sprintf(extra, "\n%pM %s",
> > +		pwdinfo->groupid_info.go_device_addr,
> >  		pwdinfo->groupid_info.ssid);
> 
> We can just use the lower-case one here, no need for a new modifier just
> for something like this (i.e. log file output)

That was just a trivial conversion of log file output and is lower case
as log file output is not ABI.

The copy_to_user bit (2nd diff block) is nominally an ABI and is upper case.
IMO at a minimum, it's bad form to change it.

@@ -3075,8 +3073,7 @@ static int rtw_p2p_get_go_device_address(struct net_device *dev,
        if (!blnMatch)
                sprintf(go_devadd_str, "\n\ndev_add = NULL");
        else
-               sprintf(go_devadd_str, "\ndev_add =%.2X:%.2X:%.2X:%.2X:%.2X:%.2X",
-                       attr_content[0], attr_content[1], attr_content[2], attr_content[3], attr_content[4], attr_content[5]);
+               sprintf(go_devadd_str, "\ndev_add =%6phCX", attr_content);
 
        if (copy_to_user(wrqu->data.pointer, go_devadd_str, 10 + 17))
                return -EFAULT;

Re: [PATCH 5/5] staging: r8188eu: Use vsprintf extension %phCX to format a copy_to_user string

[Greg Kroah-Hartman]

On Fri, Aug 27, 2021 at 08:23:31AM -0700, Joe Perches wrote:
> On Fri, 2021-08-27 at 10:42 +0200, Greg Kroah-Hartman wrote:
> > On Thu, Aug 26, 2021 at 11:43:05AM -0700, Joe Perches wrote:
> > > This reduces object size without changing the string content.
> > > 
> > > compiled x86-64 defconfig w/ r8188eu and gcc 10.3.0
> > > 
> > > $ size drivers/staging/r8188eu/os_dep/ioctl_linux.o*
> > >    text	   data	    bss	    dec	    hex	filename
> > >   72556	   1548	      0	  74104	  12178	drivers/staging/r8188eu/os_dep/ioctl_linux.o.new
> > >   72758	   1548	      0	  74306	  12242	drivers/staging/r8188eu/os_dep/ioctl_linux.o.old
> > > 
> > > Signed-off-by: Joe Perches <joe@perches.com>
> > > ---
> > >  drivers/staging/r8188eu/os_dep/ioctl_linux.c | 9 +++------
> > >  1 file changed, 3 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > > index ab4a9200f0791..048164659d872 100644
> > > --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > > +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> > > @@ -2907,10 +2907,8 @@ static int rtw_p2p_get_groupid(struct net_device *dev,
> > >  	struct adapter *padapter = (struct adapter *)rtw_netdev_priv(dev);
> > >  	struct wifidirect_info *pwdinfo = &padapter->wdinfo;
> > >  
> > > 
> > > -	sprintf(extra, "\n%.2X:%.2X:%.2X:%.2X:%.2X:%.2X %s",
> > > -		pwdinfo->groupid_info.go_device_addr[0], pwdinfo->groupid_info.go_device_addr[1],
> > > -		pwdinfo->groupid_info.go_device_addr[2], pwdinfo->groupid_info.go_device_addr[3],
> > > -		pwdinfo->groupid_info.go_device_addr[4], pwdinfo->groupid_info.go_device_addr[5],
> > > +	sprintf(extra, "\n%pM %s",
> > > +		pwdinfo->groupid_info.go_device_addr,
> > >  		pwdinfo->groupid_info.ssid);
> > 
> > We can just use the lower-case one here, no need for a new modifier just
> > for something like this (i.e. log file output)
> 
> That was just a trivial conversion of log file output and is lower case
> as log file output is not ABI.
> 
> The copy_to_user bit (2nd diff block) is nominally an ABI and is upper case.
> IMO at a minimum, it's bad form to change it.
> 
> @@ -3075,8 +3073,7 @@ static int rtw_p2p_get_go_device_address(struct net_device *dev,
>         if (!blnMatch)
>                 sprintf(go_devadd_str, "\n\ndev_add = NULL");
>         else
> -               sprintf(go_devadd_str, "\ndev_add =%.2X:%.2X:%.2X:%.2X:%.2X:%.2X",
> -                       attr_content[0], attr_content[1], attr_content[2], attr_content[3], attr_content[4], attr_content[5]);
> +               sprintf(go_devadd_str, "\ndev_add =%6phCX", attr_content);
>  
>         if (copy_to_user(wrqu->data.pointer, go_devadd_str, 10 + 17))
>                 return -EFAULT;

That looks like a horrible driver-specific api that no one will really
be using and will be removed from the tree soon.  So it can be changed,
no need to worry about any "compatibility" issues here.

thanks,

greg k-h

Re: [PATCH 5/5] staging: r8188eu: Use vsprintf extension %phCX to format a copy_to_user string

[Joe Perches]

On Fri, 2021-08-27 at 17:27 +0200, Greg Kroah-Hartman wrote:
> On Fri, Aug 27, 2021 at 08:23:31AM -0700, Joe Perches wrote:
> > On Fri, 2021-08-27 at 10:42 +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Aug 26, 2021 at 11:43:05AM -0700, Joe Perches wrote:
> > > > This reduces object size without changing the string content.
[]
> > > > diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
[]
> > The copy_to_user bit (2nd diff block) is nominally an ABI and is upper case.
> > IMO at a minimum, it's bad form to change it.
> > 
> > @@ -3075,8 +3073,7 @@ static int rtw_p2p_get_go_device_address(struct net_device *dev,
> >         if (!blnMatch)
> >                 sprintf(go_devadd_str, "\n\ndev_add = NULL");
> >         else
> > -               sprintf(go_devadd_str, "\ndev_add =%.2X:%.2X:%.2X:%.2X:%.2X:%.2X",
> > -                       attr_content[0], attr_content[1], attr_content[2], attr_content[3], attr_content[4], attr_content[5]);
> > +               sprintf(go_devadd_str, "\ndev_add =%6phCX", attr_content);
> > 
> >         if (copy_to_user(wrqu->data.pointer, go_devadd_str, 10 + 17))
> >                 return -EFAULT;
> 
> That looks like a horrible driver-specific api

Horrible could be used to describe most realtek code in the kernel.

> that no one will really
> be using and will be removed from the tree soon.  So it can be changed,
> no need to worry about any "compatibility" issues here.

Fine with me too.  I just did it for completeness.

Re: [PATCH 0/5] vsprintf and uses: Add upper case output to %*ph extension

[Joe Perches]

On Fri, 2021-08-27 at 12:23 +0200, Greg KH wrote:
> On Fri, Aug 27, 2021 at 11:46:20AM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 27, 2021 at 01:10:41AM -0700, Joe Perches wrote:
> > > On Fri, 2021-08-27 at 10:51 +0300, Andy Shevchenko wrote:
> > > > On Thu, Aug 26, 2021 at 11:43:00AM -0700, Joe Perches wrote:
> > > > > Several sysfs uses that could use %*ph are upper case hex output.
> > > > > Add a flag to the short hex formatting routine in vsprintf to support them.
> > > > > Add documentation too.
> > > > 
> > > > Thanks!
> > > > 
> > > > Unfortunately I have got only first patch and this cover letter. Can you,
> > > > please, Cc entire series?
> > > 
> > > It's on lore.
> > > 
> > > https://lore.kernel.org/lkml/cover.1630003183.git.joe@perches.com/T/#u
> > 
> > Thanks. So, you won't me to review them in a regular way :-)
> > 
> > TBH, I think those examples may pretty much be safe to use small
> > letters always.
> 
> I agree, let's just fix the users here to use small letters instead of
> adding another modifier to the kernel.

ABI _should_ mean stability for random parsers.

I don't use these so I don't care that much.