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
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
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
On Thu, Nov 20, 2014 at 10:51 AM, Steven Walter <[email protected]> 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 <[email protected]>
On Thu, Nov 20, 2014 at 8:50 AM, Marcel Holtmann <[email protected]>
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 <[email protected]>
> >> ---
> >> 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 <[email protected]>
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
Hi Johan,
>> The bluetooth spec states that automatically flushable packets may not
>> be sent over a LE-U link.
>>
>> Signed-off-by: Steven Walter <[email protected]>
>> ---
>> 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
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 <[email protected]>
> ---
> 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
The bluetooth spec states that automatically flushable packets may not
be sent over a LE-U link.
Signed-off-by: Steven Walter <[email protected]>
---
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
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 <[email protected]> wrote:
>
> The bluetooth spec states that automatically flushable packets may not
> be sent over a LE-U link.
>
> Signed-off-by: Steven Walter <[email protected]>
> ---
> 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
The bluetooth spec states that automatically flushable packets may not
be sent over a LE-U link.
Signed-off-by: Steven Walter <[email protected]>
---
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
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