Re: [linux-usb-devel] Re: [Bluetooth] HCI USB driver update. Support for SCO over HCI USB.

[Max Krasnyansky <maxk@qualcomm.com>]

Hi David,

(sorry for not replying right away)

At 09:55 PM 4/29/2003, David Brownell wrote:
>Max Krasnyansky wrote:
>>Basically I'd like to have same kind of API that we have for SKB without
>>an overhead of SKBs.
>
>I'd also like to see most of the per-request invocation costs
>shrink.  You seem to be focussing on the queueing parts of
>SKB-ness, yes?  
Yes.

>The lifecycles aren't that close; and other
>parts of the SKB and URB models are different too.
Totally.

>Inside USB, "usbcore" and the HCDs are already working with
>queues of URBs associated with each endpoint.
>
>The USB device drivers can't do that as easily, since core/HCD
>owns the urb->urb_list field after usb_submit_urb() and before
>the completion callback is issued.  Much like the next layer
>owns the SKB after you hand it off...
Yep. That's exactly what I'd like to address.

>>Here is what I've done for Bluetooth HCI USB driver.
>>struct _urb_queue {
>>        struct list_head head;
>>        spinlock_t       lock;
>>};
>>struct _urb {
>>        struct list_head  list;
>>        struct _urb_queue *queue;
>
>Those fields (reordered) are like sk_buff_head and sk_buff.
>How much of that is "needed here" vs. "SKBs work like that"?
If we want to provide urb_queue(),urb_dequeue(),etc then we'd
need all of the above. 

>Today those spinlocks are driver-specific, and urb->context
>(or urb->hcpriv) seems to have been enough of a queue head
>backpointer for most drivers.  
I'd say it's enough for the drivers that submit only 1 or 2 request 
types and one URB per request. Drivers that use bulk queening and/or have
to deal with multiple requests have to implement some infrastructure to
keep track of it's own URBs.

>The notable omission is the lack of a list_head for device drivers to use even
>after an URB has been submitted -- a lifecycle state that
>SKBs don't have.
I guess just adding another 'struct list_head', usable by the drivers, to the 'struct urb' 
is probably enough. However like I mentioned above if we want to provide urb_queue()
and friends we'd need 'struct urb_queue' or something similar.

>>        int               type;
>>        void              *priv;
>>};
>
>
>How would "priv" differ from the current per-request state,
>urb->context (for device driver) or urb->hcpriv (for HCD)?
Oh, it's something that was useful in my driver. We don't have to add another priv.

>I've also been interested in seeing something like skb->cb[].
>HCDs could use that as pre-allocated per-request memory,
>avoiding per-request heap allocations.
Yes yes yes :). And for drivers too. That were my _urb->type and stuff would go.

>>It's now easy to do things like
>>        _urb_queue_tail(&pending_q, _urb);
>>        usb_urb_submit(&_urb->urb);
>>and
>>        while (_urb = _urb_dequeue(&pending_q))
>>                usb_unlink_urb(&_urb->urb);
>
>Those resemble the kinds of primitives I might want to see
>in a Linux 2.7 USB API ... focussed on the endpoint (queue),
>rather than making the core/HCD layers do extra work to
>figure out what queue is involved.  And ideally general
>enough that device drivers would work the same way.
>
>That first pair is pretty much what any HCD does today,
>except that the endpoint's queue is hidden/internal and
>the second step is "feed it to the hardware!".
>
>The second is pretty much what usbcore (now) does when it
>shuts down an endpoint's queue.  But again, that queue
>is hidden/internal.  Caller needs to ensure the hardware is
>not working on the queue during that loop, and there are
>some other synchronization issues too.

Do you think we can add this 
        struct urb {
                ...
                struct list_head drv_list;
                char drv_cb[X];
                char hcd_cb[X]; 
                ...
        };
now though ?

Max