On Sat, 1 May 2010, Matthew Wilcox wrote:
> On Sat, May 01, 2010 at 03:02:42PM -0400, Alan Stern wrote:
> > On Sat, 1 May 2010, Matthew Wilcox wrote:
> > > Also, with just a couple of lines extra, I included
> > > support for OHCI and UHCI controllers (because the USB midlayer handles
> > > an SGL with one element), so this feels like a big win to me.
> >
> > Are you sure about this last part? As things stand currently, the only
> > way to use SG with ohci-hcd or uhci-hcd is by way of usb_sg_init() and
> > friends -- which has the disadvantage of being synchronous and
> > requiring a process context. Although usbcore could be changed so that
> > single-element SG lists would work directly, at the moment they don't
> > (the DMA address needs to be copied from the scatterlist structure into
> > the URB -- is that your couple of extra lines?).
>
> Erm, your as1368 patch added support for single-element SG lists. This is
> what I mean:
>
> uas:
> if (udev->bus->sg_tablesize) {
> urb->num_sgs = sdb->table.nents;
> } else {
> urb->num_sgs = 0;
> }
> urb->sg = sdb->table.sgl;
>
> hcd:
> if (urb->num_sgs) {
> [...]
> } else if (urb->sg) {
> struct scatterlist *sg = urb->sg;
> urb->transfer_dma = dma_map_page(
> hcd->self.controller,
> sg_page(sg),
> sg->offset,
> urb->transfer_buffer_length,
> dir);
By golly, you're right. It was an unintended side effect. I hadn't
considered drivers directly submitting requests in this way. I thought
you meant you had set sg_tablesize to 1 in uhci-hcd and ohci-hcd.
> > Also, there's a matter of efficiency. Right now usb-storage works fine
> > with arbitrary-length SG lists on OHCI or UHCI, because it uses
> > usb_sg_*(). If you change things so that the HCDs advertise native
> > scatter-gather support with a maximum list size of 1, it will cause a
> > big performance hit. Every single SG buffer would require its own SCSI
> > command, whereas now one command can handle lots of buffers together.
> > (We could avoid this performance hit by not using an HCD's native SG
> > support if sg_tablesize is 1, but that's not an attractive hack.)
>
> I'm not planning on changing usb-storage at all. Obviously, if you want
> to change it, I'm not going to stand in your way. I don't think using
> multiple SCSI commands with UAS will be as much of a performance hit as
> using multiple SCSI commands with BOT because UAS does permit commands
> to overlap.
Okay, we've been talking past each other. Again, I thought you were
suggesting setting sg_tablesize to 1 in uhci-hcd and ohci-hcd. That
would have a detrimental effect on usb-storage.
You're right that the performance hit will be less with UAS than with
BOT, but it might still be larger than one would like. It all depends
on how the speed of the device compares to the speed of the USB bus.
If the bus speed is the bottleneck then adding extra transfers will
slow things down, otherwise it won't (or at least, not as much).
> > In principle, adding native scatterlist support to uhci-hcd and
> > ohci-hcd wouldn't be very hard -- ading it to ehci-hcd was fairly easy.
> > But I have yet to be convinced that it would be worth the effort. A
> > better approach (and one that would help all the other USB-1.1 HCDs)
> > would be to improve the usb_sg_*() family by adding an asynchronous
> > mode of operation. Pete Zaitcev and I actually implemented this a
> > while back, but it never was submitted or merged.
>
> I'm not convinced we need to go to the effort of improving the UHCI and
> OHCI drivers for the benefit of UAS. As far as I can tell, the only real
> way you'd end up talking UAS through one of those controllers (on a PC
> anyway) is either to find a system from 2001 (ICH4 onwards has support for
> EHCI), or plug a USB 1.1 hub into the mix between the device and the host.
> I don't care too much about performance in those situations, but it
> is nice for it to work.
>
> As far as an asynchronous version of usb_sg_* ... I don't really see the
> advantage to doing that over improving the usb_submit_urb() support for
> sg lists.
So now that we're both on the same page... What improved support do
you mean?
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
