[PATCH 0/8] Bluetooth: L2CAP updates for PSM validation and ERTM.

[PATCH 0/8] Bluetooth: L2CAP updates for PSM validation and ERTM.

[Mat Martineau]

This is a collection of ERTM-related patches, including some L2CAP 
configuration fixes and allowing partial-frame reads from L2CAP
SOCK_STREAM sockets.  PSM validation is also included, although that
is not restricted to ERTM.

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

[PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 9ba1e8e..aed72f2 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		goto unlock;
 
 	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
-		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
-		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
+		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
+			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
+			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
+			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
 			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
 
 		sk->sk_state = BT_CONNECTED;
@@ -3217,8 +3219,10 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	l2cap_pi(sk)->conf_state |= L2CAP_CONF_INPUT_DONE;
 
 	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_OUTPUT_DONE) {
-		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
-		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
+		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
+			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
+			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
+			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
 			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
 
 		sk->sk_state = BT_CONNECTED;
-- 
1.7.1

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

Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |   12 ++++++++----
>  1 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 9ba1e8e..aed72f2 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>  		goto unlock;
>  
>  	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
> -		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> -		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
> +		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
> +			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
> +			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> +			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
>  			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;

this becomes unreadable and my brain starts to throw a core dump. So it
clearly needs to be put into a helper inline function.

Regards

Marcel

Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.

[Gustavo F. Padovan]

Hi Mat,

* Marcel Holtmann <marcel@holtmann.org> [2010-08-02 12:38:32 -0700]:

> Hi Mat,
> 
> > Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> > ---
> >  net/bluetooth/l2cap.c |   12 ++++++++----
> >  1 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > index 9ba1e8e..aed72f2 100644
> > --- a/net/bluetooth/l2cap.c
> > +++ b/net/bluetooth/l2cap.c
> > @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> >  		goto unlock;
> >  
> >  	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
> > -		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> > -		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
> > +		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
> > +			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
> > +			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> > +			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
> >  			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
> 
> this becomes unreadable and my brain starts to throw a core dump. So it
> clearly needs to be put into a helper inline function.

Actually we don't need that, since the code that deals with Basic Mode
never check  and use the l2cap_pi(sk)->fcs. So we don't care about FCS
value in the Basic Mode.


-- 
Gustavo F. Padovan
http://padovan.org

Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.

[Mat Martineau]

Hi Gustavo -

On Mon, 2 Aug 2010, Gustavo F. Padovan wrote:

> Hi Mat,
>
> * Marcel Holtmann <marcel@holtmann.org> [2010-08-02 12:38:32 -0700]:
>
>> Hi Mat,
>>
>>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>>> ---
>>>  net/bluetooth/l2cap.c |   12 ++++++++----
>>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>>> index 9ba1e8e..aed72f2 100644
>>> --- a/net/bluetooth/l2cap.c
>>> +++ b/net/bluetooth/l2cap.c
>>> @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>>>  		goto unlock;
>>>
>>>  	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
>>> -		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
>>> -		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
>>> +		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
>>> +			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
>>> +			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
>>> +			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
>>>  			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
>>
>> this becomes unreadable and my brain starts to throw a core dump. So it
>> clearly needs to be put into a helper inline function.
>
> Actually we don't need that, since the code that deals with Basic Mode
> never check  and use the l2cap_pi(sk)->fcs. So we don't care about FCS
> value in the Basic Mode.

There isn't currently any Basic Mode code that triggers this latent 
bug, but I have a patch coming up that does require this fix.

As it stands, getsockopt() on a connected basic mode socket shows FCS 
enabled, so this bug is visible from userspace.

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

Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.

[Marcel Holtmann]

Hi Mat,

> > * Marcel Holtmann <marcel@holtmann.org> [2010-08-02 12:38:32 -0700]:
> >
> >> Hi Mat,
> >>
> >>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> >>> ---
> >>>  net/bluetooth/l2cap.c |   12 ++++++++----
> >>>  1 files changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> >>> index 9ba1e8e..aed72f2 100644
> >>> --- a/net/bluetooth/l2cap.c
> >>> +++ b/net/bluetooth/l2cap.c
> >>> @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> >>>  		goto unlock;
> >>>
> >>>  	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
> >>> -		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>> -		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
> >>> +		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
> >>> +			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
> >>> +			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>> +			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
> >>>  			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
> >>
> >> this becomes unreadable and my brain starts to throw a core dump. So it
> >> clearly needs to be put into a helper inline function.
> >
> > Actually we don't need that, since the code that deals with Basic Mode
> > never check  and use the l2cap_pi(sk)->fcs. So we don't care about FCS
> > value in the Basic Mode.
> 
> There isn't currently any Basic Mode code that triggers this latent 
> bug, but I have a patch coming up that does require this fix.
> 
> As it stands, getsockopt() on a connected basic mode socket shows FCS 
> enabled, so this bug is visible from userspace.

can we just fail the setsockopt() when trying to set basic mode and FCS
off.

And also in case fallback to basic mode happens, then FCS should be set
to be enabled. Since for FCS and basic mode we always have to use FCS.
So that seems just fine to me.

Maybe you need to explain a bit more in detail what you are trying to
achieve in conjunction with userspace API.

Regards

Marcel

Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.

[Mat Martineau]

On Mon, 2 Aug 2010, Marcel Holtmann wrote:

> Hi Mat,
>
>>> * Marcel Holtmann <marcel@holtmann.org> [2010-08-02 12:38:32 -0700]:
>>>
>>>> Hi Mat,
>>>>
>>>>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>>>>> ---
>>>>>  net/bluetooth/l2cap.c |   12 ++++++++----
>>>>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>>>>> index 9ba1e8e..aed72f2 100644
>>>>> --- a/net/bluetooth/l2cap.c
>>>>> +++ b/net/bluetooth/l2cap.c
>>>>> @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>>>>>  		goto unlock;
>>>>>
>>>>>  	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
>>>>> -		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
>>>>> -		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
>>>>> +		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
>>>>> +			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
>>>>> +			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
>>>>> +			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
>>>>>  			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
>>>>
>>>> this becomes unreadable and my brain starts to throw a core dump. So it
>>>> clearly needs to be put into a helper inline function.
>>>
>>> Actually we don't need that, since the code that deals with Basic Mode
>>> never check  and use the l2cap_pi(sk)->fcs. So we don't care about FCS
>>> value in the Basic Mode.
>>
>> There isn't currently any Basic Mode code that triggers this latent
>> bug, but I have a patch coming up that does require this fix.
>>
>> As it stands, getsockopt() on a connected basic mode socket shows FCS
>> enabled, so this bug is visible from userspace.
>
> can we just fail the setsockopt() when trying to set basic mode and FCS
> off.

It definitely makes sense to have more validation of L2CAP_OPTIONS 
passed to setsockopt().

> And also in case fallback to basic mode happens, then FCS should be set
> to be enabled. Since for FCS and basic mode we always have to use FCS.
> So that seems just fine to me.

The spec says "The FCS option shall only be used when the mode is 
being, or is already configured to Enhanced Retransmission mode or 
Streaming mode."  FCS is never used in basic mode (fallback or not).

(Maybe I've misunderstood your point)

> Maybe you need to explain a bit more in detail what you are trying to
> achieve in conjunction with userspace API.

My goal is to only have l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 when the 
FCS option is actually in use.  Otherwise, any logic checking for FCS 
also has to check the L2CAP mode.  Might as well check the mode once 
and set fcs accordingly -- which is what my patch does.

Gustavo is correct that l2cap_pi(sk)->fcs is currently only checked on 
code paths used with ERTM and streaming mode.  However, future code 
(including a patch I'll be posting soon) will depend on the fcs value 
being accurate in all modes.

I only mentioned getsockopt() to show that this issue is not 
completely invisible, and is worth patching.


Regards,

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

Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.

[Marcel Holtmann]

Hi mat,

> >>>>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> >>>>> ---
> >>>>>  net/bluetooth/l2cap.c |   12 ++++++++----
> >>>>>  1 files changed, 8 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> >>>>> index 9ba1e8e..aed72f2 100644
> >>>>> --- a/net/bluetooth/l2cap.c
> >>>>> +++ b/net/bluetooth/l2cap.c
> >>>>> @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> >>>>>  		goto unlock;
> >>>>>
> >>>>>  	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
> >>>>> -		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>>>> -		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
> >>>>> +		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
> >>>>> +			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
> >>>>> +			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>>>> +			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
> >>>>>  			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
> >>>>
> >>>> this becomes unreadable and my brain starts to throw a core dump. So it
> >>>> clearly needs to be put into a helper inline function.
> >>>
> >>> Actually we don't need that, since the code that deals with Basic Mode
> >>> never check  and use the l2cap_pi(sk)->fcs. So we don't care about FCS
> >>> value in the Basic Mode.
> >>
> >> There isn't currently any Basic Mode code that triggers this latent
> >> bug, but I have a patch coming up that does require this fix.
> >>
> >> As it stands, getsockopt() on a connected basic mode socket shows FCS
> >> enabled, so this bug is visible from userspace.
> >
> > can we just fail the setsockopt() when trying to set basic mode and FCS
> > off.
> 
> It definitely makes sense to have more validation of L2CAP_OPTIONS 
> passed to setsockopt().
> 
> > And also in case fallback to basic mode happens, then FCS should be set
> > to be enabled. Since for FCS and basic mode we always have to use FCS.
> > So that seems just fine to me.
> 
> The spec says "The FCS option shall only be used when the mode is 
> being, or is already configured to Enhanced Retransmission mode or 
> Streaming mode."  FCS is never used in basic mode (fallback or not).
> 
> (Maybe I've misunderstood your point)

So basic mode uses CRC16 as FCS. And with basic that is not changeable.
If you try basic mode + no FCS then we should clearly fail the call to
setsockopt() for that.

> > Maybe you need to explain a bit more in detail what you are trying to
> > achieve in conjunction with userspace API.
> 
> My goal is to only have l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 when the 
> FCS option is actually in use.  Otherwise, any logic checking for FCS 
> also has to check the L2CAP mode.  Might as well check the mode once 
> and set fcs accordingly -- which is what my patch does.
> 
> Gustavo is correct that l2cap_pi(sk)->fcs is currently only checked on 
> code paths used with ERTM and streaming mode.  However, future code 
> (including a patch I'll be posting soon) will depend on the fcs value 
> being accurate in all modes.

As I said, I have no problem with returning basic mode + crc16. Since
that is actually what you are doing in that case.

The only thing that we have to keep in mind to be backward compatible is
to allow setting of basic mode and fcs = 0x00 and then change that into
crc16.

Regards

Marcel

Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.

[Mat Martineau]

Hi Marcel -

On Tue, 3 Aug 2010, Marcel Holtmann wrote:

>>>>>>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>>>>>>> ---
>>>>>>>  net/bluetooth/l2cap.c |   12 ++++++++----
>>>>>>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>>>>>>> index 9ba1e8e..aed72f2 100644
>>>>>>> --- a/net/bluetooth/l2cap.c
>>>>>>> +++ b/net/bluetooth/l2cap.c
>>>>>>> @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>>>>>>>  		goto unlock;
>>>>>>>
>>>>>>>  	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
>>>>>>> -		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
>>>>>>> -		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
>>>>>>> +		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
>>>>>>> +			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
>>>>>>> +			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
>>>>>>> +			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
>>>>>>>  			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
>>>>>>
>>>>>> this becomes unreadable and my brain starts to throw a core dump. So it
>>>>>> clearly needs to be put into a helper inline function.
>>>>>
>>>>> Actually we don't need that, since the code that deals with Basic Mode
>>>>> never check  and use the l2cap_pi(sk)->fcs. So we don't care about FCS
>>>>> value in the Basic Mode.
>>>>
>>>> There isn't currently any Basic Mode code that triggers this latent
>>>> bug, but I have a patch coming up that does require this fix.
>>>>
>>>> As it stands, getsockopt() on a connected basic mode socket shows FCS
>>>> enabled, so this bug is visible from userspace.
>>>
>>> can we just fail the setsockopt() when trying to set basic mode and FCS
>>> off.
>>
>> It definitely makes sense to have more validation of L2CAP_OPTIONS
>> passed to setsockopt().
>>
>>> And also in case fallback to basic mode happens, then FCS should be set
>>> to be enabled. Since for FCS and basic mode we always have to use FCS.
>>> So that seems just fine to me.
>>
>> The spec says "The FCS option shall only be used when the mode is
>> being, or is already configured to Enhanced Retransmission mode or
>> Streaming mode."  FCS is never used in basic mode (fallback or not).
>>
>> (Maybe I've misunderstood your point)
>
> So basic mode uses CRC16 as FCS. And with basic that is not changeable.
> If you try basic mode + no FCS then we should clearly fail the call to
> setsockopt() for that.

That's the opposite of what I'm trying to say! :)

Basic mode B-frames have *no* FCS (L2CAP spec section 3.1).  The FCS 
field is only found in I-frames and S-frames used in ERTM and 
streaming modes.  If you try basic mode + FCS, that should fail.

>>> Maybe you need to explain a bit more in detail what you are trying to
>>> achieve in conjunction with userspace API.
>>
>> My goal is to only have l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 when the
>> FCS option is actually in use.  Otherwise, any logic checking for FCS
>> also has to check the L2CAP mode.  Might as well check the mode once
>> and set fcs accordingly -- which is what my patch does.
>>
>> Gustavo is correct that l2cap_pi(sk)->fcs is currently only checked on
>> code paths used with ERTM and streaming mode.  However, future code
>> (including a patch I'll be posting soon) will depend on the fcs value
>> being accurate in all modes.
>
> As I said, I have no problem with returning basic mode + crc16. Since
> that is actually what you are doing in that case.
>
> The only thing that we have to keep in mind to be backward compatible is
> to allow setting of basic mode and fcs = 0x00 and then change that into
> crc16.

The fcs field in L2CAP_OPTIONS is new for ERTM/Streaming modes, so 
there should be no backward compatibility issues with basic mode.


Regards,

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

Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.

[Marcel Holtmann]

Hi Mat,

> >>>>>>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> >>>>>>> ---
> >>>>>>>  net/bluetooth/l2cap.c |   12 ++++++++----
> >>>>>>>  1 files changed, 8 insertions(+), 4 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> >>>>>>> index 9ba1e8e..aed72f2 100644
> >>>>>>> --- a/net/bluetooth/l2cap.c
> >>>>>>> +++ b/net/bluetooth/l2cap.c
> >>>>>>> @@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> >>>>>>>  		goto unlock;
> >>>>>>>
> >>>>>>>  	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
> >>>>>>> -		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>>>>>> -		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
> >>>>>>> +		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
> >>>>>>> +			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
> >>>>>>> +			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>>>>>> +			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
> >>>>>>>  			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
> >>>>>>
> >>>>>> this becomes unreadable and my brain starts to throw a core dump. So it
> >>>>>> clearly needs to be put into a helper inline function.
> >>>>>
> >>>>> Actually we don't need that, since the code that deals with Basic Mode
> >>>>> never check  and use the l2cap_pi(sk)->fcs. So we don't care about FCS
> >>>>> value in the Basic Mode.
> >>>>
> >>>> There isn't currently any Basic Mode code that triggers this latent
> >>>> bug, but I have a patch coming up that does require this fix.
> >>>>
> >>>> As it stands, getsockopt() on a connected basic mode socket shows FCS
> >>>> enabled, so this bug is visible from userspace.
> >>>
> >>> can we just fail the setsockopt() when trying to set basic mode and FCS
> >>> off.
> >>
> >> It definitely makes sense to have more validation of L2CAP_OPTIONS
> >> passed to setsockopt().
> >>
> >>> And also in case fallback to basic mode happens, then FCS should be set
> >>> to be enabled. Since for FCS and basic mode we always have to use FCS.
> >>> So that seems just fine to me.
> >>
> >> The spec says "The FCS option shall only be used when the mode is
> >> being, or is already configured to Enhanced Retransmission mode or
> >> Streaming mode."  FCS is never used in basic mode (fallback or not).
> >>
> >> (Maybe I've misunderstood your point)
> >
> > So basic mode uses CRC16 as FCS. And with basic that is not changeable.
> > If you try basic mode + no FCS then we should clearly fail the call to
> > setsockopt() for that.
> 
> That's the opposite of what I'm trying to say! :)
> 
> Basic mode B-frames have *no* FCS (L2CAP spec section 3.1).  The FCS 
> field is only found in I-frames and S-frames used in ERTM and 
> streaming modes.  If you try basic mode + FCS, that should fail.

I confused myself right now. You are fully correct. The default for
basic mode is no fcs.

And as soon as an application wants to use ERTM we just make it to
specific the FCS option. That being CRC16 or no FCS. And of course the
getsockopt() call will return whatever we ended up configuring.

And yes, basic mode + crc16 should fail.

> >>> Maybe you need to explain a bit more in detail what you are trying to
> >>> achieve in conjunction with userspace API.
> >>
> >> My goal is to only have l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 when the
> >> FCS option is actually in use.  Otherwise, any logic checking for FCS
> >> also has to check the L2CAP mode.  Might as well check the mode once
> >> and set fcs accordingly -- which is what my patch does.
> >>
> >> Gustavo is correct that l2cap_pi(sk)->fcs is currently only checked on
> >> code paths used with ERTM and streaming mode.  However, future code
> >> (including a patch I'll be posting soon) will depend on the fcs value
> >> being accurate in all modes.
> >
> > As I said, I have no problem with returning basic mode + crc16. Since
> > that is actually what you are doing in that case.
> >
> > The only thing that we have to keep in mind to be backward compatible is
> > to allow setting of basic mode and fcs = 0x00 and then change that into
> > crc16.
> 
> The fcs field in L2CAP_OPTIONS is new for ERTM/Streaming modes, so 
> there should be no backward compatibility issues with basic mode.

There is none. I got confused. Too many emails at the same time.

Regards

Marcel

Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.

[Gustavo F. Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2010-08-03 08:58:57 -0700]:

> 
> On Mon, 2 Aug 2010, Marcel Holtmann wrote:
> 
> >Hi Mat,
> >
> >>>* Marcel Holtmann <marcel@holtmann.org> [2010-08-02 12:38:32 -0700]:
> >>>
> >>>>Hi Mat,
> >>>>
> >>>>>Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> >>>>>---
> >>>>> net/bluetooth/l2cap.c |   12 ++++++++----
> >>>>> 1 files changed, 8 insertions(+), 4 deletions(-)
> >>>>>
> >>>>>diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> >>>>>index 9ba1e8e..aed72f2 100644
> >>>>>--- a/net/bluetooth/l2cap.c
> >>>>>+++ b/net/bluetooth/l2cap.c
> >>>>>@@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> >>>>> 		goto unlock;
> >>>>>
> >>>>> 	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
> >>>>>-		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>>>>-		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
> >>>>>+		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
> >>>>>+			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
> >>>>>+			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
> >>>>>+			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
> >>>>> 			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
> >>>>
> >>>>this becomes unreadable and my brain starts to throw a core dump. So it
> >>>>clearly needs to be put into a helper inline function.
> >>>
> >>>Actually we don't need that, since the code that deals with Basic Mode
> >>>never check  and use the l2cap_pi(sk)->fcs. So we don't care about FCS
> >>>value in the Basic Mode.
> >>
> >>There isn't currently any Basic Mode code that triggers this latent
> >>bug, but I have a patch coming up that does require this fix.
> >>
> >>As it stands, getsockopt() on a connected basic mode socket shows FCS
> >>enabled, so this bug is visible from userspace.
> >
> >can we just fail the setsockopt() when trying to set basic mode and FCS
> >off.
> 
> It definitely makes sense to have more validation of L2CAP_OPTIONS
> passed to setsockopt().
> 
> >And also in case fallback to basic mode happens, then FCS should be set
> >to be enabled. Since for FCS and basic mode we always have to use FCS.
> >So that seems just fine to me.
> 
> The spec says "The FCS option shall only be used when the mode is
> being, or is already configured to Enhanced Retransmission mode or
> Streaming mode."  FCS is never used in basic mode (fallback or not).
> 
> (Maybe I've misunderstood your point)
> 
> >Maybe you need to explain a bit more in detail what you are trying to
> >achieve in conjunction with userspace API.
> 
> My goal is to only have l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 when
> the FCS option is actually in use.  Otherwise, any logic checking
> for FCS also has to check the L2CAP mode.  Might as well check the
> mode once and set fcs accordingly -- which is what my patch does.
> 
> Gustavo is correct that l2cap_pi(sk)->fcs is currently only checked
> on code paths used with ERTM and streaming mode.  However, future
> code (including a patch I'll be posting soon) will depend on the fcs
> value being accurate in all modes.

So queue this togheter with the one you are going to send soon. It will
be easy the real need for the checks you are doing here.

> 
> I only mentioned getsockopt() to show that this issue is not
> completely invisible, and is worth patching.

You can also set l2cap_pi(sk)->fcs to NOFCS when the mode configured
is Basic Mode.


-- 
Gustavo F. Padovan
http://padovan.org

[PATCH 2/8] Bluetooth: Change default ERTM retransmit timeout to 2 seconds, as the spec requires.

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 include/net/bluetooth/l2cap.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 636724b..16e412f 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -33,7 +33,7 @@
 #define L2CAP_DEFAULT_FLUSH_TO		0xffff
 #define L2CAP_DEFAULT_TX_WINDOW		63
 #define L2CAP_DEFAULT_MAX_TX		3
-#define L2CAP_DEFAULT_RETRANS_TO	1000    /* 1 second */
+#define L2CAP_DEFAULT_RETRANS_TO	2000    /* 2 seconds */
 #define L2CAP_DEFAULT_MONITOR_TO	12000   /* 12 seconds */
 #define L2CAP_DEFAULT_MAX_PDU_SIZE	672
 #define L2CAP_DEFAULT_ACK_TO		200
-- 
1.7.1

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

Re: [PATCH 2/8] Bluetooth: Change default ERTM retransmit timeout to 2 seconds, as the spec requires.

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/l2cap.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

I do expect some more details commit messages. The subject line is
suppose to around 50 chars and give an idea what the patch is about. The
actual commit message should have the full blown explanation why etc.

Regards

Marcel

[PATCH 3/8] Bluetooth: Validate PSM values in calls to connect() and bind().

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index aed72f2..44bc6ee 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -1008,6 +1008,12 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
 		goto done;
 	}
 
+	/* If specified, PSM must be odd and lsb of upper byte must be 0 */
+	if (la.l2_psm && (__le16_to_cpu(la.l2_psm) & 0x0101) != 0x0001) {
+		err = -EINVAL;
+		goto done;
+	}
+
 	if (la.l2_psm && __le16_to_cpu(la.l2_psm) < 0x1001 &&
 				!capable(CAP_NET_BIND_SERVICE)) {
 		err = -EACCES;
@@ -1190,6 +1196,12 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr, int al
 		goto done;
 	}
 
+	/* PSM must be odd and lsb of upper byte must be 0 */
+	if ((__le16_to_cpu(la.l2_psm) & 0x0101) != 0x0001) {
+		err = -EINVAL;
+		goto done;
+	}
+
 	/* Set destination address and psm */
 	bacpy(&bt_sk(sk)->dst, &la.l2_bdaddr);
 	l2cap_pi(sk)->psm = la.l2_psm;
-- 
1.7.1

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

Re: [PATCH 3/8] Bluetooth: Validate PSM values in calls to connect() and bind().

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index aed72f2..44bc6ee 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -1008,6 +1008,12 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
>  		goto done;
>  	}
>  
> +	/* If specified, PSM must be odd and lsb of upper byte must be 0 */
> +	if (la.l2_psm && (__le16_to_cpu(la.l2_psm) & 0x0101) != 0x0001) {
> +		err = -EINVAL;
> +		goto done;
> +	}
> +

this was a nice feature of L2CAP in BlueZ actually ;)

I am fine with forbidding this. However, please send a patch that fixes
l2test's default PSM first. Otherwise you gonna break that one.

Regards

Marcel

Re: [PATCH 3/8] Bluetooth: Validate PSM values in calls to connect() and bind().

[Gustavo F. Padovan]

Hi Mat,

* Marcel Holtmann <marcel@holtmann.org> [2010-08-02 12:39:51 -0700]:

> Hi Mat,
> 
> > Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> > ---
> >  net/bluetooth/l2cap.c |   12 ++++++++++++
> >  1 files changed, 12 insertions(+), 0 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > index aed72f2..44bc6ee 100644
> > --- a/net/bluetooth/l2cap.c
> > +++ b/net/bluetooth/l2cap.c
> > @@ -1008,6 +1008,12 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
> >  		goto done;
> >  	}
> >  
> > +	/* If specified, PSM must be odd and lsb of upper byte must be 0 */
> > +	if (la.l2_psm && (__le16_to_cpu(la.l2_psm) & 0x0101) != 0x0001) {
> > +		err = -EINVAL;
> > +		goto done;
> > +	}
> > +
> 
> this was a nice feature of L2CAP in BlueZ actually ;)

And please add a commit message for this patch.


-- 
Gustavo F. Padovan
http://padovan.org

[PATCH 4/8] Bluetooth: Do endianness conversion on MPS configuration value before doing comparisons.

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 44bc6ee..9780ab0 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -2708,10 +2708,10 @@ done:
 		case L2CAP_MODE_ERTM:
 			pi->remote_tx_win = rfc.txwin_size;
 			pi->remote_max_tx = rfc.max_transmit;
-			if (rfc.max_pdu_size > pi->conn->mtu - 10)
-				rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);
 
 			pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
+			if (pi->remote_mps > pi->conn->mtu - 10)
+				pi->remote_mps = pi->conn->mtu - 10;
 
 			rfc.retrans_timeout =
 				le16_to_cpu(L2CAP_DEFAULT_RETRANS_TO);
@@ -2726,10 +2726,9 @@ done:
 			break;
 
 		case L2CAP_MODE_STREAMING:
-			if (rfc.max_pdu_size > pi->conn->mtu - 10)
-				rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);
-
 			pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
+			if (pi->remote_mps > pi->conn->mtu - 10)
+				pi->remote_mps = pi->conn->mtu - 10;
 
 			pi->conf_state |= L2CAP_CONF_MODE_DONE;
 
-- 
1.7.1

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

Re: [PATCH 4/8] Bluetooth: Do endianness conversion on MPS configuration value before doing comparisons.

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)

same for subject line vs commit message. And please explain why and if
this fixes a bug.

Regards

Marcel

[PATCH 5/8] Bluetooth: Don't modify remote_tx_win when receiving a config response. Only config requests should set remote_tx_win.

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 9780ab0..582975b 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -2808,7 +2808,6 @@ static int l2cap_parse_conf_rsp(struct sock *sk, void *rsp, int len, void *data,
 	if (*result == L2CAP_CONF_SUCCESS) {
 		switch (rfc.mode) {
 		case L2CAP_MODE_ERTM:
-			pi->remote_tx_win   = rfc.txwin_size;
 			pi->retrans_timeout = le16_to_cpu(rfc.retrans_timeout);
 			pi->monitor_timeout = le16_to_cpu(rfc.monitor_timeout);
 			pi->mps    = le16_to_cpu(rfc.max_pdu_size);
-- 
1.7.1

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

Re: [PATCH 5/8] Bluetooth: Don't modify remote_tx_win when receiving a config response. Only config requests should set remote_tx_win.

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)

please write a proper commit message. Your subject is not meant to be
used for explaining what is wrong and what is right and how you did it.
It is called a SUBJECT.

Regards

Marcel

[PATCH 6/8] Bluetooth: Move stream-oriented recvmsg code so it can be used by L2CAP.

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 include/net/bluetooth/bluetooth.h |    2 +
 net/bluetooth/af_bluetooth.c      |  107 +++++++++++++++++++++++++++++++++++++
 net/bluetooth/rfcomm/sock.c       |  104 ++---------------------------------
 3 files changed, 115 insertions(+), 98 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 27a902d..08b6c2a 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -126,6 +126,8 @@ int  bt_sock_unregister(int proto);
 void bt_sock_link(struct bt_sock_list *l, struct sock *s);
 void bt_sock_unlink(struct bt_sock_list *l, struct sock *s);
 int  bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags);
+int  bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
+			struct msghdr *msg, size_t len, int flags);
 uint bt_sock_poll(struct file * file, struct socket *sock, poll_table *wait);
 int  bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
 int  bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 421c45b..73047f5 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -265,6 +265,113 @@ int bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
 }
 EXPORT_SYMBOL(bt_sock_recvmsg);
 
+static long bt_sock_data_wait(struct sock *sk, long timeo)
+{
+	DECLARE_WAITQUEUE(wait, current);
+
+	add_wait_queue(sk_sleep(sk), &wait);
+	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
+
+		if (!skb_queue_empty(&sk->sk_receive_queue) ||
+		    sk->sk_err ||
+		    (sk->sk_shutdown & RCV_SHUTDOWN) ||
+		    signal_pending(current) ||
+		    !timeo)
+			break;
+
+		set_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
+		release_sock(sk);
+		timeo = schedule_timeout(timeo);
+		lock_sock(sk);
+		clear_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
+	}
+
+	__set_current_state(TASK_RUNNING);
+	remove_wait_queue(sk_sleep(sk), &wait);
+	return timeo;
+}
+
+int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
+			       struct msghdr *msg, size_t size, int flags)
+{
+	struct sock *sk = sock->sk;
+	int err = 0;
+	size_t target, copied = 0;
+	long timeo;
+
+	if (flags & MSG_OOB)
+		return -EOPNOTSUPP;
+
+	msg->msg_namelen = 0;
+
+	BT_DBG("sk %p size %zu", sk, size);
+
+	lock_sock(sk);
+
+	target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
+	timeo  = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
+
+	do {
+		struct sk_buff *skb;
+		int chunk;
+
+		skb = skb_dequeue(&sk->sk_receive_queue);
+		if (!skb) {
+			if (copied >= target)
+				break;
+
+			if ((err = sock_error(sk)) != 0)
+				break;
+			if (sk->sk_shutdown & RCV_SHUTDOWN)
+				break;
+
+			err = -EAGAIN;
+			if (!timeo)
+				break;
+
+			timeo = bt_sock_data_wait(sk, timeo);
+
+			if (signal_pending(current)) {
+				err = sock_intr_errno(timeo);
+				goto out;
+			}
+			continue;
+		}
+
+		chunk = min_t(unsigned int, skb->len, size);
+		if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) {
+			skb_queue_head(&sk->sk_receive_queue, skb);
+			if (!copied)
+				copied = -EFAULT;
+			break;
+		}
+		copied += chunk;
+		size   -= chunk;
+
+		sock_recv_ts_and_drops(msg, sk, skb);
+
+		if (!(flags & MSG_PEEK)) {
+			skb_pull(skb, chunk);
+			if (skb->len) {
+				skb_queue_head(&sk->sk_receive_queue, skb);
+				break;
+			}
+			kfree_skb(skb);
+
+		} else {
+			/* put message back and return */
+			skb_queue_head(&sk->sk_receive_queue, skb);
+			break;
+		}
+	} while (size);
+
+out:
+	release_sock(sk);
+	return copied ? : err;
+}
+EXPORT_SYMBOL(bt_sock_stream_recvmsg);
+
 static inline unsigned int bt_accept_poll(struct sock *parent)
 {
 	struct list_head *p, *n;
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 44a6232..5c92929 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -617,121 +617,29 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 	return sent;
 }
 
-static long rfcomm_sock_data_wait(struct sock *sk, long timeo)
-{
-	DECLARE_WAITQUEUE(wait, current);
-
-	add_wait_queue(sk_sleep(sk), &wait);
-	for (;;) {
-		set_current_state(TASK_INTERRUPTIBLE);
-
-		if (!skb_queue_empty(&sk->sk_receive_queue) ||
-		    sk->sk_err ||
-		    (sk->sk_shutdown & RCV_SHUTDOWN) ||
-		    signal_pending(current) ||
-		    !timeo)
-			break;
-
-		set_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
-		release_sock(sk);
-		timeo = schedule_timeout(timeo);
-		lock_sock(sk);
-		clear_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
-	}
-
-	__set_current_state(TASK_RUNNING);
-	remove_wait_queue(sk_sleep(sk), &wait);
-	return timeo;
-}
-
 static int rfcomm_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
 			       struct msghdr *msg, size_t size, int flags)
 {
 	struct sock *sk = sock->sk;
 	struct rfcomm_dlc *d = rfcomm_pi(sk)->dlc;
-	int err = 0;
-	size_t target, copied = 0;
-	long timeo;
+	int len_or_err;
 
 	if (test_and_clear_bit(RFCOMM_DEFER_SETUP, &d->flags)) {
 		rfcomm_dlc_accept(d);
 		return 0;
 	}
 
-	if (flags & MSG_OOB)
-		return -EOPNOTSUPP;
-
-	msg->msg_namelen = 0;
-
-	BT_DBG("sk %p size %zu", sk, size);
+	len_or_err = bt_sock_stream_recvmsg(iocb, sock, msg, size, flags);
 
 	lock_sock(sk);
+	if (!(flags & MSG_PEEK) && len_or_err > 0)
+		atomic_sub(len_or_err, &sk->sk_rmem_alloc);
 
-	target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
-	timeo  = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
-
-	do {
-		struct sk_buff *skb;
-		int chunk;
-
-		skb = skb_dequeue(&sk->sk_receive_queue);
-		if (!skb) {
-			if (copied >= target)
-				break;
-
-			if ((err = sock_error(sk)) != 0)
-				break;
-			if (sk->sk_shutdown & RCV_SHUTDOWN)
-				break;
-
-			err = -EAGAIN;
-			if (!timeo)
-				break;
-
-			timeo = rfcomm_sock_data_wait(sk, timeo);
-
-			if (signal_pending(current)) {
-				err = sock_intr_errno(timeo);
-				goto out;
-			}
-			continue;
-		}
-
-		chunk = min_t(unsigned int, skb->len, size);
-		if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) {
-			skb_queue_head(&sk->sk_receive_queue, skb);
-			if (!copied)
-				copied = -EFAULT;
-			break;
-		}
-		copied += chunk;
-		size   -= chunk;
-
-		sock_recv_ts_and_drops(msg, sk, skb);
-
-		if (!(flags & MSG_PEEK)) {
-			atomic_sub(chunk, &sk->sk_rmem_alloc);
-
-			skb_pull(skb, chunk);
-			if (skb->len) {
-				skb_queue_head(&sk->sk_receive_queue, skb);
-				break;
-			}
-			kfree_skb(skb);
-
-		} else {
-			/* put message back and return */
-			skb_queue_head(&sk->sk_receive_queue, skb);
-			break;
-		}
-	} while (size);
-
-out:
 	if (atomic_read(&sk->sk_rmem_alloc) <= (sk->sk_rcvbuf >> 2))
 		rfcomm_dlc_unthrottle(rfcomm_pi(sk)->dlc);
-
 	release_sock(sk);
-	return copied ? : err;
+
+	return len_or_err;
 }
 
 static int rfcomm_sock_setsockopt_old(struct socket *sock, int optname, char __user *optval, unsigned int optlen)
-- 
1.7.1

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

Re: [PATCH 6/8] Bluetooth: Move stream-oriented recvmsg code so it can be used by L2CAP.

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/bluetooth.h |    2 +
>  net/bluetooth/af_bluetooth.c      |  107 +++++++++++++++++++++++++++++++++++++
>  net/bluetooth/rfcomm/sock.c       |  104 ++---------------------------------
>  3 files changed, 115 insertions(+), 98 deletions(-)

looks all like a good idea, but I really have to insist that the commit
message explain everything in detail. Give a reason why the code is
similar or the same and why this makes sense.

Also splitting these into two or more patches makes sense. One adding
the stream receive and the other modifying L2CAP and RFCOMM to use it.

> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 27a902d..08b6c2a 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -126,6 +126,8 @@ int  bt_sock_unregister(int proto);
>  void bt_sock_link(struct bt_sock_list *l, struct sock *s);
>  void bt_sock_unlink(struct bt_sock_list *l, struct sock *s);
>  int  bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags);
> +int  bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
> +			struct msghdr *msg, size_t len, int flags);
>  uint bt_sock_poll(struct file * file, struct socket *sock, poll_table *wait);
>  int  bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
>  int  bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> index 421c45b..73047f5 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -265,6 +265,113 @@ int bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
>  }
>  EXPORT_SYMBOL(bt_sock_recvmsg);
>  
> +static long bt_sock_data_wait(struct sock *sk, long timeo)
> +{
> +	DECLARE_WAITQUEUE(wait, current);
> +
> +	add_wait_queue(sk_sleep(sk), &wait);
> +	for (;;) {
> +		set_current_state(TASK_INTERRUPTIBLE);
> +
> +		if (!skb_queue_empty(&sk->sk_receive_queue) ||
> +		    sk->sk_err ||
> +		    (sk->sk_shutdown & RCV_SHUTDOWN) ||
> +		    signal_pending(current) ||
> +		    !timeo)
> +			break;

This makes my brain hurt. Please lets do this readable and with proper
coding style and/or a helper function.

Regards

Marcel

Re: [PATCH 6/8] Bluetooth: Move stream-oriented recvmsg code so it can be used by L2CAP.

[Mat Martineau]

Marcel,

On Mon, 2 Aug 2010, Marcel Holtmann wrote:

> Hi Mat,
>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  include/net/bluetooth/bluetooth.h |    2 +
>>  net/bluetooth/af_bluetooth.c      |  107 +++++++++++++++++++++++++++++++++++++
>>  net/bluetooth/rfcomm/sock.c       |  104 ++---------------------------------
>>  3 files changed, 115 insertions(+), 98 deletions(-)
>
> looks all like a good idea, but I really have to insist that the commit
> message explain everything in detail. Give a reason why the code is
> similar or the same and why this makes sense.
>
> Also splitting these into two or more patches makes sense. One adding
> the stream receive and the other modifying L2CAP and RFCOMM to use it.

Agreed.


>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> index 27a902d..08b6c2a 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -126,6 +126,8 @@ int  bt_sock_unregister(int proto);
>>  void bt_sock_link(struct bt_sock_list *l, struct sock *s);
>>  void bt_sock_unlink(struct bt_sock_list *l, struct sock *s);
>>  int  bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags);
>> +int  bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
>> +			struct msghdr *msg, size_t len, int flags);
>>  uint bt_sock_poll(struct file * file, struct socket *sock, poll_table *wait);
>>  int  bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
>>  int  bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
>> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
>> index 421c45b..73047f5 100644
>> --- a/net/bluetooth/af_bluetooth.c
>> +++ b/net/bluetooth/af_bluetooth.c
>> @@ -265,6 +265,113 @@ int bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
>>  }
>>  EXPORT_SYMBOL(bt_sock_recvmsg);
>>
>> +static long bt_sock_data_wait(struct sock *sk, long timeo)
>> +{
>> +	DECLARE_WAITQUEUE(wait, current);
>> +
>> +	add_wait_queue(sk_sleep(sk), &wait);
>> +	for (;;) {
>> +		set_current_state(TASK_INTERRUPTIBLE);
>> +
>> +		if (!skb_queue_empty(&sk->sk_receive_queue) ||
>> +		    sk->sk_err ||
>> +		    (sk->sk_shutdown & RCV_SHUTDOWN) ||
>> +		    signal_pending(current) ||
>> +		    !timeo)
>> +			break;
>
> This makes my brain hurt. Please lets do this readable and with proper
> coding style and/or a helper function.

That's unchanged code from rfcomm_sock_data_wait() in rfcomm/sock.c - 
but I'll go ahead and break it up using multiple 'if' statements.


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

[PATCH 7/8] Bluetooth: Use a stream-oriented recvmsg with SOCK_STREAM L2CAP sockets.

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 582975b..8e9fa51 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -1923,6 +1923,7 @@ done:
 static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags)
 {
 	struct sock *sk = sock->sk;
+	int len_or_err;
 
 	lock_sock(sk);
 
@@ -1956,7 +1957,13 @@ static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct ms
 
 	release_sock(sk);
 
-	return bt_sock_recvmsg(iocb, sock, msg, len, flags);
+	if (sock->type == SOCK_STREAM)
+		len_or_err = bt_sock_stream_recvmsg(iocb, sock, msg, len,
+						flags);
+	else
+		len_or_err = bt_sock_recvmsg(iocb, sock, msg, len, flags);
+
+	return len_or_err;
 }
 
 static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __user *optval, unsigned int optlen)
-- 
1.7.1

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

Re: [PATCH 7/8] Bluetooth: Use a stream-oriented recvmsg with SOCK_STREAM L2CAP sockets.

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 582975b..8e9fa51 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -1923,6 +1923,7 @@ done:
>  static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags)
>  {
>  	struct sock *sk = sock->sk;
> +	int len_or_err;
>  
>  	lock_sock(sk);
>  
> @@ -1956,7 +1957,13 @@ static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct ms
>  
>  	release_sock(sk);
>  
> -	return bt_sock_recvmsg(iocb, sock, msg, len, flags);
> +	if (sock->type == SOCK_STREAM)
> +		len_or_err = bt_sock_stream_recvmsg(iocb, sock, msg, len,
> +						flags);
> +	else
> +		len_or_err = bt_sock_recvmsg(iocb, sock, msg, len, flags);
> +
> +	return len_or_err;
>  }

I don't like this variable name. Just call it "len" or just use "ret"
here. Check what other parts of net/bluetooth/ are using.

Also since you are not checking is value anyway, why not return right
away from the if.

	if (sock->type == SOCK_STREAM)
		return bt_sock_stream_recvmsg(...);

	return bt_sock_recvmsg(...);

That way the compiler should also not complain. And in addition your
patch looks dead simple ;)

Regards

Marcel

[PATCH 8/8] Bluetooth: Use 3-DH5 payload size for default ERTM max PDU size.

[Mat Martineau]

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 include/net/bluetooth/l2cap.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 16e412f..93aba17 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -35,7 +35,7 @@
 #define L2CAP_DEFAULT_MAX_TX		3
 #define L2CAP_DEFAULT_RETRANS_TO	2000    /* 2 seconds */
 #define L2CAP_DEFAULT_MONITOR_TO	12000   /* 12 seconds */
-#define L2CAP_DEFAULT_MAX_PDU_SIZE	672
+#define L2CAP_DEFAULT_MAX_PDU_SIZE	1011    /* Sized for 3-DH5 packet */
 #define L2CAP_DEFAULT_ACK_TO		200
 #define L2CAP_LOCAL_BUSY_TRIES		12
 
-- 
1.7.1

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

Re: [PATCH 8/8] Bluetooth: Use 3-DH5 payload size for default ERTM max PDU size.

[Marcel Holtmann]

Hi Mat,

> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  include/net/bluetooth/l2cap.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

this should have a commit message with technical details to explain why
it is a good idea.

> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 16e412f..93aba17 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -35,7 +35,7 @@
>  #define L2CAP_DEFAULT_MAX_TX		3
>  #define L2CAP_DEFAULT_RETRANS_TO	2000    /* 2 seconds */
>  #define L2CAP_DEFAULT_MONITOR_TO	12000   /* 12 seconds */
> -#define L2CAP_DEFAULT_MAX_PDU_SIZE	672
> +#define L2CAP_DEFAULT_MAX_PDU_SIZE	1011    /* Sized for 3-DH5 packet */
>  #define L2CAP_DEFAULT_ACK_TO		200
>  #define L2CAP_LOCAL_BUSY_TRIES		12

Also the default PDU size for basic mode should stay with 672. And only
for ERTM move it to 1010. Is this ensured?

And then again here. Is that explained in the commit message?

Regards

Marcel

 

Re: [PATCH 8/8] Bluetooth: Use 3-DH5 payload size for default ERTM max PDU size.

[Mat Martineau]

Marcel,

On Mon, 2 Aug 2010, Marcel Holtmann wrote:

> Hi Mat,
>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  include/net/bluetooth/l2cap.h |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> this should have a commit message with technical details to explain why
> it is a good idea.
>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 16e412f..93aba17 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -35,7 +35,7 @@
>>  #define L2CAP_DEFAULT_MAX_TX		3
>>  #define L2CAP_DEFAULT_RETRANS_TO	2000    /* 2 seconds */
>>  #define L2CAP_DEFAULT_MONITOR_TO	12000   /* 12 seconds */
>> -#define L2CAP_DEFAULT_MAX_PDU_SIZE	672
>> +#define L2CAP_DEFAULT_MAX_PDU_SIZE	1011    /* Sized for 3-DH5 packet */
>>  #define L2CAP_DEFAULT_ACK_TO		200
>>  #define L2CAP_LOCAL_BUSY_TRIES		12
>
> Also the default PDU size for basic mode should stay with 672. And only
> for ERTM move it to 1010. Is this ensured?
>
> And then again here. Is that explained in the commit message?

It is ensured that the max PDU size is only used for ERTM and 
streaming mode.  The default MTU for basic mode and ERTM/streaming 
remains unchanged.

I will update the commit message to fully describe this.

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

Re: [PATCH 0/8] Bluetooth: L2CAP updates for PSM validation and ERTM.

[Marcel Holtmann]

Hi Mat,

> This is a collection of ERTM-related patches, including some L2CAP 
> configuration fixes and allowing partial-frame reads from L2CAP
> SOCK_STREAM sockets.  PSM validation is also included, although that
> is not restricted to ERTM.

so the merge window for 2.6.36 is basically closed and these patches
came in too late. If they fix a bug, I like you to explain that in
detail in the commit message so I can pick them out and include them for
submission to John.

Regards

Marcel

Re: [PATCH 0/8] Bluetooth: L2CAP updates for PSM validation and ERTM.

[Mat Martineau]

On Mon, 2 Aug 2010, Marcel Holtmann wrote:

> Hi Mat,
>
>> This is a collection of ERTM-related patches, including some L2CAP
>> configuration fixes and allowing partial-frame reads from L2CAP
>> SOCK_STREAM sockets.  PSM validation is also included, although that
>> is not restricted to ERTM.
>
> so the merge window for 2.6.36 is basically closed and these patches
> came in too late. If they fix a bug, I like you to explain that in
> detail in the commit message so I can pick them out and include them for
> submission to John.

Marcel -

I'll fix up my commit messages and make the other changes you 
recommended.  I don't have a specific need for these to be in 2.6.36, 
but with more detail on the bug fixes you can decide if they're urgent 
enough.

Thanks for the feedback.

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

Re: [PATCH 0/8] Bluetooth: L2CAP updates for PSM validation and ERTM.

[Marcel Holtmann]

Hi Mat,

> >> This is a collection of ERTM-related patches, including some L2CAP
> >> configuration fixes and allowing partial-frame reads from L2CAP
> >> SOCK_STREAM sockets.  PSM validation is also included, although that
> >> is not restricted to ERTM.
> >
> > so the merge window for 2.6.36 is basically closed and these patches
> > came in too late. If they fix a bug, I like you to explain that in
> > detail in the commit message so I can pick them out and include them for
> > submission to John.
> 
> I'll fix up my commit messages and make the other changes you 
> recommended.  I don't have a specific need for these to be in 2.6.36, 
> but with more detail on the bug fixes you can decide if they're urgent 
> enough.

since 2.6.36 will be the first kernel we enable ERTM by default, I
prefer that we have known bugs/regressions actually fixed.

Regards

Marcel

ERTM known bugs/regressions (was Re: [PATCH 0/8] ...)

[Mat Martineau]

Marcel & Gustavo -

On Mon, 2 Aug 2010, Marcel Holtmann wrote:

> Hi Mat,

<snip>

> since 2.6.36 will be the first kernel we enable ERTM by default, I
> prefer that we have known bugs/regressions actually fixed.


Gustavo has done a lot of great work to get ERTM ready to be turned on 
by default, but there is one potential issue I see with ERTM that I'd 
like to discuss: locking.

Kernel timers are used for the ack, retransmit, and monitor timeouts. 
The handler functions for these timers use bh_lock_sock() and 
bh_unlock_sock() to protect socket state.  While this does protect 
against simultaneous timeouts, or access in tasklet context for 
incoming HCI data, it doesn't protect against access in user context.

Here's a scenario:

  * Application calls send()
  * lock_sock() is called in l2cap_sock_sendmsg()
  * L2CAP code starts to modify l2cap_pinfo(sk) state in user context
  * Timer fires, preempting l2cap_sock_sendmsg() or on another CPU
  * Timer does not spin on bh_lock_sock()
  * Timer handler changes l2cap_pinfo(sk) state
  * Timer calls bh_unlock_sock() and finishes
  * Code called by l2cap_sock_sendmsg() continues running, but
    l2cap_pinfo(sk) was changed by the timer.
  * release_sock() is called and l2cap_sock_sendmsg() returns.

It doesn't look like this would lead to a kernel panic, but can
cause data coherency problems in l2cap_pinfo(sk) (especially 
l2cap_pinfo(sk)->conn_state).  The most likely symptom would be 
spurious disconnects when the other end of the ERTM connection sees 
some odd behavior.


One solution is to use a workqueue for these timeouts, and then use 
lock_sock() and release_sock() to protect l2cap_pinfo(sk) in the 
workqueue handlers.  There's a big catch, though:  the socket might be 
locked for a while if bt_skb_send_alloc() blocks and ERTM is not 
sending data from the TX queue (it might be retransmitting frames 
instead).  As long as the lock is held, all the incoming data is put 
in the socket backlog and any ack/retrans/monitor timers that fire on 
the workqueue will block everything on that queue.  The ERTM 
connection can stall.

I think the ERTM code would need to either release the socket lock 
when calling bt_skb_send_alloc(), or another spinlock is needed to 
protect l2cap_pi(sk).  Per-socket workqueues might be a good idea too, 
so one locked socket can't block timeouts on other sockets.

This is a non-issue in basic mode, because no socket state is changed 
when data is sent or received.


Do you agree that there's an ERTM issue here to be addressed?  Any 
ideas regarding the workqueue solution?


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