* Re: [PATCH] Add NULL checking for name and path params
From: Johan Hedberg @ 2010-08-03 8:42 UTC (permalink / raw)
To: Radoslaw Jablonski; +Cc: linux-bluetooth
In-Reply-To: <1280823784-14551-1-git-send-email-ext-jablonski.radoslaw@nokia.com>
Hi Radek,
On Tue, Aug 03, 2010, Radoslaw Jablonski wrote:
> Added NULL checking for "name" and "path" parameters in pbap_open_*
> functions. If this variables will be NULL, then setting -EBADR error
> code and ending function. This fixes many obexd crashes.
> ---
> plugins/pbap.c | 15 ++++++++++++++-
> 1 files changed, 14 insertions(+), 1 deletions(-)
Please update your tree against latest upstream. Your patch doesn't
apply anymore (it seems like a new version of the previous one, but that
one got already pushed upstream). Also fix the commit message to explain
what issue in the previous patch this one fixes.
Johan
^ permalink raw reply
* Re: [PATCH 6/6] Support for generating dictionary value of service UUIDs.
From: Johan Hedberg @ 2010-08-03 8:36 UTC (permalink / raw)
To: Inga Stotland; +Cc: linux-bluetooth, rshaffer, marcel
In-Reply-To: <1280576713-1398-7-git-send-email-ingas@codeaurora.org>
Hi Inga,
On Sat, Jul 31, 2010, Inga Stotland wrote:
> Add service UUIDs from EIR to device properties in "Device Found" signal.
> ---
> src/adapter.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> src/adapter.h | 4 +-
> src/dbus-hci.c | 6 ++--
> 3 files changed, 97 insertions(+), 8 deletions(-)
A few more coding style issues:
> + while (len < EIR_DATA_LENGTH) {
> + uint8_t type = eir_data[1];
> + uint8_t field_len = eir_data[0];
Add an empty line here (it's almost always a good thing to have after
variable declarations).
> + if (type == EIR_UUID16_SOME || type == EIR_UUID16_ALL)
> + count += field_len/2;
Space before and after /
> + else if (type == EIR_UUID32_SOME || type == EIR_UUID32_ALL)
> + count += field_len/4;
Same here.
> + else if (type == EIR_UUID128_SOME || type == EIR_UUID128_ALL)
> + count += field_len/16;
And here
> + count = field_len/size;
And here
> + count = field_len/size;
And here
> + val32 = (val32<<8) + data[k];
Space before and after <<
> + count = field_len/size;
Space before and after /
> + service.value.uuid128.data[k] = data[size-k-1];
Space before and after -
> + /* Extract UUIDs from extended inquiry response if any*/
> + if (eir_data != NULL)
> + uuid_count = get_uuid_count_eir(eir_data);
> +
> + if (uuid_count > 0) {
> + uuids = g_new0(char *, uuid_count + 1);
> + get_uuids_eir(uuids, eir_data);
> + }
> +
I don't think these two get_uuids_eir and get_uuid_count_eir functions
are really needed. Could you change it to just one function:
char **get_eir_uuids(const uint8_t *eir_data, size_t *count)
Johan
^ permalink raw reply
* Re: [PATCH] bluetooth: fix not setting security level when creating a rfcomm session
From: Luiz Augusto von Dentz @ 2010-08-03 8:27 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1280822396.12579.80.camel@localhost.localdomain>
Hi Marcel,
On Tue, Aug 3, 2010 at 10:59 AM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Luiz,
>
>> This cause 'No Bonding' to be used if userspace has not yet been paired
>> with remote device since the l2cap socket used to create the rfcomm
>> session does not have any security level set.
>>
>> Signed-off-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
>> ---
>> =A0net/bluetooth/rfcomm/core.c | =A0 13 ++++++++++---
>> =A01 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
>> index 7dca91b..08f5757 100644
>> --- a/net/bluetooth/rfcomm/core.c
>> +++ b/net/bluetooth/rfcomm/core.c
>> @@ -79,7 +79,10 @@ static void rfcomm_make_uih(struct sk_buff *skb, u8 a=
ddr);
>>
>> =A0static void rfcomm_process_connect(struct rfcomm_session *s);
>>
>> -static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdad=
dr_t *dst, int *err);
>> +static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bdaddr_t *dst,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u8 sec_level,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int *err);
>> =A0static struct rfcomm_session *rfcomm_session_get(bdaddr_t *src, bdadd=
r_t *dst);
>> =A0static void rfcomm_session_del(struct rfcomm_session *s);
>>
>> @@ -402,7 +405,7 @@ static int __rfcomm_dlc_open(struct rfcomm_dlc *d, b=
daddr_t *src, bdaddr_t *dst,
>>
>> =A0 =A0 =A0 s =3D rfcomm_session_get(src, dst);
>> =A0 =A0 =A0 if (!s) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 s =3D rfcomm_session_create(src, dst, &err);
>> + =A0 =A0 =A0 =A0 =A0 =A0 s =3D rfcomm_session_create(src, dst, d->sec_l=
evel, &err);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!s)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return err;
>> =A0 =A0 =A0 }
>> @@ -680,7 +683,10 @@ static void rfcomm_session_close(struct rfcomm_sess=
ion *s, int err)
>> =A0 =A0 =A0 rfcomm_session_put(s);
>> =A0}
>>
>> -static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdad=
dr_t *dst, int *err)
>> +static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bdaddr_t *dst,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u8 sec_level,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int *err)
>> =A0{
>> =A0 =A0 =A0 struct rfcomm_session *s =3D NULL;
>> =A0 =A0 =A0 struct sockaddr_l2 addr;
>> @@ -705,6 +711,7 @@ static struct rfcomm_session *rfcomm_session_create(=
bdaddr_t *src, bdaddr_t *dst
>> =A0 =A0 =A0 sk =3D sock->sk;
>> =A0 =A0 =A0 lock_sock(sk);
>> =A0 =A0 =A0 l2cap_pi(sk)->imtu =3D l2cap_mtu;
>> + =A0 =A0 l2cap_pi(sk)->sec_level =3D sec_level;
>> =A0 =A0 =A0 if (l2cap_ertm)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 l2cap_pi(sk)->mode =3D L2CAP_MODE_ERTM;
>> =A0 =A0 =A0 release_sock(sk);
>
> I see what you are trying to do here, but why is this needed? Shouldn't
> it be already correctly set for PSM 3?
I couldn't find any special check for PSM 3, but I guess it would make
sense to make sure it always set the security level from dlc which
triggered the session, doesn't it? As it is now it first triggers No
Bonding and then when we actually connect the dlc it might have a
different sec_level which might triggers another round of io
capability negotiation, not only this doesn't work always, because
there is a bug in bluetoothd that won't invalidate/remove stored link
keys once we got a new one that is not supposed to be stored (No
Bonding), but IMO much simpler too. Maybe we can do as SDP which seems
to do the check in l2cap.c:l2cap_do_connect, but for rfcomm we
actually have to look in the dlcs to check witch security level should
be use.
--=20
Luiz Augusto von Dentz
Computer Engineer
^ permalink raw reply
* Re: [PATCH 4/6] Fix in logic to write EIR when SDP records are changed.
From: Johan Hedberg @ 2010-08-03 8:23 UTC (permalink / raw)
To: Inga Stotland; +Cc: linux-bluetooth, rshaffer, marcel
In-Reply-To: <1280576713-1398-5-git-send-email-ingas@codeaurora.org>
Hi Inga,
On Sat, Jul 31, 2010, Inga Stotland wrote:
> Whenver SDP service record is added/deleted/modified check for whether
> class of device needs to be updated as well. If the update is
> needed, proceed as before and new EIR will be written subsequently.
> If the class of device is already present, just update EIR and return.
> ---
> src/adapter.c | 14 ++++++++++----
> 1 files changed, 10 insertions(+), 4 deletions(-)
A few minor coding style issues
> - /* If we already have the CoD we want or the cache is enabled or an
> - * existing CoD write is in progress just bail out */
> - if (adapter->current_cod == adapter->wanted_cod ||
> - adapter->cache_enable || adapter->pending_cod)
> + /* If the cache is enabled or an existing CoD write is in progress
> + * just bail out */
> + if(adapter->cache_enable || adapter->pending_cod)
Missing space beteween if and (
> return 0;
Add an empty line here.
> + /* If we already have the CoD we want, update EIR and return */
> + if (adapter->current_cod == adapter->wanted_cod) {
> + update_ext_inquiry_response(adapter);
> + return 0;
> + }
Johan
^ permalink raw reply
* [PATCH] Add NULL checking for name and path params
From: Radoslaw Jablonski @ 2010-08-03 8:23 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Radoslaw Jablonski
Added NULL checking for "name" and "path" parameters in pbap_open_*
functions. If this variables will be NULL, then setting -EBADR error
code and ending function. This fixes many obexd crashes.
---
plugins/pbap.c | 15 ++++++++++++++-
1 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/plugins/pbap.c b/plugins/pbap.c
index af4b452..af4a1bc 100644
--- a/plugins/pbap.c
+++ b/plugins/pbap.c
@@ -594,6 +594,9 @@ static int pbap_get(struct obex_session *os, obex_object_t *obj,
} else
return -EBADR;
+ if (path == NULL)
+ return -EBADR;
+
pbap->params = params;
ret = obex_get_stream_start(os, path);
@@ -689,6 +692,11 @@ static void *vobject_pull_open(const char *name, int oflag, mode_t mode,
goto fail;
}
+ if (name == NULL) {
+ ret = -EBADR;
+ goto fail;
+ }
+
if (pbap->params->maxlistcount == 0)
cb = phonebook_size_result;
else
@@ -720,6 +728,11 @@ static void *vobject_list_open(const char *name, int oflag, mode_t mode,
goto fail;
}
+ if (name == NULL) {
+ ret = -EBADR;
+ goto fail;
+ }
+
/* PullvCardListing always get the contacts from the cache */
if (pbap->cache.valid) {
@@ -758,7 +771,7 @@ static void *vobject_vcard_open(const char *name, int oflag, mode_t mode,
goto fail;
}
- if (sscanf(name, "%u.vcf", &handle) != 1) {
+ if (name == NULL || sscanf(name, "%u.vcf", &handle) != 1) {
ret = -EBADR;
goto fail;
}
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH 3/6] Support for adding UUID128 to extended inquiry response
From: Johan Hedberg @ 2010-08-03 8:18 UTC (permalink / raw)
To: Inga Stotland; +Cc: linux-bluetooth, rshaffer, marcel
In-Reply-To: <1280576713-1398-4-git-send-email-ingas@codeaurora.org>
Hi Inga,
On Sat, Jul 31, 2010, Inga Stotland wrote:
> ---
> src/sdpd-service.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 62 insertions(+), 0 deletions(-)
A couple of coding style comments here:
> + /* Check for duplicates, EIR data is Little Endian */
> + for (i = 0; i < index; i++) {
> + for (k = 0; k < SIZEOF_UUID128; k++) {
> + if (uuid128[i*SIZEOF_UUID128 + k] !=
Space before and after *
> + rec->svclass.value.uuid128.data[SIZEOF_UUID128 - k])
Looks like you're going beyond 80-characters. Maybe you could create an
extra variable and assign rec->svclass.value.uuid.data to it.
> + for (k = 0; k < SIZEOF_UUID128; k++)
> + uuid128[index*SIZEOF_UUID128 + k] =
Space before and after *
> + rec->svclass.value.uuid128.data[SIZEOF_UUID128 - 1 - k];
Again over 80-characters, but if you use the extra variable that I
previously suggested for rec->svclass.value.uuid128.data it should help.
> + ptr[1] = (truncated) ? EIR_UUID128_SOME : EIR_UUID128_ALL;
Since truncated is just a gboolean and not a more complex statement I'd
just leave out the parenthesis around it, but quite a minor thing in the
end.
Johan
^ permalink raw reply
* Re: [PATCH 2/6] Clean up code that generates extended inquiry response.
From: Johan Hedberg @ 2010-08-03 8:12 UTC (permalink / raw)
To: Inga Stotland; +Cc: linux-bluetooth, rshaffer, marcel
In-Reply-To: <1280576713-1398-3-git-send-email-ingas@codeaurora.org>
Hi Inga,
On Sat, Jul 31, 2010, Inga Stotland wrote:
> Minor fix when skipping duplicate UUID16 from EIR.
> ---
> src/sdpd-service.c | 46 ++++++++++++++++++++++++++++++----------------
> 1 files changed, 30 insertions(+), 16 deletions(-)
So your commit message says two things: "Clean up code..." and "Minor
fix..". Cleanups and fixes should be in separate patches. Don't be
afraid of "too small" or trivial patches. As long as a patch contains a
single logically independent change it's fine.
Johan
> + uint16_t uuid16[EIR_DATA_LENGTH/2];
Minor nitpick: space before and after /
Johan
^ permalink raw reply
* Re: Enhancements to allow l2cap channel to use either AMP or BR/EDR
From: Gustavo F. Padovan @ 2010-08-03 8:07 UTC (permalink / raw)
To: Peter Krystad
Cc: Kevin Hayes, Marcel Holtmann, tmonahan@codeaurora.org,
David Vrabel, Inga Stotland, linux-bluetooth@vger.kernel.org,
rshaffer@codeaurora.org, johan.hedberg@gmail.com
In-Reply-To: <3e0b0d621e7c3dcee98b2a16c6e1f23b.squirrel@www.codeaurora.org>
Hi Peter,
* Peter Krystad <pkrystad@codeaurora.org> [2010-08-02 18:30:34 -0700]:
>
> Hi Marcel and Kevin,
>
> >> >
> >> > Agree that it should be done "in background" and that a size
> >> threshold would be useful. But who evaluates that threshold? The
> >> bluez kernel components, which essentially implement a transport
> >> driver, should not be examining objects (files, phonebooks, etc) size
> >> to see if this threshold is met. Therefore, it would seem Tim's
> >> suggestion of having the profile send a 'prefer_amp' bit would be
> >> useful, right?
> >>
> >> I would do something like "prefer_amp when over 100kb" or something.
> >> And
> >> then the kernel needs to count. Meaning the L2CAP layer could easily
> >> count this by itself.
> >
> > What? Let's say the effect we want is that if the object is greater than,
> > say, 10 megabytes, then we want to use AMP for the transfer. With your
> > scheme, you want the kernel to count up to 10 megabytes, THEN switch over
> > to AMP?
> > No, you don't, he says rhetorically. :)
>
> If the policy was defined as "prefer_amp after n seconds" we would get the
> desired effect without having to do any counting in the kernel.
In both cases we have to take in account the total size of the file, or
the estimated time of transfer, so we can avoid to move to AMP when the
transfer is about to end.
--
Gustavo F. Padovan
http://padovan.org
^ permalink raw reply
* Re: [PATCH] bluetooth: fix not setting security level when creating a rfcomm session
From: Marcel Holtmann @ 2010-08-03 7:59 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <1280820383-10029-1-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
> This cause 'No Bonding' to be used if userspace has not yet been paired
> with remote device since the l2cap socket used to create the rfcomm
> session does not have any security level set.
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
> ---
> net/bluetooth/rfcomm/core.c | 13 ++++++++++---
> 1 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> index 7dca91b..08f5757 100644
> --- a/net/bluetooth/rfcomm/core.c
> +++ b/net/bluetooth/rfcomm/core.c
> @@ -79,7 +79,10 @@ static void rfcomm_make_uih(struct sk_buff *skb, u8 addr);
>
> static void rfcomm_process_connect(struct rfcomm_session *s);
>
> -static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdaddr_t *dst, int *err);
> +static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
> + bdaddr_t *dst,
> + u8 sec_level,
> + int *err);
> static struct rfcomm_session *rfcomm_session_get(bdaddr_t *src, bdaddr_t *dst);
> static void rfcomm_session_del(struct rfcomm_session *s);
>
> @@ -402,7 +405,7 @@ static int __rfcomm_dlc_open(struct rfcomm_dlc *d, bdaddr_t *src, bdaddr_t *dst,
>
> s = rfcomm_session_get(src, dst);
> if (!s) {
> - s = rfcomm_session_create(src, dst, &err);
> + s = rfcomm_session_create(src, dst, d->sec_level, &err);
> if (!s)
> return err;
> }
> @@ -680,7 +683,10 @@ static void rfcomm_session_close(struct rfcomm_session *s, int err)
> rfcomm_session_put(s);
> }
>
> -static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdaddr_t *dst, int *err)
> +static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
> + bdaddr_t *dst,
> + u8 sec_level,
> + int *err)
> {
> struct rfcomm_session *s = NULL;
> struct sockaddr_l2 addr;
> @@ -705,6 +711,7 @@ static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdaddr_t *dst
> sk = sock->sk;
> lock_sock(sk);
> l2cap_pi(sk)->imtu = l2cap_mtu;
> + l2cap_pi(sk)->sec_level = sec_level;
> if (l2cap_ertm)
> l2cap_pi(sk)->mode = L2CAP_MODE_ERTM;
> release_sock(sk);
I see what you are trying to do here, but why is this needed? Shouldn't
it be already correctly set for PSM 3?
Regards
Marcel
^ permalink raw reply
* Re: [PATCH] Notify to device drivers when the SDP records change
From: José Antonio Santos Cadenas @ 2010-08-03 7:30 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <AANLkTin1khXOxvZTdazRYFbb3VZCf39YFYaZnwa3cbmN@mail.gmail.com>
Hi Luiz,
El Monday 02 August 2010 16:39:18 Luiz Augusto von Dentz escribió:
> Hi,
>
> On Mon, Jul 26, 2010 at 10:55 AM, Jose Antonio Santos Cadenas
>
> <santoscadenas@gmail.com> wrote:
> > When the remote SDP records that make a driver to be loaded change
> > (added, remove, or updated) And the driver is not going to be
> > removed, a notification is sent to the device driver. This
> > notification is optional annnd does not fail if the driver does
> > not implemment it.
>
> How this is supposed to work?
This tries to be a generic way to update the remote services. When we planned
the HDP API some weeks ago in Recife, we decided that we need a way to update
the services that a device offers (this implies to make some calls to the
HealtAgent). Marcel and Johan tell me that it will be better to implement it
in a generic way using a generic update services. May be I misunderstood
something or this is not the best way to do this, but that's the objective.
The objective is to provide an API to the user that notifies them about new
health services.
Can you see better ways to do this? We are open to any new ideas.
> If does seems to me that you guys are
> planning to keep polling the services records to update the drivers
> which IMO is not a good idea, DiscoverServices is not meant to update
> the drivers either it is basically a replacement to sdptool to so
> application can do it over dbus.
Of course I am not trying to poll remote SDP nor having it cached. Just
offering a way to update services for the user.
> Also if you take a closer look in the
> plugins that use rfcomm channel, which can be changed at any point,
> they basically fetch the record when attempting to connect, avdtp also
> work in a similar way, we have to do the discover + get capabilities
> every time we want to connect to a sink/source since there is no way
> to keep this information updated locally.
Of course, every time that you are going to connect is when you check the
remote record. In the HDP implementation is doing the same way.
>
> In the other hand, this can be useful for LE which we can subscribe to
> changes, but for sdp I don't see the point.
For HDP is also useful in SDP, because the device can update services and the
device driver will not receive any notification. This way we are offering a
way to update it when the user wants. This is almost the same than making a
service discovery and notify a new UUID discovered, but also notifying the
changes in the records.
Regards.
Jose.
^ permalink raw reply
* [PATCH] bluetooth: fix not setting security level when creating a rfcomm session
From: Luiz Augusto von Dentz @ 2010-08-03 7:26 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
This cause 'No Bonding' to be used if userspace has not yet been paired
with remote device since the l2cap socket used to create the rfcomm
session does not have any security level set.
Signed-off-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
---
net/bluetooth/rfcomm/core.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index 7dca91b..08f5757 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -79,7 +79,10 @@ static void rfcomm_make_uih(struct sk_buff *skb, u8 addr);
static void rfcomm_process_connect(struct rfcomm_session *s);
-static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdaddr_t *dst, int *err);
+static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
+ bdaddr_t *dst,
+ u8 sec_level,
+ int *err);
static struct rfcomm_session *rfcomm_session_get(bdaddr_t *src, bdaddr_t *dst);
static void rfcomm_session_del(struct rfcomm_session *s);
@@ -402,7 +405,7 @@ static int __rfcomm_dlc_open(struct rfcomm_dlc *d, bdaddr_t *src, bdaddr_t *dst,
s = rfcomm_session_get(src, dst);
if (!s) {
- s = rfcomm_session_create(src, dst, &err);
+ s = rfcomm_session_create(src, dst, d->sec_level, &err);
if (!s)
return err;
}
@@ -680,7 +683,10 @@ static void rfcomm_session_close(struct rfcomm_session *s, int err)
rfcomm_session_put(s);
}
-static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdaddr_t *dst, int *err)
+static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
+ bdaddr_t *dst,
+ u8 sec_level,
+ int *err)
{
struct rfcomm_session *s = NULL;
struct sockaddr_l2 addr;
@@ -705,6 +711,7 @@ static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src, bdaddr_t *dst
sk = sock->sk;
lock_sock(sk);
l2cap_pi(sk)->imtu = l2cap_mtu;
+ l2cap_pi(sk)->sec_level = sec_level;
if (l2cap_ertm)
l2cap_pi(sk)->mode = L2CAP_MODE_ERTM;
release_sock(sk);
--
1.7.0.4
^ permalink raw reply related
* RE: Enhancements to allow l2cap channel to use either AMP or BR/EDR
From: Kevin Hayes @ 2010-08-03 4:47 UTC (permalink / raw)
To: Marcel Holtmann
Cc: tmonahan@codeaurora.org, David Vrabel, Inga Stotland,
linux-bluetooth@vger.kernel.org, rshaffer@codeaurora.org,
johan.hedberg@gmail.com
In-Reply-To: <1280800744.12579.71.camel@localhost.localdomain>
SGkgYWdhaW4gTWFyY2VsLA0KDQo+IEhvd2V2ZXIgd2l0aCBPQkVYIGl0IG1pZ2h0IGJlIHRoYXQg
eW91IHdhbm5hIHJlYWQgdGhlIGZpcnN0IHBhY2thZ2UNCj4gZmlyc3Qgd2l0aCBCUi9FRFIgb25s
eSBhbmQgb25seSBhZnRlciBrbm93aW5nIHRoZSBmaWxlIHNpemUgbWFrZSBhDQo+IGRlY2lzaW9u
IHRvIHByZWZlciBBTVAgb3Igbm90Lg0KDQpFeGFjdGx5LiAgRm9yIE9QUCwgUEJBUCwgQklQLCBG
VFAsIHlvdSBrbm93IHRoZSBmaWxlIHNpemUgcHJldHR5IHF1aWNrbHksIG5vIGNvdW50aW5nIHJl
cXVpcmVkLg0KQW5kIGlmIHlvdSBoYXZlIGEgcG9saWN5IG9mICJ1c2UgQU1QIGlmIG9iamVjdCBi
aWdnZXIgdGhhbiBYIiwgdGhlbiB5b3UgZG9uJ3Qgd2FudCB0byBzcGVuZCBjeWNsZXMgdXNpbmcg
QlIvRURSIGlmIHlvdSBuZWVkIHRvIHhmZXIgYW4gb2JqZWN0IGV2ZW4gb25lIHNpbmdsZSBvY3Rl
dCBiaWdnZXIgdGhhbiBYLiAgQU1QIGVhcmx5LCBBTVAgb2Z0ZW4uICA6KQ0KDQo+IA0KPiBXZSB3
aWxsIHNlZSB3aGF0IHdvcmtzIG91dCBiZXN0Lg0KUG9saWN5IGlzIGEgd29uZGVyZnVsIHRoYW5n
LiAgOikNCg0KCUsrKw0KDQoNCg0K
^ permalink raw reply
* RE: Enhancements to allow l2cap channel to use either AMP or BR/EDR
From: Marcel Holtmann @ 2010-08-03 1:59 UTC (permalink / raw)
To: Peter Krystad
Cc: Kevin Hayes, tmonahan@codeaurora.org, David Vrabel, Inga Stotland,
linux-bluetooth@vger.kernel.org, rshaffer@codeaurora.org,
johan.hedberg@gmail.com
In-Reply-To: <3e0b0d621e7c3dcee98b2a16c6e1f23b.squirrel@www.codeaurora.org>
Hi Peter,
> >> > Agree that it should be done "in background" and that a size
> >> threshold would be useful. But who evaluates that threshold? The
> >> bluez kernel components, which essentially implement a transport
> >> driver, should not be examining objects (files, phonebooks, etc) size
> >> to see if this threshold is met. Therefore, it would seem Tim's
> >> suggestion of having the profile send a 'prefer_amp' bit would be
> >> useful, right?
> >>
> >> I would do something like "prefer_amp when over 100kb" or something.
> >> And
> >> then the kernel needs to count. Meaning the L2CAP layer could easily
> >> count this by itself.
> >
> > What? Let's say the effect we want is that if the object is greater than,
> > say, 10 megabytes, then we want to use AMP for the transfer. With your
> > scheme, you want the kernel to count up to 10 megabytes, THEN switch over
> > to AMP?
> > No, you don't, he says rhetorically. :)
>
> If the policy was defined as "prefer_amp after n seconds" we would get the
> desired effect without having to do any counting in the kernel.
that would work as well. We will see what works out best by giving
application the least amount of hassle :)
Regards
Marcel
^ permalink raw reply
* RE: Enhancements to allow l2cap channel to use either AMP or BR/EDR
From: Marcel Holtmann @ 2010-08-03 1:59 UTC (permalink / raw)
To: Kevin Hayes
Cc: tmonahan@codeaurora.org, David Vrabel, Inga Stotland,
linux-bluetooth@vger.kernel.org, rshaffer@codeaurora.org,
johan.hedberg@gmail.com
In-Reply-To: <B7132A25476D334D9130FE7532F2A56310A0B08A61@SC1EXMB-MBCL.global.atheros.com>
Hi Kevin,
> > > Agree that it should be done "in background" and that a size
> > threshold would be useful. But who evaluates that threshold? The
> > bluez kernel components, which essentially implement a transport
> > driver, should not be examining objects (files, phonebooks, etc) size
> > to see if this threshold is met. Therefore, it would seem Tim's
> > suggestion of having the profile send a 'prefer_amp' bit would be
> > useful, right?
> >
> > I would do something like "prefer_amp when over 100kb" or something.
> > And
> > then the kernel needs to count. Meaning the L2CAP layer could easily
> > count this by itself.
>
> What? Let's say the effect we want is that if the object is greater than, say, 10 megabytes, then we want to use AMP for the transfer. With your scheme, you want the kernel to count up to 10 megabytes, THEN switch over to AMP?
> No, you don't, he says rhetorically. :)
we will see what works out best. I am open for ideas. As I said before,
I know what I don't want for sure. The rest is open for testing and
figuring out if it works or not.
> > Also just starting with setsockopt(bredr_only) and then later on just
> > doing setsockopt(prefer_amp) the userspace application would have
> > control over switching manually.
>
> How is this really different from your ioctl(switch_now_to_amp) below, which looks very wrong?
The difference is that you change a policy. setsockopt(prefer_amp)
doesn't mean you get switched. It just says that you prefer using an AMP
at this point.
Most application that expect a lot of data would always set the policy
to prefer AMP and if we have the PAL availability then it will switch
right away to AMP.
However with OBEX it might be that you wanna read the first package
first with BR/EDR only and only after knowing the file size make a
decision to prefer AMP or not.
We will see what works out best.
Regards
Marcel
^ permalink raw reply
* RE: Enhancements to allow l2cap channel to use either AMP or BR/EDR
From: Peter Krystad @ 2010-08-03 1:30 UTC (permalink / raw)
To: Kevin Hayes
Cc: Marcel Holtmann, tmonahan@codeaurora.org, David Vrabel,
Inga Stotland, linux-bluetooth@vger.kernel.org,
rshaffer@codeaurora.org, johan.hedberg@gmail.com
In-Reply-To: <B7132A25476D334D9130FE7532F2A56310A0B08A61@SC1EXMB-MBCL.global.athero s.com>
Hi Marcel and Kevin,
>> >
>> > Agree that it should be done "in background" and that a size
>> threshold would be useful. But who evaluates that threshold? The
>> bluez kernel components, which essentially implement a transport
>> driver, should not be examining objects (files, phonebooks, etc) size
>> to see if this threshold is met. Therefore, it would seem Tim's
>> suggestion of having the profile send a 'prefer_amp' bit would be
>> useful, right?
>>
>> I would do something like "prefer_amp when over 100kb" or something.
>> And
>> then the kernel needs to count. Meaning the L2CAP layer could easily
>> count this by itself.
>
> What? Let's say the effect we want is that if the object is greater than,
> say, 10 megabytes, then we want to use AMP for the transfer. With your
> scheme, you want the kernel to count up to 10 megabytes, THEN switch over
> to AMP?
> No, you don't, he says rhetorically. :)
If the policy was defined as "prefer_amp after n seconds" we would get the
desired effect without having to do any counting in the kernel.
>>
>> Also just starting with setsockopt(bredr_only) and then later on just
>> doing setsockopt(prefer_amp) the userspace application would have
>> control over switching manually.
>
> How is this really different from your ioctl(switch_now_to_amp) below,
> which looks very wrong?
>
>>
>> Since the polices are on a per socket basis, I don't see a big problem
>> with doing it like this.
>>
>> The only thing that I don't want is a ioctl(switch_now_to_amp) since
>> that looks very wrong to me.
> Agreed.
>
>>
>> > > The other problem here is also that besides whatever the profiles
>> > > wants,
>> > > it might happen that the WiFi policy is stronger and only allows
>> using
>> > > the WiFi device as WiFi or as AMP. And if WiFi wins, then we have
>> to
>> > > rip
>> > > the AMP out of the system. At that point whatever the Bluetooth
>> > > profiles
>> > > wanted is pointless since the AMP controller is gone anyway.
>> >
>> > Well, I think this is best managed by having the PAL announce its
>> availability (and unavailability) to A2MP, using the spec's HCI events
>> for this purpose. A2MP can then be the agent to move the channel to
>> AMP or not, accordingly. For example, it might be that the 802.11
>> device is being used for an infrastructure link, and then that link
>> goes away. The PAL would then announce its availability to the BT
>> stack (A2MP), and A2MP would then move the channel to AMP.
>>
>> Agreed. My point was just that application can only set a policy. They
>> can't switch manually. If application would switch manually then they
>> need to check PAL availability etc. I don't want that complexity inside
>> the applications.
>>
>> > > That said, nothing stops us from allowing to make our AMP polices
>> > > dynamic and allow the application to change it threshold during
>> > > lifetime. For example it starts out as BR/EDR only and then during
>> the
>> > > usage of the link it decides that now AMP preferred would make
>> sense.
>> > > In
>> > > that sense you would have manual switching to some level.
>> >
>> > The only use case I see for manual switching is debugging.
>>
>> That is fine. We can do that via debugfs.
>>
>> Regards
>>
>> Marcel
>>
>
> \x04èº{.nÇ+‰·Ÿ®‰†+%ŠËlzwm…éb맲æìr¸›zX§»\x16å¹ëh¢ØbžØ^n‡r¡ö¦zË\x1aëh™¨èÚ&¢ø\x1e®G«éh®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš
Regards,
Peter.
--
Peter Krystad
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply
* RE: Enhancements to allow l2cap channel to use either AMP or BR/EDR
From: Kevin Hayes @ 2010-08-03 0:51 UTC (permalink / raw)
To: Marcel Holtmann
Cc: tmonahan@codeaurora.org, David Vrabel, Inga Stotland,
linux-bluetooth@vger.kernel.org, rshaffer@codeaurora.org,
johan.hedberg@gmail.com
In-Reply-To: <1280789882.12579.66.camel@localhost.localdomain>
SGkgTWFyY2VsLA0KDQo+ID4NCj4gPiBBZ3JlZSB0aGF0IGl0IHNob3VsZCBiZSBkb25lICJpbiBi
YWNrZ3JvdW5kIiBhbmQgdGhhdCBhIHNpemUNCj4gdGhyZXNob2xkIHdvdWxkIGJlIHVzZWZ1bC4g
IEJ1dCB3aG8gZXZhbHVhdGVzIHRoYXQgdGhyZXNob2xkPyAgVGhlDQo+IGJsdWV6IGtlcm5lbCBj
b21wb25lbnRzLCB3aGljaCBlc3NlbnRpYWxseSBpbXBsZW1lbnQgYSB0cmFuc3BvcnQNCj4gZHJp
dmVyLCBzaG91bGQgbm90IGJlIGV4YW1pbmluZyBvYmplY3RzIChmaWxlcywgcGhvbmVib29rcywg
ZXRjKSBzaXplDQo+IHRvIHNlZSBpZiB0aGlzIHRocmVzaG9sZCBpcyBtZXQuICBUaGVyZWZvcmUs
IGl0IHdvdWxkIHNlZW0gVGltJ3MNCj4gc3VnZ2VzdGlvbiBvZiBoYXZpbmcgdGhlIHByb2ZpbGUg
c2VuZCBhICdwcmVmZXJfYW1wJyBiaXQgd291bGQgYmUNCj4gdXNlZnVsLCByaWdodD8NCj4gDQo+
IEkgd291bGQgZG8gc29tZXRoaW5nIGxpa2UgInByZWZlcl9hbXAgd2hlbiBvdmVyIDEwMGtiIiBv
ciBzb21ldGhpbmcuDQo+IEFuZA0KPiB0aGVuIHRoZSBrZXJuZWwgbmVlZHMgdG8gY291bnQuIE1l
YW5pbmcgdGhlIEwyQ0FQIGxheWVyIGNvdWxkIGVhc2lseQ0KPiBjb3VudCB0aGlzIGJ5IGl0c2Vs
Zi4NCg0KV2hhdD8gIExldCdzIHNheSB0aGUgZWZmZWN0IHdlIHdhbnQgaXMgdGhhdCBpZiB0aGUg
b2JqZWN0IGlzIGdyZWF0ZXIgdGhhbiwgc2F5LCAxMCBtZWdhYnl0ZXMsIHRoZW4gd2Ugd2FudCB0
byB1c2UgQU1QIGZvciB0aGUgdHJhbnNmZXIuICBXaXRoIHlvdXIgc2NoZW1lLCB5b3Ugd2FudCB0
aGUga2VybmVsIHRvIGNvdW50IHVwIHRvIDEwIG1lZ2FieXRlcywgVEhFTiBzd2l0Y2ggb3ZlciB0
byBBTVA/DQpObywgeW91IGRvbid0LCBoZSBzYXlzIHJoZXRvcmljYWxseS4gIDopICANCg0KPiAN
Cj4gQWxzbyBqdXN0IHN0YXJ0aW5nIHdpdGggc2V0c29ja29wdChicmVkcl9vbmx5KSBhbmQgdGhl
biBsYXRlciBvbiBqdXN0DQo+IGRvaW5nIHNldHNvY2tvcHQocHJlZmVyX2FtcCkgdGhlIHVzZXJz
cGFjZSBhcHBsaWNhdGlvbiB3b3VsZCBoYXZlDQo+IGNvbnRyb2wgb3ZlciBzd2l0Y2hpbmcgbWFu
dWFsbHkuDQoNCkhvdyBpcyB0aGlzIHJlYWxseSBkaWZmZXJlbnQgZnJvbSB5b3VyIGlvY3RsKHN3
aXRjaF9ub3dfdG9fYW1wKSBiZWxvdywgd2hpY2ggbG9va3MgdmVyeSB3cm9uZz8NCg0KPiANCj4g
U2luY2UgdGhlIHBvbGljZXMgYXJlIG9uIGEgcGVyIHNvY2tldCBiYXNpcywgSSBkb24ndCBzZWUg
YSBiaWcgcHJvYmxlbQ0KPiB3aXRoIGRvaW5nIGl0IGxpa2UgdGhpcy4NCj4gDQo+IFRoZSBvbmx5
IHRoaW5nIHRoYXQgSSBkb24ndCB3YW50IGlzIGEgaW9jdGwoc3dpdGNoX25vd190b19hbXApIHNp
bmNlDQo+IHRoYXQgbG9va3MgdmVyeSB3cm9uZyB0byBtZS4NCkFncmVlZC4NCg0KPiANCj4gPiA+
IFRoZSBvdGhlciBwcm9ibGVtIGhlcmUgaXMgYWxzbyB0aGF0IGJlc2lkZXMgd2hhdGV2ZXIgdGhl
IHByb2ZpbGVzDQo+ID4gPiB3YW50cywNCj4gPiA+IGl0IG1pZ2h0IGhhcHBlbiB0aGF0IHRoZSBX
aUZpIHBvbGljeSBpcyBzdHJvbmdlciBhbmQgb25seSBhbGxvd3MNCj4gdXNpbmcNCj4gPiA+IHRo
ZSBXaUZpIGRldmljZSBhcyBXaUZpIG9yIGFzIEFNUC4gQW5kIGlmIFdpRmkgd2lucywgdGhlbiB3
ZSBoYXZlDQo+IHRvDQo+ID4gPiByaXANCj4gPiA+IHRoZSBBTVAgb3V0IG9mIHRoZSBzeXN0ZW0u
IEF0IHRoYXQgcG9pbnQgd2hhdGV2ZXIgdGhlIEJsdWV0b290aA0KPiA+ID4gcHJvZmlsZXMNCj4g
PiA+IHdhbnRlZCBpcyBwb2ludGxlc3Mgc2luY2UgdGhlIEFNUCBjb250cm9sbGVyIGlzIGdvbmUg
YW55d2F5Lg0KPiA+DQo+ID4gV2VsbCwgSSB0aGluayB0aGlzIGlzIGJlc3QgbWFuYWdlZCBieSBo
YXZpbmcgdGhlIFBBTCBhbm5vdW5jZSBpdHMNCj4gYXZhaWxhYmlsaXR5IChhbmQgdW5hdmFpbGFi
aWxpdHkpIHRvIEEyTVAsIHVzaW5nIHRoZSBzcGVjJ3MgSENJIGV2ZW50cw0KPiBmb3IgdGhpcyBw
dXJwb3NlLiAgQTJNUCBjYW4gdGhlbiBiZSB0aGUgYWdlbnQgdG8gbW92ZSB0aGUgY2hhbm5lbCB0
bw0KPiBBTVAgb3Igbm90LCBhY2NvcmRpbmdseS4gIEZvciBleGFtcGxlLCBpdCBtaWdodCBiZSB0
aGF0IHRoZSA4MDIuMTENCj4gZGV2aWNlIGlzIGJlaW5nIHVzZWQgZm9yIGFuIGluZnJhc3RydWN0
dXJlIGxpbmssIGFuZCB0aGVuIHRoYXQgbGluaw0KPiBnb2VzIGF3YXkuICBUaGUgUEFMIHdvdWxk
IHRoZW4gYW5ub3VuY2UgaXRzIGF2YWlsYWJpbGl0eSB0byB0aGUgQlQNCj4gc3RhY2sgKEEyTVAp
LCBhbmQgQTJNUCB3b3VsZCB0aGVuIG1vdmUgdGhlIGNoYW5uZWwgdG8gQU1QLg0KPiANCj4gQWdy
ZWVkLiBNeSBwb2ludCB3YXMganVzdCB0aGF0IGFwcGxpY2F0aW9uIGNhbiBvbmx5IHNldCBhIHBv
bGljeS4gVGhleQ0KPiBjYW4ndCBzd2l0Y2ggbWFudWFsbHkuIElmIGFwcGxpY2F0aW9uIHdvdWxk
IHN3aXRjaCBtYW51YWxseSB0aGVuIHRoZXkNCj4gbmVlZCB0byBjaGVjayBQQUwgYXZhaWxhYmls
aXR5IGV0Yy4gSSBkb24ndCB3YW50IHRoYXQgY29tcGxleGl0eSBpbnNpZGUNCj4gdGhlIGFwcGxp
Y2F0aW9ucy4NCj4gDQo+ID4gPiBUaGF0IHNhaWQsIG5vdGhpbmcgc3RvcHMgdXMgZnJvbSBhbGxv
d2luZyB0byBtYWtlIG91ciBBTVAgcG9saWNlcw0KPiA+ID4gZHluYW1pYyBhbmQgYWxsb3cgdGhl
IGFwcGxpY2F0aW9uIHRvIGNoYW5nZSBpdCB0aHJlc2hvbGQgZHVyaW5nDQo+ID4gPiBsaWZldGlt
ZS4gRm9yIGV4YW1wbGUgaXQgc3RhcnRzIG91dCBhcyBCUi9FRFIgb25seSBhbmQgdGhlbiBkdXJp
bmcNCj4gdGhlDQo+ID4gPiB1c2FnZSBvZiB0aGUgbGluayBpdCBkZWNpZGVzIHRoYXQgbm93IEFN
UCBwcmVmZXJyZWQgd291bGQgbWFrZQ0KPiBzZW5zZS4NCj4gPiA+IEluDQo+ID4gPiB0aGF0IHNl
bnNlIHlvdSB3b3VsZCBoYXZlIG1hbnVhbCBzd2l0Y2hpbmcgdG8gc29tZSBsZXZlbC4NCj4gPg0K
PiA+IFRoZSBvbmx5IHVzZSBjYXNlIEkgc2VlIGZvciBtYW51YWwgc3dpdGNoaW5nIGlzIGRlYnVn
Z2luZy4NCj4gDQo+IFRoYXQgaXMgZmluZS4gV2UgY2FuIGRvIHRoYXQgdmlhIGRlYnVnZnMuDQo+
IA0KPiBSZWdhcmRzDQo+IA0KPiBNYXJjZWwNCj4gDQoNCg==
^ permalink raw reply
* ERTM known bugs/regressions (was Re: [PATCH 0/8] ...)
From: Mat Martineau @ 2010-08-02 23:56 UTC (permalink / raw)
To: Marcel Holtmann, gustavo; +Cc: linux-bluetooth, rshaffer
In-Reply-To: <1280786940.12579.57.camel@localhost.localdomain>
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
^ permalink raw reply
* Re: seeking debug advice for a hanging serial device
From: Steve Castellotti @ 2010-08-02 23:21 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1280521176.2338.45.camel@localhost>
On Fri, 2010-07-30 at 13:19 -0700, Steve Castellotti wrote:
> However under Linux, the device frequently hangs within 5 seconds of
> attempting to read the data stream.
>
> This occurs whether through my code (which is platform agnostic) or
> simply cat'ing the RFCOMM device from a console (both in normal and raw
> mode).
Just to follow up on my own email (for the sake of future Google
searchers), I did arrive at a solution to my problem.
I switched to using the pybluez "bluetooth" module under Linux and
talking to the Bluetooth device using sockets instead of the RFCOMM
serial device:
http://pybluez.googlecode.com/svn/www/docs-0.7/index.html
Some example code:
import bluetooth
address = "XX:XX:XX:XX:XX:XX"
port = 3 # connect to channel 3
socket = bluetooth.BluetoothSocket( bluetooth.RFCOMM )
socket.connect((address, port))
while True:
try:
byte = sock.recv(1)
print byte
except Exception, e:
print e
sock.close()
I still don't understand why attempting to access the RFCOMM serial
device directly was failing under Linux but at least I am able to
proceed with my driver development.
Cheers
Steve Castellotti
^ permalink raw reply
* Re: Getting link quality or RSSI
From: Davide Pesavento @ 2010-08-02 22:58 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1280786989.12579.58.camel@localhost.localdomain>
On Tue, Aug 3, 2010 at 00:09, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Davide,
>
>> >> >> I'm writing an application to retrieve the current link quality (or
>> >> >> RSSI) between my laptop's Bluetooth adapter and a specific remote
>> >> >> device (my mobile phone). In order to do that I'm of course using the
>> >> >> latest version of BlueZ, but I've encountered several issues so far.
>> >> >>
>> >> >> The dbus API exposed by bluetoothd doesn't have any GetRSSI or
>> >> >> GetLinkQuality methods. Would it be possible to export these methods
>> >> >> in the public API?
>> >> >
>> >> > the link quality is vendor specific according to the specification and
>> >> > the RSSI of an existing connection is rather useless. So we don't bother
>> >> > to export those.
>> >> >
>> >>
>> >> Does this mean you're going to reject a patch which adds those methods
>> >> to the dbus API?
>> >
>> > constantly polling them via D-Bus, yes I would reject such a patch. To
>> > make this proper you would need a kernel patch first that polls the RSSI
>> > and/or link quality when a ACL is established and not in power saving
>> > mode. Then you need to use this data to send async signals via D-Bus.
>> >
>> > I have done both, let me assure you that some chips don't provide proper
>> > RSSI values. Then link quality is vendor specific and we can't do
>> > anything real useful with it (except it is a CSR chip). So I have don't
>> > the whole exercise and figured out that it is rather useless feature of
>> > Bluetooth.
>> >
>> >> >> Then I looked at the bluetooth HCI library that comes with bluez.
>> >> >> First of all, is there any reasons why it's totally undocumented?
>> >> >> Anyway, I noticed hci_read_link_quality() and hci_read_rssi() in
>> >> >> hci_lib.h and tried using them. Since they seem to require an
>> >> >> established connection, I also used hci_create_connection(). However I
>> >> >> soon discovered that creating a connection requires root privileges,
>> >> >> is that right or did I do something wrong?
>> >> >>
>> >> >> Furthermore, even when running the program as root, the connection
>> >> >> gets established only for a few seconds and then it disconnects from
>> >> >> the remote device. Is this behaviour intended? How can I specify to
>> >> >> keep the connection alive indefinitely?
>> >> >
>> >> > If you don't have an active connection that is used, the kernel will
>> >> > terminate any idle ones. So using hcitool for this is rather pointless
>> >> > unless you have a profile already using that connection.
>> >> >
>> >>
>> >> "using a connection" means pushing some traffic over it?
>> >> Is there no other way to prevent the kernel from terminating idle connections?
>> >
>> > You don't need to transfer data, but you need a reference count on the
>> > ACL link. And that can only happen by opening a L2CAP socket.
>> >
>> >> By the way, I've read in the Bluetooth specs that there exists an
>> >> extended inquiry mode which allows the host to gather the RSSI of
>> >> available devices too. How can I perform such kind of inquiry from my
>> >> application using bluez?
>> >
>> > Nice idea, but RSSI from inquiry result and RSSI from an ACL are not the
>> > same. You can't compare them properly to make sense out of them. I tried
>> > that as well. Please read the specification again to see their
>> > difference when it comes to power control on the low level baseband.
>> >
>>
>> Well, that's a non-issue. If I decide to go that way, I'll only use
>> RSSI from inquiry results so I don't need to compare RSSI values
>> coming from different sources.
>> How could I trigger an inquiry with RSSI reporting?
>
> that is one by default if the hardware supports it.
>
Very nice! I quickly checked using qdbusviewer and it does seem to
work. Thanks a lot for you help!
Regards,
Davide
^ permalink raw reply
* RE: Enhancements to allow l2cap channel to use either AMP or BR/EDR
From: Marcel Holtmann @ 2010-08-02 22:58 UTC (permalink / raw)
To: Kevin Hayes
Cc: tmonahan@codeaurora.org, David Vrabel, Inga Stotland,
linux-bluetooth@vger.kernel.org, rshaffer@codeaurora.org,
johan.hedberg@gmail.com
In-Reply-To: <B7132A25476D334D9130FE7532F2A56310A0B0896E@SC1EXMB-MBCL.global.atheros.com>
Hi Kevin,
> > please fix your mail client to NOT break threading.
> >
> > > >> AMP vs BR/EDR preference for L2CAP channel can be configured as
> > command
> > > line argument using new option "-J". Possible values:
> > > >> "require_br_edr",
> > > >> "prefer_amp",
> > > >> "prefer_br_edr"
> > > >> If no preference indicated, the default is set to require BR/EDR.
> > > >
> > > > I think an explicit channel move command and a channel move
> > complete
> > > event are what many applications/profiles will need to be able to use
> > an
> > > AMP effectively.
> > >
> > > A future AMP policy setting of "manual" could certainly give each
> > > application the freedom to move an l2cap channel at will, after
> > applying
> > > its own decision making process. However, with that freedom comes the
> > > responsibility of processing the AMP related events and initiating
> > the AMP
> > > commands to make the decision. For example, the application needs to
> > know
> > > if AMP controllers are available both locally and remotely, which in
> > turn
> > > requires initiating the AMP discovery process and looking over the
> > > available local HCI devices; a loss of the physical link event means
> > the
> > > application must move the link back to BR/EDR, etc. All of the
> > additional
> > > machinery to accomplish that could indeed be exposed for use by
> > > applications. I think such an approach, if really needed, would not
> > > conflict with these proposed policies that hide all of that within
> > the
> > > kernel.
> > >
> > > To illustrate how these policies simplify applications, consider
> > OBEX. An
> > > OBEX server application would choose "prefer_br_edr", thus leaving
> > the
> > > remote OBEX client to decide and initiate the channel move to an AMP,
> > and
> > > thus granting advance permission to the kernel for the move to take
> > place.
> > > The OBEX client could examine the size of an upcoming transfer and
> > choose
> > > "prefer_amp", thereby instructing the kernel to initiate the move to
> > an
> > > AMP if all local and remote conditions allow it. Thus, the OBEX
> > client and
> > > server applications make simplistic decisions to set up AMP, and can
> > focus
> > > on moving data over the l2cap channel, leaving the management of what
> > > medium the data flows over up to the kernel, who keeps it
> > transparent. In
> > > this way, work to make an existing profile "AMP aware" is also
> > minimal.
> > >
> > > In the discussion with Marcel regarding our AMP proposal in March
> > ("RFC:
> > > QuIC's AMP + eL2CAP Technical Plans"), where we had listed these AMP
> > > control policies, he seemed to indicate that these policies would
> > > automatically trigger AMP discovery within the kernel, which it will.
> > As
> > > he said, "Less options are less confusing for users" (apologies in
> > advance
> > > if I misinterpreted his answer).
> >
> > my current approach here is still that we wanna do this automatically
> > in
> > the background for the profiles. I think at most the profiles should
> > just provide a threshold for either a) staying on BR/EDR or b) moving
> > over to AMP.
> >
> > I know that the Bluetooth SIG tries to define such policies as part of
> > the profiles (like FTP for example), but I think this makes sense. I
> > doubt the real usage of making this manual. A profile/application
> > should
> > just give its preference or intent and then we try to accommodate them
> > as best as possible.
>
> Agree that it should be done "in background" and that a size threshold would be useful. But who evaluates that threshold? The bluez kernel components, which essentially implement a transport driver, should not be examining objects (files, phonebooks, etc) size to see if this threshold is met. Therefore, it would seem Tim's suggestion of having the profile send a 'prefer_amp' bit would be useful, right?
I would do something like "prefer_amp when over 100kb" or something. And
then the kernel needs to count. Meaning the L2CAP layer could easily
count this by itself.
Also just starting with setsockopt(bredr_only) and then later on just
doing setsockopt(prefer_amp) the userspace application would have
control over switching manually.
Since the polices are on a per socket basis, I don't see a big problem
with doing it like this.
The only thing that I don't want is a ioctl(switch_now_to_amp) since
that looks very wrong to me.
> > The other problem here is also that besides whatever the profiles
> > wants,
> > it might happen that the WiFi policy is stronger and only allows using
> > the WiFi device as WiFi or as AMP. And if WiFi wins, then we have to
> > rip
> > the AMP out of the system. At that point whatever the Bluetooth
> > profiles
> > wanted is pointless since the AMP controller is gone anyway.
>
> Well, I think this is best managed by having the PAL announce its availability (and unavailability) to A2MP, using the spec's HCI events for this purpose. A2MP can then be the agent to move the channel to AMP or not, accordingly. For example, it might be that the 802.11 device is being used for an infrastructure link, and then that link goes away. The PAL would then announce its availability to the BT stack (A2MP), and A2MP would then move the channel to AMP.
Agreed. My point was just that application can only set a policy. They
can't switch manually. If application would switch manually then they
need to check PAL availability etc. I don't want that complexity inside
the applications.
> > That said, nothing stops us from allowing to make our AMP polices
> > dynamic and allow the application to change it threshold during
> > lifetime. For example it starts out as BR/EDR only and then during the
> > usage of the link it decides that now AMP preferred would make sense.
> > In
> > that sense you would have manual switching to some level.
>
> The only use case I see for manual switching is debugging.
That is fine. We can do that via debugfs.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.
From: Marcel Holtmann @ 2010-08-02 22:51 UTC (permalink / raw)
To: Mat Martineau; +Cc: Gustavo F. Padovan, linux-bluetooth, rshaffer
In-Reply-To: <alpine.DEB.2.00.1008021519430.18432@linux-sea-02>
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
^ permalink raw reply
* Re: [PATCH] Add NULL checking for name param
From: Johan Hedberg @ 2010-08-02 22:50 UTC (permalink / raw)
To: Radoslaw Jablonski; +Cc: linux-bluetooth
In-Reply-To: <1280759046-11571-1-git-send-email-ext-jablonski.radoslaw@nokia.com>
Hi Radek,
On Mon, Aug 02, 2010, Radoslaw Jablonski wrote:
> Added NULL checking for "name" param in pbap_open_* functions. If on
> this level this variable will be NULL, then setting -EBADR error code
> and ending function. This fixes many obexd crashes.
> ---
> plugins/pbap.c | 13 +++++++++++--
> 1 files changed, 11 insertions(+), 2 deletions(-)
The patch is now upstream. Thanks.
Johan
^ permalink raw reply
* RE: Enhancements to allow l2cap channel to use either AMP or BR/EDR
From: Kevin Hayes @ 2010-08-02 22:41 UTC (permalink / raw)
To: Marcel Holtmann, tmonahan@codeaurora.org
Cc: David Vrabel, Inga Stotland, linux-bluetooth@vger.kernel.org,
rshaffer@codeaurora.org, johan.hedberg@gmail.com
In-Reply-To: <1280775200.12579.30.camel@localhost.localdomain>
SGkgTWFyY2VsLA0KDQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogbGlu
dXgtYmx1ZXRvb3RoLW93bmVyQHZnZXIua2VybmVsLm9yZyBbbWFpbHRvOmxpbnV4LWJsdWV0b290
aC0NCj4gb3duZXJAdmdlci5rZXJuZWwub3JnXSBPbiBCZWhhbGYgT2YgTWFyY2VsIEhvbHRtYW5u
DQo+IFNlbnQ6IE1vbmRheSwgQXVndXN0IDAyLCAyMDEwIDExOjUzIEFNDQo+IFRvOiB0bW9uYWhh
bkBjb2RlYXVyb3JhLm9yZw0KPiBTdWJqZWN0OiBSZTogRW5oYW5jZW1lbnRzIHRvIGFsbG93IGwy
Y2FwIGNoYW5uZWwgdG8gdXNlIGVpdGhlciBBTVAgb3INCj4gQlIvRURSDQo+IA0KPiBIaSBUaW0s
DQo+IA0KPiBwbGVhc2UgZml4IHlvdXIgbWFpbCBjbGllbnQgdG8gTk9UIGJyZWFrIHRocmVhZGlu
Zy4NCj4gDQo+ID4gPj4gQU1QIHZzIEJSL0VEUiBwcmVmZXJlbmNlIGZvciBMMkNBUCBjaGFubmVs
IGNhbiBiZSBjb25maWd1cmVkIGFzDQo+IGNvbW1hbmQNCj4gPiBsaW5lIGFyZ3VtZW50IHVzaW5n
IG5ldyBvcHRpb24gIi1KIi4gUG9zc2libGUgdmFsdWVzOg0KPiA+ID4+ICAgICAgICAgICAgICAg
ICAgInJlcXVpcmVfYnJfZWRyIiwNCj4gPiA+PiAgICAgICAgICAgICAgICAgICJwcmVmZXJfYW1w
IiwNCj4gPiA+PiAgICAgICAgICAgICAgICAgICJwcmVmZXJfYnJfZWRyIg0KPiA+ID4+IElmIG5v
IHByZWZlcmVuY2UgaW5kaWNhdGVkLCB0aGUgZGVmYXVsdCBpcyBzZXQgdG8gcmVxdWlyZSBCUi9F
RFIuDQo+ID4gPg0KPiA+ID4gSSB0aGluayBhbiBleHBsaWNpdCBjaGFubmVsIG1vdmUgY29tbWFu
ZCBhbmQgYSBjaGFubmVsIG1vdmUNCj4gY29tcGxldGUNCj4gPiBldmVudCBhcmUgd2hhdCBtYW55
IGFwcGxpY2F0aW9ucy9wcm9maWxlcyB3aWxsIG5lZWQgdG8gYmUgYWJsZSB0byB1c2UNCj4gYW4N
Cj4gPiBBTVAgZWZmZWN0aXZlbHkuDQo+ID4NCj4gPiBBIGZ1dHVyZSBBTVAgcG9saWN5IHNldHRp
bmcgb2YgIm1hbnVhbCIgY291bGQgY2VydGFpbmx5IGdpdmUgZWFjaA0KPiA+IGFwcGxpY2F0aW9u
IHRoZSBmcmVlZG9tIHRvIG1vdmUgYW4gbDJjYXAgY2hhbm5lbCBhdCB3aWxsLCBhZnRlcg0KPiBh
cHBseWluZw0KPiA+IGl0cyBvd24gZGVjaXNpb24gbWFraW5nIHByb2Nlc3MuIEhvd2V2ZXIsIHdp
dGggdGhhdCBmcmVlZG9tIGNvbWVzIHRoZQ0KPiA+IHJlc3BvbnNpYmlsaXR5IG9mIHByb2Nlc3Np
bmcgdGhlIEFNUCByZWxhdGVkIGV2ZW50cyBhbmQgaW5pdGlhdGluZw0KPiB0aGUgQU1QDQo+ID4g
Y29tbWFuZHMgdG8gbWFrZSB0aGUgZGVjaXNpb24uIEZvciBleGFtcGxlLCB0aGUgYXBwbGljYXRp
b24gbmVlZHMgdG8NCj4ga25vdw0KPiA+IGlmIEFNUCBjb250cm9sbGVycyBhcmUgYXZhaWxhYmxl
IGJvdGggbG9jYWxseSBhbmQgcmVtb3RlbHksIHdoaWNoIGluDQo+IHR1cm4NCj4gPiByZXF1aXJl
cyBpbml0aWF0aW5nIHRoZSBBTVAgZGlzY292ZXJ5IHByb2Nlc3MgYW5kIGxvb2tpbmcgb3ZlciB0
aGUNCj4gPiBhdmFpbGFibGUgbG9jYWwgSENJIGRldmljZXM7IGEgbG9zcyBvZiB0aGUgcGh5c2lj
YWwgbGluayBldmVudCBtZWFucw0KPiB0aGUNCj4gPiBhcHBsaWNhdGlvbiBtdXN0IG1vdmUgdGhl
IGxpbmsgYmFjayB0byBCUi9FRFIsIGV0Yy4gQWxsIG9mIHRoZQ0KPiBhZGRpdGlvbmFsDQo+ID4g
bWFjaGluZXJ5IHRvIGFjY29tcGxpc2ggdGhhdCBjb3VsZCBpbmRlZWQgYmUgZXhwb3NlZCBmb3Ig
dXNlIGJ5DQo+ID4gYXBwbGljYXRpb25zLiBJIHRoaW5rIHN1Y2ggYW4gYXBwcm9hY2gsIGlmIHJl
YWxseSBuZWVkZWQsIHdvdWxkIG5vdA0KPiA+IGNvbmZsaWN0IHdpdGggdGhlc2UgcHJvcG9zZWQg
cG9saWNpZXMgdGhhdCBoaWRlIGFsbCBvZiB0aGF0IHdpdGhpbg0KPiB0aGUNCj4gPiBrZXJuZWwu
DQo+ID4NCj4gPiBUbyBpbGx1c3RyYXRlIGhvdyB0aGVzZSBwb2xpY2llcyBzaW1wbGlmeSBhcHBs
aWNhdGlvbnMsIGNvbnNpZGVyDQo+IE9CRVguIEFuDQo+ID4gT0JFWCBzZXJ2ZXIgYXBwbGljYXRp
b24gd291bGQgY2hvb3NlICJwcmVmZXJfYnJfZWRyIiwgdGh1cyBsZWF2aW5nDQo+IHRoZQ0KPiA+
IHJlbW90ZSBPQkVYIGNsaWVudCB0byBkZWNpZGUgYW5kIGluaXRpYXRlIHRoZSBjaGFubmVsIG1v
dmUgdG8gYW4gQU1QLA0KPiBhbmQNCj4gPiB0aHVzIGdyYW50aW5nIGFkdmFuY2UgcGVybWlzc2lv
biB0byB0aGUga2VybmVsIGZvciB0aGUgbW92ZSB0byB0YWtlDQo+IHBsYWNlLg0KPiA+IFRoZSBP
QkVYIGNsaWVudCBjb3VsZCBleGFtaW5lIHRoZSBzaXplIG9mIGFuIHVwY29taW5nIHRyYW5zZmVy
IGFuZA0KPiBjaG9vc2UNCj4gPiAicHJlZmVyX2FtcCIsIHRoZXJlYnkgaW5zdHJ1Y3RpbmcgdGhl
IGtlcm5lbCB0byBpbml0aWF0ZSB0aGUgbW92ZSB0bw0KPiBhbg0KPiA+IEFNUCBpZiBhbGwgbG9j
YWwgYW5kIHJlbW90ZSBjb25kaXRpb25zIGFsbG93IGl0LiBUaHVzLCB0aGUgT0JFWA0KPiBjbGll
bnQgYW5kDQo+ID4gc2VydmVyIGFwcGxpY2F0aW9ucyBtYWtlIHNpbXBsaXN0aWMgZGVjaXNpb25z
IHRvIHNldCB1cCBBTVAsIGFuZCBjYW4NCj4gZm9jdXMNCj4gPiBvbiBtb3ZpbmcgZGF0YSBvdmVy
IHRoZSBsMmNhcCBjaGFubmVsLCBsZWF2aW5nIHRoZSBtYW5hZ2VtZW50IG9mIHdoYXQNCj4gPiBt
ZWRpdW0gdGhlIGRhdGEgZmxvd3Mgb3ZlciB1cCB0byB0aGUga2VybmVsLCB3aG8ga2VlcHMgaXQN
Cj4gdHJhbnNwYXJlbnQuIEluDQo+ID4gdGhpcyB3YXksIHdvcmsgdG8gbWFrZSBhbiBleGlzdGlu
ZyBwcm9maWxlICJBTVAgYXdhcmUiIGlzIGFsc28NCj4gbWluaW1hbC4NCj4gPg0KPiA+IEluIHRo
ZSBkaXNjdXNzaW9uIHdpdGggTWFyY2VsIHJlZ2FyZGluZyBvdXIgQU1QIHByb3Bvc2FsIGluIE1h
cmNoDQo+ICgiUkZDOg0KPiA+IFF1SUMncyBBTVAgKyBlTDJDQVAgVGVjaG5pY2FsIFBsYW5zIiks
IHdoZXJlIHdlIGhhZCBsaXN0ZWQgdGhlc2UgQU1QDQo+ID4gY29udHJvbCBwb2xpY2llcywgaGUg
c2VlbWVkIHRvIGluZGljYXRlIHRoYXQgdGhlc2UgcG9saWNpZXMgd291bGQNCj4gPiBhdXRvbWF0
aWNhbGx5IHRyaWdnZXIgQU1QIGRpc2NvdmVyeSB3aXRoaW4gdGhlIGtlcm5lbCwgd2hpY2ggaXQg
d2lsbC4NCj4gQXMNCj4gPiBoZSBzYWlkLCAiTGVzcyBvcHRpb25zIGFyZSBsZXNzIGNvbmZ1c2lu
ZyBmb3IgdXNlcnMiIChhcG9sb2dpZXMgaW4NCj4gYWR2YW5jZQ0KPiA+IGlmIEkgbWlzaW50ZXJw
cmV0ZWQgaGlzIGFuc3dlcikuDQo+IA0KPiBteSBjdXJyZW50IGFwcHJvYWNoIGhlcmUgaXMgc3Rp
bGwgdGhhdCB3ZSB3YW5uYSBkbyB0aGlzIGF1dG9tYXRpY2FsbHkNCj4gaW4NCj4gdGhlIGJhY2tn
cm91bmQgZm9yIHRoZSBwcm9maWxlcy4gSSB0aGluayBhdCBtb3N0IHRoZSBwcm9maWxlcyBzaG91
bGQNCj4ganVzdCBwcm92aWRlIGEgdGhyZXNob2xkIGZvciBlaXRoZXIgYSkgc3RheWluZyBvbiBC
Ui9FRFIgb3IgYikgbW92aW5nDQo+IG92ZXIgdG8gQU1QLg0KPiANCj4gSSBrbm93IHRoYXQgdGhl
IEJsdWV0b290aCBTSUcgdHJpZXMgdG8gZGVmaW5lIHN1Y2ggcG9saWNpZXMgYXMgcGFydCBvZg0K
PiB0aGUgcHJvZmlsZXMgKGxpa2UgRlRQIGZvciBleGFtcGxlKSwgYnV0IEkgdGhpbmsgdGhpcyBt
YWtlcyBzZW5zZS4gSQ0KPiBkb3VidCB0aGUgcmVhbCB1c2FnZSBvZiBtYWtpbmcgdGhpcyBtYW51
YWwuIEEgcHJvZmlsZS9hcHBsaWNhdGlvbg0KPiBzaG91bGQNCj4ganVzdCBnaXZlIGl0cyBwcmVm
ZXJlbmNlIG9yIGludGVudCBhbmQgdGhlbiB3ZSB0cnkgdG8gYWNjb21tb2RhdGUgdGhlbQ0KPiBh
cyBiZXN0IGFzIHBvc3NpYmxlLg0KDQpBZ3JlZSB0aGF0IGl0IHNob3VsZCBiZSBkb25lICJpbiBi
YWNrZ3JvdW5kIiBhbmQgdGhhdCBhIHNpemUgdGhyZXNob2xkIHdvdWxkIGJlIHVzZWZ1bC4gIEJ1
dCB3aG8gZXZhbHVhdGVzIHRoYXQgdGhyZXNob2xkPyAgVGhlIGJsdWV6IGtlcm5lbCBjb21wb25l
bnRzLCB3aGljaCBlc3NlbnRpYWxseSBpbXBsZW1lbnQgYSB0cmFuc3BvcnQgZHJpdmVyLCBzaG91
bGQgbm90IGJlIGV4YW1pbmluZyBvYmplY3RzIChmaWxlcywgcGhvbmVib29rcywgZXRjKSBzaXpl
IHRvIHNlZSBpZiB0aGlzIHRocmVzaG9sZCBpcyBtZXQuICBUaGVyZWZvcmUsIGl0IHdvdWxkIHNl
ZW0gVGltJ3Mgc3VnZ2VzdGlvbiBvZiBoYXZpbmcgdGhlIHByb2ZpbGUgc2VuZCBhICdwcmVmZXJf
YW1wJyBiaXQgd291bGQgYmUgdXNlZnVsLCByaWdodD8NCg0KPiANCj4gVGhlIG90aGVyIHByb2Js
ZW0gaGVyZSBpcyBhbHNvIHRoYXQgYmVzaWRlcyB3aGF0ZXZlciB0aGUgcHJvZmlsZXMNCj4gd2Fu
dHMsDQo+IGl0IG1pZ2h0IGhhcHBlbiB0aGF0IHRoZSBXaUZpIHBvbGljeSBpcyBzdHJvbmdlciBh
bmQgb25seSBhbGxvd3MgdXNpbmcNCj4gdGhlIFdpRmkgZGV2aWNlIGFzIFdpRmkgb3IgYXMgQU1Q
LiBBbmQgaWYgV2lGaSB3aW5zLCB0aGVuIHdlIGhhdmUgdG8NCj4gcmlwDQo+IHRoZSBBTVAgb3V0
IG9mIHRoZSBzeXN0ZW0uIEF0IHRoYXQgcG9pbnQgd2hhdGV2ZXIgdGhlIEJsdWV0b290aA0KPiBw
cm9maWxlcw0KPiB3YW50ZWQgaXMgcG9pbnRsZXNzIHNpbmNlIHRoZSBBTVAgY29udHJvbGxlciBp
cyBnb25lIGFueXdheS4NCg0KV2VsbCwgSSB0aGluayB0aGlzIGlzIGJlc3QgbWFuYWdlZCBieSBo
YXZpbmcgdGhlIFBBTCBhbm5vdW5jZSBpdHMgYXZhaWxhYmlsaXR5IChhbmQgdW5hdmFpbGFiaWxp
dHkpIHRvIEEyTVAsIHVzaW5nIHRoZSBzcGVjJ3MgSENJIGV2ZW50cyBmb3IgdGhpcyBwdXJwb3Nl
LiAgQTJNUCBjYW4gdGhlbiBiZSB0aGUgYWdlbnQgdG8gbW92ZSB0aGUgY2hhbm5lbCB0byBBTVAg
b3Igbm90LCBhY2NvcmRpbmdseS4gIEZvciBleGFtcGxlLCBpdCBtaWdodCBiZSB0aGF0IHRoZSA4
MDIuMTEgZGV2aWNlIGlzIGJlaW5nIHVzZWQgZm9yIGFuIGluZnJhc3RydWN0dXJlIGxpbmssIGFu
ZCB0aGVuIHRoYXQgbGluayBnb2VzIGF3YXkuICBUaGUgUEFMIHdvdWxkIHRoZW4gYW5ub3VuY2Ug
aXRzIGF2YWlsYWJpbGl0eSB0byB0aGUgQlQgc3RhY2sgKEEyTVApLCBhbmQgQTJNUCB3b3VsZCB0
aGVuIG1vdmUgdGhlIGNoYW5uZWwgdG8gQU1QLg0KDQo+IA0KPiBUaGF0IHNhaWQsIG5vdGhpbmcg
c3RvcHMgdXMgZnJvbSBhbGxvd2luZyB0byBtYWtlIG91ciBBTVAgcG9saWNlcw0KPiBkeW5hbWlj
IGFuZCBhbGxvdyB0aGUgYXBwbGljYXRpb24gdG8gY2hhbmdlIGl0IHRocmVzaG9sZCBkdXJpbmcN
Cj4gbGlmZXRpbWUuIEZvciBleGFtcGxlIGl0IHN0YXJ0cyBvdXQgYXMgQlIvRURSIG9ubHkgYW5k
IHRoZW4gZHVyaW5nIHRoZQ0KPiB1c2FnZSBvZiB0aGUgbGluayBpdCBkZWNpZGVzIHRoYXQgbm93
IEFNUCBwcmVmZXJyZWQgd291bGQgbWFrZSBzZW5zZS4NCj4gSW4NCj4gdGhhdCBzZW5zZSB5b3Ug
d291bGQgaGF2ZSBtYW51YWwgc3dpdGNoaW5nIHRvIHNvbWUgbGV2ZWwuDQoNClRoZSBvbmx5IHVz
ZSBjYXNlIEkgc2VlIGZvciBtYW51YWwgc3dpdGNoaW5nIGlzIGRlYnVnZ2luZy4NCg0KCUsrKw0K
^ permalink raw reply
* Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.
From: Mat Martineau @ 2010-08-02 22:40 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: Marcel Holtmann, linux-bluetooth, rshaffer
In-Reply-To: <20100802212014.GB20149@vigoh>
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
^ permalink raw reply
* Re: [PATCH] obexd: ebook backend: return all TEL attribs from vcard in string
From: Johan Hedberg @ 2010-08-02 22:20 UTC (permalink / raw)
To: Marcel Mol; +Cc: linux-bluetooth
In-Reply-To: <201008022209.o72M94DV017676@joshua.mesa.nl>
Hi Marcel,
On Tue, Aug 03, 2010, Marcel Mol wrote:
> evcard_to_string() only took one TEL attrib from a vcard. But
> multiple TEL attribs are supported. This patch will convert
> all TEL attribs to strings.
> ---
> plugins/phonebook-ebook.c | 22 ++++++++++++++--------
> 1 files changed, 14 insertions(+), 8 deletions(-)
Thanks for the quick update. The patch is now pushed upstream with a
couple more minor coding style cleanups (for which I didn't bother
having yet another review round ;) Take a look at the upstream tree if
you're interested in how exactly the final patch (and commit message)
ended up looking like.
Johan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox