On Sat, Jun 22, 2013 at 1:42 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Sat, 22 Jun 2013, Ming Lei wrote: > >> On Sat, Jun 22, 2013 at 12:45 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: >> >> Sorry, could you explain why that won't work? I understand the URBs >> >> with maxp-unaligned length still can be completed. >> > >> > Suppose the SG list has two elements, where the first element's length >> > is 1000 (not a multiple of 64) and the second element's length is 512. >> > Then usb_sg_init() will create two URBs to carry out the transfer. The >> > transfer_length of the first URB will be 1000 and the transfer_length >> > of the second URB will be 512. >> > >> > When the first URB is submitted to uhci-hcd, it will be broken up into >> > 15 TDs of length 64 (the maxpacket size) followed by a TD of length 40. >> > The second URB will be broken up into 8 TDs of length 64. >> > >> > Therefore the device will see 15 full-size packets, a short packet, and >> > then 8 more full-size packets. This is not what you want. >> >> From view of driver, maybe they only want to transfer 1512 bytes, and >> it is hard to say what the driver or device wants. Maybe they don't depend >> on the middle short packet at all, maybe they do, who knows? > > Nonsense. Transfers do not have short packets in the middle, only at > the end. If the driver wants to have 1512 bytes in a single transfer > then there must be 23 packets of length 64 followed one packet of > length 40. > > If the driver wants to send 15 packets of length 64 followed by one > packet of length 40 and 8 more packets of length 64, then it must use > two transfers. > >> Denis's patch still can't do what you hope(short packet is at the end of the >> transfer), can it? > > What I hope is that we can prevent SIGBUS or other memory access > errors. The patch will do that. If we only want to prevent this errors, there should be better approach, IMO. Since you mentioned it doesn't make sense to generate short packet in the middle of transfer, also it may not be what the driver/device expected, I suggest to add a check in usb_submit_urb() for this case and returns failure on it simply, because all HCDs shouldn't support this sort of thing. The check in usb_submit_urb() can avoid unnecessary change in HCD. Any comments on the idea? > > The test case mentioned in the patch description (testusb -a -t 7 -c > 2000 -s 4096 -v 41) is expected to fail, because 41 is not a multiple > of 64. But it shouldn't crash. Maybe the usbtest driver should refuse > to run test 7 and 8 if the "vary" parameter isn't a multiple of the > endpoint's maxpacket length. > >> >> > Furthermore, URBs using SG can be submitted directly, without going >> >> > through usb_sg_init() at all. So changing usb_sg_init() won't fix the >> >> > problem. >> >> >> >> Looks usbtest use it, so the reported problem can be fixed. >> > >> > Sometimes it's hard to tell whether you are serious or joking. >> >> I mean other places still can do this way like usb_sg_init(). > > You mean other places still can submit SG transfers with invalid > element lengths? Yes, they can. That's why the fix has to be in > uhci-hcd, not in usb_sg_init(). > > Alan Stern > Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
