[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b8ca96d0-d1e3-4f2f-5541-a6c82489243a@gateware.de>
Date: Thu, 27 Apr 2023 16:26:05 +0200
From: Konrad Gräfe <k.graefe@...eware.de>
To: Rasmus Villemoes <linux@...musvillemoes.dk>,
Quentin Schulz <quentin.schulz@...obroma-systems.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Petr Mladek <pmladek@...e.com>,
Steven Rostedt <rostedt@...dmis.org>,
Sergey Senozhatsky <senozhatsky@...omium.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
Kyungmin Park <kyungmin.park@...sung.com>,
Andrzej Pietrasiewicz <andrzej.p@...labora.com>,
Felipe Balbi <balbi@...com>
Cc: stable@...r.kernel.org
Subject: Re: [PATCH v3 1/2] vsprintf: Add %p[mM]U for uppercase MAC address
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@...r.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@...eware.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
>
Powered by blists - more mailing lists