reverse SDP issue

reverse SDP issue

[Mike]

Hi,

I notice a change in commit d2920be715974795b51f9cc3279947104da3647b
[1] that changes the "reverse" variable for an SDP query:

 -       device_browse_sdp(device, NULL, NULL, NULL, TRUE);
+       if (device_is_bredr(device))
+               device_browse_sdp(device, NULL, NULL, NULL, FALSE);
+       else
+               device_browse_primary(device, NULL, NULL, FALSE);

You can see the original had reverse as TRUE, but the patch may have
inadvertently changed it to FALSE.  The only reason I notice this is
that for some reason I get a debug output that BlueZ is removing
"0000110d-0000-1000-8000-00805f9b34fb" (ADVANCED_AUDIO_UUID) which
then knocks out my HFP and A2DP profiles.

bluetoothd[766]: src/device.c:device_remove_drivers() UUID
0000110d-0000-1000-8000-00805f9b34fb was removed from device
00:17:E3:3B:4F:DD
bluetoothd[766]: audio/unix.c:unix_device_removed()
unix_device_removed(0x2a0d0358)
bluetoothd[766]: audio/gateway.c:path_unregister() Unregistered
interface org.bluez.HandsfreeGateway on path
/org/bluez/766/hci0/dev_00_17_E3_3B_4F_DD
bluetoothd[766]: audio/source.c:path_unregister() Unregistered
interface org.bluez.AudioSource on path
/org/bluez/766/hci0/dev_00_17_E3_3B_4F_DD

There may be another subtle bug that is the real culprit here (e.g.,
why does removing 0x110d knock out the other profiles), but I thought
I'd check on this first.

Mike

[1] http://git.kernel.org/?p=bluetooth/bluez.git;a=commitdiff;h=d2920be715974795b51f9cc3279947104da3647b

Re: reverse SDP issue

[Mike]

On Mon, Mar 19, 2012 at 5:19 PM, Mike <puffy.taco@gmail.com> wrote:
>
> There may be another subtle bug that is the real culprit here (e.g.,
> why does removing 0x110d knock out the other profiles), but I thought
> I'd check on this first.
>

Ah, I can see that Mikel Astiz's patches (from inspection) should
solve this part of the problem:

[PATCH v0 2/3] Add device driver support for partial unloading
http://www.spinics.net/lists/linux-bluetooth/msg22427.html

[PATCH v0 3/3] audio: add partial unloading to audio driver
http://www.spinics.net/lists/linux-bluetooth/msg22428.html

Re: reverse SDP issue

[Johan Hedberg]

Hi Mike,

On Mon, Mar 19, 2012, Mike wrote:
> I notice a change in commit d2920be715974795b51f9cc3279947104da3647b
> [1] that changes the "reverse" variable for an SDP query:
> 
>  -       device_browse_sdp(device, NULL, NULL, NULL, TRUE);
> +       if (device_is_bredr(device))
> +               device_browse_sdp(device, NULL, NULL, NULL, FALSE);
> +       else
> +               device_browse_primary(device, NULL, NULL, FALSE);
> 
> You can see the original had reverse as TRUE, but the patch may have
> inadvertently changed it to FALSE..kernel.org/majordomo-info.html

That looks like a definitive bug. Good that you caught it. Vinicius
(author of the commit) care to comment?

As for the partial service removal changes to the device driver API, it
looks pretty messy to me. Is there any reason why we couldn't just
restrict calling remove() for when removing the entire device object
(and never call it when doing subsequent service discoveries)?

Johan

Re: reverse SDP issue

[Vinicius Costa Gomes]

Hi,

On 20:04 Mon 19 Mar, Johan Hedberg wrote:
> Hi Mike,
> 
> On Mon, Mar 19, 2012, Mike wrote:
> > I notice a change in commit d2920be715974795b51f9cc3279947104da3647b
> > [1] that changes the "reverse" variable for an SDP query:
> > 
> >  -       device_browse_sdp(device, NULL, NULL, NULL, TRUE);
> > +       if (device_is_bredr(device))
> > +               device_browse_sdp(device, NULL, NULL, NULL, FALSE);
> > +       else
> > +               device_browse_primary(device, NULL, NULL, FALSE);
> > 
> > You can see the original had reverse as TRUE, but the patch may have
> > inadvertently changed it to FALSE..kernel.org/majordomo-info.html
> 
> That looks like a definitive bug. Good that you caught it. Vinicius
> (author of the commit) care to comment?

It was changed to false by mistake, a patch to fix it should be arriving
on the mailing list in a few moments.

The comment on src/device.c around line 1683 gives some hints on what
my mistake may have caused: it could be that the device while connected
"hides" some items from its service record. So with reverse set to
false, some records that are present are being removed by mistake.

> 
> As for the partial service removal changes to the device driver API, it
> looks pretty messy to me. Is there any reason why we couldn't just
> restrict calling remove() for when removing the entire device object
> (and never call it when doing subsequent service discoveries)?

I agree that the logic is quite messy. Just some comments:

That assumes that the service list that a device implements is very much
static, which only breaks for development (in which the services of an 
device are always changing) and you will need to remove that device and 
create it again.

> 
> Johan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers,
-- 
Vinicius

Re: reverse SDP issue

[Mike]

On Tue, Mar 20, 2012 at 12:35 PM, Vinicius Costa Gomes
<vinicius.gomes@openbossa.org> wrote:
> Hi,
>
> On 20:04 Mon 19 Mar, Johan Hedberg wrote:
>> Hi Mike,
>>
>> On Mon, Mar 19, 2012, Mike wrote:
>> > I notice a change in commit d2920be715974795b51f9cc3279947104da3647b
>> > [1] that changes the "reverse" variable for an SDP query:
>> >
>> >  -       device_browse_sdp(device, NULL, NULL, NULL, TRUE);
>> > +       if (device_is_bredr(device))
>> > +               device_browse_sdp(device, NULL, NULL, NULL, FALSE);
>> > +       else
>> > +               device_browse_primary(device, NULL, NULL, FALSE);
>> >
>> > You can see the original had reverse as TRUE, but the patch may have
>> > inadvertently changed it to FALSE..kernel.org/majordomo-info.html
>>
>> That looks like a definitive bug. Good that you caught it. Vinicius
>> (author of the commit) care to comment?
>
> It was changed to false by mistake, a patch to fix it should be arriving
> on the mailing list in a few moments.
>
> The comment on src/device.c around line 1683 gives some hints on what
> my mistake may have caused: it could be that the device while connected
> "hides" some items from its service record. So with reverse set to
> false, some records that are present are being removed by mistake.

I'm not sure that's the case here.  I added some code to print out the
contents of "device->uuids" at the beginning of update_services
(src/device.c), and here is what I got:

UUID 0000110d-0000-1000-8000-00805f9b34fb
UUID 0000111f-0000-1000-8000-00805f9b34fb
UUID 0000110d-0000-1000-8000-00805f9b34fb
UUID 0000111f-0000-1000-8000-00805f9b34fb

A little curious why the UUIDs are duplicated, but besides that, we
can see that my phone already attempted to connect _before_ it was
paired, because it thought it was paired already (pairing deleted on
the BlueZ side only).  The UUIDs were added then before SDP could be
done.

This has me suspecting these lines of code in update_services:

		l = g_slist_find_custom(device->uuids, profile_uuid,
							(GCompareFunc) strcmp);
		if (!l)
			req->profiles_added =
					g_slist_append(req->profiles_added,
							profile_uuid);
		else {
			req->profiles_removed =
					g_slist_remove(req->profiles_removed,
							l->data);
			g_free(profile_uuid);
		}

It appears that if a UUID already existed when you did the SDP browse,
it is removed?  Why would that be?

Re: reverse SDP issue

[Mike]

On Tue, Mar 20, 2012 at 1:27 PM, Mike <puffy.taco@gmail.com> wrote:
> On Tue, Mar 20, 2012 at 12:35 PM, Vinicius Costa Gomes
> <vinicius.gomes@openbossa.org> wrote:
>> Hi,
>>
>> On 20:04 Mon 19 Mar, Johan Hedberg wrote:
>>> Hi Mike,
>>>
>>> On Mon, Mar 19, 2012, Mike wrote:
>>> > I notice a change in commit d2920be715974795b51f9cc3279947104da3647b
>>> > [1] that changes the "reverse" variable for an SDP query:
>>> >
>>> >  -       device_browse_sdp(device, NULL, NULL, NULL, TRUE);
>>> > +       if (device_is_bredr(device))
>>> > +               device_browse_sdp(device, NULL, NULL, NULL, FALSE);
>>> > +       else
>>> > +               device_browse_primary(device, NULL, NULL, FALSE);
>>> >
>>> > You can see the original had reverse as TRUE, but the patch may have
>>> > inadvertently changed it to FALSE..kernel.org/majordomo-info.html
>>>
>>> That looks like a definitive bug. Good that you caught it. Vinicius
>>> (author of the commit) care to comment?
>>
>> It was changed to false by mistake, a patch to fix it should be arriving
>> on the mailing list in a few moments.
>>
>> The comment on src/device.c around line 1683 gives some hints on what
>> my mistake may have caused: it could be that the device while connected
>> "hides" some items from its service record. So with reverse set to
>> false, some records that are present are being removed by mistake.
>
> I'm not sure that's the case here.  I added some code to print out the
> contents of "device->uuids" at the beginning of update_services
> (src/device.c), and here is what I got:
>
> UUID 0000110d-0000-1000-8000-00805f9b34fb
> UUID 0000111f-0000-1000-8000-00805f9b34fb
> UUID 0000110d-0000-1000-8000-00805f9b34fb
> UUID 0000111f-0000-1000-8000-00805f9b34fb
>
> A little curious why the UUIDs are duplicated, but besides that, we
> can see that my phone already attempted to connect _before_ it was
> paired, because it thought it was paired already (pairing deleted on
> the BlueZ side only).  The UUIDs were added then before SDP could be
> done.
>
> This has me suspecting these lines of code in update_services:
>
>                l = g_slist_find_custom(device->uuids, profile_uuid,
>                                                        (GCompareFunc) strcmp);
>                if (!l)
>                        req->profiles_added =
>                                        g_slist_append(req->profiles_added,
>                                                        profile_uuid);
>                else {
>                        req->profiles_removed =
>                                        g_slist_remove(req->profiles_removed,
>                                                        l->data);
>                        g_free(profile_uuid);
>                }
>
> It appears that if a UUID already existed when you did the SDP browse,
> it is removed?  Why would that be?

Ah, I read that wrong.  The UUID is removed from the list of profiles
to be removed!  So that's sane, but I guess the problem is that
somehow we have the UUIDs in there twice, which makes this code fail.

Re: reverse SDP issue

[Mike]

On Tue, Mar 20, 2012 at 1:39 PM, Mike <puffy.taco@gmail.com> wrote:
> On Tue, Mar 20, 2012 at 1:27 PM, Mike <puffy.taco@gmail.com> wrote:
>> On Tue, Mar 20, 2012 at 12:35 PM, Vinicius Costa Gomes
>> <vinicius.gomes@openbossa.org> wrote:
>>> Hi,
>>>
>>> On 20:04 Mon 19 Mar, Johan Hedberg wrote:
>>>> Hi Mike,
>>>>
>>>> On Mon, Mar 19, 2012, Mike wrote:
>>>> > I notice a change in commit d2920be715974795b51f9cc3279947104da3647b
>>>> > [1] that changes the "reverse" variable for an SDP query:
>>>> >
>>>> >  -       device_browse_sdp(device, NULL, NULL, NULL, TRUE);
>>>> > +       if (device_is_bredr(device))
>>>> > +               device_browse_sdp(device, NULL, NULL, NULL, FALSE);
>>>> > +       else
>>>> > +               device_browse_primary(device, NULL, NULL, FALSE);
>>>> >
>>>> > You can see the original had reverse as TRUE, but the patch may have
>>>> > inadvertently changed it to FALSE..kernel.org/majordomo-info.html
>>>>
>>>> That looks like a definitive bug. Good that you caught it. Vinicius
>>>> (author of the commit) care to comment?
>>>
>>> It was changed to false by mistake, a patch to fix it should be arriving
>>> on the mailing list in a few moments.
>>>
>>> The comment on src/device.c around line 1683 gives some hints on what
>>> my mistake may have caused: it could be that the device while connected
>>> "hides" some items from its service record. So with reverse set to
>>> false, some records that are present are being removed by mistake.
>>
>> I'm not sure that's the case here.  I added some code to print out the
>> contents of "device->uuids" at the beginning of update_services
>> (src/device.c), and here is what I got:
>>
>> UUID 0000110d-0000-1000-8000-00805f9b34fb
>> UUID 0000111f-0000-1000-8000-00805f9b34fb
>> UUID 0000110d-0000-1000-8000-00805f9b34fb
>> UUID 0000111f-0000-1000-8000-00805f9b34fb
>>
>> A little curious why the UUIDs are duplicated, but besides that, we
>> can see that my phone already attempted to connect _before_ it was
>> paired, because it thought it was paired already (pairing deleted on
>> the BlueZ side only).  The UUIDs were added then before SDP could be
>> done.
>>
>> This has me suspecting these lines of code in update_services:
>>
>>                l = g_slist_find_custom(device->uuids, profile_uuid,
>>                                                        (GCompareFunc) strcmp);
>>                if (!l)
>>                        req->profiles_added =
>>                                        g_slist_append(req->profiles_added,
>>                                                        profile_uuid);
>>                else {
>>                        req->profiles_removed =
>>                                        g_slist_remove(req->profiles_removed,
>>                                                        l->data);
>>                        g_free(profile_uuid);
>>                }
>>
>> It appears that if a UUID already existed when you did the SDP browse,
>> it is removed?  Why would that be?
>
> Ah, I read that wrong.  The UUID is removed from the list of profiles
> to be removed!  So that's sane, but I guess the problem is that
> somehow we have the UUIDs in there twice, which makes this code fail.

Argh.  I guess update_services was being called twice, so the UUIDS
aren't duplicated.  I'm still tracing what the real issue is then.

Re: reverse SDP issue

[Mike]

On Tue, Mar 20, 2012 at 2:21 PM, Mike <puffy.taco@gmail.com> wrote:
> On Tue, Mar 20, 2012 at 1:39 PM, Mike <puffy.taco@gmail.com> wrote:
>> On Tue, Mar 20, 2012 at 1:27 PM, Mike <puffy.taco@gmail.com> wrote:
>>> On Tue, Mar 20, 2012 at 12:35 PM, Vinicius Costa Gomes
>>> <vinicius.gomes@openbossa.org> wrote:
>>>> Hi,
>>>>
>>>> On 20:04 Mon 19 Mar, Johan Hedberg wrote:
>>>>> Hi Mike,
>>>>>
>>>>> On Mon, Mar 19, 2012, Mike wrote:
>>>>> > I notice a change in commit d2920be715974795b51f9cc3279947104da3647b
>>>>> > [1] that changes the "reverse" variable for an SDP query:
>>>>> >
>>>>> >  -       device_browse_sdp(device, NULL, NULL, NULL, TRUE);
>>>>> > +       if (device_is_bredr(device))
>>>>> > +               device_browse_sdp(device, NULL, NULL, NULL, FALSE);
>>>>> > +       else
>>>>> > +               device_browse_primary(device, NULL, NULL, FALSE);
>>>>> >
>>>>> > You can see the original had reverse as TRUE, but the patch may have
>>>>> > inadvertently changed it to FALSE..kernel.org/majordomo-info.html
>>>>>
>>>>> That looks like a definitive bug. Good that you caught it. Vinicius
>>>>> (author of the commit) care to comment?
>>>>
>>>> It was changed to false by mistake, a patch to fix it should be arriving
>>>> on the mailing list in a few moments.
>>>>
>>>> The comment on src/device.c around line 1683 gives some hints on what
>>>> my mistake may have caused: it could be that the device while connected
>>>> "hides" some items from its service record. So with reverse set to
>>>> false, some records that are present are being removed by mistake.
>>>
>>> I'm not sure that's the case here.  I added some code to print out the
>>> contents of "device->uuids" at the beginning of update_services
>>> (src/device.c), and here is what I got:
>>>
>>> UUID 0000110d-0000-1000-8000-00805f9b34fb
>>> UUID 0000111f-0000-1000-8000-00805f9b34fb
>>> UUID 0000110d-0000-1000-8000-00805f9b34fb
>>> UUID 0000111f-0000-1000-8000-00805f9b34fb
>>>
>>> A little curious why the UUIDs are duplicated, but besides that, we
>>> can see that my phone already attempted to connect _before_ it was
>>> paired, because it thought it was paired already (pairing deleted on
>>> the BlueZ side only).  The UUIDs were added then before SDP could be
>>> done.
>>>
>>> This has me suspecting these lines of code in update_services:
>>>
>>>                l = g_slist_find_custom(device->uuids, profile_uuid,
>>>                                                        (GCompareFunc) strcmp);
>>>                if (!l)
>>>                        req->profiles_added =
>>>                                        g_slist_append(req->profiles_added,
>>>                                                        profile_uuid);
>>>                else {
>>>                        req->profiles_removed =
>>>                                        g_slist_remove(req->profiles_removed,
>>>                                                        l->data);
>>>                        g_free(profile_uuid);
>>>                }
>>>
>>> It appears that if a UUID already existed when you did the SDP browse,
>>> it is removed?  Why would that be?
>>
>> Ah, I read that wrong.  The UUID is removed from the list of profiles
>> to be removed!  So that's sane, but I guess the problem is that
>> somehow we have the UUIDs in there twice, which makes this code fail.
>
> Argh.  I guess update_services was being called twice, so the UUIDS
> aren't duplicated.  I'm still tracing what the real issue is then.

Alright, now I think I understand what's going on.  The avdtp.c code
adds ADVANCED_AUDIO_UUID to the UUID list if the device does not
exist.  The problem is that ADVANCED_AUDIO_UUID, according to the
assigned numbers document, is only allowed as a profile class.  The
code in update_services is checking for service classes, and finds
0x110a (audio source) rather than 0x110d (advanced audio).  So, the
real problem in my case is that ADVANCED_AUDIO_UUID is being used as a
service class when it should not.  Below is the SDP record from my
phone for the Audio Source:

Service Name: Audio Source
Service Description: Audio Source
Service Provider: /a/mobile/system/cl.gif
Service RecHandle: 0x1000d
Service Class ID List:
  "Audio Source" (0x110a)
Protocol Descriptor List:
  "L2CAP" (0x0100)
    PSM: 25
  "AVDTP" (0x0019)
    uint16: 0x100
Language Base Attr List:
  code_ISO639: 0x656e
  encoding:    0x6a
  base_offset: 0x100
  code_ISO639: 0x6672
  encoding:    0x6a
  base_offset: 0xc800
  code_ISO639: 0x6573
  encoding:    0x6a
  base_offset: 0xc803
  code_ISO639: 0x7074
  encoding:    0x6a
  base_offset: 0xc806
Profile Descriptor List:
  "Advanced Audio" (0x110d)
    Version: 0x0100

Re: reverse SDP issue

[Johan Hedberg]

Hi Vinicius,

On Tue, Mar 20, 2012, Vinicius Costa Gomes wrote:
> > As for the partial service removal changes to the device driver API, it
> > looks pretty messy to me. Is there any reason why we couldn't just
> > restrict calling remove() for when removing the entire device object
> > (and never call it when doing subsequent service discoveries)?
> 
> I agree that the logic is quite messy. Just some comments:
> 
> That assumes that the service list that a device implements is very much
> static, which only breaks for development (in which the services of an 
> device are always changing) and you will need to remove that device and 
> create it again.

We would still be able to handle added UUIDs just fine. The only issue
is with removed services in which case you'd just wouldn't be able to
connect to the remote device through the corresponding D-Bus interface
(something which can already now happen and which I think is ok).

Johan

Query on attribute api service object path

[Jaganath]

Hello,

I have a doubt in attribute-api.txt.
In Device service hierarchy object path is 
"[prefix]/{hci0}/{device0}/{service0, service1, ...}.
which is device path + service"x".
When I checked the code "x" is "prim->start" which is service start handle.
But how user will get this handle to create object path?
Only uuids are passed to user in UUIDs signal afaik.

Thanks
Jaganath 

Re: Query on attribute api service object path

[Anderson Lizardo]

Hi,

On Wed, Mar 21, 2012 at 9:11 AM, Jaganath <jaganath.k@samsung.com> wrote:
> Hello,
>
> I have a doubt in attribute-api.txt.
> In Device service hierarchy object path is
> "[prefix]/{hci0}/{device0}/{service0, service1, ...}.
> which is device path + service"x".
> When I checked the code "x" is "prim->start" which is service start handle.
> But how user will get this handle to create object path?
> Only uuids are passed to user in UUIDs signal afaik.

Take a look on how to use the API on the test/test-attrib python
script. You should not try to parse the path itself. it is returned by
Methods and you simply use then with a D-Bus interface (e.g.
Bluez.Attribute).

Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: Query on attribute api service object path

[Jaganath]

Hi Anderson,

--------------------------------------------------
From: "Anderson Lizardo" <anderson.lizardo@openbossa.org>
Sent: Wednesday, March 21, 2012 7:48 PM
To: "Jaganath" <jaganath.k@samsung.com>
Cc: <linux-bluetooth@vger.kernel.org>
Subject: Re: Query on attribute api service object path

> Hi,
>
> On Wed, Mar 21, 2012 at 9:11 AM, Jaganath <jaganath.k@samsung.com> wrote:
>> Hello,
>>
>> I have a doubt in attribute-api.txt.
>> In Device service hierarchy object path is
>> "[prefix]/{hci0}/{device0}/{service0, service1, ...}.
>> which is device path + service"x".
>> When I checked the code "x" is "prim->start" which is service start 
>> handle.
>> But how user will get this handle to create object path?
>> Only uuids are passed to user in UUIDs signal afaik.
>
> Take a look on how to use the API on the test/test-attrib python
> script. You should not try to parse the path itself. it is returned by
> Methods and you simply use then with a D-Bus interface (e.g.
> Bluez.Attribute).
>
Thanks for your reply. Characteristic based Service object path is there in 
device properties.

Jaganath