* [PATCH] l2cap_core: automatically flushable packets aren't supported by LE-only devices @ 2014-11-18 14:46 Steven Walter 2014-11-18 22:01 ` Marcel Holtmann 0 siblings, 1 reply; 13+ messages in thread From: Steven Walter @ 2014-11-18 14:46 UTC (permalink / raw) To: linux-kernel, linux-bluetooth, marcel, gustavo, johan.hedberg Cc: Steven Walter The bluetooth spec states that automatically flushable packets may not be sent to a LE-only controller. The code already supports non-automatically-flushable packets, but uses a bit in the controller feature field to determine whether to use them. That bit is always zero for LE-only devices, so we need to check for the LE-only case explicitly. --- net/bluetooth/l2cap_core.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c index 4af3821..29d9b9c 100644 --- a/net/bluetooth/l2cap_core.c +++ b/net/bluetooth/l2cap_core.c @@ -764,7 +764,7 @@ static void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, if (!skb) return; - if (lmp_no_flush_capable(conn->hcon->hdev)) + if (lmp_no_flush_capable(conn->hcon->hdev) || !lmp_bredr_capable(conn->hcon->hdev)) flags = ACL_START_NO_FLUSH; else flags = ACL_START; @@ -798,8 +798,9 @@ static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb) return; } - if (!test_bit(FLAG_FLUSHABLE, &chan->flags) && - lmp_no_flush_capable(hcon->hdev)) + if (!lmp_bredr_capable(hcon->hdev) || + (!test_bit(FLAG_FLUSHABLE, &chan->flags) && + lmp_no_flush_capable(hcon->hdev))) flags = ACL_START_NO_FLUSH; else flags = ACL_START; -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] l2cap_core: automatically flushable packets aren't supported by LE-only devices 2014-11-18 14:46 [PATCH] l2cap_core: automatically flushable packets aren't supported by LE-only devices Steven Walter @ 2014-11-18 22:01 ` Marcel Holtmann 2014-11-19 14:41 ` [PATCH v2] l2cap_core: automatically flushable packets aren't allowed on LE links Steven Walter 0 siblings, 1 reply; 13+ messages in thread From: Marcel Holtmann @ 2014-11-18 22:01 UTC (permalink / raw) To: Steven Walter Cc: linux-kernel, BlueZ development, Gustavo F. Padovan, Johan Hedberg Hi Steven, > The bluetooth spec states that automatically flushable packets may not > be sent to a LE-only controller. The code already supports > non-automatically-flushable packets, but uses a bit in the controller > feature field to determine whether to use them. That bit is always zero > for LE-only devices, so we need to check for the LE-only case explicitly. > --- > net/bluetooth/l2cap_core.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > index 4af3821..29d9b9c 100644 > --- a/net/bluetooth/l2cap_core.c > +++ b/net/bluetooth/l2cap_core.c > @@ -764,7 +764,7 @@ static void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, > if (!skb) > return; > > - if (lmp_no_flush_capable(conn->hcon->hdev)) > + if (lmp_no_flush_capable(conn->hcon->hdev) || !lmp_bredr_capable(conn->hcon->hdev)) > flags = ACL_START_NO_FLUSH; > else > flags = ACL_START; > @@ -798,8 +798,9 @@ static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb) > return; > } > > - if (!test_bit(FLAG_FLUSHABLE, &chan->flags) && > - lmp_no_flush_capable(hcon->hdev)) > + if (!lmp_bredr_capable(hcon->hdev) || > + (!test_bit(FLAG_FLUSHABLE, &chan->flags) && > + lmp_no_flush_capable(hcon->hdev))) > flags = ACL_START_NO_FLUSH; > else > flags = ACL_START; I do not think doing it this way is correct. I am actually surprised that it using fine right now, but I assume that is because all dual-mode controllers also support non-flushable packets. We should check the link type of the connection and if it is LE then we should always use non-flushable packets. The feature bits have really nothing to do with this. They only apply to the BR/EDR side of things. LE has its own supported feature bits. Regards Marcel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] l2cap_core: automatically flushable packets aren't allowed on LE links 2014-11-18 22:01 ` Marcel Holtmann @ 2014-11-19 14:41 ` Steven Walter 2014-11-19 14:48 ` Marcel Holtmann 0 siblings, 1 reply; 13+ messages in thread From: Steven Walter @ 2014-11-19 14:41 UTC (permalink / raw) To: marcel, gustavo, johan.hedberg, linux-bluetooth, linux-kernel Cc: Steven Walter The bluetooth spec states that automatically flushable packets may not be sent over a LE-U link. Signed-off-by: Steven Walter <stevenrwalter@gmail.com> --- net/bluetooth/l2cap_core.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c index 4af3821..028fcc6 100644 --- a/net/bluetooth/l2cap_core.c +++ b/net/bluetooth/l2cap_core.c @@ -764,7 +764,7 @@ static void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, if (!skb) return; - if (lmp_no_flush_capable(conn->hcon->hdev)) + if (lmp_no_flush_capable(conn->hcon->hdev) || (conn->hcon->type == LE_LINK)) flags = ACL_START_NO_FLUSH; else flags = ACL_START; @@ -798,8 +798,9 @@ static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb) return; } - if (!test_bit(FLAG_FLUSHABLE, &chan->flags) && - lmp_no_flush_capable(hcon->hdev)) + if ((hcon->type == LE_LINK) || + (!test_bit(FLAG_FLUSHABLE, &chan->flags) && + lmp_no_flush_capable(hcon->hdev))) flags = ACL_START_NO_FLUSH; else flags = ACL_START; -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] l2cap_core: automatically flushable packets aren't allowed on LE links 2014-11-19 14:41 ` [PATCH v2] l2cap_core: automatically flushable packets aren't allowed on LE links Steven Walter @ 2014-11-19 14:48 ` Marcel Holtmann 2014-11-19 17:59 ` [PATCH v3] Bluetooth: " Steven Walter 0 siblings, 1 reply; 13+ messages in thread From: Marcel Holtmann @ 2014-11-19 14:48 UTC (permalink / raw) To: Steven Walter Cc: Gustavo F. Padovan, Johan Hedberg, BlueZ development, linux-kernel Hi Steven, you need to prefix the subject line with Bluetooth: like all other patches do. > On Nov 19, 2014, at 22:41, Steven Walter <stevenrwalter@gmail.com> wrote: > > The bluetooth spec states that automatically flushable packets may not > be sent over a LE-U link. > > Signed-off-by: Steven Walter <stevenrwalter@gmail.com> > --- > net/bluetooth/l2cap_core.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > index 4af3821..028fcc6 100644 > --- a/net/bluetooth/l2cap_core.c > +++ b/net/bluetooth/l2cap_core.c > @@ -764,7 +764,7 @@ static void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, > if (!skb) > return; > > - if (lmp_no_flush_capable(conn->hcon->hdev)) > + if (lmp_no_flush_capable(conn->hcon->hdev) || (conn->hcon->type == LE_LINK)) no need for ( ) around that new statement. > flags = ACL_START_NO_FLUSH; > else > flags = ACL_START; > @@ -798,8 +798,9 @@ static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb) > return; > } > > - if (!test_bit(FLAG_FLUSHABLE, &chan->flags) && > - lmp_no_flush_capable(hcon->hdev)) > + if ((hcon->type == LE_LINK) || Same here, the ( ) are not needed. > + (!test_bit(FLAG_FLUSHABLE, &chan->flags) && > + lmp_no_flush_capable(hcon->hdev))) > flags = ACL_START_NO_FLUSH; > else > flags = ACL_START; I wonder actually if we should have a helper function or add comments to explain why we are doing it this complicated. Regards Marcel ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] Bluetooth: automatically flushable packets aren't allowed on LE links 2014-11-19 14:48 ` Marcel Holtmann @ 2014-11-19 17:59 ` Steven Walter 2014-11-20 11:38 ` Johan Hedberg 0 siblings, 1 reply; 13+ messages in thread From: Steven Walter @ 2014-11-19 17:59 UTC (permalink / raw) To: marcel, gustavo, johan.hedberg, linux-bluetooth, linux-kernel Cc: Steven Walter The bluetooth spec states that automatically flushable packets may not be sent over a LE-U link. Signed-off-by: Steven Walter <stevenrwalter@gmail.com> --- net/bluetooth/l2cap_core.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c index 4af3821..7c4350f 100644 --- a/net/bluetooth/l2cap_core.c +++ b/net/bluetooth/l2cap_core.c @@ -764,7 +764,7 @@ static void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, if (!skb) return; - if (lmp_no_flush_capable(conn->hcon->hdev)) + if (lmp_no_flush_capable(conn->hcon->hdev) || conn->hcon->type == LE_LINK) flags = ACL_START_NO_FLUSH; else flags = ACL_START; @@ -784,7 +784,7 @@ static bool __chan_is_moving(struct l2cap_chan *chan) static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb) { struct hci_conn *hcon = chan->conn->hcon; - u16 flags; + u16 flags = ACL_START; BT_DBG("chan %p, skb %p len %d priority %u", chan, skb, skb->len, skb->priority); @@ -798,11 +798,13 @@ static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb) return; } - if (!test_bit(FLAG_FLUSHABLE, &chan->flags) && - lmp_no_flush_capable(hcon->hdev)) + if (hcon->type == LE_LINK) { + /* LE-U does not support auto-flushing packets */ flags = ACL_START_NO_FLUSH; - else - flags = ACL_START; + } else if (!test_bit(FLAG_FLUSHABLE, &chan->flags) && + lmp_no_flush_capable(hcon->hdev)) { + flags = ACL_START_NO_FLUSH; + } bt_cb(skb)->force_active = test_bit(FLAG_FORCE_ACTIVE, &chan->flags); hci_send_acl(chan->conn->hchan, skb, flags); -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3] Bluetooth: automatically flushable packets aren't allowed on LE links 2014-11-19 17:59 ` [PATCH v3] Bluetooth: " Steven Walter @ 2014-11-20 11:38 ` Johan Hedberg 2014-11-20 13:50 ` Marcel Holtmann 0 siblings, 1 reply; 13+ messages in thread From: Johan Hedberg @ 2014-11-20 11:38 UTC (permalink / raw) To: Steven Walter; +Cc: marcel, gustavo, linux-bluetooth, linux-kernel Hi Steven, On Wed, Nov 19, 2014, Steven Walter wrote: > The bluetooth spec states that automatically flushable packets may not > be sent over a LE-U link. > > Signed-off-by: Steven Walter <stevenrwalter@gmail.com> > --- > net/bluetooth/l2cap_core.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > index 4af3821..7c4350f 100644 > --- a/net/bluetooth/l2cap_core.c > +++ b/net/bluetooth/l2cap_core.c > @@ -764,7 +764,7 @@ static void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, > if (!skb) > return; > > - if (lmp_no_flush_capable(conn->hcon->hdev)) > + if (lmp_no_flush_capable(conn->hcon->hdev) || conn->hcon->type == LE_LINK) > flags = ACL_START_NO_FLUSH; > else > flags = ACL_START; > @@ -784,7 +784,7 @@ static bool __chan_is_moving(struct l2cap_chan *chan) > static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb) > { > struct hci_conn *hcon = chan->conn->hcon; > - u16 flags; > + u16 flags = ACL_START; > > BT_DBG("chan %p, skb %p len %d priority %u", chan, skb, skb->len, > skb->priority); > @@ -798,11 +798,13 @@ static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb) > return; > } > > - if (!test_bit(FLAG_FLUSHABLE, &chan->flags) && > - lmp_no_flush_capable(hcon->hdev)) > + if (hcon->type == LE_LINK) { > + /* LE-U does not support auto-flushing packets */ > flags = ACL_START_NO_FLUSH; > - else > - flags = ACL_START; > + } else if (!test_bit(FLAG_FLUSHABLE, &chan->flags) && > + lmp_no_flush_capable(hcon->hdev)) { > + flags = ACL_START_NO_FLUSH; > + } I think Marcel was after just providing a clarifying code comment in both places - having two branches of an if-statement doing exactly the same thing looks a bit weird to me. To make thins completely clear I'd suggest adding a simple helper function that you can call from both places to get the needed flags, something like the following: static u16 get_acl_flags(struct hci_conn *hcon, struct l2cap_chan *chan) { /* LE-U does not support auto-flushing packets */ if (hcon->type == LE_LINK) return ACL_START_NO_FLUSH; /* If non-flushable packets are not supported don't request them */ if (!lmp_no_flush_capable(hcon->hdev)) return ACL_START; /* If the chan has requested auto-flushing go with that */ if (chan && test_bit(FLAG_FLUSHABLE, &chan->flags)) return ACL_START; /* Otherwise go with a non-flushable packet */ return ACL_START_NO_FLUSH; } This way we'd avoid complex if-statements and can clearly document each condition independently. Johan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] Bluetooth: automatically flushable packets aren't allowed on LE links 2014-11-20 11:38 ` Johan Hedberg @ 2014-11-20 13:50 ` Marcel Holtmann 2014-11-20 14:57 ` Johan Hedberg 2014-11-20 15:51 ` Steven Walter 0 siblings, 2 replies; 13+ messages in thread From: Marcel Holtmann @ 2014-11-20 13:50 UTC (permalink / raw) To: Johan Hedberg Cc: Steven Walter, Gustavo F. Padovan, BlueZ development, linux-kernel Hi Johan, >> The bluetooth spec states that automatically flushable packets may not >> be sent over a LE-U link. >> >> Signed-off-by: Steven Walter <stevenrwalter@gmail.com> >> --- >> net/bluetooth/l2cap_core.c | 14 ++++++++------ >> 1 file changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c >> index 4af3821..7c4350f 100644 >> --- a/net/bluetooth/l2cap_core.c >> +++ b/net/bluetooth/l2cap_core.c >> @@ -764,7 +764,7 @@ static void l2cap_send_cmd(struct l2cap_conn *conn, u8 ident, u8 code, u16 len, >> if (!skb) >> return; >> >> - if (lmp_no_flush_capable(conn->hcon->hdev)) >> + if (lmp_no_flush_capable(conn->hcon->hdev) || conn->hcon->type == LE_LINK) >> flags = ACL_START_NO_FLUSH; >> else >> flags = ACL_START; >> @@ -784,7 +784,7 @@ static bool __chan_is_moving(struct l2cap_chan *chan) >> static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb) >> { >> struct hci_conn *hcon = chan->conn->hcon; >> - u16 flags; >> + u16 flags = ACL_START; >> >> BT_DBG("chan %p, skb %p len %d priority %u", chan, skb, skb->len, >> skb->priority); >> @@ -798,11 +798,13 @@ static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb) >> return; >> } >> >> - if (!test_bit(FLAG_FLUSHABLE, &chan->flags) && >> - lmp_no_flush_capable(hcon->hdev)) >> + if (hcon->type == LE_LINK) { >> + /* LE-U does not support auto-flushing packets */ >> flags = ACL_START_NO_FLUSH; >> - else >> - flags = ACL_START; >> + } else if (!test_bit(FLAG_FLUSHABLE, &chan->flags) && >> + lmp_no_flush_capable(hcon->hdev)) { >> + flags = ACL_START_NO_FLUSH; >> + } > > I think Marcel was after just providing a clarifying code comment in > both places - having two branches of an if-statement doing exactly the > same thing looks a bit weird to me. To make thins completely clear I'd > suggest adding a simple helper function that you can call from both > places to get the needed flags, something like the following: I am actually fine with just adding a comment explaining the complex if statement on why it is correct. It is just a helper for everybody to understand what and why it is done that way. > static u16 get_acl_flags(struct hci_conn *hcon, struct l2cap_chan *chan) > { If we do it with a helper functions, then it should only provide the l2cap_chan since we can get the hci_conn from there. > /* LE-U does not support auto-flushing packets */ > if (hcon->type == LE_LINK) > return ACL_START_NO_FLUSH; > > /* If non-flushable packets are not supported don't request them */ > if (!lmp_no_flush_capable(hcon->hdev)) > return ACL_START; > > /* If the chan has requested auto-flushing go with that */ > if (chan && test_bit(FLAG_FLUSHABLE, &chan->flags)) > return ACL_START; This seems to be a bit too much. The FLAG_FLUSABLE is only settable if the controller supports it. I wonder why we need the LMP features check here in the first place. Can we not just trust the flag on the channel is set correctly and enforce it before setting the flag. > > /* Otherwise go with a non-flushable packet */ > return ACL_START_NO_FLUSH; > } > > This way we'd avoid complex if-statements and can clearly document each > condition independently. Regards Marcel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] Bluetooth: automatically flushable packets aren't allowed on LE links 2014-11-20 13:50 ` Marcel Holtmann @ 2014-11-20 14:57 ` Johan Hedberg 2014-11-20 15:51 ` Steven Walter 1 sibling, 0 replies; 13+ messages in thread From: Johan Hedberg @ 2014-11-20 14:57 UTC (permalink / raw) To: Marcel Holtmann Cc: Steven Walter, Gustavo F. Padovan, BlueZ development, linux-kernel Hi Marcel, On Thu, Nov 20, 2014, Marcel Holtmann wrote: > > static u16 get_acl_flags(struct hci_conn *hcon, struct l2cap_chan *chan) > > { > > If we do it with a helper functions, then it should only provide the > l2cap_chan since we can get the hci_conn from there. One of the places it'd get called from (l2cap_send_cmd) doesn't have a l2cap_chan context at all which is why I split this into one mandatory (hci_conn) and another optoinal (l2cap_chan) parameter. Anyway, if you're happy with having this inline with a code comment explaining the (rather complex) if-statement then I'm fine with that too. Johan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] Bluetooth: automatically flushable packets aren't allowed on LE links 2014-11-20 13:50 ` Marcel Holtmann 2014-11-20 14:57 ` Johan Hedberg @ 2014-11-20 15:51 ` Steven Walter 2014-11-25 14:53 ` Steven Walter 1 sibling, 1 reply; 13+ messages in thread From: Steven Walter @ 2014-11-20 15:51 UTC (permalink / raw) To: Marcel Holtmann Cc: Johan Hedberg, Gustavo F. Padovan, BlueZ development, linux-kernel@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 2930 bytes --] On Thu, Nov 20, 2014 at 8:50 AM, Marcel Holtmann <marcel@holtmann.org> wrote: > Hi Johan, > > >> The bluetooth spec states that automatically flushable packets may not > >> be sent over a LE-U link. > >> > >> Signed-off-by: Steven Walter <stevenrwalter@gmail.com> > >> --- > >> net/bluetooth/l2cap_core.c | 14 ++++++++------ > >> 1 file changed, 8 insertions(+), 6 deletions(-) > >> > >> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c > >> index 4af3821..7c4350f 100644 > >> --- a/net/bluetooth/l2cap_core.c > >> +++ b/net/bluetooth/l2cap_core.c > >> @@ -764,7 +764,7 @@ static void l2cap_send_cmd(struct l2cap_conn *conn, > u8 ident, u8 code, u16 len, > >> if (!skb) > >> return; > >> > >> - if (lmp_no_flush_capable(conn->hcon->hdev)) > >> + if (lmp_no_flush_capable(conn->hcon->hdev) || conn->hcon->type == > LE_LINK) > >> flags = ACL_START_NO_FLUSH; > >> else > >> flags = ACL_START; > >> @@ -784,7 +784,7 @@ static bool __chan_is_moving(struct l2cap_chan > *chan) > >> static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb) > >> { > >> struct hci_conn *hcon = chan->conn->hcon; > >> - u16 flags; > >> + u16 flags = ACL_START; > >> > >> BT_DBG("chan %p, skb %p len %d priority %u", chan, skb, skb->len, > >> skb->priority); > >> @@ -798,11 +798,13 @@ static void l2cap_do_send(struct l2cap_chan > *chan, struct sk_buff *skb) > >> return; > >> } > >> > >> - if (!test_bit(FLAG_FLUSHABLE, &chan->flags) && > >> - lmp_no_flush_capable(hcon->hdev)) > >> + if (hcon->type == LE_LINK) { > >> + /* LE-U does not support auto-flushing packets */ > >> flags = ACL_START_NO_FLUSH; > >> - else > >> - flags = ACL_START; > >> + } else if (!test_bit(FLAG_FLUSHABLE, &chan->flags) && > >> + lmp_no_flush_capable(hcon->hdev)) { > >> + flags = ACL_START_NO_FLUSH; > >> + } > > > > I think Marcel was after just providing a clarifying code comment in > > both places - having two branches of an if-statement doing exactly the > > same thing looks a bit weird to me. To make thins completely clear I'd > > suggest adding a simple helper function that you can call from both > > places to get the needed flags, something like the following: > > I am actually fine with just adding a comment explaining the complex if > statement on why it is correct. It is just a helper for everybody to > understand what and why it is done that way. > Is the comment I added sufficient, or should I add one for the other if condition as well? To me, the second condition is pretty straightforward: if the caller requested it and the hardware supports it, use NO_FLUSH. The relationship between FLUSH/NO_FLUSH and low-energy is much less clear and more justifies a comment, in my opinion. -- -Steven Walter <stevenrwalter@gmail.com> [-- Attachment #2: Type: text/html, Size: 4053 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] Bluetooth: automatically flushable packets aren't allowed on LE links 2014-11-20 15:51 ` Steven Walter @ 2014-11-25 14:53 ` Steven Walter 2014-11-26 5:16 ` Marcel Holtmann 0 siblings, 1 reply; 13+ messages in thread From: Steven Walter @ 2014-11-25 14:53 UTC (permalink / raw) To: Marcel Holtmann Cc: Johan Hedberg, Gustavo F. Padovan, BlueZ development, linux-kernel@vger.kernel.org On Thu, Nov 20, 2014 at 10:51 AM, Steven Walter <stevenrwalter@gmail.com> wrote: >> > I think Marcel was after just providing a clarifying code comment in >> > both places - having two branches of an if-statement doing exactly the >> > same thing looks a bit weird to me. To make thins completely clear I'd >> > suggest adding a simple helper function that you can call from both >> > places to get the needed flags, something like the following: >> >> I am actually fine with just adding a comment explaining the complex if >> statement on why it is correct. It is just a helper for everybody to >> understand what and why it is done that way. > > > Is the comment I added sufficient, or should I add one for the other if > condition as well? To me, the second condition is pretty straightforward: > if the caller requested it and the hardware supports it, use NO_FLUSH. The > relationship between FLUSH/NO_FLUSH and low-energy is much less clear and > more justifies a comment, in my opinion. Did a miss a reply to this? How would you like the next iteration of the patch to look? -- -Steven Walter <stevenrwalter@gmail.com> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] Bluetooth: automatically flushable packets aren't allowed on LE links 2014-11-25 14:53 ` Steven Walter @ 2014-11-26 5:16 ` Marcel Holtmann 2014-11-27 10:14 ` Johan Hedberg 0 siblings, 1 reply; 13+ messages in thread From: Marcel Holtmann @ 2014-11-26 5:16 UTC (permalink / raw) To: Steven Walter Cc: Johan Hedberg, Gustavo F. Padovan, BlueZ development, linux-kernel@vger.kernel.org Hi Steven, >>>> I think Marcel was after just providing a clarifying code comment in >>>> both places - having two branches of an if-statement doing exactly the >>>> same thing looks a bit weird to me. To make thins completely clear I'd >>>> suggest adding a simple helper function that you can call from both >>>> places to get the needed flags, something like the following: >>> >>> I am actually fine with just adding a comment explaining the complex if >>> statement on why it is correct. It is just a helper for everybody to >>> understand what and why it is done that way. >> >> >> Is the comment I added sufficient, or should I add one for the other if >> condition as well? To me, the second condition is pretty straightforward: >> if the caller requested it and the hardware supports it, use NO_FLUSH. The >> relationship between FLUSH/NO_FLUSH and low-energy is much less clear and >> more justifies a comment, in my opinion. > > Did a miss a reply to this? How would you like the next iteration of > the patch to look? can you just send a v4 and I have a look at it. I thing it is best to keep the original patch with the rather complicated if statement you had. And then add a comment in front of it, why it is that way and that it is correct this way. Regards Marcel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] Bluetooth: automatically flushable packets aren't allowed on LE links 2014-11-26 5:16 ` Marcel Holtmann @ 2014-11-27 10:14 ` Johan Hedberg 2014-11-27 15:06 ` Steven Walter 0 siblings, 1 reply; 13+ messages in thread From: Johan Hedberg @ 2014-11-27 10:14 UTC (permalink / raw) To: Marcel Holtmann Cc: Steven Walter, Gustavo F. Padovan, BlueZ development, linux-kernel@vger.kernel.org Hi Marcel, On Wed, Nov 26, 2014, Marcel Holtmann wrote: > >>>> I think Marcel was after just providing a clarifying code comment in > >>>> both places - having two branches of an if-statement doing exactly the > >>>> same thing looks a bit weird to me. To make thins completely clear I'd > >>>> suggest adding a simple helper function that you can call from both > >>>> places to get the needed flags, something like the following: > >>> > >>> I am actually fine with just adding a comment explaining the complex if > >>> statement on why it is correct. It is just a helper for everybody to > >>> understand what and why it is done that way. > >> > >> > >> Is the comment I added sufficient, or should I add one for the other if > >> condition as well? To me, the second condition is pretty straightforward: > >> if the caller requested it and the hardware supports it, use NO_FLUSH. The > >> relationship between FLUSH/NO_FLUSH and low-energy is much less clear and > >> more justifies a comment, in my opinion. > > > > Did a miss a reply to this? How would you like the next iteration of > > the patch to look? > > can you just send a v4 and I have a look at it. I thing it is best to > keep the original patch with the rather complicated if statement you > had. And then add a comment in front of it, why it is that way and > that it is correct this way. Since this is moving way too slow for such a trivial patch I went ahead and added the necessary comments myself and pushed the patch upstream (to bluetooth-next). So no need to send new revisions of this one. Johan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] Bluetooth: automatically flushable packets aren't allowed on LE links 2014-11-27 10:14 ` Johan Hedberg @ 2014-11-27 15:06 ` Steven Walter 0 siblings, 0 replies; 13+ messages in thread From: Steven Walter @ 2014-11-27 15:06 UTC (permalink / raw) To: Marcel Holtmann, Steven Walter, Gustavo F. Padovan, BlueZ development, linux-kernel@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 8 bytes --] Thanks! [-- Attachment #2: Type: text/html, Size: 25 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-11-27 15:06 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-18 14:46 [PATCH] l2cap_core: automatically flushable packets aren't supported by LE-only devices Steven Walter 2014-11-18 22:01 ` Marcel Holtmann 2014-11-19 14:41 ` [PATCH v2] l2cap_core: automatically flushable packets aren't allowed on LE links Steven Walter 2014-11-19 14:48 ` Marcel Holtmann 2014-11-19 17:59 ` [PATCH v3] Bluetooth: " Steven Walter 2014-11-20 11:38 ` Johan Hedberg 2014-11-20 13:50 ` Marcel Holtmann 2014-11-20 14:57 ` Johan Hedberg 2014-11-20 15:51 ` Steven Walter 2014-11-25 14:53 ` Steven Walter 2014-11-26 5:16 ` Marcel Holtmann 2014-11-27 10:14 ` Johan Hedberg 2014-11-27 15:06 ` Steven Walter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).