USB: gadget: dummy_hcd: switch char * to u8 *
Greg Kroah-Hartman
maintainerPATCH 1/1 USB: gadget: dummy_hcd: switch char * to u8 *
The function handle_control_request() casts the urb buffer to a char *, and then treats it like a unsigned char buffer when assigning data to it. On some architectures, "char" is really signed, so let's just properly set this pointer to a u8 to take away any potential problems as that's what is really wanted here.
--- drivers/usb/gadget/udc/dummy_hcd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/usb/gadget/udc/dummy_hcd.c +2 -2
@@ -1740,13 +1740,13 @@ static int handle_control_request(struct dummy_hcd *dum_hcd, struct urb *urb,if (setup->bRequestType == Dev_InRequest || setup->bRequestType == Intf_InRequest || setup->bRequestType == Ep_InRequest) { -char *buf; +u8 *buf;/* * device: remote wakeup, selfpowered * interface: nothing * endpoint: halt */ - buf = (char *)urb->transfer_buffer; + buf = urb->transfer_buffer;if (urb->transfer_buffer_length >0 ) {if (setup->bRequestType == Ep_InRequest) { ep2 =find_endpoint (dum, w_index);
Signature · 1 line
2.38.1
Quoted 6 lines · Greg Kroah-Hartman
The function
handle_control_request()casts the urb buffer to a char *, and then treats it like a unsigned char buffer when assigning data to it. On some architectures, "char" is really signed, so let's just properly set this pointer to a u8 to take away any potential problems as that's what is really wanted here.
I think you might as well also remove the cast that was always a bit odd:
buf[0] = (u8)dum->devstatus;
although maybe it's intentional ("look, ma, I'm truncating this value") because 'devstatus' is a 'u16' type?
I suspect a comment would be more readable than an odd cast that doesn't actually change anything (since the assignment does it anyway).
Or maybe people wrote it that way on purpose, and used that variable name on purpose.
Because 'dum' is Swedish for 'stupid', and maybe there's a coded message in that driver?
Linus
From: Linus Torvalds
Quoted 1 line
Sent: 21 October 2022 18:31
...
Quoted 3 lines
I think you might as well also remove the cast that was always a bit odd:
buf[0] = (u8)dum->devstatus;
...
Quoted 3 lines
I suspect a comment would be more readable than an odd cast that doesn't actually change anything (since the assignment does it anyway).
I've seen a compiler generate an '& 0xff' for the cast before storing the low byte.
Don't even think about what that compiler would generated for:
buf[0] = (u8)(dum->devstatus & 0xff);
The code is much easier to read as just an assignment :-)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Greg Kroah-Hartman
maintainerQuoted 9 lines · Linus Torvalds
Quoted 6 lines · Greg Kroah-Hartman
The function
handle_control_request()casts the urb buffer to a char *, and then treats it like a unsigned char buffer when assigning data to it. On some architectures, "char" is really signed, so let's just properly set this pointer to a u8 to take away any potential problems as that's what is really wanted here.
Quoted 6 lines
I think you might as well also remove the cast that was always a bit odd:
buf[0] = (u8)dum->devstatus;although maybe it's intentional ("look, ma, I'm truncating this value") because 'devstatus' is a 'u16' type?
(adding Alan as he's the owner of this file now)
Yes, devstatus is a u16 as that's what the USB spec says it should be, but so far only 7 of the lower bits have been used. I guess to do this properly we should also copy the upper 8 bits in to buf[1], eventhough in reality it's only ever going to be 0x00 for now.
Although if we ever do get another 2 status bits defined, this code will break so we probably should do that too.
I'll go do that for a v2 of this and properly comment it.
Quoted 9 lines
I suspect a comment would be more readable than an odd cast that doesn't actually change anything (since the assignment does it anyway).
Or maybe people wrote it that way on purpose, and used that variable name on purpose.
Because 'dum' is Swedish for 'stupid', and maybe there's a coded message in that driver?
That whole driver is called "dummy" as it's a "fake" driver, not a "stupid" one :)
thanks,
greg k-h
Greg Kroah-Hartman
maintainerQuoted 17 lines · Greg Kroah-Hartman
Quoted 9 lines · Linus Torvalds
Quoted 6 lines · Greg Kroah-Hartman
The function
handle_control_request()casts the urb buffer to a char *, and then treats it like a unsigned char buffer when assigning data to it. On some architectures, "char" is really signed, so let's just properly set this pointer to a u8 to take away any potential problems as that's what is really wanted here.Quoted 6 lines
I think you might as well also remove the cast that was always a bit odd:
buf[0] = (u8)dum->devstatus;although maybe it's intentional ("look, ma, I'm truncating this value") because 'devstatus' is a 'u16' type?
Quoted 6 lines
(adding Alan as he's the owner of this file now)
Yes, devstatus is a u16 as that's what the USB spec says it should be, but so far only 7 of the lower bits have been used. I guess to do this properly we should also copy the upper 8 bits in to buf[1], eventhough in reality it's only ever going to be
0x00for now.
Along these lines, do we really not have a predefined macro/inline function that does:
(value >> 8)
to give you the "high byte" of a 16bit value? I keep seeing people write their own macros for this in staging drivers, but I just attributed that to them not using the correct in-kernel macro, but I can't seem to find anything at the moment to do this (same with "give me just the lower 8 bits of a 16bit value").
Am I just blind?
It's not like it's complex or tricky stuff, I just thought we had something in bits.h or bitops.h or the like. Oh well...
thanks,
greg k-h
Quoted 5 lines · Greg Kroah-Hartman
Along these lines, do we really not have a predefined macro/inline function that does:
(value >> 8)to give you the "high byte" of a 16bit value?
No macros like that. And honestly, why would you want a macro that is more complicated than the operation itself?
But it sounds like what you actually want is just
put_unaligned_le16(dum->devstatus, buf);
which does both bytes correctly (and turns into a plain 16-bit store on sane architectures)..
Linus
Quoted 17 lines · Greg Kroah-Hartman
Quoted 9 lines · Linus Torvalds
Quoted 6 lines · Greg Kroah-Hartman
The function
handle_control_request()casts the urb buffer to a char *, and then treats it like a unsigned char buffer when assigning data to it. On some architectures, "char" is really signed, so let's just properly set this pointer to a u8 to take away any potential problems as that's what is really wanted here.Quoted 6 lines
I think you might as well also remove the cast that was always a bit odd:
buf[0] = (u8)dum->devstatus;although maybe it's intentional ("look, ma, I'm truncating this value") because 'devstatus' is a 'u16' type?
Quoted 6 lines
(adding Alan as he's the owner of this file now)
Yes, devstatus is a u16 as that's what the USB spec says it should be, but so far only 7 of the lower bits have been used. I guess to do this properly we should also copy the upper 8 bits in to buf[1], eventhough in reality it's only ever going to be
0x00for now.
I count only 5 of the bits being used, not 7. (See Figure 9-4 in section 9.4.5 of the USB-3.1 spec.) Maybe you're thinking of Feature flags rather than Status bits? They do have a lot of overlap.
Dave Brownell wrote that code initially, so we'll never know why he included the cast. My guess is the same as Linus's: It's there to alert the reader that a 16-bit value is being shortened to squeeze into an 8-bit slot.
Quoted 4 lines
Although if we ever do get another 2 status bits defined, this code will break so we probably should do that too.
I'll go do that for a v2 of this and properly comment it.
Note that there's an out-of-date comment line just above this part of the code:
* device: remote wakeup, selfpowered
Thanks to USB-3 support, the device recipient now defines three more bits in devstatus: LTM_ENABLED, U1_ENABLED, and U2_ENABLED.
Alan Stern