Hi Marcel, On Wed, Feb 08, 2012 at 06:27:35AM +0100, Marcel Holtmann wrote: > Hello everyone, > > we currently have the problem on our hand that our upstream L2CAP > support is not as good as it should be. And many Android products start > deviating from it to fix issues with. That is of course something we can > not have. Fragmentation on this level is not useful for anyone. It will > hurt everybody in the longterm. So I asked Mat to port over the QUIC > code to the latest upstream so we can start a discussion here. > > https://www.codeaurora.org/gitweb/quic/bluetooth/?p=bluetooth-next.git;a=commit;h=d02ef660d1ec9e9312798561fd688a4c717d339e > > To make one thing perfectly clear here. I want that our upstream code > reflects the state machine transition from the L2CAP specification. I > know that historically this was not the case and that was for a reason. > All early specification where pretty unclear in this area, but with the > introduction of ERTM the Bluetooth SIG got this fixed and now it is our > time to get this clearly into our code as well. > > With the separation of L2CAP channels from L2CAP sockets this should > make it even more easier to use since we do not need and should not to > use socket states anymore anyway. 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. Best regards Andrei Emeltchenko -- 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
