[patch 2/3] hci_usb: remove code obfuscation

[patch 2/3] hci_usb: remove code obfuscation

[akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b]

From: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>

_urb_free is an alias for kfree... making code longer & harder to
read. Remove it.

Signed-off-by: Pavel Machek <pavel-AlSwsSmVLrQ@public.gmane.org>
Cc: Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>
Cc: Dave Young <hidave.darkstar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
---

 drivers/bluetooth/hci_usb.c |   10 +++++-----
 drivers/bluetooth/hci_usb.h |    5 -----
 2 files changed, 5 insertions(+), 10 deletions(-)

diff -puN drivers/bluetooth/hci_usb.c~hci_usb-remove-code-obfuscation drivers/bluetooth/hci_usb.c
--- a/drivers/bluetooth/hci_usb.c~hci_usb-remove-code-obfuscation
+++ a/drivers/bluetooth/hci_usb.c
@@ -265,7 +265,7 @@ static int hci_usb_intr_rx_submit(struct
 		BT_ERR("%s intr rx submit failed urb %p err %d",
 				husb->hdev->name, urb, err);
 		_urb_unlink(_urb);
-		_urb_free(_urb);
+		kfree(_urb);
 		kfree(buf);
 	}
 	return err;
@@ -302,7 +302,7 @@ static int hci_usb_bulk_rx_submit(struct
 		BT_ERR("%s bulk rx submit failed urb %p err %d",
 				husb->hdev->name, urb, err);
 		_urb_unlink(_urb);
-		_urb_free(_urb);
+		kfree(_urb);
 		kfree(buf);
 	}
 	return err;
@@ -353,7 +353,7 @@ static int hci_usb_isoc_rx_submit(struct
 		BT_ERR("%s isoc rx submit failed urb %p err %d",
 				husb->hdev->name, urb, err);
 		_urb_unlink(_urb);
-		_urb_free(_urb);
+		kfree(_urb);
 		kfree(buf);
 	}
 	return err;
@@ -431,7 +431,7 @@ static void hci_usb_unlink_urbs(struct h
 					husb->hdev->name, _urb, _urb->type, urb);
 			kfree(urb->setup_packet);
 			kfree(urb->transfer_buffer);
-			_urb_free(_urb);
+			kfree(_urb);
 		}
 	}
 }
@@ -490,7 +490,7 @@ static inline int hci_usb_send_ctrl(stru
 
 		dr = kmalloc(sizeof(*dr), GFP_ATOMIC);
 		if (!dr) {
-			_urb_free(_urb);
+			kfree(_urb);
 			return -ENOMEM;
 		}
 	} else
diff -puN drivers/bluetooth/hci_usb.h~hci_usb-remove-code-obfuscation drivers/bluetooth/hci_usb.h
--- a/drivers/bluetooth/hci_usb.h~hci_usb-remove-code-obfuscation
+++ a/drivers/bluetooth/hci_usb.h
@@ -60,11 +60,6 @@ struct _urb {
 	struct urb        urb;
 };
 
-static inline void _urb_free(struct _urb *_urb)
-{
-	kfree(_urb);
-}
-
 static inline void _urb_queue_init(struct _urb_queue *q)
 {
 	INIT_LIST_HEAD(&q->head);
_

Re: [patch 2/3] hci_usb: remove code obfuscation

[Marcel Holtmann]

Hi Andrew,

> From: Pavel Machek <pavel@ucw.cz>
>
> _urb_free is an alias for kfree... making code longer & harder to
> read. Remove it.

our own URB handling here is broken anyway. It was a bad idea when we  
did it, but at that time the USB susbsystem was not in that good shape  
that it is today. So I would say leave this code as it is and  
concentrate on the new btusb driver, but if it helps anybody I am  
happy to ACK this one.

Regards

Marcel

Re: [patch 2/3] hci_usb: remove code obfuscation

[Pavel Machek]

On Fri 2008-04-18 23:07:10, Marcel Holtmann wrote:
> Hi Andrew,
>
>> From: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
>>
>> _urb_free is an alias for kfree... making code longer & harder to
>> read. Remove it.
>
> our own URB handling here is broken anyway. It was a bad idea when we did 
> it, but at that time the USB susbsystem was not in that good shape that it 
> is today. So I would say leave this code as it is and concentrate on the 
> new btusb driver, but if it helps anybody I am happy to ACK this one.

It currently corrupts memory (use after free) during suspend, and I'm
trying to get that fixed. It is 90% reproducible.

And yes, fixing clean code is easier.

It could be easily fixed by just not freeing the urbs during
disconnect (introducing very very slow memory leak). Would that be
acceptable?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 2/3] hci_usb: remove code obfuscation

[Marcel Holtmann]

Hi Pavel,

>>> _urb_free is an alias for kfree... making code longer & harder to
>>> read. Remove it.
>>
>> our own URB handling here is broken anyway. It was a bad idea when  
>> we did
>> it, but at that time the USB susbsystem was not in that good shape  
>> that it
>> is today. So I would say leave this code as it is and concentrate  
>> on the
>> new btusb driver, but if it helps anybody I am happy to ACK this one.
>
> It currently corrupts memory (use after free) during suspend, and I'm
> trying to get that fixed. It is 90% reproducible.

that is good, because I can't reproduce it with any of my system.  
Don't ask me why. I really simply works. So if you figure it out, it  
would be great.

> It could be easily fixed by just not freeing the urbs during
> disconnect (introducing very very slow memory leak). Would that be
> acceptable?

I don't think so.

Have you ever tried btusb driver? That code is a lot cleaner and it  
uses USB anchors for the queued URBs (no home grown broken URB queues).

Regards

Marcel

Re: [patch 2/3] hci_usb: remove code obfuscation

[Pavel Machek]

Hi!

>>>> _urb_free is an alias for kfree... making code longer & harder to
>>>> read. Remove it.
>>>
>>> our own URB handling here is broken anyway. It was a bad idea when we did
>>> it, but at that time the USB susbsystem was not in that good shape that 
>>> it
>>> is today. So I would say leave this code as it is and concentrate on the
>>> new btusb driver, but if it helps anybody I am happy to ACK this one.
>>
>> It currently corrupts memory (use after free) during suspend, and I'm
>> trying to get that fixed. It is 90% reproducible.
>
> that is good, because I can't reproduce it with any of my system. Don't ask 
> me why. I really simply works. So if you figure it out, it would be great.

There needs to be some load on the usb. I use
rsync-over-bt-over-gprs/umts.

>> It could be easily fixed by just not freeing the urbs during
>> disconnect (introducing very very slow memory leak). Would that be
>> acceptable?
>
> I don't think so.
>
> Have you ever tried btusb driver? That code is a lot cleaner and it uses 
> USB anchors for the queued URBs (no home grown broken URB queues).

I wish I knew about  this one earlier. Yes, it seems to work for me.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 2/3] hci_usb: remove code obfuscation

[Marcel Holtmann]

Hi Pavel,

>>>>> _urb_free is an alias for kfree... making code longer & harder to
>>>>> read. Remove it.
>>>>
>>>> our own URB handling here is broken anyway. It was a bad idea  
>>>> when we did
>>>> it, but at that time the USB susbsystem was not in that good  
>>>> shape that
>>>> it
>>>> is today. So I would say leave this code as it is and concentrate  
>>>> on the
>>>> new btusb driver, but if it helps anybody I am happy to ACK this  
>>>> one.
>>>
>>> It currently corrupts memory (use after free) during suspend, and  
>>> I'm
>>> trying to get that fixed. It is 90% reproducible.
>>
>> that is good, because I can't reproduce it with any of my system.  
>> Don't ask
>> me why. I really simply works. So if you figure it out, it would be  
>> great.
>
> There needs to be some load on the usb. I use
> rsync-over-bt-over-gprs/umts.

you told me before and I believe you. This driver is not in its best  
shape. Weird thing is that I never got it re-produced. If you find the  
real cause. That would be great.

>>> It could be easily fixed by just not freeing the urbs during
>>> disconnect (introducing very very slow memory leak). Would that be
>>> acceptable?
>>
>> I don't think so.
>>
>> Have you ever tried btusb driver? That code is a lot cleaner and it  
>> uses
>> USB anchors for the queued URBs (no home grown broken URB queues).
>
> I wish I knew about  this one earlier. Yes, it seems to work for me.

Does this mean you are not fixing hci_usb anymore ;)

Regards

Marcel

Re: [patch 2/3] hci_usb: remove code obfuscation

[Pavel Machek]

Hi!

>> There needs to be some load on the usb. I use
>> rsync-over-bt-over-gprs/umts.
>
> you told me before and I believe you. This driver is not in its best shape. 
> Weird thing is that I never got it re-produced. If you find the real cause. 
> That would be great.

The cause seems to be hci_usb_close() racing with the USB core and
completion handlers...

>>>> It could be easily fixed by just not freeing the urbs during
>>>> disconnect (introducing very very slow memory leak). Would that be
>>>> acceptable?
>>>
>>> I don't think so.
>>>
>>> Have you ever tried btusb driver? That code is a lot cleaner and it uses
>>> USB anchors for the queued URBs (no home grown broken URB queues).
>>
>> I wish I knew about  this one earlier. Yes, it seems to work for me.
>
> Does this mean you are not fixing hci_usb anymore ;)

I'm not sure what's next, as hci_usb is still in opensuse11...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 2/3] hci_usb: remove code obfuscation

[Marcel Holtmann]

Hi Pavel,

>>> There needs to be some load on the usb. I use
>>> rsync-over-bt-over-gprs/umts.
>>
>> you told me before and I believe you. This driver is not in its  
>> best shape.
>> Weird thing is that I never got it re-produced. If you find the  
>> real cause.
>> That would be great.
>
> The cause seems to be hci_usb_close() racing with the USB core and
> completion handlers...
>
>>>>> It could be easily fixed by just not freeing the urbs during
>>>>> disconnect (introducing very very slow memory leak). Would that be
>>>>> acceptable?
>>>>
>>>> I don't think so.
>>>>
>>>> Have you ever tried btusb driver? That code is a lot cleaner and  
>>>> it uses
>>>> USB anchors for the queued URBs (no home grown broken URB queues).
>>>
>>> I wish I knew about  this one earlier. Yes, it seems to work for me.
>>
>> Does this mean you are not fixing hci_usb anymore ;)
>
> I'm not sure what's next, as hci_usb is still in opensuse11...

the goal should be improve btusb. Especially add all the needed quirks  
from the hci_usb driver to it. I have a patch for it, but I need to  
reverse the HCI_RESET logic.

However if you have hci_usb patches, send them, I will review them and  
David will happily apply them I guess.

Regards

Marcel

Re: [patch 2/3] hci_usb: remove code obfuscation

[Pavel Machek]

Hi!

>>>>>> It could be easily fixed by just not freeing the urbs during
>>>>>> disconnect (introducing very very slow memory leak). Would that be
>>>>>> acceptable?
>>>>>
>>>>> I don't think so.
>>>>>
>>>>> Have you ever tried btusb driver? That code is a lot cleaner and it 
>>>>> uses
>>>>> USB anchors for the queued URBs (no home grown broken URB queues).
>>>>
>>>> I wish I knew about  this one earlier. Yes, it seems to work for me.
>>>
>>> Does this mean you are not fixing hci_usb anymore ;)
>>
>> I'm not sure what's next, as hci_usb is still in opensuse11...
>
> the goal should be improve btusb. Especially add all the needed quirks from 
> the hci_usb driver to it. I have a patch for it, but I need to reverse the 
> HCI_RESET logic.

Could I get a copy of that patch?

> However if you have hci_usb patches, send them, I will review them and 
> David will happily apply them I guess.

No, I given up. I was not able to fix close vs. finishing urbs race
in a reliable way :-(.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 2/3] hci_usb: remove code obfuscation

[David Miller]

From: akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org
Date: Fri, 18 Apr 2008 13:46:53 -0700

> _urb_free is an alias for kfree... making code longer & harder to
> read. Remove it.
> 
> Signed-off-by: Pavel Machek <pavel-AlSwsSmVLrQ@public.gmane.org>
> Cc: Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>
> Cc: Dave Young <hidave.darkstar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>

As long as the code is in the tree, we should accept
simplifications that aid to code understanding.

Marking drivers broken or deprecated or to-be-replaced is
a secondary concern.

Therefore I'm applying this patch, thanks Pavel.