On Wed, Feb 04, 2009 at 06:54:04PM +0000, David Vrabel wrote:
> Here's a new version of the patch:
>
> No new fields are added to struct urb. urb->transfer_buffer points to a
> struct sg_table if urb->transfer_flags & URB_HAS_SG.
>
> The sg table stores both the original number of entries and the number
> of DMA mapped entries allowing HCDs to choose whether to iterate over
> the DMA mapped entries or the original unmapped ones.
I'm a bit confused as to why a host controller would want to limit the
number of entries in an sg list (instead of say, the size of the overall
transfer or some other metric). I can't see why I wouldn't want to
limit the number of sg list entries for xHCI, but I'm not familiar with
what the scsi core does with Scsi_Host->sg_tablesize.
The only usage of bus->sg_tablesize is here:
Index: linux-working/drivers/usb/core/message.c
...
+ if (dev->bus->sg_tablesize) {
+ ret = usb_sg_init_with_sg(io, dev, pipe, period, sg, length, mem_flags);
+ } else {
+ ret = usb_sg_init_without_sg(io, dev, pipe, period, sg, length, mem_flags);
+ }
Index: linux-working/drivers/usb/storage/usb.c
...
+ /*
+ * If the USB HCD supports sg lists, limit sg_tablesize.
+ */
+ {
+ struct usb_device *usb_dev = interface_to_usbdev(intf);
+ if (usb_dev->bus->sg_tablesize) {
+ host->sg_tablesize = usb_dev->bus->sg_tablesize;
+ }
+ }
us = host_to_us(host);
memset(us, 0, sizeof(struct us_data));
mutex_init(&(us->dev_mutex));
Index: linux-working/include/linux/usb.h
...
u8 otg_port; /* 0, or number of OTG/HNP port */
unsigned is_b_host:1; /* true during some HNP roleswitches */
unsigned b_hnp_enable:1; /* OTG: did A-Host enable HNP? */
+ int sg_tablesize; /* 0 or largest number of sg list entries */
int devnum_next; /* Next open device number in
* round-robin allocation */
@@ -1114,6 +1115,7 @@
I think there's also a memory leak:
Index: linux-working/drivers/usb/core/message.c
...
+ /* initialize one urb with transfer_buffer pointing to a new sg_table */
+ io->urbs = kmalloc(sizeof(struct urb *) + sizeof(struct sg_table), mem_flags);
+ if (!io->urbs)
+ return -ENOMEM;
+
+ sgt = (struct sg_table *)&io->urbs[1];
+ sgt->sgl = sg;
+ sgt->orig_nents = io->nents;
+
+ /* not all host controllers use DMA (like the mainstream pci ones);
+ * they can use PIO (sl811) or be software over another transport.
+ */
+ if (dev->dev.dma_mask != NULL)
+ sgt->nents = usb_buffer_map_sg(dev, usb_pipein(pipe), sg, io->nents);
+ else
+ sgt->nents = sgt->orig_nents;
+ if (sgt->nents <= 0)
+ return sgt->nents;
+
+ io->urbs[0] = usb_alloc_urb(0, mem_flags);
+ if (!io->urbs[0])
+ return -ENOMEM;
+ io->entries = 1;
Do you want to free io->urbs[0] and sg_table if usb_alloc_urb() fails?
sg_clean() won't free anything io->urbs points to because io->entries is
set to zero by sg_init().
Other than that, and my confusion with bus->sg_tablesize, the patch
looks fine. I think can easily rework my xHCI patches to look for
URB_HAS_SG and pull the sg list out of urb->transfer_buffer.
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
