If we register a uuid other than uuid16, especially custom 128 bit uuid
then nothing is updated in the EIR and it was broken.
After registering a 16 bit uuid. ex: "sdptool add SP", we can see the
uuid in the EIR as below.
< 0000: 01 52 0c f1 00 08 09 52 65 64 77 6f 6f 64 15 03 .R.....Redwood..
0010: 01 11 32 11 2f 11 06 11 05 11 0a 11 0e 11 0c 11 ..2./...........
0020: 1f 11 12 11 00 00 00 00 00 00 00 00 00 00 00 00 ................
0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00f0: 00 00 00 00 00 .....
> 0000: 04 0e 04 01 52 0c 00 ....R..
But after register a user defined 128 bit uuid, nothing is
updated in the EIR.
< 0000: 01 52 0c f1 00 08 09 52 65 64 77 6f 6f 64 00 00 .R.....Redwood..
0010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00f0: 00 00 00 00 00 .....
> 0000: 04 0e 04 01 52 0c 00 ....R..
With this fix, we can see the EIR is updated properly.
Signed-off-by: Syam Sidhardhan <[email protected]>
---
net/bluetooth/mgmt.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index f559b96..512a3f5 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -514,8 +514,6 @@ static void create_eir(struct hci_dev *hdev, u8 *data)
u16 uuid16;
uuid16 = get_uuid16(uuid->uuid);
- if (uuid16 == 0)
- return;
if (uuid16 < 0x1100)
continue;
--
1.7.9.5
Hi Syam,
> >> > If we register a uuid other than uuid16, especially custom 128 bit uuid
> >> > then nothing is updated in the EIR and it was broken.
> >> >
> >> > After registering a 16 bit uuid. ex: "sdptool add SP", we can see the
> >> > uuid in the EIR as below.
> >> > < 0000: 01 52 0c f1 00 08 09 52 65 64 77 6f 6f 64 15 03 .R.....Redwood..
> >> > 0010: 01 11 32 11 2f 11 06 11 05 11 0a 11 0e 11 0c 11 ..2./...........
> >> > 0020: 1f 11 12 11 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 0070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 0090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 00a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 00b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 00c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 00d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 00e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 00f0: 00 00 00 00 00 .....
> >> > > 0000: 04 0e 04 01 52 0c 00 ....R..
> >> >
> >> > But after register a user defined 128 bit uuid, nothing is
> >> > updated in the EIR.
> >> >
> >> > < 0000: 01 52 0c f1 00 08 09 52 65 64 77 6f 6f 64 00 00 .R.....Redwood..
> >> > 0010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 0020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 0070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 0090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 00a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 00b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 00c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 00d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 00e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> > 00f0: 00 00 00 00 00 .....
> >> > > 0000: 04 0e 04 01 52 0c 00 ....R..
> >> >
> >> > With this fix, we can see the EIR is updated properly.
> >> >
> >> > Signed-off-by: Syam Sidhardhan <[email protected]>
> >> > ---
> >> > net/bluetooth/mgmt.c | 2 --
> >> > 1 file changed, 2 deletions(-)
> >> >
> >> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> >> > index f559b96..512a3f5 100644
> >> > --- a/net/bluetooth/mgmt.c
> >> > +++ b/net/bluetooth/mgmt.c
> >> > @@ -514,8 +514,6 @@ static void create_eir(struct hci_dev *hdev, u8 *data)
> >> > u16 uuid16;
> >> >
> >> > uuid16 = get_uuid16(uuid->uuid);
> >> > - if (uuid16 == 0)
> >> > - return;
> >> >
> >> > if (uuid16 < 0x1100)
> >> > continue;
> >>
> >> Nak. The bug is real and should be fixed but your fix is wrong. The
> >> right fix it to convert this return statement into a continue statement
> >> since we do still want to check for a 0 return value from get_uuid16.
> >>
>
> Since the next statements (uuid16 < 0x1100) indirectly do this logic,
> I intentionally removed it in order to avoid duplication.
> Probably for more clarity and readability, I can do it as per your
> suggestion.
>
> >> Along with this patch please prepare another one to increment the mgmt
> >> revision. These two should go together to upstream trees so that we can
> >> introduce a check in user space to know whether it's safe to pass
> >> non-16bit UUIDs to the kernel or not.
> >
> Ok.
> > I want a fix that introduces also support for 32-bit and 128-bit UUIDs
> > now. No paper over the hole fixing here.
> >
>
> As per the specification, "To reduce interference, the host should try
> to minimize the amount of EIR data such that the baseband can use
> a 1-slot or 3-slot EIR packet. This is advantageous because it reduces
> interference and maximizes the probability that the EIR packet will be
> received."
>
> Does the addition of 128-bit and 32-bit uuid decreases the probability of the
> reception of EIR packet, if any application register more of these types?
that is not the point here. If you want to put a 128-bit UUID into the
EIR data, then you should be able to. Let bluetoothd make the decision
on what to give the kernel and what not.
Regards
Marcel
Hi Marcel, Johan,
On Sat, Dec 22, 2012 at 10:09 PM, Marcel Holtmann <[email protected]> wrote:
> Hi Johan,
>
>> > If we register a uuid other than uuid16, especially custom 128 bit uuid
>> > then nothing is updated in the EIR and it was broken.
>> >
>> > After registering a 16 bit uuid. ex: "sdptool add SP", we can see the
>> > uuid in the EIR as below.
>> > < 0000: 01 52 0c f1 00 08 09 52 65 64 77 6f 6f 64 15 03 .R.....Redwood..
>> > 0010: 01 11 32 11 2f 11 06 11 05 11 0a 11 0e 11 0c 11 ..2./...........
>> > 0020: 1f 11 12 11 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 0070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 0090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 00a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 00b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 00c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 00d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 00e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 00f0: 00 00 00 00 00 .....
>> > > 0000: 04 0e 04 01 52 0c 00 ....R..
>> >
>> > But after register a user defined 128 bit uuid, nothing is
>> > updated in the EIR.
>> >
>> > < 0000: 01 52 0c f1 00 08 09 52 65 64 77 6f 6f 64 00 00 .R.....Redwood..
>> > 0010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 0020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 0070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 0090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 00a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 00b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 00c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 00d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 00e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> > 00f0: 00 00 00 00 00 .....
>> > > 0000: 04 0e 04 01 52 0c 00 ....R..
>> >
>> > With this fix, we can see the EIR is updated properly.
>> >
>> > Signed-off-by: Syam Sidhardhan <[email protected]>
>> > ---
>> > net/bluetooth/mgmt.c | 2 --
>> > 1 file changed, 2 deletions(-)
>> >
>> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> > index f559b96..512a3f5 100644
>> > --- a/net/bluetooth/mgmt.c
>> > +++ b/net/bluetooth/mgmt.c
>> > @@ -514,8 +514,6 @@ static void create_eir(struct hci_dev *hdev, u8 *data)
>> > u16 uuid16;
>> >
>> > uuid16 = get_uuid16(uuid->uuid);
>> > - if (uuid16 == 0)
>> > - return;
>> >
>> > if (uuid16 < 0x1100)
>> > continue;
>>
>> Nak. The bug is real and should be fixed but your fix is wrong. The
>> right fix it to convert this return statement into a continue statement
>> since we do still want to check for a 0 return value from get_uuid16.
>>
Since the next statements (uuid16 < 0x1100) indirectly do this logic,
I intentionally removed it in order to avoid duplication.
Probably for more clarity and readability, I can do it as per your
suggestion.
>> Along with this patch please prepare another one to increment the mgmt
>> revision. These two should go together to upstream trees so that we can
>> introduce a check in user space to know whether it's safe to pass
>> non-16bit UUIDs to the kernel or not.
>
Ok.
> I want a fix that introduces also support for 32-bit and 128-bit UUIDs
> now. No paper over the hole fixing here.
>
As per the specification, "To reduce interference, the host should try
to minimize the amount of EIR data such that the baseband can use
a 1-slot or 3-slot EIR packet. This is advantageous because it reduces
interference and maximizes the probability that the EIR packet will be
received."
Does the addition of 128-bit and 32-bit uuid decreases the probability of the
reception of EIR packet, if any application register more of these types?
Regards,
Syam
Hi Johan,
> > If we register a uuid other than uuid16, especially custom 128 bit uuid
> > then nothing is updated in the EIR and it was broken.
> >
> > After registering a 16 bit uuid. ex: "sdptool add SP", we can see the
> > uuid in the EIR as below.
> > < 0000: 01 52 0c f1 00 08 09 52 65 64 77 6f 6f 64 15 03 .R.....Redwood..
> > 0010: 01 11 32 11 2f 11 06 11 05 11 0a 11 0e 11 0c 11 ..2./...........
> > 0020: 1f 11 12 11 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 0070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 0090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00f0: 00 00 00 00 00 .....
> > > 0000: 04 0e 04 01 52 0c 00 ....R..
> >
> > But after register a user defined 128 bit uuid, nothing is
> > updated in the EIR.
> >
> > < 0000: 01 52 0c f1 00 08 09 52 65 64 77 6f 6f 64 00 00 .R.....Redwood..
> > 0010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 0020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 0070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 0090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> > 00f0: 00 00 00 00 00 .....
> > > 0000: 04 0e 04 01 52 0c 00 ....R..
> >
> > With this fix, we can see the EIR is updated properly.
> >
> > Signed-off-by: Syam Sidhardhan <[email protected]>
> > ---
> > net/bluetooth/mgmt.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index f559b96..512a3f5 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -514,8 +514,6 @@ static void create_eir(struct hci_dev *hdev, u8 *data)
> > u16 uuid16;
> >
> > uuid16 = get_uuid16(uuid->uuid);
> > - if (uuid16 == 0)
> > - return;
> >
> > if (uuid16 < 0x1100)
> > continue;
>
> Nak. The bug is real and should be fixed but your fix is wrong. The
> right fix it to convert this return statement into a continue statement
> since we do still want to check for a 0 return value from get_uuid16.
>
> Along with this patch please prepare another one to increment the mgmt
> revision. These two should go together to upstream trees so that we can
> introduce a check in user space to know whether it's safe to pass
> non-16bit UUIDs to the kernel or not.
I want a fix that introduces also support for 32-bit and 128-bit UUIDs
now. No paper over the hole fixing here.
Regards
Marcel
Hi Syam,
On Fri, Dec 21, 2012, Syam Sidhardhan wrote:
> If we register a uuid other than uuid16, especially custom 128 bit uuid
> then nothing is updated in the EIR and it was broken.
>
> After registering a 16 bit uuid. ex: "sdptool add SP", we can see the
> uuid in the EIR as below.
> < 0000: 01 52 0c f1 00 08 09 52 65 64 77 6f 6f 64 15 03 .R.....Redwood..
> 0010: 01 11 32 11 2f 11 06 11 05 11 0a 11 0e 11 0c 11 ..2./...........
> 0020: 1f 11 12 11 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 0070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 0090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00f0: 00 00 00 00 00 .....
> > 0000: 04 0e 04 01 52 0c 00 ....R..
>
> But after register a user defined 128 bit uuid, nothing is
> updated in the EIR.
>
> < 0000: 01 52 0c f1 00 08 09 52 65 64 77 6f 6f 64 00 00 .R.....Redwood..
> 0010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 0020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 0030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 0040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 0070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 0090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00f0: 00 00 00 00 00 .....
> > 0000: 04 0e 04 01 52 0c 00 ....R..
>
> With this fix, we can see the EIR is updated properly.
>
> Signed-off-by: Syam Sidhardhan <[email protected]>
> ---
> net/bluetooth/mgmt.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index f559b96..512a3f5 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -514,8 +514,6 @@ static void create_eir(struct hci_dev *hdev, u8 *data)
> u16 uuid16;
>
> uuid16 = get_uuid16(uuid->uuid);
> - if (uuid16 == 0)
> - return;
>
> if (uuid16 < 0x1100)
> continue;
Nak. The bug is real and should be fixed but your fix is wrong. The
right fix it to convert this return statement into a continue statement
since we do still want to check for a 0 return value from get_uuid16.
Along with this patch please prepare another one to increment the mgmt
revision. These two should go together to upstream trees so that we can
introduce a check in user space to know whether it's safe to pass
non-16bit UUIDs to the kernel or not.
Johan
For certain devices (ex: HID mouse), support for authentication,
pairing and bonding is optional. For such devices, the ACL alive
for too long after the l2cap disconnection.
To avoid keep ACL alive for too long, set the ACL timeout back to
HCI_DISCONN_TIMEOUT when l2cap is connected.
commit id:a9ea3ed9b71cc3271dd59e76f65748adcaa76422 might have introduce
this issue.
Signed-off-by: Sang-Ki Park <[email protected]>
Signed-off-by: Syam Sidhardhan <[email protected]>
---
I'm not sure whether we need hci_conn_hold() and hci_conn_put() across
while updating the disc_timeout. In certain other places in the code
it's done. Ex: hci_auth_complete_evt(), hci_link_key_notify_evt() etc.
Here I took that as the reference.
net/bluetooth/l2cap_core.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 82a3bdc..7a544c2 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1360,7 +1360,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
sk = chan->sk;
hci_conn_hold(conn->hcon);
- conn->hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
bacpy(&bt_sk(sk)->src, conn->src);
bacpy(&bt_sk(sk)->dst, conn->dst);
@@ -1380,6 +1379,10 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
BT_DBG("conn %p", conn);
+ hci_conn_hold(conn->hcon);
+ conn->hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
+ hci_conn_put(conn->hcon);
+
if (!hcon->out && hcon->type == LE_LINK)
l2cap_le_conn_ready(conn);
--
1.7.9.5
Hi,
On Fri, Dec 21, 2012 at 7:14 PM, Syam Sidhardhan <[email protected]> wrote:
> For certain devices (ex: HID mouse), support for authentication,
> pairing and bonding is optional. For such devices, the ACL alive
> for too long after the l2cap disconnection.
>
> To avoid keep ACL alive for too long, set the ACL timeout back to
> HCI_DISCONN_TIMEOUT when l2cap is connected.
>
> commit id:a9ea3ed9b71cc3271dd59e76f65748adcaa76422 might have introduce
> this issue.
>
> Signed-off-by: Sang-Ki Park <[email protected]>
> Signed-off-by: Syam Sidhardhan <[email protected]>
> ---
> I'm not sure whether we need hci_conn_hold() and hci_conn_put() across
> while updating the disc_timeout. In certain other places in the code
> it's done. Ex: hci_auth_complete_evt(), hci_link_key_notify_evt() etc.
> Here I took that as the reference.
>
> net/bluetooth/l2cap_core.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 82a3bdc..7a544c2 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1360,7 +1360,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
> sk = chan->sk;
>
> hci_conn_hold(conn->hcon);
> - conn->hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
>
> bacpy(&bt_sk(sk)->src, conn->src);
> bacpy(&bt_sk(sk)->dst, conn->dst);
> @@ -1380,6 +1379,10 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
>
> BT_DBG("conn %p", conn);
>
> + hci_conn_hold(conn->hcon);
> + conn->hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
> + hci_conn_put(conn->hcon);
> +
> if (!hcon->out && hcon->type == LE_LINK)
> l2cap_le_conn_ready(conn);
>
> --
> 1.7.9.5
>
ping.
Thanks,
Syam.
Hi
On Wed, Jun 26, 2013 at 6:18 PM, Szymon Janc <[email protected]> wrote:
>
> Hi,
>
> On Wednesday 26 of June 2013 17:03:23 Syam Sidhardhan wrote:
> > Hi,
> >
> > On Tue, Jan 8, 2013 at 2:19 AM, Syam Sidhardhan <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Dec 21, 2012 at 7:14 PM, Syam Sidhardhan <[email protected]> wrote:
> > > > For certain devices (ex: HID mouse), support for authentication,
> > > > pairing and bonding is optional. For such devices, the ACL alive
> > > > for too long after the l2cap disconnection.
> > > >
> > > > To avoid keep ACL alive for too long, set the ACL timeout back to
> > > > HCI_DISCONN_TIMEOUT when l2cap is connected.
> > > >
> > > > commit id:a9ea3ed9b71cc3271dd59e76f65748adcaa76422 might have introduce
> > > > this issue.
> > > >
> > > > Signed-off-by: Sang-Ki Park <[email protected]>
> > > > Signed-off-by: Syam Sidhardhan <[email protected]>
> > > > ---
> > > > I'm not sure whether we need hci_conn_hold() and hci_conn_put() across
> > > > while updating the disc_timeout. In certain other places in the code
> > > > it's done. Ex: hci_auth_complete_evt(), hci_link_key_notify_evt() etc.
> > > > Here I took that as the reference.
> > > >
> > > > net/bluetooth/l2cap_core.c | 5 ++++-
> > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > > > index 82a3bdc..7a544c2 100644
> > > > --- a/net/bluetooth/l2cap_core.c
> > > > +++ b/net/bluetooth/l2cap_core.c
> > > > @@ -1360,7 +1360,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
> > > > sk = chan->sk;
> > > >
> > > > hci_conn_hold(conn->hcon);
> > > > - conn->hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
The above change has been recently applied to the bluetooth-next by Johan.
https://git.kernel.org/cgit/linux/kernel/git/bluetooth/bluetooth-next.git/commit/net/bluetooth/l2cap_core.c?id=0cc59a72c723979cf8973aff4df874a5f7a697c7
>
> > > >
> > > > bacpy(&bt_sk(sk)->src, conn->src);
> > > > bacpy(&bt_sk(sk)->dst, conn->dst);
> > > > @@ -1380,6 +1379,10 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
> > > >
> > > > BT_DBG("conn %p", conn);
> > > >
> > > > + hci_conn_hold(conn->hcon);
> > > > + conn->hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
> > > > + hci_conn_put(conn->hcon);
> > > > +
> > > > if (!hcon->out && hcon->type == LE_LINK)
> > > > l2cap_le_conn_ready(conn);
> > > >
> > > > --
> > > > 1.7.9.5
> > > >
> > >
> > > ping.
> > >
> >
> > Is there any comment on this patch?
> > If this patch looks valid I can rebase it with the latest code and
> > send it once again.
> >
>
> The funny thing is that original patch send to ML has this timer set back to
> HCI_DISCONN_TIMEOUT in l2cap_connect_req(), not in l2cap_le_conn_ready().
> See [1]. There were some big changes in l2cap code in bluetooth-next.git
> while this was committed to bluetooth.git so maybe some merge conflict was
> incorrectly resolved or sth...
>
> This patch looks ok to me. Just please make sure it works correctly
> in scenario described in original commit.
>
Unfortunately I'm unable to find such an Android device to check the correctness
describing in original commit.
>
>
> ps
> hold/put was to restart timer with new value, I think now it is hold/drop and
> timer is replaced with workqueue and is needed if you want new timeout to be
> used.
>
> [1] http://comments.gmane.org/gmane.linux.bluez.kernel/27532
Thanks Szymon for the clarification.
I'll send another version based on the latest bluetooth-next.git.
Hi,
On Wednesday 26 of June 2013 17:03:23 Syam Sidhardhan wrote:
> Hi,
>
> On Tue, Jan 8, 2013 at 2:19 AM, Syam Sidhardhan <[email protected]> wrote:
> >
> > Hi,
> >
> > On Fri, Dec 21, 2012 at 7:14 PM, Syam Sidhardhan <[email protected]> wrote:
> > > For certain devices (ex: HID mouse), support for authentication,
> > > pairing and bonding is optional. For such devices, the ACL alive
> > > for too long after the l2cap disconnection.
> > >
> > > To avoid keep ACL alive for too long, set the ACL timeout back to
> > > HCI_DISCONN_TIMEOUT when l2cap is connected.
> > >
> > > commit id:a9ea3ed9b71cc3271dd59e76f65748adcaa76422 might have introduce
> > > this issue.
> > >
> > > Signed-off-by: Sang-Ki Park <[email protected]>
> > > Signed-off-by: Syam Sidhardhan <[email protected]>
> > > ---
> > > I'm not sure whether we need hci_conn_hold() and hci_conn_put() across
> > > while updating the disc_timeout. In certain other places in the code
> > > it's done. Ex: hci_auth_complete_evt(), hci_link_key_notify_evt() etc.
> > > Here I took that as the reference.
> > >
> > > net/bluetooth/l2cap_core.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > > index 82a3bdc..7a544c2 100644
> > > --- a/net/bluetooth/l2cap_core.c
> > > +++ b/net/bluetooth/l2cap_core.c
> > > @@ -1360,7 +1360,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
> > > sk = chan->sk;
> > >
> > > hci_conn_hold(conn->hcon);
> > > - conn->hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
> > >
> > > bacpy(&bt_sk(sk)->src, conn->src);
> > > bacpy(&bt_sk(sk)->dst, conn->dst);
> > > @@ -1380,6 +1379,10 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
> > >
> > > BT_DBG("conn %p", conn);
> > >
> > > + hci_conn_hold(conn->hcon);
> > > + conn->hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
> > > + hci_conn_put(conn->hcon);
> > > +
> > > if (!hcon->out && hcon->type == LE_LINK)
> > > l2cap_le_conn_ready(conn);
> > >
> > > --
> > > 1.7.9.5
> > >
> >
> > ping.
> >
>
> Is there any comment on this patch?
> If this patch looks valid I can rebase it with the latest code and
> send it once again.
>
The funny thing is that original patch send to ML has this timer set back to
HCI_DISCONN_TIMEOUT in l2cap_connect_req(), not in l2cap_le_conn_ready().
See [1]. There were some big changes in l2cap code in bluetooth-next.git
while this was committed to bluetooth.git so maybe some merge conflict was
incorrectly resolved or sth...
This patch looks ok to me. Just please make sure it works correctly
in scenario described in original commit.
ps
hold/put was to restart timer with new value, I think now it is hold/drop and
timer is replaced with workqueue and is needed if you want new timeout to be
used.
[1] http://comments.gmane.org/gmane.linux.bluez.kernel/27532
--
BR
Szymon Janc
Hi,
On Tue, Jan 8, 2013 at 2:19 AM, Syam Sidhardhan <[email protected]> wrote:
>
> Hi,
>
> On Fri, Dec 21, 2012 at 7:14 PM, Syam Sidhardhan <[email protected]> wrote:
> > For certain devices (ex: HID mouse), support for authentication,
> > pairing and bonding is optional. For such devices, the ACL alive
> > for too long after the l2cap disconnection.
> >
> > To avoid keep ACL alive for too long, set the ACL timeout back to
> > HCI_DISCONN_TIMEOUT when l2cap is connected.
> >
> > commit id:a9ea3ed9b71cc3271dd59e76f65748adcaa76422 might have introduce
> > this issue.
> >
> > Signed-off-by: Sang-Ki Park <[email protected]>
> > Signed-off-by: Syam Sidhardhan <[email protected]>
> > ---
> > I'm not sure whether we need hci_conn_hold() and hci_conn_put() across
> > while updating the disc_timeout. In certain other places in the code
> > it's done. Ex: hci_auth_complete_evt(), hci_link_key_notify_evt() etc.
> > Here I took that as the reference.
> >
> > net/bluetooth/l2cap_core.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 82a3bdc..7a544c2 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -1360,7 +1360,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
> > sk = chan->sk;
> >
> > hci_conn_hold(conn->hcon);
> > - conn->hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
> >
> > bacpy(&bt_sk(sk)->src, conn->src);
> > bacpy(&bt_sk(sk)->dst, conn->dst);
> > @@ -1380,6 +1379,10 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
> >
> > BT_DBG("conn %p", conn);
> >
> > + hci_conn_hold(conn->hcon);
> > + conn->hcon->disc_timeout = HCI_DISCONN_TIMEOUT;
> > + hci_conn_put(conn->hcon);
> > +
> > if (!hcon->out && hcon->type == LE_LINK)
> > l2cap_le_conn_ready(conn);
> >
> > --
> > 1.7.9.5
> >
>
> ping.
>
Is there any comment on this patch?
If this patch looks valid I can rebase it with the latest code and
send it once again.
Regards,
Syam