Re: PATCH: (as265) Fix bluetooth driver's wait_for_urb()

Re: PATCH: (as265) Fix bluetooth driver's wait_for_urb()

[Greg KH]

On Wed, Apr 28, 2004 at 01:25:08PM -0400, Alan Stern wrote:
> Greg:
> 
> Your recent change to struct urb broke this function in the bluetooth 
> driver.

I know.  I really hate what the Bluetooth driver does, and it's up to
them to keep up with the changes in urbs due to them statically
including a urb in their structures.  That is what they agreed to when
they did this a while ago.  They are on their own here...

> You know, I think usb_wait_for_urb() would make an excellent 
> addition to usbcore.

No, no one should do that.  Just use the proper usb_alloc_urb() and
usb_free_urb() and you will be fine.

> At some future time we could consider replacing 
> synchronous unlink_urb with asynchronous unlink plus wait_for_urb.

Why?  Does it cause undue hardship in the host controllers to have a
synchronous unlink_urb?

> ===== drivers/bluetooth/hci_usb.c 1.43 vs edited =====
> --- 1.43/drivers/bluetooth/hci_usb.c	Wed Apr 21 01:11:06 2004
> +++ edited/drivers/bluetooth/hci_usb.c	Wed Apr 28 13:20:22 2004
> @@ -342,7 +342,7 @@
>  
>  static inline void hci_usb_wait_for_urb(struct urb *urb)
>  {
> -	while (atomic_read(&urb->count) > 1) {
> +	while (atomic_read(&urb->kref.refcount) > 1) {
>  		current->state = TASK_UNINTERRUPTIBLE;
>  		schedule_timeout((5 * HZ + 999) / 1000);
>  	}

I really just hate that whole function, it's such a hack.

So, Bluetooth developers, have you reconsidered your "need" to put a
static urb in your structure?  Are you convinced yet of the wrongness of
your ways?  Do you want a patch to change your subsystem to follow the
rest of the kernel with regards to USB?

thanks,

greg k-h

[Bluez-devel] Re: PATCH: (as265) Fix bluetooth driver's wait_for_urb()

[Marcel Holtmann]

Hi Greg,

> I know.  I really hate what the Bluetooth driver does, and it's up to
> them to keep up with the changes in urbs due to them statically
> including a urb in their structures.  That is what they agreed to when
> they did this a while ago.  They are on their own here...

I know that Max accepted this. So I also have to accept this, but this
doesn't count for the bfusb driver and inserting this hack prevents it
from oopsing on a UHCI controller, too.

> So, Bluetooth developers, have you reconsidered your "need" to put a
> static urb in your structure?  Are you convinced yet of the wrongness of
> your ways?  Do you want a patch to change your subsystem to follow the
> rest of the kernel with regards to USB?

Right now I don't have enough time to rewrite the hci_usb driver, but it
is on my todo list. Actually the bulk and isoc URB's should only be
started when they are needed. This means when an ACL or SCO link is
really established. However I think we should go back to the discussion
how we can combine an URB with a SKB in a nice way. This would also be
helpful for the bfusb driver.

Regards

Marcel




-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: PATCH: (as265) Fix bluetooth driver's wait_for_urb()

[Greg KH]

On Wed, Apr 28, 2004 at 08:08:53PM +0200, Marcel Holtmann wrote:
> Hi Greg,
> 
> > I know.  I really hate what the Bluetooth driver does, and it's up to
> > them to keep up with the changes in urbs due to them statically
> > including a urb in their structures.  That is what they agreed to when
> > they did this a while ago.  They are on their own here...
> 
> I know that Max accepted this. So I also have to accept this, but this
> doesn't count for the bfusb driver and inserting this hack prevents it
> from oopsing on a UHCI controller, too.

If you unlink the urb synchronously there should be no more problems, as
Alan has fixed this in the UHCI driver, right?

> > So, Bluetooth developers, have you reconsidered your "need" to put a
> > static urb in your structure?  Are you convinced yet of the wrongness of
> > your ways?  Do you want a patch to change your subsystem to follow the
> > rest of the kernel with regards to USB?
> 
> Right now I don't have enough time to rewrite the hci_usb driver, but it
> is on my todo list. Actually the bulk and isoc URB's should only be
> started when they are needed. This means when an ACL or SCO link is
> really established. However I think we should go back to the discussion
> how we can combine an URB with a SKB in a nice way. This would also be
> helpful for the bfusb driver.

What's wrong with just a pointer to the urb in the skb?

Some of the main reason I want to do this (becides all of the reference
counting stuff) is to see if a separate pool of urbs could increase
throughput any.  If we change the usb core to do this, and there is a
speed increase, then the bluetooth drivers would also see this
improvement.  But by trying to manage the memory yourself, you wouldn't
:)

Good enough bribe?

Hey, I'll even write the patch for the hci_usb driver :)

thanks,

greg k-h

[Bluez-devel] Re: PATCH: (as265) Fix bluetooth driver's wait_for_urb()

[Marcel Holtmann]

Hi Greg,

> > I know that Max accepted this. So I also have to accept this, but this
> > doesn't count for the bfusb driver and inserting this hack prevents it
> > from oopsing on a UHCI controller, too.
> 
> If you unlink the urb synchronously there should be no more problems, as
> Alan has fixed this in the UHCI driver, right?

last time I checked Alan's disable_endpoint patch it still crashes both
drivers and as you see the bfusb uses usb_free_urb().

> > Right now I don't have enough time to rewrite the hci_usb driver, but it
> > is on my todo list. Actually the bulk and isoc URB's should only be
> > started when they are needed. This means when an ACL or SCO link is
> > really established. However I think we should go back to the discussion
> > how we can combine an URB with a SKB in a nice way. This would also be
> > helpful for the bfusb driver.
> 
> What's wrong with just a pointer to the urb in the skb?

>>From my point the URB should be central point in an USB driver. The SKB
is only used as data buffer and if we use a SKB in the first place we
don't have to copy the data twice to send it to the HCI layer. I really
like to point to the SKB inside the URB, because otherwise I am still
playing tricks to combine these two data types.

> Some of the main reason I want to do this (becides all of the reference
> counting stuff) is to see if a separate pool of urbs could increase
> throughput any.  If we change the usb core to do this, and there is a
> speed increase, then the bluetooth drivers would also see this
> improvement.  But by trying to manage the memory yourself, you wouldn't
> :)
> 
> Good enough bribe?

I always got your point over a year ago, but the world is not perfect
and the lack of time keeps me from writing a new hci_usb driver from
scratch so it will working nicely with dynamic creation for the URB's
for the Bluetooth data and audio links links.

> Hey, I'll even write the patch for the hci_usb driver :)

I am glad to test every patch you send me, but it may take some days. I
still have another patch from Alan that is in my queue and he expects an
answer.

Regards

Marcel




-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. 
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: PATCH: (as265) Fix bluetooth driver's wait_for_urb()

[Alan Stern]

On Wed, 28 Apr 2004, Greg KH wrote:

> On Wed, Apr 28, 2004 at 01:25:08PM -0400, Alan Stern wrote:
> > Greg:
> > 
> > Your recent change to struct urb broke this function in the bluetooth 
> > driver.
> 
> I know.  I really hate what the Bluetooth driver does, and it's up to
> them to keep up with the changes in urbs due to them statically
> including a urb in their structures.  That is what they agreed to when
> they did this a while ago.  They are on their own here...

I have to agree that life would be easier all 'round if the bluetooth code 
was changed to use dynamically-allocated URBs.

> > You know, I think usb_wait_for_urb() would make an excellent 
> > addition to usbcore.
> 
> No, no one should do that.  Just use the proper usb_alloc_urb() and
> usb_free_urb() and you will be fine.

There's more to it than that.  Yes, if all you're worried about is freeing 
an URB then usb_alloc_urb() and usb_free_urb() will take care of 
everything -- you don't even need to use synchronous unlinks.

However...  Suppose you want to cancel an outstanding URB and then reuse 
that URB for a new request.  Or suppose a driver's disconnect() routine 
has been called and it needs to cancel all outstanding URBs before 
allowing the driver to be unloaded from memory.  In both cases it's 
necessary to wait until _after_ the completion handler has finished -- in 
the first case because you don't want to reuse the URB's data fields while 
the completion handler may still be looking at them, in the second case 
because you don't want the completion handler to try to run when its code 
segment has been unloaded.

The second case is no longer an issue, even for the UHCI driver, because
it now implements the appropriate endpoint_disable method.  This will
prevent the majority of possible problems.

But the first case still applies.  (As does the case of maverick drivers
like Bluetooth that don't use usb_free_urb().)  So there has to be a way
for a driver to wait until an URB's completion handler has finished.  
That's the need addressed by usb_wait_for_urb().

> > At some future time we could consider replacing 
> > synchronous unlink_urb with asynchronous unlink plus wait_for_urb.
> 
> Why?  Does it cause undue hardship in the host controllers to have a
> synchronous unlink_urb?

There's two points of confusion here.  First, the host controller drivers
don't implement synchronous unlinking -- it's all handled higher up in
core/hcd.c.  As far as the HCDs are concerned everything is asynchronous.

Second, synchronous unlink_urb doesn't do what is needed because it 
doesn't guarantee that the completion handler will have finished when it 
returns.  It only guarantees that the completion handler will have 
finished if it returns _with no error_.  A race can cause such an error, 
but the driver will still have to wait until the completion handler has 
finished.  That's why usb_wait_for_urb() is needed.

Alan Stern

Re: [linux-usb-devel] Re: PATCH: (as265) Fix bluetooth driver's wait_for_urb()

[Oliver Neukum]

> However...  Suppose you want to cancel an outstanding URB and then reuse
> that URB for a new request.  Or suppose a driver's disconnect() routine
> has been called and it needs to cancel all outstanding URBs before
> allowing the driver to be unloaded from memory.  In both cases it's
> necessary to wait until _after_ the completion handler has finished -- in
> the first case because you don't want to reuse the URB's data fields while
> the completion handler may still be looking at them, in the second case
> because you don't want the completion handler to try to run when its code
> segment has been unloaded.
>
> The second case is no longer an issue, even for the UHCI driver, because
> it now implements the appropriate endpoint_disable method.  This will
> prevent the majority of possible problems.

Ceterum censeo: Until you've solved all problems, the issue still exists.

> But the first case still applies.  (As does the case of maverick drivers
> like Bluetooth that don't use usb_free_urb().)  So there has to be a way
> for a driver to wait until an URB's completion handler has finished.
> That's the need addressed by usb_wait_for_urb().

Is there anybody using a synchronous unlink who doesn't want that?

> > > At some future time we could consider replacing
> > > synchronous unlink_urb with asynchronous unlink plus wait_for_urb.
> >
> > Why?  Does it cause undue hardship in the host controllers to have a
> > synchronous unlink_urb?
>
> There's two points of confusion here.  First, the host controller drivers
> don't implement synchronous unlinking -- it's all handled higher up in
> core/hcd.c.  As far as the HCDs are concerned everything is asynchronous.

As it should be.

> Second, synchronous unlink_urb doesn't do what is needed because it
> doesn't guarantee that the completion handler will have finished when it
> returns.  It only guarantees that the completion handler will have
> finished if it returns _with no error_.  A race can cause such an error,
> but the driver will still have to wait until the completion handler has
> finished.  That's why usb_wait_for_urb() is needed.

But is very poorly implemented.

	Regards
		Oliver