[PATCH 1/2] Bluetooth: Add debug print to l2cap_chan_create

[PATCH 1/2] Bluetooth: Add debug print to l2cap_chan_create

[Szymon Janc]

Signed-off-by: Szymon Janc <szymon.janc@tieto.com>
---
 net/bluetooth/l2cap_core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index af3e8c5..9dcbf3d 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -294,6 +294,8 @@ struct l2cap_chan *l2cap_chan_create(struct sock *sk)
 
 	atomic_set(&chan->refcnt, 1);
 
+	BT_DBG("sk %p chan %p", sk, chan);
+
 	return chan;
 }
 
-- 
on behalf of ST-Ericsson

[PATCH 2/2] Bluetooth: Add ability to force local busy condition on L2CAP ERTM

[Szymon Janc]

This is required to pass PTS TP/ERM/BV-07-C (Send S-Frame [RNR]).

Signed-off-by: Szymon Janc <szymon.janc@tieto.com>
---
 net/bluetooth/l2cap_core.c |   43 ++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 42 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 9dcbf3d..6202009 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -59,6 +59,8 @@
 int disable_ertm;
 int enable_hs;
 
+static int force_local_busy;
+
 static u32 l2cap_feat_mask = L2CAP_FEAT_FIXED_CHAN;
 static u8 l2cap_fixed_chan[8] = { 0x02, };
 
@@ -3522,7 +3524,7 @@ void l2cap_chan_busy(struct l2cap_chan *chan, int busy)
 	if (chan->mode == L2CAP_MODE_ERTM) {
 		if (busy)
 			l2cap_ertm_enter_local_busy(chan);
-		else
+		else if (!force_local_busy)
 			l2cap_ertm_exit_local_busy(chan);
 	}
 }
@@ -4498,7 +4500,38 @@ static const struct file_operations l2cap_debugfs_fops = {
 	.release	= single_release,
 };
 
+static int force_local_busy_set(void *data, u64 val)
+{
+	struct l2cap_chan *chan;
+
+	force_local_busy = val;
+
+	read_lock_bh(&chan_list_lock);
+
+	list_for_each_entry(chan, &chan_list, global_l) {
+		bh_lock_sock(chan->sk);
+		if (chan->state == BT_CONNECTED)
+			l2cap_chan_busy(chan, force_local_busy);
+		bh_unlock_sock(chan->sk);
+	}
+
+	read_unlock_bh(&chan_list_lock);
+
+	return 0;
+}
+
+static int force_local_busy_get(void *data, u64 *val)
+{
+	*val = force_local_busy;
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(force_local_busy_fops, force_local_busy_get,
+					force_local_busy_set, "%llu\n");
+
 static struct dentry *l2cap_debugfs;
+static struct dentry *force_local_busy_debugfs;
 
 static struct hci_proto l2cap_hci_proto = {
 	.name		= "L2CAP",
@@ -4531,6 +4564,13 @@ int __init l2cap_init(void)
 					bt_debugfs, NULL, &l2cap_debugfs_fops);
 		if (!l2cap_debugfs)
 			BT_ERR("Failed to create L2CAP debug file");
+
+		force_local_busy_debugfs =
+				debugfs_create_file("force_local_busy", 0444,
+							bt_debugfs, NULL,
+							&force_local_busy_fops);
+		if (!force_local_busy_debugfs)
+			BT_ERR("Failed to create ERTM debug file");
 	}
 
 	return 0;
@@ -4543,6 +4583,7 @@ error:
 void l2cap_exit(void)
 {
 	debugfs_remove(l2cap_debugfs);
+	debugfs_remove(force_local_busy_debugfs);
 
 	if (hci_unregister_proto(&l2cap_hci_proto) < 0)
 		BT_ERR("L2CAP protocol unregistration failed");
-- 
on behalf of ST-Ericsson

Re: [PATCH 2/2] Bluetooth: Add ability to force local busy condition on L2CAP ERTM

[Gustavo Padovan]

Hi Szymon,

* Szymon Janc <szymon.janc@tieto.com> [2011-11-03 16:05:45 +0100]:

> This is required to pass PTS TP/ERM/BV-07-C (Send S-Frame [RNR]).
> 
> Signed-off-by: Szymon Janc <szymon.janc@tieto.com>
> ---
>  net/bluetooth/l2cap_core.c |   43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 42 insertions(+), 1 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 9dcbf3d..6202009 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -59,6 +59,8 @@
>  int disable_ertm;
>  int enable_hs;
>  
> +static int force_local_busy;
> +
>  static u32 l2cap_feat_mask = L2CAP_FEAT_FIXED_CHAN;
>  static u8 l2cap_fixed_chan[8] = { 0x02, };
>  
> @@ -3522,7 +3524,7 @@ void l2cap_chan_busy(struct l2cap_chan *chan, int busy)
>  	if (chan->mode == L2CAP_MODE_ERTM) {
>  		if (busy)
>  			l2cap_ertm_enter_local_busy(chan);
> -		else
> +		else if (!force_local_busy)
>  			l2cap_ertm_exit_local_busy(chan);

NACK on this. I don't wanna move code with the specific purpose of pass a PTS
test into the tree. This kinda of thing can survive outside of the tree.

	Gustavo

Re: [PATCH 2/2] Bluetooth: Add ability to force local busy condition on L2CAP ERTM

[Marcel Holtmann]

Hi Gustavo,

> > This is required to pass PTS TP/ERM/BV-07-C (Send S-Frame [RNR]).
> > 
> > Signed-off-by: Szymon Janc <szymon.janc@tieto.com>
> > ---
> >  net/bluetooth/l2cap_core.c |   43 ++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 42 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 9dcbf3d..6202009 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -59,6 +59,8 @@
> >  int disable_ertm;
> >  int enable_hs;
> >  
> > +static int force_local_busy;
> > +
> >  static u32 l2cap_feat_mask = L2CAP_FEAT_FIXED_CHAN;
> >  static u8 l2cap_fixed_chan[8] = { 0x02, };
> >  
> > @@ -3522,7 +3524,7 @@ void l2cap_chan_busy(struct l2cap_chan *chan, int busy)
> >  	if (chan->mode == L2CAP_MODE_ERTM) {
> >  		if (busy)
> >  			l2cap_ertm_enter_local_busy(chan);
> > -		else
> > +		else if (!force_local_busy)
> >  			l2cap_ertm_exit_local_busy(chan);
> 
> NACK on this. I don't wanna move code with the specific purpose of pass a PTS
> test into the tree. This kinda of thing can survive outside of the tree.

actually it can not life outside the tree. We need to be able to pass
the PTS test cases with an upstream source.

However instead of hacking this in, what is the actual test case details
here that we need to have support for?

Regards

Marcel

Re: [PATCH 2/2] Bluetooth: Add ability to force local busy condition on L2CAP ERTM

[Mat Martineau]

Hi everyone -

On Fri, 4 Nov 2011, Marcel Holtmann wrote:

> Hi Gustavo,
>
>>> This is required to pass PTS TP/ERM/BV-07-C (Send S-Frame [RNR]).
>>>
>>> Signed-off-by: Szymon Janc <szymon.janc@tieto.com>
>>> ---
>>>  net/bluetooth/l2cap_core.c |   43 ++++++++++++++++++++++++++++++++++++++++++-
>>>  1 files changed, 42 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>>> index 9dcbf3d..6202009 100644
>>> --- a/net/bluetooth/l2cap_core.c
>>> +++ b/net/bluetooth/l2cap_core.c
>>> @@ -59,6 +59,8 @@
>>>  int disable_ertm;
>>>  int enable_hs;
>>>
>>> +static int force_local_busy;
>>> +
>>>  static u32 l2cap_feat_mask = L2CAP_FEAT_FIXED_CHAN;
>>>  static u8 l2cap_fixed_chan[8] = { 0x02, };
>>>
>>> @@ -3522,7 +3524,7 @@ void l2cap_chan_busy(struct l2cap_chan *chan, int busy)
>>>  	if (chan->mode == L2CAP_MODE_ERTM) {
>>>  		if (busy)
>>>  			l2cap_ertm_enter_local_busy(chan);
>>> -		else
>>> +		else if (!force_local_busy)
>>>  			l2cap_ertm_exit_local_busy(chan);
>>
>> NACK on this. I don't wanna move code with the specific purpose of pass a PTS
>> test into the tree. This kinda of thing can survive outside of the tree.
>
> actually it can not life outside the tree. We need to be able to pass
> the PTS test cases with an upstream source.
>
> However instead of hacking this in, what is the actual test case details
> here that we need to have support for?

ERTM will send an RNR when the socket receive buffer fills up.  For 
this test case, it works to just set the SO_RCVBUF sockopt to 
a small enough value that the recv buffer fills before the transmit 
window does - no kernel changes required.

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

Re: [PATCH 2/2] Bluetooth: Add ability to force local busy condition on L2CAP ERTM

[Szymon Janc]

> Hi everyone -

Hi All,

> >>
> >> NACK on this. I don't wanna move code with the specific purpose of pass a PTS
> >> test into the tree. This kinda of thing can survive outside of the tree.
> >
> > actually it can not life outside the tree. We need to be able to pass
> > the PTS test cases with an upstream source.
> >
> > However instead of hacking this in, what is the actual test case details
> > here that we need to have support for?
> 
> ERTM will send an RNR when the socket receive buffer fills up.  For 
> this test case, it works to just set the SO_RCVBUF sockopt to 
> a small enough value that the recv buffer fills before the transmit 
> window does - no kernel changes required.

@Marcel
This test case is to "Verify the IUT will send an S-Frame [RNR] when it detects a Local Busy condition."

"Pass verdict:
- ALT 1: The IUT immediately sends an S-Frame with function RNR after the Local
Busy condition is set by the Upper Tester.
- ALT 2: The IUT sends an S-Frame with function RNR after receiving I-Frame(s) from
the Tester when the Local Busy condition is set by the Upper Tester."


I was trying to pass ALT2 with Mat's suggestions but couldn't trigger local busy.
Minimum possible value for SO_RCVBUF (at least here on 3.0 kernel) is 2224 bytes and this is 
still too high to allow PTS to trigger local busy (PTS tries to send txwin i-frames with 4 bytes of 
data only).
[If it would possible to force PTS to send larger i-frames than it should work as well but I wasn't
able to find any option that would do that... please correct me if it is possible]

With forcing local busy on channels it is possible to pass ALT1 scenario: PTS ask IUT to enter
local busy condition and just waits for RNR to be send.

@Gustavo
If you prefer not to temper with normal code path I could just get rid of force_local_busy variable
and not hold channels in local busy state. This should be enough to pass test.


-- 
BR
Szymon Janc

Re: [PATCH 2/2] Bluetooth: Add ability to force local busy condition on L2CAP ERTM

[Marcel Holtmann]

Hi Syzmon,

> > >> NACK on this. I don't wanna move code with the specific purpose of pass a PTS
> > >> test into the tree. This kinda of thing can survive outside of the tree.
> > >
> > > actually it can not life outside the tree. We need to be able to pass
> > > the PTS test cases with an upstream source.
> > >
> > > However instead of hacking this in, what is the actual test case details
> > > here that we need to have support for?
> > 
> > ERTM will send an RNR when the socket receive buffer fills up.  For 
> > this test case, it works to just set the SO_RCVBUF sockopt to 
> > a small enough value that the recv buffer fills before the transmit 
> > window does - no kernel changes required.
> 
> @Marcel
> This test case is to "Verify the IUT will send an S-Frame [RNR] when it detects a Local Busy condition."
> 
> "Pass verdict:
> - ALT 1: The IUT immediately sends an S-Frame with function RNR after the Local
> Busy condition is set by the Upper Tester.
> - ALT 2: The IUT sends an S-Frame with function RNR after receiving I-Frame(s) from
> the Tester when the Local Busy condition is set by the Upper Tester."
> 
> 
> I was trying to pass ALT2 with Mat's suggestions but couldn't trigger local busy.
> Minimum possible value for SO_RCVBUF (at least here on 3.0 kernel) is 2224 bytes and this is 
> still too high to allow PTS to trigger local busy (PTS tries to send txwin i-frames with 4 bytes of 
> data only).
> [If it would possible to force PTS to send larger i-frames than it should work as well but I wasn't
> able to find any option that would do that... please correct me if it is possible]
> 
> With forcing local busy on channels it is possible to pass ALT1 scenario: PTS ask IUT to enter
> local busy condition and just waits for RNR to be send.
> 
> @Gustavo
> If you prefer not to temper with normal code path I could just get rid of force_local_busy variable
> and not hold channels in local busy state. This should be enough to pass test.

so I like the idea to make this work with standard socket options. That
is how we should be doing this. So lets figure out on how to make Mat's
suggestion work for us.

Regards

Marcel

Re: [PATCH 2/2] Bluetooth: Add ability to force local busy condition on L2CAP ERTM

[Luiz Augusto von Dentz]

Hi Marcel,

On Wed, Nov 9, 2011 at 4:48 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>> @Marcel
>> This test case is to "Verify the IUT will send an S-Frame [RNR] when it detects a Local Busy condition."
>>
>> "Pass verdict:
>> - ALT 1: The IUT immediately sends an S-Frame with function RNR after the Local
>> Busy condition is set by the Upper Tester.
>> - ALT 2: The IUT sends an S-Frame with function RNR after receiving I-Frame(s) from
>> the Tester when the Local Busy condition is set by the Upper Tester."
>>
>>
>> I was trying to pass ALT2 with Mat's suggestions but couldn't trigger local busy.
>> Minimum possible value for SO_RCVBUF (at least here on 3.0 kernel) is 2224 bytes and this is
>> still too high to allow PTS to trigger local busy (PTS tries to send txwin i-frames with 4 bytes of
>> data only).
>> [If it would possible to force PTS to send larger i-frames than it should work as well but I wasn't
>> able to find any option that would do that... please correct me if it is possible]
>>
>> With forcing local busy on channels it is possible to pass ALT1 scenario: PTS ask IUT to enter
>> local busy condition and just waits for RNR to be send.
>>
>> @Gustavo
>> If you prefer not to temper with normal code path I could just get rid of force_local_busy variable
>> and not hold channels in local busy state. This should be enough to pass test.
>
> so I like the idea to make this work with standard socket options. That
> is how we should be doing this. So lets figure out on how to make Mat's
> suggestion work for us.

Any idea why it is not possible to overwrite the minimum for the
socket buffer, its a bit annoying not being able to set them to 0
which would be useful for making readonly/writeonly sockets when using
the force socket option.

-- 
Luiz Augusto von Dentz

Re: [PATCH 2/2] Bluetooth: Add ability to force local busy condition on L2CAP ERTM

[Mat Martineau]

On Wed, 9 Nov 2011, Marcel Holtmann wrote:

> Hi Syzmon,
>
>>>>> NACK on this. I don't wanna move code with the specific purpose of pass a PTS
>>>>> test into the tree. This kinda of thing can survive outside of the tree.
>>>>
>>>> actually it can not life outside the tree. We need to be able to pass
>>>> the PTS test cases with an upstream source.
>>>>
>>>> However instead of hacking this in, what is the actual test case details
>>>> here that we need to have support for?
>>>
>>> ERTM will send an RNR when the socket receive buffer fills up.  For
>>> this test case, it works to just set the SO_RCVBUF sockopt to
>>> a small enough value that the recv buffer fills before the transmit
>>> window does - no kernel changes required.
>>
>> @Marcel
>> This test case is to "Verify the IUT will send an S-Frame [RNR] when it detects a Local Busy condition."
>>
>> "Pass verdict:
>> - ALT 1: The IUT immediately sends an S-Frame with function RNR after the Local
>> Busy condition is set by the Upper Tester.
>> - ALT 2: The IUT sends an S-Frame with function RNR after receiving I-Frame(s) from
>> the Tester when the Local Busy condition is set by the Upper Tester."
>>
>>
>> I was trying to pass ALT2 with Mat's suggestions but couldn't trigger local busy.
>> Minimum possible value for SO_RCVBUF (at least here on 3.0 kernel) is 2224 bytes and this is
>> still too high to allow PTS to trigger local busy (PTS tries to send txwin i-frames with 4 bytes of
>> data only).
>> [If it would possible to force PTS to send larger i-frames than it should work as well but I wasn't
>> able to find any option that would do that... please correct me if it is possible]
>>
>> With forcing local busy on channels it is possible to pass ALT1 scenario: PTS ask IUT to enter
>> local busy condition and just waits for RNR to be send.
>>
>> @Gustavo
>> If you prefer not to temper with normal code path I could just get rid of force_local_busy variable
>> and not hold channels in local busy state. This should be enough to pass test.
>
> so I like the idea to make this work with standard socket options. That
> is how we should be doing this. So lets figure out on how to make Mat's
> suggestion work for us.

Even with only 4 bytes of data, the buffer size limits also include 
sk_buff overhead of several hundred bytes per packet, so 2224 is small 
enough.  You also have to slow down (or stop) the reader of the data 
in userspace - otherwise, the reader pulls data out of the socket 
rcvbuf as soon as it arrives, and the buffer stays nearly empty.

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

Re: [PATCH 1/2] Bluetooth: Add debug print to l2cap_chan_create

[Gustavo Padovan]

Hi Szymon,

* Szymon Janc <szymon.janc@tieto.com> [2011-11-03 16:05:44 +0100]:

> Signed-off-by: Szymon Janc <szymon.janc@tieto.com>
> ---
>  net/bluetooth/l2cap_core.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

Applied, thanks.

	Gustavo