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.
Document that we are only using the lower 8 bits for the devstatus variable (only 7 are currently used), as the cast from 16 to 8 is not obvious.
--- drivers/usb/gadget/udc/dummy_hcd.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/usb/gadget/udc/dummy_hcd.c +7 -6
@@ -1739,13 +1739,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 +* device: remote wakeup, selfpowered, LTM, U1, or U2 * 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);
@@ -1754,11 +1754,12 @@ static int handle_control_request(struct dummy_hcd *dum_hcd, struct urb *urb,break ; } buf[0 ] = ep2->halted; - }else if (setup->bRequestType == - Dev_InRequest) { + }else if (setup->bRequestType == Dev_InRequest) { +/* Only take the lower 8 bits */ buf[0 ] = (u8 )dum->devstatus; - }else + }else { buf[0 ] =0 ; + } }if (urb->transfer_buffer_length >1 ) buf[1 ] =0 ;
Signature · 1 line
2.44.0
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 1 line
Reported-by Linus Torvalds <[email protected]>
Well, I assume this goes back to the discussions almost two years ago that then just led us to use '-funsigned-char' for the kernel.
So the patch is still correct, but it's not like it's strictly necessary. I have no idea how this re-surfaced now.
Linus
Greg Kroah-Hartman
maintainerQuoted 8 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 5 lines
[..]
Quoted 1 line
Reported-by Linus Torvalds <[email protected]>Well, I assume this goes back to the discussions almost two years ago that then just led us to use '-funsigned-char' for the kernel.
Yes.
Quoted 2 lines
So the patch is still correct, but it's not like it's strictly necessary. I have no idea how this re-surfaced now.
It was in my really old patch queue and I noticed it had never been updated or merged, so I dug it up and fixed it based on Alan's review.
thanks,
greg k-h
Quoted 5 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.
Yes, it certainly ought to be u8 rather than char.
Quoted 3 lines
Document that we are only using the lower 8 bits for the devstatus variable (only 7 are currently used), as the cast from 16 to 8 is not obvious.
It wouldn't hurt to change the code so that it copies all 16 bits. I think it was originally done the way it is now because that was easier, not because there was any significance to the 8-bit/16-bit alteration.
Quoted 16 lines
Cc Felipe Balbi <[email protected]>Cc Jakob Koschel <[email protected]>Cc Randy Dunlap <[email protected]>Cc Ira Weiny <[email protected]>Cc Alan Stern <[email protected]>Reported-by Linus Torvalds <[email protected]>Signed-off-by Greg Kroah-Hartman <[email protected]>--- drivers/usb/gadget/udc/dummy_hcd.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)drivers/usb/gadget/udc/dummy_hcd.c +0
-0
drivers/usb/gadget/udc/dummy_hcd.c static int handle_control_request(struct dummy_hcd *dum_hcd, struct urb *urb, · line 1744
Quoted 8 lines
drivers/usb/gadget/udc/dummy_hcd.c +2
-2@@ -1739,13 +1739,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 +* device: remote wakeup, selfpowered, LTM, U1, or U2
Also B_HNP_ENABLE, A_HNP_SUPPORT, A_ALT_HNP_SUPPORT. Not sure it's really worthwhile to list all these things in a comment.
Also, it would be good to reorder the comment lines so that they match the code: endpoint first, then device, then interface.
Alan Stern
Quoted 27 lines
drivers/usb/gadget/udc/dummy_hcd.c +5
-4* 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);@@ -1754,11 +1754,12 @@ static int handle_control_request(struct dummy_hcd *dum_hcd, struct urb *urb,break ; } buf[0 ] = ep2->halted; - }else if (setup->bRequestType == - Dev_InRequest) { + }else if (setup->bRequestType == Dev_InRequest) { +/* Only take the lower 8 bits */ buf[0 ] = (u8 )dum->devstatus; - }else + }else { buf[0 ] =0 ; + } }if (urb->transfer_buffer_length >1 ) buf[1 ] =0 ;Signature · 1 line
2.44.0