On Fri, 11 May 2012, Sarah Sharp wrote: > On Fri, May 11, 2012 at 10:18:47AM -0400, Alan Stern wrote: > > On Fri, 11 May 2012, Andiry Xu wrote: > > > > > > Okay. Why do you care about short packets on isochronous transfers? I > > > > wouldn't think they make any difference. > > > > > > > > > > If we do not set ISP for ISOC IN transfers, there is no event produced > > > when a short packet occurs and the driver cannot update the status and > > > length field of iso_packet_descriptor accordingly. > > > > You are begging the question. Why do you care if there is no event > > produced when a short packet occurs? After all, there is no event when > > a full-sized packet occurs; what's special about short packets? > > > > As for the iso_packet_descriptor status field... Surely that is > > already being updated. Otherwise it would always contain -EINPROGRESS, > > because that's what it gets set to when URBs are submitted. > > When we get an event for the last TD in an isochronous URB, we go > through the frame list and mark any iso_packet_descriptor status field > as successful that was between the last TD that generated an event and > the last TD in the frame list. So if we don't get an event that tells > us a short packet occurred on a TD somewhere in the middle of the frame > list, we will mark it as successful, and the driver will consume random > bytes. Well, this is just a choice of driver implementation. Obviously it's entirely up to you. I will merely point out that instead of marking all those status fields as successful, you could mark each of them with whatever status code is appropriate. As for whether the driver consumes random bytes, that doesn't depend on the status -- it depends on the actual_length field. It's also worth mentioning that none of uhci-hcd, ohci-hcd, or ehci-hcd bother to set the iso_packet_descriptor status to -EREMOTEIO when a short packet is received. There's even a comment about it in ohci-q.c:td_done(): /* short reads are always OK for ISO */ if (cc == TD_DATAUNDERRUN) cc = TD_CC_NOERROR; usb_hcd_giveback_urb() will in any case automatically set the URB's status to -EREMOTEIO if the status is 0, URB_SHORT_NOT_OK is set, and urb->actual_length is less than urb->transfer_buffer_length. But for isochronous transfers this is basically irrelevant, because they are unreliable by definition. > > Much the same is true for the length field. In fact, you must really > > mean the actual_length field, because the length field does not get > > updated. Besides, doesn't Sarah's most recent patch set the > > actual_length field correctly when a short packet occurs? > > The actual_length field in the event TRB is only for the last TD in the > frame list. It is not an accumulation of all the lengths in the frame > list. The xHC hardware has no idea where the frame list for an URB ends > or begins. It only sees that one TRB in a number of TDs has the IOC > flag set, and so it will only generate an event for that TD. So we need > to know if anything bad happened (including a short packet) for the > other TDs in the frame list. The main point I'm trying to make is that you don't really need an event to be generated whenever a short packet is received. You could handle short packets and full-sized ones the same -- just add up the number of bytes actually transferred. Full-sized packets don't generate events (except for the last one in an URB), so short packets shouldn't need to either. The secondary point is that short packets in isochronous transfers aren't "bad". > My patch only changes the completion status, not the actual_length > field. It actually doesn't touch the hardware's event TRB at all, it > just modifies a local copy of the completion status from the event TRB. That same completion status is available in the transfer TRB, right? Which suggests the event TRB is unnecessary. Alan Stern -- 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
