Re: [PATCH 16/22] Store new configuration values in l2cap_pinfo

["Gustavo F. Padovan" <padovan@profusion.mobi>]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2010-09-03 13:58:46 -0700]:

> 
> On Fri, 3 Sep 2010, haijun liu wrote:
> 
> > On Fri, Sep 3, 2010 at 7:02 AM, Mat Martineau <mathewm@codeaurora.org> wrote:
> >>
> >> On Thu, 26 Aug 2010, haijun liu wrote:
> >>
> >>> From d093975dde6d85c824a5aaac943d676100810010 Mon Sep 17 00:00:00 2001
> >>> From: haijun.liu <haijun.liu@atheros.com>
> >>> Date: Mon, 23 Aug 2010 00:09:56 +0800
> >>> Subject: [PATCH 16/22] Store new configuration values in l2cap_pinfo.
> >>>
> >>> ---
> >>> include/net/bluetooth/l2cap.h |    9 +++++++++
> >>> 1 files changed, 9 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> >>> index 2d864d4..f2dd65d 100644
> >>> --- a/include/net/bluetooth/l2cap.h
> >>> +++ b/include/net/bluetooth/l2cap.h
> >>> @@ -406,6 +406,15 @@ struct l2cap_pinfo {
> >>>        __u16           remote_mps;
> >>>        __u16           mps;
> >>>
> >>> +       __u8            ext_flowspec_enable;
> >>> +       struct ext_flow_spec    loc_efs;
> >>> +       struct ext_flow_spec    rem_efs;
> >>> +
> >>> +       __u8            extwin_enable;
> >>> +       __u16           extwin_size;
> >>> +       __u8            rem_extwin_enable;
> >>> +       __u16           rem_extwin_size;
> >>> +
> >>>        __le16          sport;
> >>>
> >>>        struct timer_list       retrans_timer;
> >>
> >> Regarding the new "extwin" structure members, have you considered changing
> >> the existing tx_win and remote_tx_win members to __u16 and using them with
> >> both standard and extended window sizes?
> >>
> >> The spec also requires that both directions of the link use the same type of
> >> control field (standard or extended).  After L2CAP configuration is done,
> >> all the information required for the transmit window is the control field
> >> type, tx_win, and remote_tx_win.  The control field would be set to
> >> 'extended' if a successful configuration response is sent or received for
> >> the extended window size option.
> >
> > Yes, we do, please look into the patch, we use __u16 for extwin_size &
> > rem_extwin_size
> > +       __u8            extwin_enable;
> > +       __u16           extwin_size;
> > +       __u8            rem_extwin_enable;
> > +       __u16           rem_extwin_size;
> >
> > You are exactly right, in our implementation, choosing standard or 
> > extended window, it depends whether successful configuration 
> > response contain the extended window size option.
> 
> Haijin -
> 
> Thank you for your explanation.  I was trying to suggest something 
> different - sorry I did not explain myself well.
> 
> I think that extwin_enable, extwin_size, rem_extwin_enable, and 
> rem_extwin_size are not needed in l2cap_pinfo.  Instead, I suggest 
> this:
> 
> @@ -349,15 +349,17 @@ struct l2cap_pinfo {
> 
>          __u8            ident;
> 
> -       __u8            tx_win;
> +       __u16           tx_win;
>          __u8            max_tx;
> -       __u8            remote_tx_win;
> +       __u16           remote_tx_win;
>          __u8            remote_max_tx;
>          __u16           retrans_timeout;
>          __u16           monitor_timeout;
>          __u16           remote_mps;
>          __u16           mps;
> 
> +       __u8            extended_control;
> +
>          __le16          sport;
> 
>          struct timer_list       retrans_timer;
> 
> Here, tx_win and remote_tx_win are always used to record the window 
> size, whether extended or standard.  extended_control is used to 
> specify standard or extended control fields.

Then you don't need extended_control here, a bit in the conf_state field
should be enough.

-- 
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi