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
<andrei.emeltchenko.news@xxxxxxxxx> wrote:
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.
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?
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.
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?
I'm not sure about the patch status - but I do know this function isn't
needed with the new ERTM code.
...
-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.
+ 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);
+ }
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!
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.
-
-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
--
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
