hci_usb: remove macro code obfuscation - Kernel

This is a discussion on hci_usb: remove macro code obfuscation - Kernel ; 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 I had trouble figuring out what the code does. atomic_inc/dec management is actually ...

+ Reply to Thread
Results 1 to 8 of 8

Thread: hci_usb: remove macro code obfuscation

  1. hci_usb: remove macro code obfuscation


    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

    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

    ---
    commit 88c139ba08abddb52636e0ac25af0cabf8345e54
    tree 30f9fd7c7051e63edaaa24a93ad9686492a0bc63
    parent 43cfc0427c14f482e36adac447409c82b5cbbe09
    author Pavel Wed, 16 Apr 2008 12:42:32 +0200
    committer Pavel 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/~pav...rses/blog.html
    --
    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/

  2. Re: hci_usb: remove macro code obfuscation

    On Wed, Apr 16, 2008 at 1:42 PM, Pavel Machek 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
    >
    > 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



    Got it from the first time

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

    V>
    --
    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/

  3. Re: hci_usb: remove macro code obfuscation

    On Wed 2008-04-16 13:51:37, Vitaliy Ivanov wrote:
    > On Wed, Apr 16, 2008 at 1:42 PM, Pavel Machek 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
    > >
    > > 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

    >
    >
    > 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/~pav...rses/blog.html
    --
    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/

  4. references:in-reply-to:content-type:

    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 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
    >>>
    >>> 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

    >>
    >> 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



    --
    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/

  5. references:in-reply-to:content-type:

    Andrew Morton wrote:
    > On Wed, 16 Apr 2008 13:51:37 +0300
    > "Vitaliy Ivanov" wrote:
    >
    >> On Wed, Apr 16, 2008 at 1:42 PM, Pavel Machek 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
    >>>
    >>> 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

    >>
    >> 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
    --
    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/

  6. Re: hci_usb: remove macro code obfuscation

    On Wed, 16 Apr 2008 13:51:37 +0300
    "Vitaliy Ivanov" wrote:

    > On Wed, Apr 16, 2008 at 1:42 PM, Pavel Machek 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
    > >
    > > 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

    >
    >
    > 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.
    --
    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/

  7. Re: hci_usb: remove macro code obfuscation

    On Wed, 16 Apr 2008 11:37:41 -0700
    Max Krasnyanskiy 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.
    --
    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/

  8. Re: hci_usb: remove macro code obfuscation

    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 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
    >>>>
    >>>> 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
    >>>
    >>> 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/~pav...rses/blog.html
    --
    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/

+ Reply to Thread