On Thu, Jul 08, 2010 at 07:03:11PM +0800, Xu, Andiry wrote: > > -----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. Yes, I'll try to get to the other patches as soon as I can. It's just the first thing I noticed. Sarah Sharp -- 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
