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

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

[Konrad Gräfe]

[-- Attachment #1: Type: text/plain, Size: 851 bytes --]


As the CDC-ECM specification states the host MAC address must be sent to
the host as an uppercase hexadecimal string:
     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.

Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
---
  drivers/usb/gadget/function/u_ether.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)


[-- Attachment #2: 0001-usb-gadget-u_ether-Fix-host-MAC-address-case.patch --]
[-- Type: text/x-patch, Size: 815 bytes --]

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index 6956ad8ba8dd..250734e090fc 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -958,6 +958,7 @@ EXPORT_SYMBOL_GPL(gether_get_host_addr);
 int gether_get_host_addr_cdc(struct net_device *net, char *host_addr, int len)
 {
 	struct eth_dev *dev;
+	int i, slen;
 
 	if (len < 13)
 		return -EINVAL;
@@ -965,7 +966,13 @@ 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);
 
-	return strlen(host_addr);
+
+	slen = strlen(host_addr);
+
+	for (i = 0; i < slen; i++)
+		host_addr[i] = toupper(host_addr[i]);
+
+	return slen;
 }
 EXPORT_SYMBOL_GPL(gether_get_host_addr_cdc);
 

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

[Greg KH]

On Tue, Apr 25, 2023 at 03:15:08PM +0200, Konrad Gräfe wrote:
> 
> As the CDC-ECM specification states the host MAC address must be sent to
> the host as an uppercase hexadecimal string:
>     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.
> 
> Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
> ---
>  drivers/usb/gadget/function/u_ether.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 

> diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
> index 6956ad8ba8dd..250734e090fc 100644
> --- a/drivers/usb/gadget/function/u_ether.c
> +++ b/drivers/usb/gadget/function/u_ether.c
> @@ -958,6 +958,7 @@ EXPORT_SYMBOL_GPL(gether_get_host_addr);
>  int gether_get_host_addr_cdc(struct net_device *net, char *host_addr, int len)
>  {
>  	struct eth_dev *dev;
> +	int i, slen;
>  
>  	if (len < 13)
>  		return -EINVAL;
> @@ -965,7 +966,13 @@ 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);
>  
> -	return strlen(host_addr);
> +
> +	slen = strlen(host_addr);
> +
> +	for (i = 0; i < slen; i++)
> +		host_addr[i] = toupper(host_addr[i]);
> +
> +	return slen;
>  }
>  EXPORT_SYMBOL_GPL(gether_get_host_addr_cdc);
>  
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

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

[Konrad Gräfe]

[-- Attachment #1: Type: text/plain, Size: 891 bytes --]


As the CDC-ECM specification states the host MAC address must be sent to
the host as an uppercase hexadecimal string:
     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.

Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
---
V1 -> V2: Fixed checkpatch.pl warnings

  drivers/usb/gadget/function/u_ether.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)


[-- Attachment #2: 0001-usb-gadget-u_ether-Fix-host-MAC-address-case.patch --]
[-- Type: text/x-patch, Size: 815 bytes --]

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index 6956ad8ba8dd..250734e090fc 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -958,6 +958,7 @@ EXPORT_SYMBOL_GPL(gether_get_host_addr);
 int gether_get_host_addr_cdc(struct net_device *net, char *host_addr, int len)
 {
 	struct eth_dev *dev;
+	int i, slen;
 
 	if (len < 13)
 		return -EINVAL;
@@ -965,7 +966,13 @@ 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);
 
-	return strlen(host_addr);
+
+	slen = strlen(host_addr);
+
+	for (i = 0; i < slen; i++)
+		host_addr[i] = toupper(host_addr[i]);
+
+	return slen;
 }
 EXPORT_SYMBOL_GPL(gether_get_host_addr_cdc);
 

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

[Quentin Schulz]

Hi Konrad,

On 4/26/23 12:17, Konrad Gräfe wrote:
> 
> As the CDC-ECM specification states the host MAC address must be sent to
> the host as an uppercase hexadecimal string:
>      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.
> 
> Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
> ---
> V1 -> V2: Fixed checkpatch.pl warnings
> 

When sending a new version, please use the -v option of git format-patch 
to add the v2 prefix in the [PATCH] header, see 
https://lore.kernel.org/lkml/20230417065005.24967-1-yu.tu@amlogic.com/ 
(here a v7).

c.f. 
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format

Additionally, please use scripts/get_maintainer.pl on your patch to know 
who to put in the recipient list, here you're missing 
linux-kernel@vger.kernel.org which is always added. If I'm not mistaken, 
it's basically used for archival purposes but also so people don't have 
to subscribe or search through all mailing list to know what's going on 
globally in the kernel community.

>   drivers/usb/gadget/function/u_ether.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 

The patch should be the mail body and not attached, git send-email 
should be able to get you through the hoops. I've seen 
https://git-send-email.io/ mentioned in various places so maybe that 
could help you get started.
See 
https://lore.kernel.org/lkml/20230417065005.24967-2-yu.tu@amlogic.com/ 
and yours: 
https://lore.kernel.org/all/f147dabd-5c39-31c2-0ff0-f72745d7cd3f@gateware.de/

In yours, you can see
"""
[-- Attachment #1.1.2: 
0001-usb-gadget-u_ether-Fix-host-MAC-address-case.patch --]
[-- Type: text/x-patch, Size: 812 bytes --]
"""
this is not good.

The point of the mailing list patch submission process is to allow 
in-line review, people will typically comment in-line directly, see 
https://lore.kernel.org/lkml/20230426111358.xh3gbhlvxj46ggi5@CAB-WSD-L081021/ 
for example. Adding the patch as an attachment doesn't allow this review 
process to happen. There also are a few robots applying patches directly 
from the various mailing lists and running some build tests 
automatically, I am not sure they are developed with patch as an 
attachment in mind.

c.f. 
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#no-mime-no-links-no-compression-no-attachments-just-plain-text

For the "i'm not so sure" part of my mail now.
I don't know if this matches stable backport policy but I believe it 
could/should. So maybe add:
"""
Cc: stable@vger.kernel.org
"""
before your Signed-off-by, c.f. 
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

I think you should add:
"""
Fixes: bcd4a1c40bee ("usb: gadget: u_ether: construct with default 
values and add setters/getters")
"""
since (I believe) this is fixing an issue introduced in that commit.
c.f. 
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes 
and 
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

Cheers,
Quentin

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

[Greg KH]

On Wed, Apr 26, 2023 at 12:17:53PM +0200, Konrad Gräfe wrote:
> 
> As the CDC-ECM specification states the host MAC address must be sent to
> the host as an uppercase hexadecimal string:
>     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.
> 
> Signed-off-by: Konrad Gräfe <k.graefe@gateware.de>
> ---
> V1 -> V2: Fixed checkpatch.pl warnings

There is no "v2" in the subject line, so our tools will get confused and
have no idea this is a newer patch.

Please fix up and send a v3?

>  	snprintf(host_addr, len, "%pm", dev->host_mac);

Is there no option to print a mac address with all uppercase?  If not,
why not add that instead as it's needed here and maybe other places,
right?

thanks,

greg k-h

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