Hi Andrei,
On Fri, Feb 24, 2012 at 7:48 AM, Andrei Emeltchenko
<andrei.emeltchenko.news@xxxxxxxxx> wrote:
> Hi Mat,
>
> It is better to have normal patches for a better review.
> I think that we can minimize amount of changes by redefining defines
> when they cannot be used.
>
> I also think think that patches shall be logically split like:
> - change control field handling
> - working with FCS, etc which do not affect state machine
> - adding states
>
> Also check some comments below: (I copied some code from the link you sent)
>
> On Thu, Feb 23, 2012 at 12:37:48PM -0800, Mat Martineau wrote:
>> This change affects data structures storing ERTM state and control
>> fields, and adds new definitions for states and events. An
>> l2cap_seq_list structure is added for tracking ERTM sequence numbers
>> without repeated memory allocations. Control fields are carried in
>> the bt_skb_cb struct rather than constantly doing shift and mask
>> operations.
>>
>> Signed-off-by: Mat Martineau <mathewm@xxxxxxxxxxxxxx>
>> ---
>> include/net/bluetooth/bluetooth.h | 14 ++-
>> include/net/bluetooth/l2cap.h | 260 +++++++++----------------------------
>> 2 files changed, 73 insertions(+), 201 deletions(-)
>
> ...
>
>> -static inline int l2cap_tx_window_full(struct l2cap_chan *ch)
>> -{
>> - int sub;
>> -
>> - sub = (ch->next_tx_seq - ch->expected_ack_seq) % 64;
>> -
>> - if (sub < 0)
>> - sub += 64;
>> -
>> - return sub == ch->remote_tx_win;
>> -}
>
> BTW: was it already changed? What is the status with Luiz's patch?
>
> ...
>
>> -static inline __u32 __get_control(struct l2cap_chan *chan, void *p)
>> -{
>> - if (test_bit(FLAG_EXT_CTRL, &chan->flags))
>> - return get_unaligned_le32(p);
>> - else
>> - return get_unaligned_le16(p);
>> -}
>
> Cannot it still be used?
>
> + if (test_bit(FLAG_EXT_CTRL, &chan->flags)) {
> + __get_extended_control(get_unaligned_le32(skb->data),
> + control);
> + skb_pull(skb, L2CAP_EXT_CTRL_SIZE);
> + } else {
> + __get_enhanced_control(get_unaligned_le16(skb->data),
> + control);
> + skb_pull(skb, L2CAP_ENH_CTRL_SIZE);
> + }
>
> - control = __get_control(chan, skb->data);
> - skb_pull(skb, __ctrl_size(chan));
>
> ...
>
>> -static inline void __put_control(struct l2cap_chan *chan, __u32 control,
>> - void *p)
>> -{
>> - if (test_bit(FLAG_EXT_CTRL, &chan->flags))
>> - return put_unaligned_le32(control, p);
>> - else
>> - return put_unaligned_le16(control, p);
>> -}
>
> Can it be used in the code below:
>
> + if (test_bit(FLAG_EXT_CTRL, &chan->flags)) {
> + put_unaligned_le32(__pack_extended_control(control),
> + skb->data + L2CAP_HDR_SIZE);
> + } else {
> + put_unaligned_le16(__pack_enhanced_control(control),
> + skb->data + L2CAP_HDR_SIZE);
> + }
>
> and for example here:
>
> - __put_control(chan, control, skb_put(skb, __ctrl_size(chan)));
> + /* Control header is populated later */
> + if (test_bit(FLAG_EXT_CTRL, &chan->flags))
> + put_unaligned_le32(0, skb_put(skb, 4));
> + else
> + put_unaligned_le16(0, skb_put(skb, 2));
>
>
>> -
>> -static inline __u8 __ctrl_size(struct l2cap_chan *chan)
>> -{
>> - if (test_bit(FLAG_EXT_CTRL, &chan->flags))
>> - return L2CAP_EXT_HDR_SIZE - L2CAP_HDR_SIZE;
>> - else
>> - return L2CAP_ENH_HDR_SIZE - L2CAP_HDR_SIZE;
>> -}
Do we have that many places with this test in Mat's code? If not, then
we might not need to bother having all of these helpers, I think. And
if we add them, I do think it makes sense to add them to l2cap_core.c
than in l2cap.h, right?
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
