[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 1/5] vsprintf/Documentation: Add X to %*ph extension to output upper case hex

[Joe Perches]

A few sysfs output uses of hex arrays are uppercase and are nominally ABI.

Add a mechanism to the existing vsprintf %*ph hex output extension to
support upper case hex output.

Signed-off-by: Joe Perches <joe@perches.com>
---
 Documentation/core-api/printk-formats.rst |  6 ++++
 lib/vsprintf.c                            | 42 ++++++++++++++---------
 2 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index e08bbe9b0cbf3..ca750274992e6 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -284,10 +284,16 @@ Raw buffer as a hex string
 
 ::
 
+	The preferred output is lowercase
 	%*ph	00 01 02  ...  3f
 	%*phC	00:01:02: ... :3f
 	%*phD	00-01-02- ... -3f
 	%*phN	000102 ... 3f
+	Formats with X are uppercase, used for backwards compatibility
+	%*phX	00 01 02  ...  3F
+	%*phCX	00:01:02: ... :3F
+	%*phDX	00-01-02- ... -3F
+	%*phNX	000102 ... 3F
 
 For printing small buffers (up to 64 bytes long) as a hex string with a
 certain separator. For larger buffers consider using
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 134216c45980e..5c22a07bbe3a7 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1147,7 +1147,10 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 {
 	int i, len = 1;		/* if we pass '%ph[CDN]', field width remains
 				   negative value, fallback to the default */
-	char separator;
+	char separator = ' ';
+	int count = 1;
+	bool found = true;
+	char locase = 0x20;	/* ASCII OR'd for lower case see: number() */
 
 	if (spec.field_width == 0)
 		/* nothing to print */
@@ -1156,30 +1159,35 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 	if (check_pointer(&buf, end, addr, spec))
 		return buf;
 
-	switch (fmt[1]) {
-	case 'C':
-		separator = ':';
-		break;
-	case 'D':
-		separator = '-';
-		break;
-	case 'N':
-		separator = 0;
-		break;
-	default:
-		separator = ' ';
-		break;
-	}
+	do {
+		switch (fmt[count++]) {
+		case 'C':
+			separator = ':';
+			break;
+		case 'D':
+			separator = '-';
+			break;
+		case 'N':
+			separator = 0;
+			break;
+		case 'X':
+			locase = 0;
+			break;
+		default:
+			found = false;
+			break;
+		}
+	} while (found);
 
 	if (spec.field_width > 0)
 		len = min_t(int, spec.field_width, 64);
 
 	for (i = 0; i < len; ++i) {
 		if (buf < end)
-			*buf = hex_asc_hi(addr[i]);
+			*buf = hex_asc_upper_hi(addr[i]) | locase;
 		++buf;
 		if (buf < end)
-			*buf = hex_asc_lo(addr[i]);
+			*buf = hex_asc_upper_lo(addr[i]) | locase;
 		++buf;
 
 		if (separator && i != len - 1) {
-- 
2.30.0

[PATCH 2/5] scsi: aacraid: Use vsprintf %phNX extension

[Joe Perches]

Reduce object size by using the %phNX extension to format a sysfs output
buffer with identical output.

compiled x86-64 defconfig w/ aacraid and gcc 10.3.0

$ size drivers/scsi/aacraid/linit.o*
   text	   data	    bss	    dec	    hex	filename
  18616	   4056	      0	  22672	   5890	drivers/scsi/aacraid/linit.o.new
  18951	   4056	      0	  23007	   59df	drivers/scsi/aacraid/linit.o.old

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/scsi/aacraid/linit.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 3168915adaa75..165e6e10f07b9 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -587,12 +587,7 @@ static ssize_t aac_show_unique_id(struct device *dev,
 	if (sdev_channel(sdev) == CONTAINER_CHANNEL)
 		memcpy(sn, aac->fsa_dev[sdev_id(sdev)].identifier, sizeof(sn));
 
-	return snprintf(buf, 16 * 2 + 2,
-		"%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X%02X\n",
-		sn[0], sn[1], sn[2], sn[3],
-		sn[4], sn[5], sn[6], sn[7],
-		sn[8], sn[9], sn[10], sn[11],
-		sn[12], sn[13], sn[14], sn[15]);
+	return snprintf(buf, 16 * 2 + 2, "%16phNX\n", sn);
 }
 
 static struct device_attribute aac_unique_id_attr = {
-- 
2.30.0

[PATCH 3/5] scsi: hpsa: Use vsprintf %phNX extension

[Joe Perches]

Reduce object size by using the %phNX extension to format a sysfs output
buffer with identical output.

compiled x86-64 defconfig w/ hpsa and gcc 10.3.0

$ size drivers/scsi/hpsa.o*
   text	   data	    bss	    dec	    hex	filename
  84041	   2181	     20	  86242	  150e2	drivers/scsi/hpsa.o.new
  84226	   2181	     20	  86427	  1519b	drivers/scsi/hpsa.o.old

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/scsi/hpsa.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 3faa87fa296a2..c56871e8ce1b7 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -743,13 +743,7 @@ static ssize_t unique_id_show(struct device *dev,
 	}
 	memcpy(sn, hdev->device_id, sizeof(sn));
 	spin_unlock_irqrestore(&h->lock, flags);
-	return snprintf(buf, 16 * 2 + 2,
-			"%02X%02X%02X%02X%02X%02X%02X%02X"
-			"%02X%02X%02X%02X%02X%02X%02X%02X\n",
-			sn[0], sn[1], sn[2], sn[3],
-			sn[4], sn[5], sn[6], sn[7],
-			sn[8], sn[9], sn[10], sn[11],
-			sn[12], sn[13], sn[14], sn[15]);
+	return snprintf(buf, 16 * 2 + 2, "%16phNX\n", sn);
 }
 
 static ssize_t sas_address_show(struct device *dev,
-- 
2.30.0

[PATCH 4/5] scsi: smartpqi: Use vsprintf %phNX extension

[Joe Perches]

Reduce object size by using the %phNX extension to format a sysfs output
buffer with identical output.

compiled x86-64 defconfig w/ smartpqi and gcc 10.3.0

$ size drivers/scsi/smartpqi/smartpqi_init.o*
   text	   data	    bss	    dec	    hex	filename
  69791	   2205	     48	  72044	  1196c	drivers/scsi/smartpqi/smartpqi_init.o.new
  69950	   2205	     48	  72203	  11a0b	drivers/scsi/smartpqi/smartpqi_init.o.old

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/scsi/smartpqi/smartpqi_init.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
index ecb2af3f43ca3..eb39490b196cc 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -6674,13 +6674,7 @@ static ssize_t pqi_unique_id_show(struct device *dev,
 
 	spin_unlock_irqrestore(&ctrl_info->scsi_device_list_lock, flags);
 
-	return scnprintf(buffer, PAGE_SIZE,
-		"%02X%02X%02X%02X%02X%02X%02X%02X"
-		"%02X%02X%02X%02X%02X%02X%02X%02X\n",
-		unique_id[0], unique_id[1], unique_id[2], unique_id[3],
-		unique_id[4], unique_id[5], unique_id[6], unique_id[7],
-		unique_id[8], unique_id[9], unique_id[10], unique_id[11],
-		unique_id[12], unique_id[13], unique_id[14], unique_id[15]);
+	return scnprintf(buffer, PAGE_SIZE, "%16phNX\n", unique_id);
 }
 
 static ssize_t pqi_lunid_show(struct device *dev,
-- 
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 1/5] vsprintf/Documentation: Add X to %*ph extension to output upper case hex

[Andy Shevchenko]

On Thu, Aug 26, 2021 at 11:43:01AM -0700, Joe Perches wrote:
> A few sysfs output uses of hex arrays are uppercase and are nominally ABI.
> 
> Add a mechanism to the existing vsprintf %*ph hex output extension to
> support upper case hex output.

...

> +	The preferred output is lowercase
>  	%*ph	00 01 02  ...  3f
>  	%*phC	00:01:02: ... :3f
>  	%*phD	00-01-02- ... -3f
>  	%*phN	000102 ... 3f
> +	Formats with X are uppercase, used for backwards compatibility
> +	%*phX	00 01 02  ...  3F
> +	%*phCX	00:01:02: ... :3F
> +	%*phDX	00-01-02- ... -3F
> +	%*phNX	000102 ... 3F

Why not using %*pH...?

...

> +	char locase = 0x20;	/* ASCII OR'd for lower case see: number() */

If you use h vs H, you may derive this from (fmt[...] & SMALL).

-- 
With Best Regards,
Andy Shevchenko

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 1/5] vsprintf/Documentation: Add X to %*ph extension to output upper case hex

[Joe Perches]

On Fri, 2021-08-27 at 10:48 +0300, Andy Shevchenko wrote:
> On Thu, Aug 26, 2021 at 11:43:01AM -0700, Joe Perches wrote:
> > A few sysfs output uses of hex arrays are uppercase and are nominally ABI.
> > 
> > Add a mechanism to the existing vsprintf %*ph hex output extension to
> > support upper case hex output.
> 
> ...
> 
> > +	The preferred output is lowercase
> >  	%*ph	00 01 02  ...  3f
> >  	%*phC	00:01:02: ... :3f
> >  	%*phD	00-01-02- ... -3f
> >  	%*phN	000102 ... 3f
> > +	Formats with X are uppercase, used for backwards compatibility
> > +	%*phX	00 01 02  ...  3F
> > +	%*phCX	00:01:02: ... :3F
> > +	%*phDX	00-01-02- ... -3F
> > +	%*phNX	000102 ... 3F
> 
> Why not using %*pH...?

I find X more intelligible.

> > +	char locase = 0x20;	/* ASCII OR'd for lower case see: number() */
> 
> If you use h vs H, you may derive this from (fmt[...] & SMALL).

It's not necessary to use any more of the rather limited vsprintf
extension namespace.

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 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 1/5] vsprintf/Documentation: Add X to %*ph extension to output upper case hex

[Andy Shevchenko]

On Fri, Aug 27, 2021 at 01:08:10AM -0700, Joe Perches wrote:
> On Fri, 2021-08-27 at 10:48 +0300, Andy Shevchenko wrote:
> > On Thu, Aug 26, 2021 at 11:43:01AM -0700, Joe Perches wrote:
> > > A few sysfs output uses of hex arrays are uppercase and are nominally ABI.
> > > 
> > > Add a mechanism to the existing vsprintf %*ph hex output extension to
> > > support upper case hex output.
> > 
> > ...
> > 
> > > +	The preferred output is lowercase
> > >  	%*ph	00 01 02  ...  3f
> > >  	%*phC	00:01:02: ... :3f
> > >  	%*phD	00-01-02- ... -3f
> > >  	%*phN	000102 ... 3f
> > > +	Formats with X are uppercase, used for backwards compatibility
> > > +	%*phX	00 01 02  ...  3F
> > > +	%*phCX	00:01:02: ... :3F
> > > +	%*phDX	00-01-02- ... -3F
> > > +	%*phNX	000102 ... 3F
> > 
> > Why not using %*pH...?
> 
> I find X more intelligible.
> 
> > > +	char locase = 0x20;	/* ASCII OR'd for lower case see: number() */
> > 
> > If you use h vs H, you may derive this from (fmt[...] & SMALL).
> 
> It's not necessary to use any more of the rather limited vsprintf
> extension namespace.

I understand your concern, but %*ph is quite widely used (I guess top 1 or 2
among all %p extensions), its performance degradation with your code may affect
a lot of other users and hence a kernel as a whole.

So, that's why my proposal stays.

Of course you may provide a benchmark (btw, where are the test cases for this?)
for yours and mine variant and we will see if it makes sense to optimize.

-- 
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 1/5] vsprintf/Documentation: Add X to %*ph extension to output upper case hex

[Petr Mladek]

On Fri 2021-08-27 11:49:07, Andy Shevchenko wrote:
> On Fri, Aug 27, 2021 at 01:08:10AM -0700, Joe Perches wrote:
> > On Fri, 2021-08-27 at 10:48 +0300, Andy Shevchenko wrote:
> > > On Thu, Aug 26, 2021 at 11:43:01AM -0700, Joe Perches wrote:
> > > > A few sysfs output uses of hex arrays are uppercase and are nominally ABI.
> > > > 
> > > > Add a mechanism to the existing vsprintf %*ph hex output extension to
> > > > support upper case hex output.
> > > 
> > > ...
> > > 
> > > > +	The preferred output is lowercase
> > > >  	%*ph	00 01 02  ...  3f
> > > >  	%*phC	00:01:02: ... :3f
> > > >  	%*phD	00-01-02- ... -3f
> > > >  	%*phN	000102 ... 3f
> > > > +	Formats with X are uppercase, used for backwards compatibility
> > > > +	%*phX	00 01 02  ...  3F
> > > > +	%*phCX	00:01:02: ... :3F
> > > > +	%*phDX	00-01-02- ... -3F
> > > > +	%*phNX	000102 ... 3F
> > > 
> > > Why not using %*pH...?

I though about this as well.

> > I find X more intelligible.

I would slightly prefer %pH. I always have problems to parse long
sequences of modifiers. So, the shorter format the better.

Of course, it means that 'H' won't be usable for another purpose.
But it will happen one day anyway. Well, this is why I do not
have strong opinion.

I am more and more convinced that we will need another approach.
Mathew Wilcox has had an idea to add support for custom callbacks
that would be able to format the string, something like:

   vsprintf("Date: %pX(%p)\n", format_date, time_stamp);

I think that it might even be possible to do something like:

   vsprintf("Date: %pX\n", format_date(time));

, where the format_date() would be a macro that would create
a struct at stack a pass it as a pointer:

#define format_date(time)			   \
({						   \
	struct vsprintf_callback c = {		   \
		.func = vsprintf_format_date,	   \
		.arg1 = time,			   \
	}					   \
						   \
	&c;					   \
})

and vsprintf would internally do something like:

char *custom_format(char *buf, char *end, vsprintf_callback *c,
			 struct printf_spec spec, const char *fmt)
{
	return c->func(buf, end, c->arg1, spec);
}

It would allow to replace all the magic %pXYZ modifiers with
self-explanatory callbacks. While still keeping it easy to use.

Best Regards,
Petr

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

[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 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.

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.

Re: [PATCH 1/5] vsprintf/Documentation: Add X to %*ph extension to output upper case hex

[Joe Perches]

On Fri, 2021-08-27 at 11:49 +0300, Andy Shevchenko wrote:
> On Fri, Aug 27, 2021 at 01:08:10AM -0700, Joe Perches wrote:
> > On Fri, 2021-08-27 at 10:48 +0300, Andy Shevchenko wrote:
> > > On Thu, Aug 26, 2021 at 11:43:01AM -0700, Joe Perches wrote:
> > > > A few sysfs output uses of hex arrays are uppercase and are nominally ABI.
> > > > 
> > > > Add a mechanism to the existing vsprintf %*ph hex output extension to
> > > > support upper case hex output.
> > > 
> > > ...
> > > 
> > > > +	The preferred output is lowercase
> > > >  	%*ph	00 01 02  ...  3f
> > > >  	%*phC	00:01:02: ... :3f
> > > >  	%*phD	00-01-02- ... -3f
> > > >  	%*phN	000102 ... 3f
> > > > +	Formats with X are uppercase, used for backwards compatibility
> > > > +	%*phX	00 01 02  ...  3F
> > > > +	%*phCX	00:01:02: ... :3F
> > > > +	%*phDX	00-01-02- ... -3F
> > > > +	%*phNX	000102 ... 3F
> > > 
> > > Why not using %*pH...?
> > 
> > I find X more intelligible.
> > 
> > > > +	char locase = 0x20;	/* ASCII OR'd for lower case see: number() */
> > > 
> > > If you use h vs H, you may derive this from (fmt[...] & SMALL).
> > 
> > It's not necessary to use any more of the rather limited vsprintf
> > extension namespace.
> 
> I understand your concern, but %*ph is quite widely used (I guess top 1 or 2
> among all %p extensions),

Cumulatively 3rd after %pM and %pOF

> its performance degradation with your code may affect
> a lot of other users and hence a kernel as a whole.
> 
> So, that's why my proposal stays.

Knock yourself out.

> Of course you may provide a benchmark (btw, where are the test cases for this?)

You are welcome to provide both test cases and benchmarks.
I find the whole thing rather dull.

> for yours and mine variant and we will see if it makes sense to optimize.

It doesn't.  Anyone thinking there is a required printf/vsprintf
optimization in the kernel is decidedly barking up the wrong tree.