[PATCH v3 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[PATCH v3 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[Konrad Gräfe]

The CDC-ECM specification requires an USB gadget to send the host MAC
address as uppercase hex string. This change adds the appropriate
modifier.

Cc: stable@vger.kernel.org
Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
---
Added in v3

 lib/vsprintf.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index be71a03c936a..8aee1caabd9e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1269,9 +1269,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
 {
 	char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
 	char *p = mac_addr;
-	int i;
+	int i, pos;
 	char separator;
 	bool reversed = false;
+	bool uppercase = false;
 
 	if (check_pointer(&buf, end, addr, spec))
 		return buf;
@@ -1281,6 +1282,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
 		separator = '-';
 		break;
 
+	case 'U':
+		uppercase = true;
+		break;
+
 	case 'R':
 		reversed = true;
 		fallthrough;
@@ -1292,9 +1297,14 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
 
 	for (i = 0; i < 6; i++) {
 		if (reversed)
-			p = hex_byte_pack(p, addr[5 - i]);
+			pos = 5 - i;
+		else
+			pos = i;
+
+		if (uppercase)
+			p = hex_byte_pack_upper(p, addr[pos]);
 		else
-			p = hex_byte_pack(p, addr[i]);
+			p = hex_byte_pack(p, addr[pos]);
 
 		if (fmt[0] == 'M' && i != 5)
 			*p++ = separator;
@@ -2279,6 +2289,7 @@ char *rust_fmt_argument(char *buf, char *end, void *ptr);
  * - 'm' For a 6-byte MAC address, it prints the hex address without colons
  * - 'MF' For a 6-byte MAC FDDI address, it prints the address
  *       with a dash-separated hex notation
+ * - '[mM]U' For a 6-byte MAC address in uppercase hex
  * - '[mM]R' For a 6-byte MAC address, Reverse order (Bluetooth)
  * - 'I' [46] for IPv4/IPv6 addresses printed in the usual way
  *       IPv4 uses dot-separated decimal without leading 0's (1.2.3.4)
@@ -2407,6 +2418,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'M':			/* Colon separated: 00:01:02:03:04:05 */
 	case 'm':			/* Contiguous: 000102030405 */
 					/* [mM]F (FDDI) */
+					/* [mM]U (Uppercase hex) */
 					/* [mM]R (Reverse order; Bluetooth) */
 		return mac_address_string(buf, end, ptr, spec, fmt);
 	case 'I':			/* Formatted IP supported
-- 
2.34.1

[PATCH v3 2/2] usb: gadget: u_ether: Fix host MAC address case

[Konrad Gräfe]

The CDC-ECM specification [1] requires to send the host MAC address as
an uppercase hexadecimal string in chapter "5.4 Ethernet Networking
Functional Descriptor":
    The Unicode character is chosen from the set of values 30h through
    39h and 41h through 46h (0-9 and A-F).

However, snprintf(.., "%pm", ..) generates a lowercase MAC address
string. While most host drivers are tolerant to this, UsbNcm.sys on
Windows 10 is not. Instead it uses a different MAC address with all
bytes set to zero including and after the first byte containing a
lowercase letter. On Windows 11 Microsoft fixed it, but apparently they
did not backport the fix.

This change fixes the issue by using "%pmU" to generate an uppercase hex
string to comply with the specification.

[1]: https://www.usb.org/document-library/class-definitions-communication-devices-12, file ECM120.pdf

Fixes: bcd4a1c40bee ("usb: gadget: u_ether: construct with default values and add setters/getters")
Cc: stable@vger.kernel.org
Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
---
Changes since v2:
* Add uppercase MAC address format string and use that instead of
  manually uppercasing the resulting MAC address string.

Changes since v1:
* Fixed checkpatch.pl warnings

 drivers/usb/gadget/function/u_ether.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index 6956ad8ba8dd..70e6b825654c 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -963,7 +963,7 @@ int gether_get_host_addr_cdc(struct net_device *net, char *host_addr, int len)
 		return -EINVAL;
 
 	dev = netdev_priv(net);
-	snprintf(host_addr, len, "%pm", dev->host_mac);
+	snprintf(host_addr, len, "%pmU", dev->host_mac);
 
 	return strlen(host_addr);
 }
-- 
2.34.1

[PATCH v4 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[Konrad Gräfe]

The CDC-ECM specification requires an USB gadget to send the host MAC
address as uppercase hex string. This change adds the appropriate
modifier.

Cc: stable@vger.kernel.org
Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
---

Changes since v3:
* Added documentation
* Added test cases
* Use string_upper() after conversion to simplify conversion loop
* Fixed maybe-uninitalized variable warning

Added in v3

 Documentation/core-api/printk-formats.rst | 15 ++++++++++-----
 lib/test_printf.c                         |  2 ++
 lib/vsprintf.c                            |  5 +++++
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index dbe1aacc79d0..1ec682bdfe94 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -298,11 +298,13 @@ MAC/FDDI addresses
 
 ::
 
-	%pM	00:01:02:03:04:05
-	%pMR	05:04:03:02:01:00
-	%pMF	00-01-02-03-04-05
-	%pm	000102030405
-	%pmR	050403020100
+	%pM	00:01:02:03:aa:bb
+	%pMR	aa:bb:03:02:01:00
+	%pMF	00-01-02-03-aa-bb
+	%pMU	00:01:02:03:AA:BB
+	%pm	00010203aabb
+	%pmR	bbaa03020100
+	%pmU	00010203AABB
 
 For printing 6-byte MAC/FDDI addresses in hex notation. The ``M`` and ``m``
 specifiers result in a printed address with (M) or without (m) byte
@@ -316,6 +318,9 @@ For Bluetooth addresses the ``R`` specifier shall be used after the ``M``
 specifier to use reversed byte order suitable for visual interpretation
 of Bluetooth addresses which are in the little endian order.
 
+For uppercase hex notation the ``U`` specifier shall be used after the ``M``
+and ``m`` specifiers.
+
 Passed by reference.
 
 IPv4 addresses
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 46b4e6c414a3..7f4de2ecafbc 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -416,9 +416,11 @@ mac(void)
 	const u8 addr[6] = {0x2d, 0x48, 0xd6, 0xfc, 0x7a, 0x05};
 
 	test("2d:48:d6:fc:7a:05", "%pM", addr);
+	test("2D:48:D6:FC:7A:05", "%pMU", addr);
 	test("05:7a:fc:d6:48:2d", "%pMR", addr);
 	test("2d-48-d6-fc-7a-05", "%pMF", addr);
 	test("2d48d6fc7a05", "%pm", addr);
+	test("2D48D6FC7A05", "%pmU", addr);
 	test("057afcd6482d", "%pmR", addr);
 }
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index be71a03c936a..c82616c335e0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1301,6 +1301,9 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
 	}
 	*p = '\0';
 
+	if (fmt[1] == 'U')
+		string_upper(mac_addr, mac_addr);
+
 	return string_nocheck(buf, end, mac_addr, spec);
 }
 
@@ -2280,6 +2283,7 @@ char *rust_fmt_argument(char *buf, char *end, void *ptr);
  * - 'MF' For a 6-byte MAC FDDI address, it prints the address
  *       with a dash-separated hex notation
  * - '[mM]R' For a 6-byte MAC address, Reverse order (Bluetooth)
+ * - '[mM]U' For a 6-byte MAC address in uppercase hex
  * - 'I' [46] for IPv4/IPv6 addresses printed in the usual way
  *       IPv4 uses dot-separated decimal without leading 0's (1.2.3.4)
  *       IPv6 uses colon separated network-order 16 bit hex with leading 0's
@@ -2407,6 +2411,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'M':			/* Colon separated: 00:01:02:03:04:05 */
 	case 'm':			/* Contiguous: 000102030405 */
 					/* [mM]F (FDDI) */
+					/* [mM]U (Uppercase hex) */
 					/* [mM]R (Reverse order; Bluetooth) */
 		return mac_address_string(buf, end, ptr, spec, fmt);
 	case 'I':			/* Formatted IP supported
-- 
2.34.1

[PATCH v4 2/2] usb: gadget: u_ether: Fix host MAC address case

[Konrad Gräfe]

The CDC-ECM specification [1] requires to send the host MAC address as
an uppercase hexadecimal string in chapter "5.4 Ethernet Networking
Functional Descriptor":
    The Unicode character is chosen from the set of values 30h through
    39h and 41h through 46h (0-9 and A-F).

However, snprintf(.., "%pm", ..) generates a lowercase MAC address
string. While most host drivers are tolerant to this, UsbNcm.sys on
Windows 10 is not. Instead it uses a different MAC address with all
bytes set to zero including and after the first byte containing a
lowercase letter. On Windows 11 Microsoft fixed it, but apparently they
did not backport the fix.

This change fixes the issue by using "%pmU" to generate an uppercase hex
string to comply with the specification.

[1]: https://www.usb.org/document-library/class-definitions-communication-devices-12, file ECM120.pdf

Fixes: bcd4a1c40bee ("usb: gadget: u_ether: construct with default values and add setters/getters")
Cc: stable@vger.kernel.org
Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
---

Changes since v3: None

Changes since v2:
* Add uppercase MAC address format string and use that instead of
  manually uppercasing the resulting MAC address string.

Changes since v1:
* Fixed checkpatch.pl warnings

 drivers/usb/gadget/function/u_ether.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index 6956ad8ba8dd..70e6b825654c 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -963,7 +963,7 @@ int gether_get_host_addr_cdc(struct net_device *net, char *host_addr, int len)
 		return -EINVAL;
 
 	dev = netdev_priv(net);
-	snprintf(host_addr, len, "%pm", dev->host_mac);
+	snprintf(host_addr, len, "%pmU", dev->host_mac);
 
 	return strlen(host_addr);
 }
-- 
2.34.1

Re: [PATCH v4 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[Andy Shevchenko]

On Fri, Apr 28, 2023 at 08:49:04AM +0200, Konrad Gräfe wrote:
> The CDC-ECM specification requires an USB gadget to send the host MAC
> address as uppercase hex string. This change adds the appropriate
> modifier.

Why not teaching %ph to provide an uppercase? Would be much more useful than
this.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5] usb: gadget: u_ether: Fix host MAC address case

[Konrad Gräfe]

The CDC-ECM specification [1] requires to send the host MAC address as
an uppercase hexadecimal string in chapter "5.4 Ethernet Networking
Functional Descriptor":
    The Unicode character is chosen from the set of values 30h through
    39h and 41h through 46h (0-9 and A-F).

However, snprintf(.., "%pm", ..) generates a lowercase MAC address
string. While most host drivers are tolerant to this, UsbNcm.sys on
Windows 10 is not. Instead it uses a different MAC address with all
bytes set to zero including and after the first byte containing a
lowercase letter. On Windows 11 Microsoft fixed it, but apparently they
did not backport the fix.

This change fixes the issue by upper-casing the MAC to comply with the
specification.

[1]: https://www.usb.org/document-library/class-definitions-communication-devices-12, file ECM120.pdf

Fixes: bcd4a1c40bee ("usb: gadget: u_ether: construct with default values and add setters/getters")
Cc: stable@vger.kernel.org
Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
---
Changes since v4:
* Use string_upper() instead of a special format string

Changes since v3: None

Changes since v2:
* Add uppercase MAC address format string and use that instead of
  manually uppercasing the resulting MAC address string.

Changes since v1:
* Fixed checkpatch.pl warnings

 drivers/usb/gadget/function/u_ether.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index 6956ad8ba8dd..a366abb45623 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -17,6 +17,7 @@
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
 #include <linux/if_vlan.h>
+#include <linux/string_helpers.h>
 #include <linux/usb/composite.h>
 
 #include "u_ether.h"
@@ -965,6 +966,8 @@ int gether_get_host_addr_cdc(struct net_device *net, char *host_addr, int len)
 	dev = netdev_priv(net);
 	snprintf(host_addr, len, "%pm", dev->host_mac);
 
+	string_upper(host_addr, host_addr);
+
 	return strlen(host_addr);
 }
 EXPORT_SYMBOL_GPL(gether_get_host_addr_cdc);
-- 
2.34.1

Re: [PATCH v3 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[Greg Kroah-Hartman]

On Thu, Apr 27, 2023 at 01:51:19PM +0200, Konrad Gräfe wrote:
> The CDC-ECM specification requires an USB gadget to send the host MAC
> address as uppercase hex string. This change adds the appropriate
> modifier.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
> ---
> Added in v3
> 
>  lib/vsprintf.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index be71a03c936a..8aee1caabd9e 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1269,9 +1269,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
>  {
>  	char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
>  	char *p = mac_addr;
> -	int i;
> +	int i, pos;
>  	char separator;
>  	bool reversed = false;
> +	bool uppercase = false;
>  
>  	if (check_pointer(&buf, end, addr, spec))
>  		return buf;
> @@ -1281,6 +1282,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
>  		separator = '-';
>  		break;
>  
> +	case 'U':
> +		uppercase = true;
> +		break;

No documentation update as well?

thanks,

greg k-h

Re: [PATCH v3 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[Rasmus Villemoes]

On 27/04/2023 13.51, Konrad Gräfe wrote:
> The CDC-ECM specification requires an USB gadget to send the host MAC
> address as uppercase hex string. This change adds the appropriate
> modifier.
> 
> Cc: stable@vger.kernel.org

Why cc stable?

> Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
> ---
> Added in v3
> 
>  lib/vsprintf.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)

The diffstat here, or for some other patch in the same series,
definitely ought to mention lib/test_printf.c.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index be71a03c936a..8aee1caabd9e 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1269,9 +1269,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
>  {
>  	char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
>  	char *p = mac_addr;
> -	int i;
> +	int i, pos;
>  	char separator;
>  	bool reversed = false;
> +	bool uppercase = false;
>  
>  	if (check_pointer(&buf, end, addr, spec))
>  		return buf;
> @@ -1281,6 +1282,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
>  		separator = '-';
>  		break;
>  
> +	case 'U':
> +		uppercase = true;
> +		break;
> +
>  	case 'R':
>  		reversed = true;
>  		fallthrough;

This seems broken, and I'm surprised the compiler doesn't warn about
separator possibly being uninitialized further down. I'm also surprised
your testing hasn't caught this. For reference, the full switch
statement is currently

        switch (fmt[1]) {
        case 'F':
                separator = '-';
                break;

        case 'R':
                reversed = true;
                fallthrough;

        default:
                separator = ':';
                break;
        }

> @@ -1292,9 +1297,14 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
>  
>  	for (i = 0; i < 6; i++) {
>  		if (reversed)
> -			p = hex_byte_pack(p, addr[5 - i]);
> +			pos = 5 - i;
> +		else
> +			pos = i;
> +
> +		if (uppercase)
> +			p = hex_byte_pack_upper(p, addr[pos]);
>  		else
> -			p = hex_byte_pack(p, addr[i]);
> +			p = hex_byte_pack(p, addr[pos]);

I think this becomes quite hard to follow. We have string_upper() in
linux/string_helpers.h, so I'd rather just leave this loop alone and do

  if (uppercase)
    string_upper(mac_addr, mac_addr);

after the nul-termination.

Rasmus

Re: [PATCH v3 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[Konrad Gräfe]

On 27.04.23 14:35, Rasmus Villemoes wrote:
> On 27/04/2023 13.51, Konrad Gräfe wrote:
>> The CDC-ECM specification requires an USB gadget to send the host MAC
>> address as uppercase hex string. This change adds the appropriate
>> modifier.
>>
>> Cc: stable@vger.kernel.org
> 
> Why cc stable?

I believe the second patch matches the criteria but it uses this one.

> 
>> Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
>> ---
>> Added in v3
>>
>>   lib/vsprintf.c | 18 +++++++++++++++---
>>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> The diffstat here, or for some other patch in the same series,
> definitely ought to mention lib/test_printf.c.
> 
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index be71a03c936a..8aee1caabd9e 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -1269,9 +1269,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
>>   {
>>   	char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
>>   	char *p = mac_addr;
>> -	int i;
>> +	int i, pos;
>>   	char separator;
>>   	bool reversed = false;
>> +	bool uppercase = false;
>>   
>>   	if (check_pointer(&buf, end, addr, spec))
>>   		return buf;
>> @@ -1281,6 +1282,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
>>   		separator = '-';
>>   		break;
>>   
>> +	case 'U':
>> +		uppercase = true;
>> +		break;
>> +
>>   	case 'R':
>>   		reversed = true;
>>   		fallthrough;
> 
> This seems broken, and I'm surprised the compiler doesn't warn about
> separator possibly being uninitialized further down. I'm also surprised
> your testing hasn't caught this. For reference, the full switch
> statement is currently
> 
>          switch (fmt[1]) {
>          case 'F':
>                  separator = '-';
>                  break;
> 
>          case 'R':
>                  reversed = true;
>                  fallthrough;
> 
>          default:
>                  separator = ':';
>                  break;
>          }
> 
>> @@ -1292,9 +1297,14 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
>>   
>>   	for (i = 0; i < 6; i++) {
>>   		if (reversed)
>> -			p = hex_byte_pack(p, addr[5 - i]);
>> +			pos = 5 - i;
>> +		else
>> +			pos = i;
>> +
>> +		if (uppercase)
>> +			p = hex_byte_pack_upper(p, addr[pos]);
>>   		else
>> -			p = hex_byte_pack(p, addr[i]);
>> +			p = hex_byte_pack(p, addr[pos]);
> 
> I think this becomes quite hard to follow. We have string_upper() in
> linux/string_helpers.h, so I'd rather just leave this loop alone and do
> 
>    if (uppercase)
>      string_upper(mac_addr, mac_addr);
> 
> after the nul-termination.
> 
> Rasmus
> 

Re: [PATCH v3 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[Peter Seiderer]

Hello Rasmus, Konrad, *,

On Thu, 27 Apr 2023 14:35:19 +0200, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On 27/04/2023 13.51, Konrad Gräfe wrote:
> > The CDC-ECM specification requires an USB gadget to send the host MAC
> > address as uppercase hex string. This change adds the appropriate
> > modifier.
> > 
> > Cc: stable@vger.kernel.org  
> 
> Why cc stable?
> 
> > Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
> > ---
> > Added in v3
> > 
> >  lib/vsprintf.c | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)  
> 
> The diffstat here, or for some other patch in the same series,
> definitely ought to mention lib/test_printf.c.
> 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index be71a03c936a..8aee1caabd9e 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1269,9 +1269,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
> >  {
> >  	char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
> >  	char *p = mac_addr;
> > -	int i;
> > +	int i, pos;
> >  	char separator;
> >  	bool reversed = false;
> > +	bool uppercase = false;
> >  
> >  	if (check_pointer(&buf, end, addr, spec))
> >  		return buf;
> > @@ -1281,6 +1282,10 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
> >  		separator = '-';
> >  		break;
> >  
> > +	case 'U':
> > +		uppercase = true;
> > +		break;
> > +
> >  	case 'R':
> >  		reversed = true;
> >  		fallthrough;  
> 
> This seems broken, and I'm surprised the compiler doesn't warn about
> separator possibly being uninitialized further down. I'm also surprised
> your testing hasn't caught this. For reference, the full switch
> statement is currently

Compiler (gcc) does not warn because of Makefile:

  1038 # Enabled with W=2, disabled by default as noisy
  1039 ifdef CONFIG_CC_IS_GCC
  1040 KBUILD_CFLAGS += -Wno-maybe-uninitialized
  1041 endif

With this commented:

  lib/vsprintf.c: In function ‘mac_address_string’:
  lib/vsprintf.c:1310:30: warning: ‘separator’ may be used uninitialized [-Wmaybe-uninitialized]
   1310 |                         *p++ = separator;
        |                         ~~~~~^~~~~~~~~~~
  lib/vsprintf.c:1273:14: note: ‘separator’ was declared here
   1273 |         char separator;
        |              ^~~~~~~~~

Regards,
Peter

> 
>         switch (fmt[1]) {
>         case 'F':
>                 separator = '-';
>                 break;
> 
>         case 'R':
>                 reversed = true;
>                 fallthrough;
> 
>         default:
>                 separator = ':';
>                 break;
>         }
> 
> > @@ -1292,9 +1297,14 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
> >  
> >  	for (i = 0; i < 6; i++) {
> >  		if (reversed)
> > -			p = hex_byte_pack(p, addr[5 - i]);
> > +			pos = 5 - i;
> > +		else
> > +			pos = i;
> > +
> > +		if (uppercase)
> > +			p = hex_byte_pack_upper(p, addr[pos]);
> >  		else
> > -			p = hex_byte_pack(p, addr[i]);
> > +			p = hex_byte_pack(p, addr[pos]);  
> 
> I think this becomes quite hard to follow. We have string_upper() in
> linux/string_helpers.h, so I'd rather just leave this loop alone and do
> 
>   if (uppercase)
>     string_upper(mac_addr, mac_addr);
> 
> after the nul-termination.
> 
> Rasmus
> 

Re: [PATCH v3 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[Rasmus Villemoes]

On 27/04/2023 13.51, Konrad Gräfe wrote:
> The CDC-ECM specification requires an USB gadget to send the host MAC
> address as uppercase hex string. This change adds the appropriate
> modifier.

Thinking more about it, I'm not sure this is appropriate, not for a
single user like this. vsprintf() should not and cannot satisfy all
possible string formatting requirements for the whole kernel. The %pX
extensions are convenient for use with printk() and friends where one
needs what in other languages would be "string interpolation" (because
then the caller doesn't need to deal with temporary stack buffers and
pass them as %s arguments), but for single items like this, snprintf()
is not necessarily the right tool for the job.

In this case, the caller can just as well call string_upper() on the
result, or not use sprintf() at all and do a tiny loop with
hex_byte_pack_upper().

Rasmus

Re: [PATCH v3 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[Greg Kroah-Hartman]

On Fri, Apr 28, 2023 at 08:56:59AM +0200, Rasmus Villemoes wrote:
> On 27/04/2023 13.51, Konrad Gräfe wrote:
> > The CDC-ECM specification requires an USB gadget to send the host MAC
> > address as uppercase hex string. This change adds the appropriate
> > modifier.
> 
> Thinking more about it, I'm not sure this is appropriate, not for a
> single user like this. vsprintf() should not and cannot satisfy all
> possible string formatting requirements for the whole kernel. The %pX
> extensions are convenient for use with printk() and friends where one
> needs what in other languages would be "string interpolation" (because
> then the caller doesn't need to deal with temporary stack buffers and
> pass them as %s arguments), but for single items like this, snprintf()
> is not necessarily the right tool for the job.

But sprintf() already creates mac address strings today, adding
yet-another-modifier makes it so that we don't have to hand-roll this
type of logic in the individual drivers that require it.

thanks,

greg k-h

RE: [PATCH v3 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[David Laight]

From: Rasmus Villemoes
> Sent: 28 April 2023 07:57
> 
> On 27/04/2023 13.51, Konrad Gräfe wrote:
> > The CDC-ECM specification requires an USB gadget to send the host MAC
> > address as uppercase hex string. This change adds the appropriate
> > modifier.
> 
> Thinking more about it, I'm not sure this is appropriate, not for a
> single user like this. vsprintf() should not and cannot satisfy all
> possible string formatting requirements for the whole kernel. The %pX
> extensions are convenient for use with printk() and friends where one
> needs what in other languages would be "string interpolation" (because
> then the caller doesn't need to deal with temporary stack buffers and
> pass them as %s arguments), but for single items like this, snprintf()
> is not necessarily the right tool for the job.
> 
> In this case, the caller can just as well call string_upper() on the
> result, or not use sprintf() at all and do a tiny loop with
> hex_byte_pack_upper().

Or snprintf with "%02X:%02X:%02X:%02X:%02X:%02X".

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v3 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[Andy Shevchenko]

On Fri, Apr 28, 2023 at 07:46:14AM +0000, David Laight wrote:
> From: Rasmus Villemoes
> > Sent: 28 April 2023 07:57
> > On 27/04/2023 13.51, Konrad Gräfe wrote:
> > > The CDC-ECM specification requires an USB gadget to send the host MAC
> > > address as uppercase hex string. This change adds the appropriate
> > > modifier.
> > 
> > Thinking more about it, I'm not sure this is appropriate, not for a
> > single user like this. vsprintf() should not and cannot satisfy all
> > possible string formatting requirements for the whole kernel. The %pX
> > extensions are convenient for use with printk() and friends where one
> > needs what in other languages would be "string interpolation" (because
> > then the caller doesn't need to deal with temporary stack buffers and
> > pass them as %s arguments), but for single items like this, snprintf()
> > is not necessarily the right tool for the job.
> > 
> > In this case, the caller can just as well call string_upper() on the
> > result, or not use sprintf() at all and do a tiny loop with
> > hex_byte_pack_upper().
> 
> Or snprintf with "%02X:%02X:%02X:%02X:%02X:%02X".

Of course this is a step back. Why? Have you read actually what we have in %p
extensions already?

Also, what about stack?

Entire %pm/M exists due to reversed order. Otherwise it's an alias to %6phD or
alike.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[Petr Mladek]

On Fri 2023-04-28 08:56:59, Rasmus Villemoes wrote:
> On 27/04/2023 13.51, Konrad Gräfe wrote:
> > The CDC-ECM specification requires an USB gadget to send the host MAC
> > address as uppercase hex string. This change adds the appropriate
> > modifier.
> 
> Thinking more about it, I'm not sure this is appropriate, not for a
> single user like this. vsprintf() should not and cannot satisfy all
> possible string formatting requirements for the whole kernel. The %pX
> extensions are convenient for use with printk() and friends where one
> needs what in other languages would be "string interpolation" (because
> then the caller doesn't need to deal with temporary stack buffers and
> pass them as %s arguments), but for single items like this, snprintf()
> is not necessarily the right tool for the job.
>
> In this case, the caller can just as well call string_upper() on the
> result

I tend to agree with Rasmus. string_upper() is a super-easy solution.
One user does not look worth adding all the churn into vsprintf().

Best Regards,
Petr

Re: [PATCH v3 1/2] vsprintf: Add %p[mM]U for uppercase MAC address

[Konrad Gräfe]

[-- Attachment #1.1: Type: text/plain, Size: 1377 bytes --]

On 02.05.23 14:23, Petr Mladek wrote:
> On Fri 2023-04-28 08:56:59, Rasmus Villemoes wrote:
>> On 27/04/2023 13.51, Konrad Gräfe wrote:
>>> The CDC-ECM specification requires an USB gadget to send the host MAC
>>> address as uppercase hex string. This change adds the appropriate
>>> modifier.
>>
>> Thinking more about it, I'm not sure this is appropriate, not for a
>> single user like this. vsprintf() should not and cannot satisfy all
>> possible string formatting requirements for the whole kernel. The %pX
>> extensions are convenient for use with printk() and friends where one
>> needs what in other languages would be "string interpolation" (because
>> then the caller doesn't need to deal with temporary stack buffers and
>> pass them as %s arguments), but for single items like this, snprintf()
>> is not necessarily the right tool for the job.
>>
>> In this case, the caller can just as well call string_upper() on the
>> result
> 
> I tend to agree with Rasmus. string_upper() is a super-easy solution.
> One user does not look worth adding all the churn into vsprintf().
> 
> Best Regards,
> Petr

I do agree as well. That would basically be v1 [1] without the 
hand-crafted string_upper(). (I didn't know the function.)

Regards,
Konrad

[1]: 
https://lore.kernel.org/linux-usb/94afd6e0-7300-e8f4-d52e-c21acec04f5b@gateware.de/

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]