On Thu, Nov 20, 2014 at 8:50 AM, Marcel Holtmann <= marcel@holtmann.or= g> 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,
>>=C2=A0 =C2=A0 =C2=A0 if (!skb)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return;
>>
>> -=C2=A0 =C2=A0 if (lmp_no_flush_capable(conn->hcon->hdev)) >> +=C2=A0 =C2=A0 if (lmp_no_flush_capable(conn->hcon->hdev) ||= conn->hcon->type =3D=3D LE_LINK)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 flags =3D ACL_STAR= T_NO_FLUSH;
>>=C2=A0 =C2=A0 =C2=A0 else
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 flags =3D ACL_STAR= T;
>> @@ -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)
>> {
>>=C2=A0 =C2=A0 =C2=A0 struct hci_conn *hcon =3D chan->conn->hc= on;
>> -=C2=A0 =C2=A0 u16 flags;
>> +=C2=A0 =C2=A0 u16 flags =3D ACL_START;
>>
>>=C2=A0 =C2=A0 =C2=A0 BT_DBG("chan %p, skb %p len %d priority %= u", chan, skb, skb->len,
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0skb->priority);<= br> >> @@ -798,11 +798,13 @@ static void l2cap_do_send(struct l2cap_chan = *chan, struct sk_buff *skb)
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return;
>>=C2=A0 =C2=A0 =C2=A0 }
>>
>> -=C2=A0 =C2=A0 if (!test_bit(FLAG_FLUSHABLE, &chan->flags) = &&
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 lmp_no_flush_capable(hcon->hdev))<= br> >> +=C2=A0 =C2=A0 if (hcon->type =3D=3D LE_LINK) {
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* LE-U does not suppor= t auto-flushing packets */
>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 flags =3D ACL_STAR= T_NO_FLUSH;
>> -=C2=A0 =C2=A0 else
>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 flags =3D ACL_START; >> +=C2=A0 =C2=A0 } else if (!test_bit(FLAG_FLUSHABLE, &chan->= flags) &&
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 lmp_no_fl= ush_capable(hcon->hdev)) {
>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 flags =3D ACL_START_NO_= FLUSH;
>> +=C2=A0 =C2=A0 }
>
> 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&#= 39;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 co= mplex 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 oth= er if condition as well?=C2=A0 To me, the second condition is pretty straig= htforward: if the caller requested it and the hardware supports it, use NO_= FLUSH.=C2=A0 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>

--001a1139126c5e640205084c4d6d--