Messages in this thread | | | Date | Tue, 29 Apr 2003 13:29:41 -0700 | From | Max Krasnyansky <> | Subject | Re: [Bluetooth] HCI USB driver update. Support for SCO over HCI USB. |
| |
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
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |