* 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
* Re: [PATCH 6/8] Bluetooth: Move stream-oriented recvmsg code so it can be used by L2CAP.
From: Mat Martineau @ 2010-08-02 22:13 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1280778398.12579.46.camel@localhost.localdomain>
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
^ permalink raw reply
* Re: Getting link quality or RSSI
From: Marcel Holtmann @ 2010-08-02 22:09 UTC (permalink / raw)
To: Davide Pesavento; +Cc: linux-bluetooth
In-Reply-To: <AANLkTimrcSXZ-wTz7nmKCYtXjTEHY8FiPSiubRyfusxY@mail.gmail.com>
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.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 0/8] Bluetooth: L2CAP updates for PSM validation and ERTM.
From: Marcel Holtmann @ 2010-08-02 22:09 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <alpine.DEB.2.00.1008021450290.18432@linux-sea-02>
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
^ permalink raw reply
* [PATCH] obexd: ebook backend: return all TEL attribs from vcard in string
From: Marcel Mol @ 2010-08-02 22:08 UTC (permalink / raw)
To: linux-bluetooth
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(-)
diff --git a/plugins/phonebook-ebook.c b/plugins/phonebook-ebook.c
index 089b956..e80f797 100644
--- a/plugins/phonebook-ebook.c
+++ b/plugins/phonebook-ebook.c
@@ -97,6 +97,7 @@ static char *evcard_to_string(EVCard *evcard, unsigned int format,
uint64_t filter)
{
EVCard *evcard2;
+ GList *l;
char *vcard;
unsigned int i;
@@ -109,18 +110,23 @@ static char *evcard_to_string(EVCard *evcard, unsigned int format,
*/
filter = format == EVC_FORMAT_VCARD_30 ? filter | 0x87: filter | 0x85;
+ l = e_vcard_get_attributes(evcard);
evcard2 = e_vcard_new();
- for (i = 0; i < 29; i++) {
- EVCardAttribute *attrib;
-
- if (!(filter & (1 << i)))
- continue;
+ for (; l; l = g_list_next(l)) {
+ EVCardAttribute *attrib = l->data;
+ const char *name;
- attrib = e_vcard_get_attribute(evcard, attribute_mask[i]);
if (!attrib)
continue;
-
- e_vcard_add_attribute(evcard2, e_vcard_attribute_copy(attrib));
+ name = e_vcard_attribute_get_name(attrib);
+ for (i = 0; attribute_mask[i] != NULL; i++) {
+ if (!(filter & (1 << i)))
+ continue;
+ if (g_strcmp0(name, attribute_mask[i]) != 0)
+ continue;
+ e_vcard_add_attribute(evcard2,
+ e_vcard_attribute_copy(attrib));
+ }
}
vcard = e_vcard_to_string(evcard2, format);
--
1.7.2
^ permalink raw reply related
* Re: Getting link quality or RSSI
From: Davide Pesavento @ 2010-08-02 22:07 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1280775482.12579.34.camel@localhost.localdomain>
On Mon, Aug 2, 2010 at 20:58, 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?
Thanks,
Davide
^ permalink raw reply
* Re: [PATCH 0/8] Bluetooth: L2CAP updates for PSM validation and ERTM.
From: Mat Martineau @ 2010-08-02 22:02 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1280778649.12579.50.camel@localhost.localdomain>
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
^ permalink raw reply
* Re: [PATCH] obexd: ebook backend: return all TEL attribs from vcard in string
From: Johan Hedberg @ 2010-08-02 21:57 UTC (permalink / raw)
To: Marcel Mol; +Cc: linux-bluetooth
In-Reply-To: <201008022131.o72LVpuL011014@joshua.mesa.nl>
On Mon, Aug 02, 2010, Marcel Mol wrote:
> + for (; l; l = g_list_next(l)) {
> + EVCardAttribute *attrib = l->data;
> if (!attrib)
> continue;
Add an empty line after variable declarations.
> -
> - e_vcard_add_attribute(evcard2, e_vcard_attribute_copy(attrib));
> + const char *name = e_vcard_attribute_get_name(attrib);
plugins/phonebook.c: In function ‘evcard_to_string’:
plugins/phonebook.c:119: error: ISO C90 forbids mixed declarations and code
make[1]: *** [plugins/phonebook.o] Error 1
make: *** [all] Error 2
> + for (i = 0; attribute_mask[i] != NULL; i++) {
> + if (!(filter & (1 << i)))
> + continue;
> + if (g_strcmp(name, attribute_mask[i]) != 0)
plugins/phonebook.c:123: error: implicit declaration of function ‘g_strcmp’
Probably some missing include. Furthermore, use g_strcmp0 if there's a
risk that one of the inputs might be NULL.
Always compile test with ./bootstrap-configure (in this case
--with-phonebook=ebook) before submitting patches.
Johan
^ permalink raw reply
* Re: Getting link quality or RSSI
From: Davide Pesavento @ 2010-08-02 21:55 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: Marcel Holtmann, linux-bluetooth
In-Reply-To: <AANLkTimExH027cpFPmMS8vd7be9HkBWzok9tzWqNJFeP@mail.gmail.com>
On Mon, Aug 2, 2010 at 20:12, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi,
>
> On Mon, Aug 2, 2010 at 7:34 PM, Davide Pesavento <davidepesa@gmail.com> wrote:
>> "using a connection" means pushing some traffic over it?
>> Is there no other way to prevent the kernel from terminating idle connections?
>
> You should really think twice if you are creating a connection just to
> see the link quality, it really doesn't worth.
>
OK, thanks for the advice.
>> 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?
>
> We do expose the RSSI when doing the device discovery, take a look at
> DeviceFound documentation under doc/adapter-api.txt, as for link
> quality maybe it can be made as a Property of device object, but as
> Marcel said this is vendor specific so the use cases are very limited.
> In the other hand it would be very nice if ui could show the link
> quality and we could possible use it to route the audio back to the
> speaker if the connection quality is too low.
>
What I'm trying to accomplish is indeed similar to that: I'd like to
re-route some audio streams based on RSSI or link quality.
>
> --
> Luiz Augusto von Dentz
> Computer Engineer
>
Regards,
Davide
^ permalink raw reply
* [PATCH] obexd: ebook backend: return all TEL attribs from vcard in string
From: Marcel Mol @ 2010-08-02 21:29 UTC (permalink / raw)
To: linux-bluetooth
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, 13 insertions(+), 9 deletions(-)
diff --git a/plugins/phonebook-ebook.c b/plugins/phonebook-ebook.c
index 089b956..d2482c0 100644
--- a/plugins/phonebook-ebook.c
+++ b/plugins/phonebook-ebook.c
@@ -97,6 +97,7 @@ static char *evcard_to_string(EVCard *evcard, unsigned int format,
uint64_t filter)
{
EVCard *evcard2;
+ GList *l;
char *vcard;
unsigned int i;
@@ -109,18 +110,21 @@ static char *evcard_to_string(EVCard *evcard, unsigned int format,
*/
filter = format == EVC_FORMAT_VCARD_30 ? filter | 0x87: filter | 0x85;
+ l = e_vcard_get_attributes(evcard);
evcard2 = e_vcard_new();
- for (i = 0; i < 29; i++) {
- EVCardAttribute *attrib;
-
- if (!(filter & (1 << i)))
- continue;
-
- attrib = e_vcard_get_attribute(evcard, attribute_mask[i]);
+ for (; l; l = g_list_next(l)) {
+ EVCardAttribute *attrib = l->data;
if (!attrib)
continue;
-
- e_vcard_add_attribute(evcard2, e_vcard_attribute_copy(attrib));
+ const char *name = e_vcard_attribute_get_name(attrib);
+ for (i = 0; attribute_mask[i] != NULL; i++) {
+ if (!(filter & (1 << i)))
+ continue;
+ if (g_strcmp(name, attribute_mask[i]) != 0)
+ continue;
+ e_vcard_add_attribute(evcard2,
+ e_vcard_attribute_copy(attrib));
+ }
}
vcard = e_vcard_to_string(evcard2, format);
--
1.7.2
^ permalink raw reply related
* [PATCH] obexd: ebook backend: return all TEL attribs from vcard in string
From: Marcel Mol @ 2010-08-02 21:29 UTC (permalink / raw)
To: linux-bluetooth
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, 13 insertions(+), 9 deletions(-)
diff --git a/plugins/phonebook-ebook.c b/plugins/phonebook-ebook.c
index 089b956..d2482c0 100644
--- a/plugins/phonebook-ebook.c
+++ b/plugins/phonebook-ebook.c
@@ -97,6 +97,7 @@ static char *evcard_to_string(EVCard *evcard, unsigned int format,
uint64_t filter)
{
EVCard *evcard2;
+ GList *l;
char *vcard;
unsigned int i;
@@ -109,18 +110,21 @@ static char *evcard_to_string(EVCard *evcard, unsigned int format,
*/
filter = format == EVC_FORMAT_VCARD_30 ? filter | 0x87: filter | 0x85;
+ l = e_vcard_get_attributes(evcard);
evcard2 = e_vcard_new();
- for (i = 0; i < 29; i++) {
- EVCardAttribute *attrib;
-
- if (!(filter & (1 << i)))
- continue;
-
- attrib = e_vcard_get_attribute(evcard, attribute_mask[i]);
+ for (; l; l = g_list_next(l)) {
+ EVCardAttribute *attrib = l->data;
if (!attrib)
continue;
-
- e_vcard_add_attribute(evcard2, e_vcard_attribute_copy(attrib));
+ const char *name = e_vcard_attribute_get_name(attrib);
+ for (i = 0; attribute_mask[i] != NULL; i++) {
+ if (!(filter & (1 << i)))
+ continue;
+ if (g_strcmp0(name, attribute_mask[i]) != 0)
+ continue;
+ e_vcard_add_attribute(evcard2,
+ e_vcard_attribute_copy(attrib));
+ }
}
vcard = e_vcard_to_string(evcard2, format);
--
1.7.2
^ permalink raw reply related
* Re: [PATCH 3/8] Bluetooth: Validate PSM values in calls to connect() and bind().
From: Gustavo F. Padovan @ 2010-08-02 21:24 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Mat Martineau, linux-bluetooth, rshaffer, linux-arm-msm
In-Reply-To: <1280777991.12579.39.camel@localhost.localdomain>
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
^ permalink raw reply
* Re: [PATCH v4 0/5] Enhanced support for extended inquiry response
From: ingas @ 2010-08-02 21:24 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <20100802185542.GA28641@jh-x301>
Hi Johan,
Thanks for your reply.
> Hi Inga,
>
> First of all, sorry for the slight delay in my reply. I've been on
> holiday last week with quite limted email processing capability.
>
> On Wed, Jul 28, 2010, ingas@codeaurora.org wrote:
>> In the current implementation, EIR write is keyed off HCI Command
>> Complete events for a set of commands (in security.c).
>> - When first registering an OPP service with sdptool, the
>> update_svcclass_list() (sdpd-service.c) calls manager_update_svc()
>> (manager.c) which in turn calls adapter_update() (adapter.c). BTW, why
>> this level of indirection, since manager_update_svc() is called only
>> from
>> one place in the code and calling adapter_update() is all it does?
>> Probably some earlier architectural decision I am not aware of…
>
> That looks pretty stupid to me too. Might be some historical artifact.
> Anyway, feel free to send a patch to remove this indirection (i.e. call
> adapter_update directly).
>
Will do once i get through the initial submission.
>> The current implementation may be extended as following:
>> Since update_svcclass_list() is being called throughout sdpd-service.c
>> in
>> all the places where EIR needs to be updated as well, I propose to
>> modify
>> the corresponding function adapter_set_service_classes() (adapter.c) :
>>
>> **********************
>> Before:
>> /* 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)
>> return 0;
>>
>>
>> ***********************
>>
>> After:
>> /* 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->cache_enable || adapter->pending_cod)
>> return 0;
>>
>> /* 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;
>> }
>>
>> **********************
>> Will that be acceptable? I tested this modification and it’s working
>> with
>> both d-bus methods and sdptool.
>
> Seems good to me.
>
I just submitted an updated patch set for EIR (hopefully, not messing up
my tabs an spaces in the process). This is certainly a learning process
and your patience is appreciated :)
Regards,
Inga
^ permalink raw reply
* Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.
From: Gustavo F. Padovan @ 2010-08-02 21:20 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Mat Martineau, linux-bluetooth, rshaffer, linux-arm-msm
In-Reply-To: <1280777912.12579.37.camel@localhost.localdomain>
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
^ permalink raw reply
* Re: [PATCH 2/2] ebook backend: return all TEL attribs from vcard in string
From: Johan Hedberg @ 2010-08-02 20:06 UTC (permalink / raw)
To: Marcel Mol; +Cc: linux-bluetooth
In-Reply-To: <201007281324.o6SDO9YB010621@joshua.mesa.nl>
Hi,
On Wed, Jul 28, 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 | 24 +++++++++++++-----------
> 1 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/plugins/phonebook-ebook.c b/plugins/phonebook-ebook.c
> index 089b956..04b7900 100644
> --- a/plugins/phonebook-ebook.c
> +++ b/plugins/phonebook-ebook.c
> @@ -97,6 +97,7 @@ static char *evcard_to_string(EVCard *evcard, unsigned int format,
> uint64_t filter)
> {
> EVCard *evcard2;
> + GList *l;
> char *vcard;
> unsigned int i;
>
> @@ -109,18 +110,19 @@ static char *evcard_to_string(EVCard *evcard, unsigned int format,
> */
> filter = format == EVC_FORMAT_VCARD_30 ? filter | 0x87: filter | 0x85;
>
> + l = e_vcard_get_attributes(evcard);
> evcard2 = e_vcard_new();
> - for (i = 0; i < 29; i++) {
> - EVCardAttribute *attrib;
> -
> - if (!(filter & (1 << i)))
> - continue;
> -
> - attrib = e_vcard_get_attribute(evcard, attribute_mask[i]);
> - if (!attrib)
> - continue;
> -
> - e_vcard_add_attribute(evcard2, e_vcard_attribute_copy(attrib));
> + for (; l; l = g_list_next(l)) {
> + EVCardAttribute *attrib = l->data;
> + if (attrib) {
> + const char *name = e_vcard_attribute_get_name(attrib);
> + for (i = 0; i < 29; i++) {
> + if (!(filter & (1 << i)))
> + continue;
> + if (!strcmp(name, attribute_mask[i]))
> + e_vcard_add_attribute(evcard2, e_vcard_attribute_copy(attrib));
> + }
> + }
> }
This needs to be cleaned up a bit in order not to be so deeply nested
which additionally causes you problems with producing over 80-character
long lines. You can avoid that with keeping the same
if (!attrib)
continue;
trick as the original code had as well as
if (g_strcmp0(name, attribute_mask[i]) != 0)
continue;
Johan
^ permalink raw reply
* Re: [PATCH 7/8] Bluetooth: Use a stream-oriented recvmsg with SOCK_STREAM L2CAP sockets.
From: Marcel Holtmann @ 2010-08-02 19:53 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1280776810-18213-8-git-send-email-mathewm@codeaurora.org>
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
^ permalink raw reply
* Re: [PATCH 0/8] Bluetooth: L2CAP updates for PSM validation and ERTM.
From: Marcel Holtmann @ 2010-08-02 19:50 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1280776810-18213-1-git-send-email-mathewm@codeaurora.org>
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
^ permalink raw reply
* Re: [PATCH 8/8] Bluetooth: Use 3-DH5 payload size for default ERTM max PDU size.
From: Marcel Holtmann @ 2010-08-02 19:48 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1280776810-18213-9-git-send-email-mathewm@codeaurora.org>
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
^ permalink raw reply
* Re: [PATCH 6/8] Bluetooth: Move stream-oriented recvmsg code so it can be used by L2CAP.
From: Marcel Holtmann @ 2010-08-02 19:46 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1280776810-18213-7-git-send-email-mathewm@codeaurora.org>
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
^ permalink raw reply
* Re: [PATCH 1/2] Fix vcard filter setting
From: Johan Hedberg @ 2010-08-02 19:45 UTC (permalink / raw)
To: Marcel Mol; +Cc: linux-bluetooth
In-Reply-To: <201007281324.o6SDO2oH010611@joshua.mesa.nl>
Hi,
On Tue, Jul 27, 2010, Marcel Mol wrote:
> The bits for VCARD version 2.1 and version 3.0 were wrong:
> oin case of 2.1 the FN bit was on and for 3.0 it is off.
> The comment state that 3.0 needs FN while 2.1 does not.
> This patch fixes that.
> ---
> plugins/phonebook-ebook.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
Thanks for the patch. It's now in the upstream obexd tree.
Johan
^ permalink raw reply
* Re: [PATCH 5/8] Bluetooth: Don't modify remote_tx_win when receiving a config response. Only config requests should set remote_tx_win.
From: Marcel Holtmann @ 2010-08-02 19:42 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1280776810-18213-6-git-send-email-mathewm@codeaurora.org>
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
^ permalink raw reply
* Re: [PATCH 4/8] Bluetooth: Do endianness conversion on MPS configuration value before doing comparisons.
From: Marcel Holtmann @ 2010-08-02 19:41 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1280776810-18213-5-git-send-email-mathewm@codeaurora.org>
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
^ permalink raw reply
* Re: [PATCH 2/8] Bluetooth: Change default ERTM retransmit timeout to 2 seconds, as the spec requires.
From: Marcel Holtmann @ 2010-08-02 19:40 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1280776810-18213-3-git-send-email-mathewm@codeaurora.org>
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
^ permalink raw reply
* Re: [PATCH 3/8] Bluetooth: Validate PSM values in calls to connect() and bind().
From: Marcel Holtmann @ 2010-08-02 19:39 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1280776810-18213-4-git-send-email-mathewm@codeaurora.org>
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
^ 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 19:38 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1280776810-18213-2-git-send-email-mathewm@codeaurora.org>
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
^ 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