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: [email protected]
Signed-off-by: Konrad Gräfe <[email protected]>
---
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
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: [email protected]
Signed-off-by: Konrad Gräfe <[email protected]>
---
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
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: [email protected]
> Signed-off-by: Konrad Gr?fe <[email protected]>
> ---
> 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
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: [email protected]
Why cc stable?
> Signed-off-by: Konrad Gräfe <[email protected]>
> ---
> 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
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: [email protected]
>
> Why cc stable?
I believe the second patch matches the criteria but it uses this one.
>
>> Signed-off-by: Konrad Gräfe <[email protected]>
>> ---
>> 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
>
Hello Rasmus, Konrad, *,
On Thu, 27 Apr 2023 14:35:19 +0200, Rasmus Villemoes <[email protected]> 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: [email protected]
>
> Why cc stable?
>
> > Signed-off-by: Konrad Gräfe <[email protected]>
> > ---
> > 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
>
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: [email protected]
Signed-off-by: Konrad Gräfe <[email protected]>
---
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
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: [email protected]
Signed-off-by: Konrad Gräfe <[email protected]>
---
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
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
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
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)
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
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
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
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/[email protected]/
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: [email protected]
Signed-off-by: Konrad Gräfe <[email protected]>
---
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