> -----Original Message----- > From: Sarah Sharp [mailto:sarah.a.sharp@xxxxxxxxxxxxxxx] > Sent: Thursday, July 08, 2010 5:35 PM > To: Xu, Andiry > Cc: linux-usb@xxxxxxxxxxxxxxx; gregkh@xxxxxxx > Subject: Re: [PATCH 1/9 v7] xHCI: handle_tx_event() refactor: finish_td > > One small thing I noticed is that this changes what the xHCI driver does > with urb->status. I think that the host driver isn't supposed to > directly set urb->status. I think the host driver is only supposed to > pass the > status to usb_hcd_giveback_urb(). > > The problem is that drivers aren't supposed to look at urb->status > until their completion function is called, but I think some of them > still might. So if the URB generates multiple events, the driver might > see the change of status for the first event and deallocate the URB > before the xHCI host has a chance to deal with the second event. I > think it's best to let the USB core handle setting urb->status. > > I did a quick grep for "urb->status", and none of the major hosts (EHCI, > UHCI, OHCI) directly set urb->status. There are a few embedded ones > (fhci, imx21, isp1362, isp1760, & oxu210hp), but I have no idea if > they're doing the correct thing. > > Plus, I'm not sure if finish_td() should unconditionally set > urb->status. I would have to think hard about the stopped endpoint, > short packet, and stall cases. The problem is we can get multiple > events for an URB, and I'm not sure if the xHCI driver should overwrite > urb->status for each of those events. In some cases, taking the last > status could be OK, but I'm not sure if it's true of all cases. > > It's a pretty easy fix for this though. Just pass the address of the > status variable in handle_tx_event() to finish_td(), and let it > overwrite it if necessary. Then you don't have to set urb->status in > finish_td(). > > I think this issues effects other refactoring patches and the > isochronous patches. > > Thanks for the comment. I've thought about this problem when making the patches and searched for solutions in other host controllers. Some drivers set urb->status directly so I think it should be OK. But it seems better to leave urb->status as -EINPROGRESS when it's still possessed by host controller. I'll adopt your solution and fix the issue. Please help to review other patches so I can resubmit the patchset. For PM patches, I'm still testing them and will resubmit when they're ready. Thanks, Andiry -- 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
