Hi everybody,
On Wed, Feb 8, 2012 at 7:32 AM, Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
> Hi Andrei,
>
> On Wed, Feb 8, 2012 at 11:09 AM, Andrei Emeltchenko
> <andrei.emeltchenko.news@xxxxxxxxx> 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<-----------------------------------------------------------------
>> | -#define L2CAP_EXT_CTRL_TXSEQ 0xFFFC0000
>> | #define L2CAP_EXT_CTRL_SAR 0x00030000
>> | -#define L2CAP_EXT_CTRL_SUPERVISE 0x00030000
>> | #define L2CAP_EXT_CTRL_REQSEQ 0x0000FFFC
>> | -
>> | -#define L2CAP_EXT_CTRL_POLL 0x00040000
>> | +#define L2CAP_EXT_CTRL_TXSEQ 0xFFFC0000
>> | #define L2CAP_EXT_CTRL_FINAL 0x00000002
>> | +#define L2CAP_EXT_CTRL_POLL 0x00040000
>> | +#define L2CAP_EXT_CTRL_SUPERVISE 0x00030000
>> | #define L2CAP_EXT_CTRL_FRAME_TYPE 0x00000001 /* I- or S-Frame */
>> <------8<-----------------------------------------------------------------
>>
>> and I don't like this kind of change:
>>
>> <------8<--------------------------------------------------------------------
>> | - if (__is_sar_start(chan, control) && !__is_sframe(chan, control))
>> | - len -= L2CAP_SDULEN_SIZE;
>> | + if ((control->frame_type == 'i') &&
>> | + (control->sar == L2CAP_SAR_START))
>> | + len -= 2;
>> |
>> | if (chan->fcs == L2CAP_FCS_CRC16)
>> | - len -= L2CAP_FCS_SIZE;
>> | + len -= 2;
>> <------8<--------------------------------------------------------------------
>>
>> why not to use macros and defines for magic numbers?
>>
>> the same below:
>>
>> <------8<----------------------------------------------------------------
>> | + if (test_bit(FLAG_EXT_CTRL, &chan->flags)) {
>> | + __get_extended_control(get_unaligned_le32(skb->data),
>> | + control);
>> | + skb_pull(skb, 4);
>> | + } else {
>> | + __get_enhanced_control(get_unaligned_le16(skb->data),
>> | + control);
>> | + skb_pull(skb, 2);
>> | + }
>> |
>> | - control = __get_control(chan, skb->data);
>> | - skb_pull(skb, __ctrl_size(chan));
>> <------8<----------------------------------------------------------------
>>
>> those magic number does not look nice IMO and the code is not looking any
>> 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,
--
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
