HCI USB driver update. Support for SCO over HCI USB.

ChangeSet 1.971.22.2, 2003/03/24 13:03:28-08:00, [email protected]

[Bluetooth] HCI USB driver update. Support for SCO over HCI USB. URB and buffer managment rewrite.

(quoted diff) +6 -0

+struct _urb *_urb_dequeue(struct _urb_queue *q)
+{
+	struct _urb *_urb = NULL;
+        unsigned long flags;
+        spin_lock_irqsave(&q->lock, flags);
+	{

Quoted 4 lines · Linux Kernel Mailing List

ChangeSet 1.971.22.2, 2003/03/24 13:03:28-08:00, [email protected]

[Bluetooth] HCI USB driver update. Support for SCO over HCI USB. URB and buffer managment rewrite.

Max, you need to be very careful with this:

Review

Quoted 11 lines

(quoted diff) +11 -0

+struct _urb *_urb_alloc(int isoc, int gfp)
+{
+     struct _urb *_urb = kmalloc(sizeof(struct _urb) +
+                             sizeof(struct usb_iso_packet_descriptor) * isoc, gfp);
+     if (_urb) {
+             memset(_urb, 0, sizeof(*_urb));
+             _urb->urb.count = (atomic_t)ATOMIC_INIT(1);
+             spin_lock_init(&_urb->urb.lock);
+     }
+     return _urb;
+}

You aren't calling usb_alloc_urb() and:

Quoted 13 lines

(quoted diff) +13 -0

+struct _urb *_urb_dequeue(struct _urb_queue *q)
+{
+     struct _urb *_urb = NULL;
+        unsigned long flags;
+        spin_lock_irqsave(&q->lock, flags);
+     {
+             struct list_head *head = &q->head;
+             struct list_head *next = head->next;
+             if (next != head) {
+                     _urb = list_entry(next, struct _urb, list);
+                     list_del(next); _urb->queue = NULL;
+             }
+     }

Review

Quoted 3 lines

(quoted diff) +3 -0

+     spin_unlock_irqrestore(&q->lock, flags);
+     return _urb;
+}

You aren't calling usb_free_urb() as you are embedding a struct urb within your struct _urb structure. Any reason you can't use a struct urb * instead and call the usb core's functions to create and return a urb ?

Otherwise any changes to the internal urb structures, and the usb_alloc_urb() and usb_free_urb() functions will have to be mirrored here in your functions, and I know I will forget to do that :)

Other than that, it's nice to see Bluetooth SCO support for Linux, very nice job.

Quoted 4 lines

You aren't calling usb_free_urb() as you are embedding a struct urb within your struct _urb structure. Any reason you can't use a struct urb * instead and call the usb core's functions to create and return a urb ?

I didn't want to do two allocations (one for struct _urb and one for struct urb).

Quoted 3 lines

Otherwise any changes to the internal urb structures, and the usb_alloc_urb() and usb_free_urb() functions will have to be mirrored here in your functions, and I know I will forget to do that :)

How about

I was actually going to ask you guys if you'd be interested in generalizing this _urb_queue() stuff that I have for other drivers. Current URB api does not provide any interface for queueing/linking/etc of URBs in the _driver_ itself. Things like next, prev, etc are used in the HCD. So if driver submits bunch of different URBs (and potentially multiple URBs of the same type like hci_usb does) it has to implement its own lists, arrays and stuff. I used to use SKBs for URB queues but struct sk_buff is to big for that simple task.

drivers/usb/core/urb.c line 37

Quoted 11 lines

drivers/usb/core/urb.c +10 -0

+int usb_init_urb(struct urb *urb)
+{
+	if (!urb)
+		return -EINVAL;
+	memset(urb, 0, sizeof(*urb));
+	urb->count = (atomic_t)ATOMIC_INIT(1);
+	spin_lock_init(&urb->lock);
+
+	return 0;
+}

Greg, please don't do it this way. Somebody will try to free this urb. If the urb is part of a structure this must not lead to a kfree. Please init it to some insanely high dummy value in this case.

Quoted 7 lines

Quoted 7 lines

drivers/usb/core/urb.c +7 -0

+int usb_init_urb(struct urb *urb)
+{
+	if (!urb)
+		return -EINVAL;
+	memset(urb, 0, sizeof(*urb));
+	urb->count = (atomic_t)ATOMIC_INIT(1);
+	spin_lock_init(&urb->lock);

Quoted 8 lines

drivers/usb/core/urb.c line 37

Quoted 3 lines

drivers/usb/core/urb.c +3 -0

+
+	return 0;
+}

Greg, please don't do it this way. Somebody will try to free this urb. If the urb is part of a structure this must not lead to a kfree. Please init it to some insanely high dummy value in this case.

We can't init it to a high value, if we want to use it ourself in usb_alloc_urb().

And yes, I agree this is a very dangerous function to use on your own, I thought I conveyed that in the documentation for the function.

But if we don't have such a function, then people like Max will just roll their own, like he just did :)

Might as well make it easy for him to shoot himself in the foot if he really wants to...

Quoted 16 lines · Oliver Neukum

drivers/usb/core/urb.c line 37

Quoted 11 lines

drivers/usb/core/urb.c +10 -0

+int usb_init_urb(struct urb *urb)
+{
+   if (!urb)
+           return -EINVAL;
+   memset(urb, 0, sizeof(*urb));
+   urb->count = (atomic_t)ATOMIC_INIT(1);
+   spin_lock_init(&urb->lock);
+
+   return 0;
+}

Greg, please don't do it this way. Somebody will try to free this urb. If the urb is part of a structure this must not lead to a kfree. Please init it to some insanely high dummy value in this case.

We can't init it to a high value, if we want to use it ourself in usb_alloc_urb().

And yes, I agree this is a very dangerous function to use on your own, I thought I conveyed that in the documentation for the function.

But if we don't have such a function, then people like Max will just roll their own, like he just did :)

Might as well make it easy for him to shoot himself in the foot if he really wants to...

Quoted 16 lines · Greg KH

Quoted 7 lines · Oliver Neukum

Quoted 7 lines

drivers/usb/core/urb.c +7 -0

+int usb_init_urb(struct urb *urb)
+{
+   if (!urb)
+           return -EINVAL;
+   memset(urb, 0, sizeof(*urb));
+   urb->count = (atomic_t)ATOMIC_INIT(1);
+   spin_lock_init(&urb->lock);

Quoted 8 lines

drivers/usb/core/urb.c line 37

Quoted 3 lines

drivers/usb/core/urb.c +3 -0

+
+   return 0;
+}

Greg, please don't do it this way. Somebody will try to free this urb. If the urb is part of a structure this must not lead to a kfree. Please init it to some insanely high dummy value in this case.

Uh, I didn't think about that one. This stuff was first implemented for 2.4 which didn't have refcount in urb and then forward ported to 2.5.

Quoted 6 lines · Max Krasnyansky

Quoted 4 lines

You aren't calling usb_free_urb() as you are embedding a struct urb within your struct _urb structure. Any reason you can't use a struct urb * instead and call the usb core's functions to create and return a urb ?

I didn't want to do two allocations (one for struct _urb and one for struct urb).

I agree, and understand what you are doing, it makes sense for your case. Just wanted to make sure you were aware of the future danger :)

Quoted 4 lines

Quoted 3 lines

Otherwise any changes to the internal urb structures, and the usb_alloc_urb() and usb_free_urb() functions will have to be mirrored here in your functions, and I know I will forget to do that :)

How about

<snip>

Ok, that works for me. Does the patch below work out for you?

Quoted 9 lines

I was actually going to ask you guys if you'd be interested in generalizing this _urb_queue() stuff that I have for other drivers. Current URB api does not provide any interface for queueing/linking/etc of URBs in the _driver_ itself. Things like next, prev, etc are used in the HCD. So if driver submits bunch of different URBs (and potentially multiple URBs of the same type like hci_usb does) it has to implement its own lists, arrays and stuff. I used to use SKBs for URB queues but struct sk_buff is to big for that simple task.

Yes, generalizing that stuff would be nice to have. Lots of drivers have to manage their urbs on their own today, and making that easier to do would be a good thing.

Quoted 1 line

Ok, that works for me. Does the patch below work out for you?

Yep. I'd make it return void though.

I was actually going to ask you guys if you'd be interested in generalizing this _urb_queue() stuff that I have for other drivers. Current URB api does not provide any interface for queueing/linking/etc of URBs in the _driver_ itself.

Quoted 5 lines

I was actually going to ask you guys if you'd be interested in generalizing this _urb_queue() stuff that I have for other drivers. Current URB api does not provide any interface for queueing/linking/etc of URBs in the _driver_ itself.

I only saw fragments of the original patch -- could you be just a bit more specific?

Quoted 4 lines

I was actually going to ask you guys if you'd be interested in generalizing this _urb_queue() stuff that I have for other drivers. Current URB api does not provide any interface for queueing/linking/etc of URBs in the _driver_ itself.

I only saw fragments of the original patch -- could you be just a bit more specific?

If you're suggesting adding some "struct list_head" into "struct urb" for exclusive use of the interface's driver (instead of urb_list, which is for usbcore/hcd) ... I'd agree that'd be a good thing.

In fact I recently got around to adding that to the "gadget side" analogue of an URB. For much the same kind of reasons as you mentioned.

Basically I'd like to have same kind of API that we have for SKB without an overhead of SKBs.

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;
        struct urb        urb;

        int               type;
        void              *priv;
};

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);

etc

Max

Quoted 2 lines · Max Krasnyansky

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?

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

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...

Quoted 8 lines

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"?

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.

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.

Quoted 3 lines

       int               type;
       void              *priv;
};

How would "priv" differ from the current per-request state, urb->context (for device driver) or urb->hcpriv (for HCD)?

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.

Quoted 6 lines

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]; 
                ...
        };

I'd certainly like the list_head. Patch attached, in case Greg agrees enough.

Quoted 3 lines · David Brownell

I'd certainly like the list_head. Patch attached, in case Greg agrees enough.

I agree, but will only take the patch if a driver is modified to actually use this. I'll take both patches at once :)

Hi Max,

Quoted 8 lines

Do you think we can add this

        struct urb {
                ...
                struct list_head drv_list;
                char drv_cb[X];
                char hcd_cb[X]; 
                ...
        };

Only with kerneldoc ... :)

I'd certainly like the list_head. Patch attached, in case Greg agrees enough. On x86, this makes sizeof(struct urb) == 120, so it's using space that was previously wasted.

As for the skb->cb analogue(s), details need working.

- What should "X" be? skb->cb is 48 bytes.

- Should the cb[] arrays be "long"? They tend to be used for pointers...

- The HCDs want different amounts of per-urb data. Sizes on x86:
* UHCI wants a lot -- 60 bytes!
* OHCI typically uses 16 bytes, but more for multi-TD urbs (control 24 bytes, ISO often 36).
* EHCI doesn't allocate such extra data.

- The HCDs would need conversion to use hcd_cb[]. I once had a patch that did that, but it's not current. It made urb->cb replace urb->hcpriv.

I suppose X=60 for hcd_cb[] will be enough, at least on 32 bit CPUs. But you start to see why in the new "USB Gadget" API, the analogue of "urb" gets allocated by the USB (device) controller rather than by generic code: so that in typical cases, no additional per-request allocations are needed.

Ok. Sounds like it should be
uint32_t hcd_cb[16]; // 64 bytes for internal use by HCD uint32_t drv_cb[2]; // 8 bytes for internal use by USB driver

Quoted 3 lines · Max Krasnyansky

Ok. Sounds like it should be
uint32_t hcd_cb[16]; // 64 bytes for internal use by HCD uint32_t drv_cb[2]; // 8 bytes for internal use by USB driver

s/uint32_t/u32/ please.

And if this is going to be used for pointers, why not just say they are pointers? Otherwise people are going to have to be careful with 32 vs. 64 bit kernels to not overrun their space.

struct sk_buff uses a char, any reason not to use that here too? Has being a char made things more difficult for that structure over time?

Quoted 1 line

s/uint32_t/u32/ please.

"u32" is prettier, but is there actually a policy against using the more standard type names? (POSIX, someone had said.)

Quoted 7 lines · Greg KH

Quoted 3 lines · Max Krasnyansky

Ok. Sounds like it should be
uint32_t hcd_cb[16]; // 64 bytes for internal use by HCD uint32_t drv_cb[2]; // 8 bytes for internal use by USB driver

Quoted 1 line

s/uint32_t/u32/ please.

"u32" is prettier, but is there actually a policy against using the more standard type names? (POSIX, someone had said.)

Quoted 2 lines

"u32" is prettier, but is there actually a policy against using the more standard type names? (POSIX, someone had said.)

Yes there is. Linus has stated this a few times on lkml in the past. I have an old linux journal article that talks about this that I need to turn into docbook and add to the kernel tree to set it in stone.

Quoted 2 lines · David Brownell

Quoted 2 lines

"u32" is prettier, but is there actually a policy against using the more standard type names? (POSIX, someone had said.)

Quoted 3 lines · Greg KH

Yes there is. Linus has stated this a few times on lkml in the past. I have an old linux journal article that talks about this that I need to turn into docbook and add to the kernel tree to set it in stone.

If someone could clarify the motive I'd be much obliged.

Quoted 1 line

If someone could clarify the motive I'd be much obliged.

Read Linus's comments in this thread for more insight:

	http://marc.theaimsgroup.com/?t=102806382900001

Quoted 7 lines · Greg KH

Quoted 3 lines · Max Krasnyansky

Ok. Sounds like it should be
uint32_t hcd_cb[16]; // 64 bytes for internal use by HCD uint32_t drv_cb[2]; // 8 bytes for internal use by USB driver

s/uint32_t/u32/ please.

"u32" is prettier, but is there actually a policy against using the more standard type names? (POSIX, someone had said.)

Quoted 5 lines

And if this is going to be used for pointers, why not just say they are pointers? Otherwise people are going to have to be careful with 32 vs. 64 bit kernels to not overrun their space.
struct sk_buff uses a char, any reason not to use that here too? Has being a char made things more difficult for that structure over time?

No, it's just that in some similar cases having the value be "long" (not "32 bit unsigned") has been simpler. I'm not religious.

So, I guess in general you're ok with adding ->drv_cb and ->hcd_cb to 'struct urb' ?

Quoted 2 lines · Max Krasnyansky

So, I guess in general you're ok with adding ->drv_cb and ->hcd_cb to 'struct urb' ?

I am, as long as someone uses it :)

Quoted 2 lines · Max Krasnyansky

So, I guess in general you're ok with adding ->drv_cb and ->hcd_cb to 'struct urb' ?

I am, as long as someone uses it :)