hci_usb: remove macro code obfuscation

hci_usb: remove macro code obfuscation

[Pavel Machek]

I had trouble figuring out what the code does. atomic_inc/dec
management is actually pretty simple, but it is needlessly obfuscated
with macros. Fix that.

Signed-off-by: Pavel Machek <pavel@suse.cz>

I had trouble figuring out what the code does. atomic_inc/dec
management is actually pretty simple, but it is needlessly obfuscated
with macros. Fix that.

Signed-off-by: Pavel Machek <pavel@suse.cz>

---
commit 88c139ba08abddb52636e0ac25af0cabf8345e54
tree 30f9fd7c7051e63edaaa24a93ad9686492a0bc63
parent 43cfc0427c14f482e36adac447409c82b5cbbe09
author Pavel <pavel@amd.ucw.cz> Wed, 16 Apr 2008 12:42:32 +0200
committer Pavel <pavel@amd.ucw.cz> Wed, 16 Apr 2008 12:42:32 +0200

 drivers/bluetooth/hci_usb.c |   41 ++++++++++++++++++-----------------------
 1 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/drivers/bluetooth/hci_usb.c b/drivers/bluetooth/hci_usb.c
index 2597f63..6e0efc7 100644
--- a/drivers/bluetooth/hci_usb.c
+++ b/drivers/bluetooth/hci_usb.c
@@ -201,14 +201,9 @@ static struct _urb *_urb_dequeue(struct 
 static void hci_usb_rx_complete(struct urb *urb);
 static void hci_usb_tx_complete(struct urb *urb);
 
-#define __pending_tx(husb, type)  (&husb->pending_tx[type-1])
-#define __pending_q(husb, type)   (&husb->pending_q[type-1])
-#define __completed_q(husb, type) (&husb->completed_q[type-1])
-#define __transmit_q(husb, type)  (&husb->transmit_q[type-1])
-
 static inline struct _urb *__get_completed(struct hci_usb *husb, int type)
 {
-	return _urb_dequeue(__completed_q(husb, type)); 
+	return _urb_dequeue(&husb->completed_q[type-1]); 
 }
 
 #ifdef CONFIG_BT_HCIUSB_SCO
@@ -254,7 +249,7 @@ static int hci_usb_intr_rx_submit(struct
 		return -ENOMEM;
 	}
 	_urb->type = HCI_EVENT_PKT;
-	_urb_queue_tail(__pending_q(husb, _urb->type), _urb);
+	_urb_queue_tail(&husb->pending_q[_urb->type-1], _urb);
 
 	urb = &_urb->urb;
 	pipe     = usb_rcvintpipe(husb->udev, husb->intr_in_ep->desc.bEndpointAddress);
@@ -289,7 +284,7 @@ static int hci_usb_bulk_rx_submit(struct
 		return -ENOMEM;
 	}
 	_urb->type = HCI_ACLDATA_PKT;
-	_urb_queue_tail(__pending_q(husb, _urb->type), _urb);
+	_urb_queue_tail(&husb->pending_q[_urb->type-1], _urb);
 
 	urb  = &_urb->urb;
 	pipe = usb_rcvbulkpipe(husb->udev, husb->bulk_in_ep->desc.bEndpointAddress);
@@ -330,7 +325,7 @@ static int hci_usb_isoc_rx_submit(struct
 		return -ENOMEM;
 	}
 	_urb->type = HCI_SCODATA_PKT;
-	_urb_queue_tail(__pending_q(husb, _urb->type), _urb);
+	_urb_queue_tail(&husb->pending_q[_urb->type-1], _urb);
 
 	urb = &_urb->urb;
 
@@ -423,7 +418,7 @@ static void hci_usb_unlink_urbs(struct h
 			BT_DBG("%s unlinking _urb %p type %d urb %p", 
 					husb->hdev->name, _urb, _urb->type, urb);
 			usb_kill_urb(urb);
-			_urb_queue_tail(__completed_q(husb, _urb->type), _urb);
+			_urb_queue_tail(&husb->completed_q[_urb->type-1], _urb);
 		}
 
 		/* Release completed requests */
@@ -474,15 +469,15 @@ static int __tx_submit(struct hci_usb *h
 
 	BT_DBG("%s urb %p type %d", husb->hdev->name, urb, _urb->type);
 
-	_urb_queue_tail(__pending_q(husb, _urb->type), _urb);
+	_urb_queue_tail(&husb->pending_q[_urb->type-1], _urb);
 	err = usb_submit_urb(urb, GFP_ATOMIC);
 	if (err) {
 		BT_ERR("%s tx submit failed urb %p type %d err %d",
 				husb->hdev->name, urb, _urb->type, err);
 		_urb_unlink(_urb);
-		_urb_queue_tail(__completed_q(husb, _urb->type), _urb);
+		_urb_queue_tail(&husb->completed_q[_urb->type-1], _urb);
 	} else
-		atomic_inc(__pending_tx(husb, _urb->type));
+		atomic_inc(&husb->pending_tx[_urb->type-1]);
 
 	return err;
 }
@@ -594,8 +589,8 @@ static void hci_usb_tx_process(struct hc
 		clear_bit(HCI_USB_TX_WAKEUP, &husb->state);
 
 		/* Process command queue */
-		q = __transmit_q(husb, HCI_COMMAND_PKT);
-		if (!atomic_read(__pending_tx(husb, HCI_COMMAND_PKT)) &&
+		q = &husb->transmit_q[HCI_COMMAND_PKT-1];
+		if (!atomic_read(&husb->pending_tx[HCI_COMMAND_PKT-1]) &&
 				(skb = skb_dequeue(q))) {
 			if (hci_usb_send_ctrl(husb, skb) < 0)
 				skb_queue_head(q, skb);
@@ -603,8 +598,8 @@ static void hci_usb_tx_process(struct hc
 
 #ifdef CONFIG_BT_HCIUSB_SCO
 		/* Process SCO queue */
-		q = __transmit_q(husb, HCI_SCODATA_PKT);
-		if (atomic_read(__pending_tx(husb, HCI_SCODATA_PKT)) < HCI_MAX_ISOC_TX &&
+		q = &husb->transmit_q[HCI_SCODATA_PKT-1];
+		if (atomic_read(&husb->pending_tx[HCI_SCODATA_PKT-1]) < HCI_MAX_ISOC_TX &&
 				(skb = skb_dequeue(q))) {
 			if (hci_usb_send_isoc(husb, skb) < 0)
 				skb_queue_head(q, skb);
@@ -612,8 +607,8 @@ #ifdef CONFIG_BT_HCIUSB_SCO
 #endif
 
 		/* Process ACL queue */
-		q = __transmit_q(husb, HCI_ACLDATA_PKT);
-		while (atomic_read(__pending_tx(husb, HCI_ACLDATA_PKT)) < HCI_MAX_BULK_TX &&
+		q = &husb->transmit_q[HCI_ACLDATA_PKT-1];
+		while (atomic_read(&husb->pending_tx[HCI_ACLDATA_PKT-1]) < HCI_MAX_BULK_TX &&
 				(skb = skb_dequeue(q))) {
 			if (hci_usb_send_bulk(husb, skb) < 0) {
 				skb_queue_head(q, skb);
@@ -675,7 +670,7 @@ #endif
 
 	read_lock(&husb->completion_lock);
 
-	skb_queue_tail(__transmit_q(husb, bt_cb(skb)->pkt_type), skb);
+	skb_queue_tail(&husb->transmit_q[bt_cb(skb)->pkt_type-1], skb);
 	hci_usb_tx_wakeup(husb);
 
 	read_unlock(&husb->completion_lock);
@@ -752,7 +747,7 @@ static void hci_usb_tx_complete(struct u
 		return;
 
 	/* _urb may have been already cleared by hci_usb_unlink_urbs */
-	atomic_dec(__pending_tx(husb, _urb->type));
+	atomic_dec(&husb->pending_tx[_urb->type-1]);
 
 	urb->transfer_buffer = NULL;
 	kfree_skb((struct sk_buff *) _urb->priv);
@@ -765,7 +760,7 @@ static void hci_usb_tx_complete(struct u
 	read_lock(&husb->completion_lock);
 
 	_urb_unlink(_urb);
-	_urb_queue_tail(__completed_q(husb, _urb->type), _urb);
+	_urb_queue_tail(&husb->completed_q[_urb->type-1], _urb);
 
 	hci_usb_tx_wakeup(husb);
 


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

Re: hci_usb: remove macro code obfuscation

[Vitaliy Ivanov]

On Wed, Apr 16, 2008 at 1:42 PM, Pavel Machek <pavel@ucw.cz> wrote:
>
>  I had trouble figuring out what the code does. atomic_inc/dec
>  management is actually pretty simple, but it is needlessly obfuscated
>  with macros. Fix that.
>
>  Signed-off-by: Pavel Machek <pavel@suse.cz>
>
>  I had trouble figuring out what the code does. atomic_inc/dec
>  management is actually pretty simple, but it is needlessly obfuscated
>  with macros. Fix that.
>
>  Signed-off-by: Pavel Machek <pavel@suse.cz>


Got it from the first time;)

Do you think that now code looks better? As for me it's not...

V>

Re: hci_usb: remove macro code obfuscation

[Pavel Machek]

On Wed 2008-04-16 13:51:37, Vitaliy Ivanov wrote:
> On Wed, Apr 16, 2008 at 1:42 PM, Pavel Machek <pavel@ucw.cz> wrote:
> >
> >  I had trouble figuring out what the code does. atomic_inc/dec
> >  management is actually pretty simple, but it is needlessly obfuscated
> >  with macros. Fix that.
> >
> >  Signed-off-by: Pavel Machek <pavel@suse.cz>
> >
> >  I had trouble figuring out what the code does. atomic_inc/dec
> >  management is actually pretty simple, but it is needlessly obfuscated
> >  with macros. Fix that.
> >
> >  Signed-off-by: Pavel Machek <pavel@suse.cz>
> 
> 
> Got it from the first time;)
> 
> Do you think that now code looks better? As for me it's not...

Yes. Hiding & operator deep inside macro is evil for one thing. Plus
it is no longer clear what the code does with the macros in there.
									Pavel

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

Re: hci_usb: remove macro code obfuscation

[Max Krasnyanskiy]

Pavel Machek wrote:
> On Wed 2008-04-16 13:51:37, Vitaliy Ivanov wrote:
>> On Wed, Apr 16, 2008 at 1:42 PM, Pavel Machek <pavel@ucw.cz> wrote:
>>>  I had trouble figuring out what the code does. atomic_inc/dec
>>>  management is actually pretty simple, but it is needlessly obfuscated
>>>  with macros. Fix that.
>>>
>>>  Signed-off-by: Pavel Machek <pavel@suse.cz>
>>>
>>>  I had trouble figuring out what the code does. atomic_inc/dec
>>>  management is actually pretty simple, but it is needlessly obfuscated
>>>  with macros. Fix that.
>>>
>>>  Signed-off-by: Pavel Machek <pavel@suse.cz>
>>
>> Got it from the first time;)
>>
>> Do you think that now code looks better? As for me it's not...
> 
> Yes. Hiding & operator deep inside macro is evil for one thing. Plus
> it is no longer clear what the code does with the macros in there.

In general I would agree in this case it seems to actually make code clearer 
(I prefer original macros that is).
Anyway, I do not mind the change.

btw Marcel told me that all this queuing stuff does not actually make sense 
anymore. USB core did not support this before and HCI driver performance 
sucked without it. Marcel is telling me that things have changed.
So. Pavel, while you're at it can you maybe whack that stuff out completely ?
I mean all this custom _urb stuff that I did was eventually supposed to move 
into usb core. Then I stopped working on Bluetooth and it never happened. It'd 
be nice to clean that up since it seems that most of the latest bug reports 
are related to this urb business.

Thanx
Max

Re: hci_usb: remove macro code obfuscation

[Andrew Morton]

On Wed, 16 Apr 2008 13:51:37 +0300
"Vitaliy Ivanov" <vitalivanov@gmail.com> wrote:

> On Wed, Apr 16, 2008 at 1:42 PM, Pavel Machek <pavel@ucw.cz> wrote:
> >
> >  I had trouble figuring out what the code does. atomic_inc/dec
> >  management is actually pretty simple, but it is needlessly obfuscated
> >  with macros. Fix that.
> >
> >  Signed-off-by: Pavel Machek <pavel@suse.cz>
> >
> >  I had trouble figuring out what the code does. atomic_inc/dec
> >  management is actually pretty simple, but it is needlessly obfuscated
> >  with macros. Fix that.
> >
> >  Signed-off-by: Pavel Machek <pavel@suse.cz>
> 
> 
> Got it from the first time;)
> 
> Do you think that now code looks better? As for me it's not...
> 

Yes, I expect that the original code was easier to work with and it is not
obfuscated I don't think.  Sometimes these things take a few minutes for
new readers to become comfortable with but are good for people who work on
the code regularly.

Although it's a mystery why __pending_tx() and friends

a) have leading underscores and

b) are implemented in cpp, when C is available.

Re: hci_usb: remove macro code obfuscation

[Max Krasnyanskiy]

Andrew Morton wrote:
> On Wed, 16 Apr 2008 13:51:37 +0300
> "Vitaliy Ivanov" <vitalivanov@gmail.com> wrote:
> 
>> On Wed, Apr 16, 2008 at 1:42 PM, Pavel Machek <pavel@ucw.cz> wrote:
>>>  I had trouble figuring out what the code does. atomic_inc/dec
>>>  management is actually pretty simple, but it is needlessly obfuscated
>>>  with macros. Fix that.
>>>
>>>  Signed-off-by: Pavel Machek <pavel@suse.cz>
>>>
>>>  I had trouble figuring out what the code does. atomic_inc/dec
>>>  management is actually pretty simple, but it is needlessly obfuscated
>>>  with macros. Fix that.
>>>
>>>  Signed-off-by: Pavel Machek <pavel@suse.cz>
>>
>> Got it from the first time;)
>>
>> Do you think that now code looks better? As for me it's not...
>>
> 
> Yes, I expect that the original code was easier to work with and it is not
> obfuscated I don't think.  Sometimes these things take a few minutes for
> new readers to become comfortable with but are good for people who work on
> the code regularly.
> 
> Although it's a mystery why __pending_tx() and friends
> 
> a) have leading underscores and
At the time I wrote it, it made sense (to me :)) but it's been such a long 
time ago I would not remember. Probably to show that this is something 
internal and needs to be used with caution.

> 
> b) are implemented in cpp, when C is available.
> 
I'm not sure what you mean ? #define vs inline I suppose ?

Max

Re: hci_usb: remove macro code obfuscation

[Andrew Morton]

On Wed, 16 Apr 2008 11:37:41 -0700
Max Krasnyanskiy <maxk@qualcomm.com> wrote:

> > 
> > b) are implemented in cpp, when C is available.
> > 
> I'm not sure what you mean ? #define vs inline I suppose ?

yup.

I have a stdrant about this which I should write down permanently sometime.

Re: hci_usb: remove macro code obfuscation

[Pavel Machek]

On Wed 2008-04-16 10:19:15, Max Krasnyanskiy wrote:
> Pavel Machek wrote:
>> On Wed 2008-04-16 13:51:37, Vitaliy Ivanov wrote:
>>> On Wed, Apr 16, 2008 at 1:42 PM, Pavel Machek <pavel@ucw.cz> wrote:
>>>>  I had trouble figuring out what the code does. atomic_inc/dec
>>>>  management is actually pretty simple, but it is needlessly obfuscated
>>>>  with macros. Fix that.
>>>>
>>>>  Signed-off-by: Pavel Machek <pavel@suse.cz>
>>>>
>>>>  I had trouble figuring out what the code does. atomic_inc/dec
>>>>  management is actually pretty simple, but it is needlessly obfuscated
>>>>  with macros. Fix that.
>>>>
>>>>  Signed-off-by: Pavel Machek <pavel@suse.cz>
>>>
>>> Got it from the first time;)
>>>
>>> Do you think that now code looks better? As for me it's not...
>>
>> Yes. Hiding & operator deep inside macro is evil for one thing. Plus
>> it is no longer clear what the code does with the macros in there.
>
> In general I would agree in this case it seems to actually make code 
> clearer (I prefer original macros that is).
> Anyway, I do not mind the change.
>
> btw Marcel told me that all this queuing stuff does not actually make sense 
> anymore. USB core did not support this before and HCI driver performance 
> sucked without it. Marcel is telling me that things have changed.
> So. Pavel, while you're at it can you maybe whack that stuff out completely ?
> I mean all this custom _urb stuff that I did was eventually supposed to 
> move into usb core. Then I stopped working on Bluetooth and it never 
> happened. It'd be nice to clean that up since it seems that most of the 
> latest bug reports are related to this urb business.

It seems someone already done that in (mainline) btusb.c

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