Re: [RFC 1/2] Bluetooth: Header changes for ERTM state machine replacement

Andrei and Ulisses -

On 2/24/2012 9:42 AM, Ulisses Furquim wrote:

Hi Andrei,

On Fri, Feb 24, 2012 at 7:48 AM, Andrei Emeltchenko
[off-list ref]  wrote:

quoted

Hi Mat,

It is better to have normal patches for a better review.

I would have preferred to have more "normal" patches, but I didn't want 
to keep you guys waiting another week.

quoted

I think that we can minimize amount of changes by redefining defines
when they cannot be used.

Do you mean that it would be better to modify existing macros rather 
than remove them?

quoted

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

Sounds like a good way to divide things up.

quoted

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:

quoted

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<redacted>
---
  include/net/bluetooth/bluetooth.h |   14 ++-
  include/net/bluetooth/l2cap.h     |  260 +++++++++----------------------------
  2 files changed, 73 insertions(+), 201 deletions(-)

...

quoted

-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?

I'm not sure about the patch status - but I do know this function isn't 
needed with the new ERTM code.

quoted

...

quoted

-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?

It's not needed.  The control header is only parsed in one place now at 
the beginning of l2cap_ertm_data_rcv(), and this macro isn't a good fit 
for the code there.

quoted

+       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));

...

quoted

-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);
+               }

Since the __pack function differs based on the extended/enhanced header 
type, it doesn't seem worthwhile to check FLAG_EXT_CTRL twice.

The code I'm porting in predates the __put_control macro, so I didn't 
make an effort to remove it.  In order to avoid breaking things, I'm 
trying to only change the ported code where required.  I'm open to 
making this code better, though!

quoted

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));

This is the only place the macro would drop in cleanly - but it seems 
like a macro should be used in more than one place unless it's hiding 
some really distracting syntax.  In this case, I think it's clearer to 
see what's going on.

And - I forgot to fix some magic numbers in those skb_puts.

quoted

quoted

-
-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?

Not too many places with the FLAG_EXT_CTRL test, with use of struct 
bt_control in most of the code.  I would rather have helpers in 
l2cap_core.c if they're needed.


Thanks for the reviews!

-- 
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum