Return-Path: Date: Fri, 3 Sep 2010 14:46:58 -0700 (PDT) From: Mat Martineau To: "Gustavo F. Padovan" cc: haijun liu , "haijun.liu" , linux-bluetooth@vger.kernel.org Subject: Re: [PATCH 16/22] Store new configuration values in l2cap_pinfo In-Reply-To: <20100903210432.GA16973@vigoh> Message-ID: References: <20100903210432.GA16973@vigoh> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; format=flowed; charset=ISO-8859-15 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: 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? -- Mat Martineau Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum