Re: Getting L2CAP ERTM support into better upstream state

[Ulisses Furquim <ulisses@profusion.mobi>]

Hi everybody,

On Wed, Feb 8, 2012 at 7:32 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Andrei,
>
> On Wed, Feb 8, 2012 at 11:09 AM, Andrei Emeltchenko
> <andrei.emeltchenko.news@gmail.com> wrote:
>>
>> I think this would be good to send as patch series so that people can
>> comment. What comes to my mind is that the patch might be reduced if it
>> does not change order of functions and defines like:
>>
>> <------8<---------------------------------------------------------------=
--
>> | =A0-#define L2CAP_EXT_CTRL_TXSEQ =A0 =A0 =A0 =A0 =A0 0xFFFC0000
>> | =A0 #define L2CAP_EXT_CTRL_SAR =A0 =A0 =A0 =A0 =A0 =A0 0x00030000
>> | =A0-#define L2CAP_EXT_CTRL_SUPERVISE =A0 =A0 =A0 0x00030000
>> | =A0 #define L2CAP_EXT_CTRL_REQSEQ =A0 =A0 =A0 =A0 =A00x0000FFFC
>> | =A0-
>> | =A0-#define L2CAP_EXT_CTRL_POLL =A0 =A0 =A0 =A0 =A0 =A00x00040000
>> | =A0+#define L2CAP_EXT_CTRL_TXSEQ =A0 =A0 =A0 =A0 0xFFFC0000
>> | =A0 #define L2CAP_EXT_CTRL_FINAL =A0 =A0 =A0 =A0 =A0 0x00000002
>> | =A0+#define L2CAP_EXT_CTRL_POLL =A0 =A0 =A0 =A0 =A00x00040000
>> | =A0+#define L2CAP_EXT_CTRL_SUPERVISE =A0 =A0 0x00030000
>> | =A0 #define L2CAP_EXT_CTRL_FRAME_TYPE =A0 =A0 =A00x00000001 /* I- or S=
-Frame */
>> <------8<---------------------------------------------------------------=
--
>>
>> and I don't like this kind of change:
>>
>> <------8<---------------------------------------------------------------=
-----
>> | =A0- =A0 =A0 =A0 if (__is_sar_start(chan, control) && !__is_sframe(cha=
n, control))
>> | =A0- =A0 =A0 =A0 =A0 =A0 =A0 =A0 len -=3D L2CAP_SDULEN_SIZE;
>> | =A0+ =A0 =A0 =A0 if ((control->frame_type =3D=3D 'i') &&
>> | =A0+ =A0 =A0 =A0 =A0 =A0 (control->sar =3D=3D L2CAP_SAR_START))
>> | =A0+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 len -=3D 2;
>> |
>> | =A0 =A0 =A0 =A0 =A0if (chan->fcs =3D=3D L2CAP_FCS_CRC16)
>> | =A0- =A0 =A0 =A0 =A0 =A0 =A0 =A0 len -=3D L2CAP_FCS_SIZE;
>> | =A0+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 len -=3D 2;
>> <------8<---------------------------------------------------------------=
-----
>>
>> why not to use macros and defines for magic numbers?
>>
>> the same below:
>>
>> <------8<---------------------------------------------------------------=
-
>> | =A0+ =A0 =A0 =A0 if (test_bit(FLAG_EXT_CTRL, &chan->flags)) {
>> | =A0+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 __get_extended_control(get_unaligned_=
le32(skb->data),
>> | =A0+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0control);
>> | =A0+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 skb_pull(skb, 4);
>> | =A0+ =A0 =A0 =A0 } else {
>> | =A0+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 __get_enhanced_control(get_unaligned_=
le16(skb->data),
>> | =A0+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0control);
>> | =A0+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 skb_pull(skb, 2);
>> | =A0+ =A0 =A0 =A0 }
>> |
>> | =A0- =A0 =A0 =A0 control =3D __get_control(chan, skb->data);
>> | =A0- =A0 =A0 =A0 skb_pull(skb, __ctrl_size(chan));
>> <------8<---------------------------------------------------------------=
-
>>
>> those magic number does not look nice IMO and the code is not looking an=
y
>> better.
>
> In this aspect perhaps, but we don't need to take the code as it is,
> but the point here is following the states defined by the spec and
> that is IMO much better.

I've taken a quick look last week at their code and indeed it looks
better regarding following the states. In particular they track
rx_state and tx_state and then decide what to do based on that and
what happened. I like it because it's explicit about that instead of
demanding us to reason a lot on what state we are and what we should
do. I'm all for changing our ERTM to that so it'll be more
maintainable.

Marcel mentioned the separation of L2CAP channel and socket. That is a
work in progress by Andrei and judging by what you said, Marcel, you
want that merged before we change ERTM, is that it?

Mat, thanks a lot for doing this. I have just a few questions. Have
you tested the stack with this patch? How was it? Quickly looking
through the code I feel it's almost there. I agree with Andrei
regarding constants and control field handling but apart from that it
looks good, in general.

Regarding delayed work handling, are you sure about usage of
__cancel_delayed_work()? I do think it's safer to use
cancel_delayed_work() instead as it'll just spin on a lock if the
timer to queue the work is running on another CPU.

I saw a comment about channel ref counting and kind of audit that
would be great. It'd be good to see a patch for that after transition
to new state handling.

Best regards,

--=20
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs