On Thu, Oct 31, 2013 at 04:16:14PM -0000, David Laight wrote: > Code was recently added to the xhci driver to dynamically add extra ring > segments if there is insufficient space for all the fragments of the > URB being sent. > > ISTM that it would be better to just queue the URB (on the ring) > and process it when space becomes available. Hmm, better why? I'm reluctant to touch code that Just Works (TM). Especially anything having do with the xHCI ring code, since it's quite complex. > This wouldn't solve the problem of pathologically badly fragmented, > or very long URB, but there are simpler solutions to that. > > I believe that very long bulk transfers can be split into > separate TD on USB block boundaries - so they can be safely > added to the ring in several pieces. No, that's not a good idea. Think about the case where the device driver sets up an IN transfer to receive a specific amount of data from the device. The device can choose to send less data, resulting in a short packet TD completion event. If you queue the URB as one TD, when the short packet occurs, the host returns the short status, and stops requesting any further packets from the device. It then goes on to process the next TD in the ring. Now, assume you queue the URB as multiple TDs. Say you split the URB into five TDs, and the short packet occurs on TD three. The host will return a short packet completion event, and then go on to request data from the device for TDs four and five. This is not what you want, because it changes the bus behavior. > I don't know if any other data type is allowed to exceed the > block length? > > Badly fragmented URB are probably best copied into a linear buffer. > > This would make the code a lot simpler, especially since the > tx setup could be started if there were a 'reasonable' number > of free entries in the TRB ring without having to calculate > the actual number needed (and > > Additionally it would make it possible to ensure that LINK TRB > only happen on the appropriate boundary. This might require > some fragments be copied to a linear buffer. > > If this is a reasonable idea I'll volunteer to do the changes. > We are seeing some issues with the xhci driver, and parts of it > are just to contorted! The code is complex, especially the interaction between the code to queue to the xHCI rings, the URB cancellation code, and the command ring cancellation code. Any big changes like this would be bound to introduce new bugs. Frankly, since these are your first kernel patches, I can't trust any big sweeping changes you make unless I can throughly understand them. That's just a fact about being a kernel maintainer; I have to understand the code that gets contributed because the contributor is highly likely to disappear, leaving me to fix bugs in their code years later. So, I would recommend you work on small fixes and earn my trust before making big sweeping changes. Let's get the scatter gather issue fixed before you completely re-architect the xHCI driver, okay? :) 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
