* [PATCH 1/2] Bluetooth: Fix to update EIR for uuid16 properly @ 2012-12-21 13:44 Syam Sidhardhan 2012-12-21 13:44 ` [PATCH 2/2] Bluetooth: Fix ACL alive for long in case of non pariable devices Syam Sidhardhan 2012-12-21 19:31 ` [PATCH 1/2] Bluetooth: Fix to update EIR for uuid16 properly Johan Hedberg 0 siblings, 2 replies; 10+ messages in thread From: Syam Sidhardhan @ 2012-12-21 13:44 UTC (permalink / raw) To: linux-bluetooth 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 <s.syam@samsung.com> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] Bluetooth: Fix ACL alive for long in case of non pariable devices 2012-12-21 13:44 [PATCH 1/2] Bluetooth: Fix to update EIR for uuid16 properly Syam Sidhardhan @ 2012-12-21 13:44 ` Syam Sidhardhan 2013-01-07 20:49 ` Syam Sidhardhan 2012-12-21 19:31 ` [PATCH 1/2] Bluetooth: Fix to update EIR for uuid16 properly Johan Hedberg 1 sibling, 1 reply; 10+ messages in thread From: Syam Sidhardhan @ 2012-12-21 13:44 UTC (permalink / raw) To: linux-bluetooth 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 <sangki79.park@samsung.com> Signed-off-by: Syam Sidhardhan <s.syam@samsung.com> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Fix ACL alive for long in case of non pariable devices 2012-12-21 13:44 ` [PATCH 2/2] Bluetooth: Fix ACL alive for long in case of non pariable devices Syam Sidhardhan @ 2013-01-07 20:49 ` Syam Sidhardhan 2013-06-26 11:33 ` Syam Sidhardhan 0 siblings, 1 reply; 10+ messages in thread From: Syam Sidhardhan @ 2013-01-07 20:49 UTC (permalink / raw) To: Syam Sidhardhan; +Cc: linux-bluetooth Hi, On Fri, Dec 21, 2012 at 7:14 PM, Syam Sidhardhan <s.syam@samsung.com> 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 <sangki79.park@samsung.com> > Signed-off-by: Syam Sidhardhan <s.syam@samsung.com> > --- > 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Fix ACL alive for long in case of non pariable devices 2013-01-07 20:49 ` Syam Sidhardhan @ 2013-06-26 11:33 ` Syam Sidhardhan 2013-06-26 12:48 ` Szymon Janc 0 siblings, 1 reply; 10+ messages in thread From: Syam Sidhardhan @ 2013-06-26 11:33 UTC (permalink / raw) To: Syam Sidhardhan; +Cc: User Name Hi, On Tue, Jan 8, 2013 at 2:19 AM, Syam Sidhardhan <syamsidhardh@gmail.com> wrote: > > Hi, > > On Fri, Dec 21, 2012 at 7:14 PM, Syam Sidhardhan <s.syam@samsung.com> 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 <sangki79.park@samsung.com> > > Signed-off-by: Syam Sidhardhan <s.syam@samsung.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Fix ACL alive for long in case of non pariable devices 2013-06-26 11:33 ` Syam Sidhardhan @ 2013-06-26 12:48 ` Szymon Janc 2013-06-28 10:17 ` Syam Sidhardhan 0 siblings, 1 reply; 10+ messages in thread From: Szymon Janc @ 2013-06-26 12:48 UTC (permalink / raw) To: Syam Sidhardhan; +Cc: Syam Sidhardhan, linux-bluetooth 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 <syamsidhardh@gmail.com> wrote: > > > > Hi, > > > > On Fri, Dec 21, 2012 at 7:14 PM, Syam Sidhardhan <s.syam@samsung.com> 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 <sangki79.park@samsung.com> > > > Signed-off-by: Syam Sidhardhan <s.syam@samsung.com> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Fix ACL alive for long in case of non pariable devices 2013-06-26 12:48 ` Szymon Janc @ 2013-06-28 10:17 ` Syam Sidhardhan 0 siblings, 0 replies; 10+ messages in thread From: Syam Sidhardhan @ 2013-06-28 10:17 UTC (permalink / raw) To: Szymon Janc; +Cc: Syam Sidhardhan, User Name Hi On Wed, Jun 26, 2013 at 6:18 PM, Szymon Janc <szymon.janc@tieto.com> 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 <syamsidhardh@gmail.com> wrote: > > > > > > Hi, > > > > > > On Fri, Dec 21, 2012 at 7:14 PM, Syam Sidhardhan <s.syam@samsung.com> 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 <sangki79.park@samsung.com> > > > > Signed-off-by: Syam Sidhardhan <s.syam@samsung.com> > > > > --- > > > > 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Fix to update EIR for uuid16 properly 2012-12-21 13:44 [PATCH 1/2] Bluetooth: Fix to update EIR for uuid16 properly Syam Sidhardhan 2012-12-21 13:44 ` [PATCH 2/2] Bluetooth: Fix ACL alive for long in case of non pariable devices Syam Sidhardhan @ 2012-12-21 19:31 ` Johan Hedberg 2012-12-22 16:39 ` Marcel Holtmann 1 sibling, 1 reply; 10+ messages in thread From: Johan Hedberg @ 2012-12-21 19:31 UTC (permalink / raw) To: Syam Sidhardhan; +Cc: linux-bluetooth 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 <s.syam@samsung.com> > --- > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Fix to update EIR for uuid16 properly 2012-12-21 19:31 ` [PATCH 1/2] Bluetooth: Fix to update EIR for uuid16 properly Johan Hedberg @ 2012-12-22 16:39 ` Marcel Holtmann 2012-12-24 11:46 ` Syam Sidhardhan 0 siblings, 1 reply; 10+ messages in thread From: Marcel Holtmann @ 2012-12-22 16:39 UTC (permalink / raw) To: Johan Hedberg; +Cc: Syam Sidhardhan, linux-bluetooth 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 <s.syam@samsung.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Fix to update EIR for uuid16 properly 2012-12-22 16:39 ` Marcel Holtmann @ 2012-12-24 11:46 ` Syam Sidhardhan 2012-12-24 16:45 ` Marcel Holtmann 0 siblings, 1 reply; 10+ messages in thread From: Syam Sidhardhan @ 2012-12-24 11:46 UTC (permalink / raw) To: Marcel Holtmann; +Cc: Johan Hedberg, Syam Sidhardhan, linux-bluetooth Hi Marcel, Johan, On Sat, Dec 22, 2012 at 10:09 PM, Marcel Holtmann <marcel@holtmann.org> 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 <s.syam@samsung.com> >> > --- >> > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Fix to update EIR for uuid16 properly 2012-12-24 11:46 ` Syam Sidhardhan @ 2012-12-24 16:45 ` Marcel Holtmann 0 siblings, 0 replies; 10+ messages in thread From: Marcel Holtmann @ 2012-12-24 16:45 UTC (permalink / raw) To: Syam Sidhardhan; +Cc: Johan Hedberg, Syam Sidhardhan, linux-bluetooth 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 <s.syam@samsung.com> > >> > --- > >> > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-06-28 10:17 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-21 13:44 [PATCH 1/2] Bluetooth: Fix to update EIR for uuid16 properly Syam Sidhardhan 2012-12-21 13:44 ` [PATCH 2/2] Bluetooth: Fix ACL alive for long in case of non pariable devices Syam Sidhardhan 2013-01-07 20:49 ` Syam Sidhardhan 2013-06-26 11:33 ` Syam Sidhardhan 2013-06-26 12:48 ` Szymon Janc 2013-06-28 10:17 ` Syam Sidhardhan 2012-12-21 19:31 ` [PATCH 1/2] Bluetooth: Fix to update EIR for uuid16 properly Johan Hedberg 2012-12-22 16:39 ` Marcel Holtmann 2012-12-24 11:46 ` Syam Sidhardhan 2012-12-24 16:45 ` Marcel Holtmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).