| Message ID | 20221021064453.3341050-1-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: gadget: dummy_hcd: switch char * to u8 * | expand |
On Thu, Oct 20, 2022 at 11:44 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > 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 > Sent: 21 October 2022 18:31 ... > I think you might as well also remove the cast that was always a bit odd: > > buf[0] = (u8)dum->devstatus; ... > 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)
On Fri, Oct 21, 2022 at 10:30:37AM -0700, Linus Torvalds wrote: > On Thu, Oct 20, 2022 at 11:44 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > 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? (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. > 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
On Sun, Oct 23, 2022 at 05:53:19PM +0200, Greg Kroah-Hartman wrote: > On Fri, Oct 21, 2022 at 10:30:37AM -0700, Linus Torvalds wrote: > > On Thu, Oct 20, 2022 at 11:44 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > 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? > > (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. 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
On Sun, Oct 23, 2022 at 05:53:19PM +0200, Greg Kroah-Hartman wrote: > On Fri, Oct 21, 2022 at 10:30:37AM -0700, Linus Torvalds wrote: > > On Thu, Oct 20, 2022 at 11:44 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > 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? > > (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. 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. > 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
On Sun, Oct 23, 2022 at 9:04 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > 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
diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c index 899ac9f9c279..774781968e55 100644 --- a/drivers/usb/gadget/udc/dummy_hcd.c +++ b/drivers/usb/gadget/udc/dummy_hcd.c @@ -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);
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. Cc: Felipe Balbi <balbi@kernel.org> Cc: Jakob Koschel <jakobkoschel@gmail.com> Cc: Randy Dunlap <rdunlap@infradead.org> Cc: Ira Weiny <ira.weiny@intel.com> Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/usb/gadget/udc/dummy_hcd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)