On Mon, 2 Feb 2009, David Vrabel wrote:
> > Instead of adding new fields to struct urb, we could overload the
> > existing fields. For instance, the scatterlist address could be stored
> > in urb->transfer_buffer and the number of entries could be stored in
> > urb->number_of_packets. If that value is 0 then it's a normal URB.
> > (That's because only Bulk will use sg; Iso won't.)
>
> I think this is too confusing. I could add some inline functions for
> retrieving the sg related values if you still want the overloaded fields.
I don't know; let's see what other people think.
> > The code that takes care of setting up the transfer buffer addresses
> > should be split out and EXPORTed, because any HCD that supports sg will
> > want to use it.
>
> I'm not sure what code you're referring to?
I'm talking about all this stuff, which (rightly) got left out of
usb_sg_init_with_sg():
/*
* Some systems need to revert to PIO when DMA is temporarily
* unavailable. For their sakes, both transfer_buffer and
* transfer_dma are set when possible. However this can only
* work on systems without:
*
* - HIGHMEM, since DMA buffers located in high memory are
* not directly addressable by the CPU for PIO;
*
* - IOMMU, since dma_map_sg() is allowed to use an IOMMU to
* make virtually discontiguous buffers be "dma-contiguous"
* so that PIO and DMA need diferent numbers of URBs.
*
* So when HIGHMEM or IOMMU are in use, transfer_buffer is NULL
* to prevent stale pointers and to help spot bugs.
*/
if (dma) {
io->urbs[i]->transfer_dma = sg_dma_address(sg);
len = sg_dma_len(sg);
#if defined(CONFIG_HIGHMEM) || defined(CONFIG_GART_IOMMU)
io->urbs[i]->transfer_buffer = NULL;
#else
io->urbs[i]->transfer_buffer = sg_virt(sg);
#endif
} else {
/* hc may use _only_ transfer_buffer */
io->urbs[i]->transfer_buffer = sg_virt(sg);
len = sg->length;
}
In addition, there are complications involving the hcd_alloc_coherent()
routine in hcd.c:map_urb_for_dma(). Consider also these lines in your
patch:
> + if (dma)
> + urb_flags |= URB_NO_TRANSFER_DMA_MAP;
> + if (usb_pipein(pipe))
> + urb_flags |= URB_SHORT_NOT_OK;
At the very least, the mapping code in hcd.c will need changes to
accomodate this stuff. And I suspect URB_SHORT_NOT_OK won't be needed
at all.
> The sg list is already mapped and the HCD need only do something like this:
>
> for_each_sg(sg, sg, std->urb->sg_nents, i) {
> std->sgl_virt[i].buf_ptr = cpu_to_le64(sg_dma_address(sg));
> std->sgl_virt[i].len = cpu_to_le32(sg_dma_len(sg));
> }
>
> to fill in the hardware's sg list.
Ah, but you're forgetting the possibile cases in which the controller
doesn't support DMA. That is an independent question from whether the
HCD supports scatterlists.
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
