vsprintf and uses: Add upper case output to %*ph extension
Subsystems (8)
cover vsprintf and uses: Add upper case output to %*ph extension
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(-)
Signature · 1 line
2.30.0
PATCH 1/5 vsprintf/Documentation: Add X to %*ph extension to output upper case hex
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.
--- Documentation/core-api/printk-formats.rst | 6 ++++ lib/vsprintf.c | 42 ++++++++++++++--------- 2 files changed, 31 insertions(+), 17 deletions(-)
Documentation/core-api/printk-formats.rst +6 -0
@@ -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
lib/vsprintf.c +25 -17
62 lines not on this page · raw message
Signature · 1 line
2.30.0
Andy Shevchenko
reviewerRe: [PATCH 1/5] vsprintf/Documentation: Add X to %*ph extension to output upper case hex
Quoted 4 lines · 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.
...
Documentation/core-api/printk-formats.rst Raw buffer as a hex string · line 296
Quoted 10 lines
Documentation/core-api/printk-formats.rst +6
-0+ 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...?
...
lib/vsprintf.c char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec, · line 1153
Quoted 1 line
lib/vsprintf.c +1
-0+char locase =0x20 ;/* ASCII OR'd for lower case see: number() */
If you use h vs H, you may derive this from (fmt[...] & SMALL).
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH 1/5] vsprintf/Documentation: Add X to %*ph extension to output upper case hex
Quoted 8 lines · Andy Shevchenko
Quoted 4 lines · 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.
...
Quoted 12 lines
Documentation/core-api/printk-formats.rst Raw buffer as a hex string · line 296
Quoted 10 lines
Documentation/core-api/printk-formats.rst +1
-0+ The preferred output is lowercase%*ph 00 01 02 ... 3f
%*phC 00:01:02: ... :3f
%*phD 00-01-02- ... -3f
%*phN 000102 ... 3fDocumentation/core-api/printk-formats.rst +5
-0+ 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 ... 3FWhy not using %*pH...?
I find X more intelligible.
Quoted 3 lines
lib/vsprintf.c char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec, · line 1153
Quoted 1 line
lib/vsprintf.c +1
-0+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.
Andy Shevchenko
reviewerRe: [PATCH 1/5] vsprintf/Documentation: Add X to %*ph extension to output upper case hex
Quoted 22 lines · Joe Perches
Quoted 8 lines · Andy Shevchenko
Quoted 4 lines · 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.
...
Quoted 12 lines
Documentation/core-api/printk-formats.rst Raw buffer as a hex string · line 296
Quoted 10 lines
Documentation/core-api/printk-formats.rst +1
-0+ The preferred output is lowercase%*ph 00 01 02 ... 3f
%*phC 00:01:02: ... :3f
%*phD 00-01-02- ... -3f
%*phN 000102 ... 3fDocumentation/core-api/printk-formats.rst +5
-0+ 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 ... 3FWhy not using %*pH...?
Quoted 8 lines
I find X more intelligible.
Quoted 3 lines
lib/vsprintf.c char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec, · line 1153
Quoted 1 line
lib/vsprintf.c +1
-0+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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Petr Mladek
maintainerRe: [PATCH 1/5] vsprintf/Documentation: Add X to %*ph extension to output upper case hex
Quoted 10 lines · Andy Shevchenko
Quoted 9 lines · Joe Perches
Quoted 8 lines · Andy Shevchenko
Quoted 4 lines · 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.
...
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Documentation/core-api/printk-formats.rst +1
-0+ The preferred output is lowercase%*ph 00 01 02 ... 3f
%*phC 00:01:02: ... :3f
%*phD 00-01-02- ... -3fQuoted 8 lines
Quoted 8 lines
Documentation/core-api/printk-formats.rst Raw buffer as a hex string · line 296
Quoted 6 lines
%*phN 000102 ... 3f
Documentation/core-api/printk-formats.rst +5
-0+ 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 ... 3FWhy not using %*pH...?
I though about this as well.
Quoted 1 line
Quoted 1 line
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 1/5] vsprintf/Documentation: Add X to %*ph extension to output upper case hex
In reply to Andy ShevchenkoQuoted 29 lines · Andy Shevchenko
Quoted 28 lines · Joe Perches
Quoted 8 lines · Andy Shevchenko
Quoted 4 lines · 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.
...
Quoted 12 lines
Documentation/core-api/printk-formats.rst Raw buffer as a hex string · line 296
Quoted 10 lines
Documentation/core-api/printk-formats.rst +1
-0+ The preferred output is lowercase%*ph 00 01 02 ... 3f
%*phC 00:01:02: ... :3f
%*phD 00-01-02- ... -3f
%*phN 000102 ... 3fDocumentation/core-api/printk-formats.rst +5
-0+ 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 ... 3FWhy not using %*pH...?
I find X more intelligible.
Quoted 3 lines
lib/vsprintf.c char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec, · line 1153
Quoted 1 line
lib/vsprintf.c +1
-0+char locase =0x20 ;/* ASCII OR'd for lower case see: number() */ If you use h vs H, you may derive this from (fmt[...] & SMALL).
Quoted 5 lines
Quoted 2 lines
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
Quoted 4 lines
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.
Quoted 1 line
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.
Quoted 1 line
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.
PATCH 2/5 scsi: aacraid: Use vsprintf %phNX extension
In reply to Joe PerchesReduce 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
--- drivers/scsi/aacraid/linit.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
drivers/scsi/aacraid/linit.c +1 -6
@@ -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 = {
Signature · 1 line
2.30.0
PATCH 3/5 scsi: hpsa: Use vsprintf %phNX extension
In reply to Joe PerchesReduce 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
--- drivers/scsi/hpsa.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
drivers/scsi/hpsa.c +1 -7
@@ -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,
Signature · 1 line
2.30.0
PATCH 4/5 scsi: smartpqi: Use vsprintf %phNX extension
In reply to Joe PerchesReduce 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
--- drivers/scsi/smartpqi/smartpqi_init.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
drivers/scsi/smartpqi/smartpqi_init.c +1 -7
@@ -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,
Signature · 1 line
2.30.0
PATCH 5/5 staging: r8188eu: Use vsprintf extension %phCX to format a copy_to_user string
In reply to Joe PerchesThis 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
--- drivers/staging/r8188eu/os_dep/ioctl_linux.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/staging/r8188eu/os_dep/ioctl_linux.c +3 -6
@@ -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 ;
Signature · 1 line
2.30.0
Greg Kroah-Hartman
maintainerRe: [PATCH 5/5] staging: r8188eu: Use vsprintf extension %phCX to format a copy_to_user string
Quoted 19 lines · 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.oldSigned-off-by Joe Perches <[email protected]>--- drivers/staging/r8188eu/os_dep/ioctl_linux.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)drivers/staging/r8188eu/os_dep/ioctl_linux.c +0
-0
drivers/staging/r8188eu/os_dep/ioctl_linux.c static int rtw_p2p_get_groupid(struct net_device *dev, · line 2912
Quoted 10 lines
drivers/staging/r8188eu/os_dep/ioctl_linux.c +2
-4@@ -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 5/5] staging: r8188eu: Use vsprintf extension %phCX to format a copy_to_user string
Quoted 24 lines · Greg Kroah-Hartman
Quoted 23 lines · 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.oldSigned-off-by Joe Perches <[email protected]>---
drivers/staging/r8188eu/os_dep/ioctl_linux.c | 9 +++------1 file changed, 3 insertions(+), 6 deletions(-)drivers/staging/r8188eu/os_dep/ioctl_linux.c +0
-0@@ -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;
Quoted 10 lines
Quoted 7 lines
drivers/staging/r8188eu/os_dep/ioctl_linux.c +2
-4- 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;Greg Kroah-Hartman
maintainerRe: [PATCH 5/5] staging: r8188eu: Use vsprintf extension %phCX to format a copy_to_user string
Quoted 43 lines · Joe Perches
Quoted 24 lines · Greg Kroah-Hartman
Quoted 23 lines · 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.oldSigned-off-by Joe Perches <[email protected]>---
drivers/staging/r8188eu/os_dep/ioctl_linux.c | 9 +++------1 file changed, 3 insertions(+), 6 deletions(-)drivers/staging/r8188eu/os_dep/ioctl_linux.c +0
-0@@ -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;Quoted 10 lines
Quoted 7 lines
drivers/staging/r8188eu/os_dep/ioctl_linux.c +2
-4- 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_userbit (2nd diff block) is nominally an ABI and is upper case. IMO at a minimum, it's bad form to change it.
drivers/staging/r8188eu/os_dep/ioctl_linux.c static int rtw_p2p_get_go_device_address(struct net_device *dev, · line 3079
Quoted 9 lines
drivers/staging/r8188eu/os_dep/ioctl_linux.c +1
-2@@ -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
Quoted 4 lines · Greg Kroah-Hartman
Quoted 3 lines · Joe Perches
Quoted 2 lines · Greg Kroah-Hartman
Quoted 1 line · Joe Perches
This reduces object size without changing the string content.
[]
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/staging/r8188eu/os_dep/ioctl_linux.c
Quoted 1 line
drivers/staging/r8188eu/os_dep/ioctl_linux.c +0
-0
[]
Quoted 11 lines
Quoted 11 lines
The
copy_to_userbit (2nd diff block) is nominally an ABI and is upper case. IMO at a minimum, it's bad form to change it.drivers/staging/r8188eu/os_dep/ioctl_linux.c +0
-0@@ -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
drivers/staging/r8188eu/os_dep/ioctl_linux.c +1
-2-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);
Quoted 4 lines
Quoted 2 lines
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.
Quoted 3 lines
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.
Andy Shevchenko
reviewerQuoted 3 lines · 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.
Thanks!
Unfortunately I have got only first patch and this cover letter. Can you, please, Cc entire series?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 9 lines · Andy Shevchenko
Quoted 3 lines · 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.
Thanks!
Unfortunately I have got only first patch and this cover letter. Can you, please, Cc entire series?
It's on lore.
Andy Shevchenko
reviewerQuoted 6 lines · Joe Perches
Quoted 5 lines · Andy Shevchenko
Quoted 3 lines · 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.
Quoted 8 lines
Quoted 4 lines
Thanks!
Unfortunately I have got only first patch and this cover letter. Can you, please, Cc entire series?
It's on lore.
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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Greg KH
maintainerQuoted 12 lines · Andy Shevchenko
Quoted 11 lines · Joe Perches
Quoted 9 lines · Andy Shevchenko
Quoted 3 lines · 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.
Thanks!
Unfortunately I have got only first patch and this cover letter. Can you, please, Cc entire series?
Quoted 8 lines
Quoted 3 lines
It's on lore.
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
Quoted 17 lines · Greg KH
Quoted 16 lines · Andy Shevchenko
Quoted 14 lines · Joe Perches
Quoted 9 lines · Andy Shevchenko
Quoted 3 lines · 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.
Thanks!
Unfortunately I have got only first patch and this cover letter. Can you, please, Cc entire series?
It's on lore.
Quoted 7 lines
Quoted 4 lines
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.