Return-Path: Date: Fri, 3 Sep 2010 18:49:56 -0300 From: "Gustavo F. Padovan" To: Mat Martineau Cc: haijun liu , "haijun.liu" , linux-bluetooth@vger.kernel.org Subject: Re: [PATCH 16/22] Store new configuration values in l2cap_pinfo Message-ID: <20100903214956.GB16973@vigoh> References: <20100903210432.GA16973@vigoh> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 In-Reply-To: Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Mat, * Mat Martineau [2010-09-03 14:46:58 -0700]: > > Gustavo - > > On Fri, 3 Sep 2010, Gustavo F. Padovan wrote: > > > Hi Mat, > > > > * Mat Martineau [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 wrote: > >>>> > >>>> On Thu, 26 Aug 2010, haijun liu wrote: > >>>> > >>>>> From d093975dde6d85c824a5aaac943d676100810010 Mon Sep 17 00:00:00 2001 > >>>>> From: haijun.liu > >>>>> 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. > > It looks like conf_state is not currently used once the connection is > made. The extended_control information is accessed when parsing > headers in every incoming PDU and generating every outgoing PDU. > Usage is a lot like l2cap_pinfo.fcs > > If you still want to squeeze this extended control bit in an existing > l2cap_pinfo member, would conn_state be better? Yes, this one is better. -- Gustavo F. Padovan ProFUSION embedded systems - http://profusion.mobi