Re: [RFC] URBs and buffer management

On Mon, Jun 04, 2007 at 10:38:44AM +0200, Marcel Holtmann wrote:
> Hi,
> 
> after the discussion with Oliver at the LinuxTag last week, I started to
> re-write the hci_usb driver to remove this ugly cruft around the URB
> management in the driver. The basic driver (without ISOC support) is
> working perfectly fine and thanks to the reference count of the URB and
> the new USB anchor extension it is a really small and simple driver now.
> 
> However the buffer management for URBs that get re-submitted all the
> time is really ugly. It can be done inside the driver, but I think the
> USB core should provide some helpers here.
> 
> The attached patch is an attempt to integrate the buffer into the URB
> and let the URB take care of freeing it when it is no longer needed.
> This might not be optimal for all drivers, but it helps to reduce a lot
> of code in many drivers. And of course the old method of allocating or
> providing an external buffer is still available.
> 
> The main use cases are interrupt and bulk in endpoints where fragmented
> packets are used and the driver has to reassemble them. In this cases
> one or multiple URBs with and attached buffer can be used. When shutting
> down these URBs via an USB anchor, it will take care of freeing the
> buffers and the driver doesn't have to worry about.
> 
> The patch misses setup routines for interrupt and ISOC URBs, but they
> are straight forward. Please let me know what you think.
> 
> Regards
> 
> Marcel
> 

> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 94ea972..e683726 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -11,6 +11,11 @@
>  static void urb_destroy(struct kref *kref)
>  {
>  	struct urb *urb = to_urb(kref);
> +
> +	if (urb->transfer_flags & URB_FREE_BUFFER)
> +		usb_buffer_free(urb->dev, urb->transfer_buffer_length,
> +				urb->transfer_buffer, urb->transfer_dma);
> +
>  	kfree(urb);
>  }
>  
> @@ -478,6 +483,36 @@ void usb_kill_urb(struct urb *urb)

I like this portion, if we make it kfree() instead of usb_buffer_free.

>  	spin_unlock_irq(&urb->lock);
>  }
>  
> +/**
> + * usb_setup_bulk_urb - macro to help initialize a bulk urb

<snip>

It seems this function is not needed as per the discussion in this
thread.

Care to resend this with just the above functionality?  It will let me
clean up a number of different drivers.

thanks,

greg k-h

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel