Re: [Bluetooth] HCI USB driver update. Support for SCO over HCI USB.

Re: [Bluetooth] HCI USB driver update. Support for SCO over HCI USB.

[Greg KH]

On Mon, Mar 24, 2003 at 09:03:28PM +0000, Linux Kernel Mailing List wrote:
> ChangeSet 1.971.22.2, 2003/03/24 13:03:28-08:00, maxk@qualcomm.com
> 
> 	[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:

> -static void hci_usb_interrupt(struct urb *urb, struct pt_regs *regs);
> +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:

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

thaks,

greg k-h

Re: [Bluetooth] HCI USB driver update. Support for SCO over HCI USB.

[Max Krasnyansky]

At 09:15 PM 4/28/2003, Greg KH wrote:
>On Mon, Mar 24, 2003 at 09:03:28PM +0000, Linux Kernel Mailing List wrote:
>> ChangeSet 1.971.22.2, 2003/03/24 13:03:28-08:00, maxk@qualcomm.com
>> 
>>       [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:
I know ;-)

>> +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:
>
>> +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;
>> +             }
>> +     }
>> +     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 ?
I didn't want to do two allocations (one for struct _urb and one for struct 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 :)
How about 

static inline urb_init(urb)
{
        urb->xx = YY;
        ...
}

urb_alloc() 
{
        urb = kmalloc()
        urb_init(urb);
}

?

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

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.

Max

Re: [Bluetooth] HCI USB driver update. Support for SCO over HCI USB.

[Greg KH]

On Tue, Apr 29, 2003 at 01:29:41PM -0700, Max Krasnyansky wrote:
> >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 :)

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

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

thanks,

greg k-h


# usb: create usb_init_urb() for those people who like to live dangerously (like the bluetooth stack.)

diff -Nru a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
--- a/drivers/usb/core/urb.c	Tue Apr 29 14:07:13 2003
+++ b/drivers/usb/core/urb.c	Tue Apr 29 14:07:13 2003
@@ -14,6 +14,29 @@
 #include "hcd.h"
 
 /**
+ * usb_init_urb - initializes a urb so that it can be used by a USB driver
+ * @urb: pointer to the urb to initialize
+ *
+ * Initializes a urb so that the USB subsystem can use it properly.
+ *
+ * If a urb is created with a call to usb_alloc_urb() it is not
+ * necessary to call this function.  Only use this if you allocate the
+ * space for a struct urb on your own.  If you call this function, be
+ * careful when freeing the memory for your urb that it is no longer in
+ * use by the USB core.
+ */
+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;
+}
+
+/**
  * usb_alloc_urb - creates a new urb for a USB driver to use
  * @iso_packets: number of iso packets for this urb
  * @mem_flags: the type of memory to allocate, see kmalloc() for a list of
@@ -38,13 +61,14 @@
 		mem_flags);
 	if (!urb) {
 		err("alloc_urb: kmalloc failed");
-		return NULL;
+		goto exit;
+	}
+	if (usb_init_urb(urb)) {
+		kfree(urb);
+		urb = NULL;
 	}
 
-	memset(urb, 0, sizeof(*urb));
-	urb->count = (atomic_t)ATOMIC_INIT(1);
-	spin_lock_init(&urb->lock);
-
+exit:
 	return urb;
 }
 
@@ -387,7 +411,7 @@
 		return -ENODEV;
 }
 
-// asynchronous request completion model
+EXPORT_SYMBOL(usb_init_urb);
 EXPORT_SYMBOL(usb_alloc_urb);
 EXPORT_SYMBOL(usb_free_urb);
 EXPORT_SYMBOL(usb_get_urb);
diff -Nru a/include/linux/usb.h b/include/linux/usb.h
--- a/include/linux/usb.h	Tue Apr 29 14:07:13 2003
+++ b/include/linux/usb.h	Tue Apr 29 14:07:13 2003
@@ -766,6 +766,7 @@
 }
 
 extern struct urb *usb_alloc_urb(int iso_packets, int mem_flags);
+extern int usb_init_urb(struct urb *urb);
 extern void usb_free_urb(struct urb *urb);
 #define usb_put_urb usb_free_urb
 extern struct urb *usb_get_urb(struct urb *urb);

Re: [Bluetooth] HCI USB driver update. Support for SCO over HCI USB.

[Oliver Neukum]

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

	Regards
		Oliver

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

[Greg KH]

On Tue, Apr 29, 2003 at 02:39:39PM -0700, David Brownell wrote:
> 
> > 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?

The whole patch can be seen at:
	http://marc.theaimsgroup.com/?l=bk-commits-head&m=105158631404987


thanks,

greg k-h

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

[David Brownell]

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

- Dave

Re: [Bluetooth] HCI USB driver update. Support for SCO over HCI USB.

[Greg KH]

On Tue, Apr 29, 2003 at 11:34:19PM +0200, Oliver Neukum wrote:
> 
> > +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...

thanks,

greg k-h

Re: [Bluetooth] HCI USB driver update. Support for SCO over HCI USB.

[Max Krasnyansky]

At 02:15 PM 4/29/2003, Greg KH wrote:
>On Tue, Apr 29, 2003 at 01:29:41PM -0700, Max Krasnyansky wrote:
>> >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 :)
:)

>> >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?
Yep. I'd make it return void though.

I'll fix hci_usb to use usb_init_urb() when your patch get's in.

>> 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.
Good. I'll reply do Dave's email were he asked for more details.

Thanks. 
Max

Re: [Bluetooth] HCI USB driver update. Support for SCO over HCI USB.

[Greg KH]

On Tue, Apr 29, 2003 at 02:55:17PM -0700, Max Krasnyansky wrote:
> >Ok, that works for me.  Does the patch below work out for you?
> Yep. I'd make it return void though.

Good point, I'll change that and send it off.

thanks,

greg k-h

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

[Max Krasnyansky]

At 02:39 PM 4/29/2003, David Brownell wrote:

>> 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;
        int               type;
        void              *priv;
        struct urb        urb;
};

struct _urb *_urb_alloc(int isoc, int gfp);
static inline void _urb_free(struct _urb *_urb);
static inline void _urb_queue_init(struct _urb_queue *q);
static inline void _urb_queue_head(struct _urb_queue *q, struct _urb *_urb);
static inline void _urb_queue_tail(struct _urb_queue *q, struct _urb *_urb);
static inline void _urb_unlink(struct _urb *_urb);
struct _urb *_urb_dequeue(struct _urb_queue *q);

_urb is an ugly name I know but I couldn't come up with the better one :)

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

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

[Oliver Neukum]

Am Dienstag, 29. April 2003 23:40 schrieb Greg KH:
> On Tue, Apr 29, 2003 at 11:34:19PM +0200, Oliver Neukum wrote:
> > > +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().

So don't or make that value a parameter. 

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

It's not any more dangerous than what worked quite well for 2.4.

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

Sure, why not.

	Regards
		Oliver

Re: [Bluetooth] HCI USB driver update. Support for SCO over HCI USB.

[Max Krasnyansky]

At 02:40 PM 4/29/2003, Greg KH wrote:
>On Tue, Apr 29, 2003 at 11:34:19PM +0200, Oliver Neukum wrote:
>> 
>> > +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.
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.

>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...
Thanks. I'll wear bullet proof shoes, just in case :-).

Max

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

[David Brownell]

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


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

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.


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


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

- Dave



> etc
> 
> Max
> 
> 

Re: [Bluetooth] HCI USB driver update. Support for SCO over HCI USB.

[Oliver Neukum]

Am Mittwoch, 30. April 2003 02:44 schrieb Max Krasnyansky:
> At 02:40 PM 4/29/2003, Greg KH wrote:
> >On Tue, Apr 29, 2003 at 11:34:19PM +0200, Oliver Neukum wrote:
> >> > +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.
>
> 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.

It should work. However if you have a refcount bug, the failure case will
be spectacular.

	Regards
		Oliver

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

[Max Krasnyansky]

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

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

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 1352 bytes --]

Hi Max,

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

- Dave





[-- Attachment #2: urb.patch --]
[-- Type: text/plain, Size: 1079 bytes --]

--- 1.139/include/linux/usb.h	Wed May  7 00:02:43 2003
+++ edited/include/linux/usb.h	Thu May  8 20:06:31 2003
@@ -511,6 +511,8 @@
 /**
  * struct urb - USB Request Block
  * @urb_list: For use by current owner of the URB.
+ * @dev_list: For use by device driver, even while the URB is owned by
+ * 	usbcore and the HCD.  Th device driver must initialize this list.
  * @pipe: Holds endpoint number, direction, type, and more.
  *	Create these values with the eight macros available;
  *	usb_{snd,rcv}TYPEpipe(dev,endpoint), where the type is "ctrl"
@@ -669,7 +671,8 @@
 	spinlock_t lock;		/* lock for the URB */
 	atomic_t count;			/* reference count of the URB */
 	void *hcpriv;			/* private data for host controller */
-	struct list_head urb_list;	/* list pointer to all active urbs */
+	struct list_head urb_list;	/* private data for usbcore */
+	struct list_head dev_list;	/* private data for device driver */
 	struct usb_device *dev; 	/* (in) pointer to associated device */
 	unsigned int pipe;		/* (in) pipe information */
 	int status;			/* (return) non-ISO status */

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

[Greg KH]

On Fri, May 09, 2003 at 12:06:27PM -0700, David Brownell wrote:
> 
> 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 :)

thanks,

greg k-h

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

[Max Krasnyansky]

On Fri, 2003-05-09 at 12:06, David Brownell wrote:
> Hi Max,
> 
> > 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.
I'd say 16 will be enough for drivers. 

>   - Should the cb[] arrays be "long"?  They tend to
>     be used for pointers...
Probably doesn't matter as long as ->cb is aligned.

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

?

Max

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

[Max Krasnyansky]

On Fri, 2003-05-09 at 12:29, Greg KH wrote:
> On Fri, May 09, 2003 at 12:06:27PM -0700, David Brownell wrote:
> > 
> > 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 :)

Ok. Sounds good to me. 
But I also need ->drv_cb[].   

Max

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

[Greg KH]

On Fri, May 09, 2003 at 03:35:36PM -0700, Max Krasnyansky wrote:
> 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?

thanks,

greg k-h

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

[David Brownell]

Greg KH wrote:
> On Fri, May 09, 2003 at 03:35:36PM -0700, Max Krasnyansky wrote:
> 
>>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.)


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

- Dave

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

[Brad Hards]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Sat, 10 May 2003 10:48 am, David Brownell wrote:
> > 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.)
Not Invented Here?

Brad
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.7 (GNU/Linux)

iD8DBQE+vIjiW6pHgIdAuOMRAm6+AKCg3P6rUZOvUEcv4emlpwXiGwAA8ACfUlLG
MuHfr7Pmc8yk017a1aWGLYw=
=8zSh
-----END PGP SIGNATURE-----

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

[Greg KH]

On Fri, May 09, 2003 at 05:48:16PM -0700, David Brownell wrote:
> Greg KH wrote:
> >On Fri, May 09, 2003 at 03:35:36PM -0700, Max Krasnyansky wrote:
> >
> >>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.)

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.

thanks,

greg k-h

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

[William Lee Irwin III]

On Fri, May 09, 2003 at 05:48:16PM -0700, David Brownell wrote:
>> "u32" is prettier, but is there actually a policy against using
>> the more standard type names?  (POSIX, someone had said.)

On Fri, May 09, 2003 at 10:40:15PM -0700, Greg KH wrote:
> 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.

Thanks.


-- wli

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

[Greg KH]

On Fri, May 09, 2003 at 10:55:16PM -0700, William Lee Irwin III wrote:
> On Fri, May 09, 2003 at 05:48:16PM -0700, David Brownell wrote:
> >> "u32" is prettier, but is there actually a policy against using
> >> the more standard type names?  (POSIX, someone had said.)
> 
> On Fri, May 09, 2003 at 10:40:15PM -0700, Greg KH wrote:
> > 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.

Read Linus's comments in this thread for more insight:
	http://marc.theaimsgroup.com/?t=102806382900001

Hope this helps,

greg k-h

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

[William Lee Irwin III]

On Fri, May 09, 2003 at 10:55:16PM -0700, William Lee Irwin III wrote:
>> If someone could clarify the motive I'd be much obliged.

On Fri, May 09, 2003 at 11:11:14PM -0700, Greg KH wrote:
> Read Linus's comments in this thread for more insight:
> 	http://marc.theaimsgroup.com/?t=102806382900001

That looks relatively arbitrary but I honestly can't be arsed to have
an opinion, only to be informed.

Thanks.


-- wli

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

[Max Krasnyansky]

At 05:48 PM 5/9/2003, David Brownell wrote:
>Greg KH wrote:
>>On Fri, May 09, 2003 at 03:35:36PM -0700, Max Krasnyansky wrote:
>>
>>>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.)
>
>
>>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.
I don't care either. 'char' is ok for me.

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

Max

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

[Greg KH]

On Mon, May 12, 2003 at 10:53:48AM -0700, Max Krasnyansky wrote:
> 
> 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 :)

thanks,

greg k-h

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

[Max Krasnyansky]

At 11:01 AM 5/12/2003, Greg KH wrote:
>On Mon, May 12, 2003 at 10:53:48AM -0700, Max Krasnyansky wrote:
>> 
>> 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 :)
Ok then. 
I'll go ahead and change my driver to use ->drv_list and ->drv_cb and send the patch to you.

Thanks
Max

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

[David Brownell]

Greg KH wrote:
> On Mon, May 12, 2003 at 10:53:48AM -0700, Max Krasnyansky wrote:
> 
>>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 :)

I'm unlikely to dig up that patch making ohci-hcd use a hcd_cb,
which would mean that the submit path (a) gets rid of one fault
mode, (b) slightly speeds up the submit path, and (c) all but
stops using the heap.  Even though they'd be fine updates ...
I'd be glad to see someone else contribute such cleanups though!

The uhci-hcd version would be trickier, and on 64bit CPUs it
would not fit in the 64 bytes Max suggested -- without some
work to shrink that driver.

- Dave