* [PATCH] Bluetooth-next: Add incremental indexing in sysfs HCI connection name. @ 2011-08-17 16:01 doron.keren.bluez 2011-08-17 21:42 ` Marcel Holtmann 0 siblings, 1 reply; 9+ messages in thread From: doron.keren.bluez @ 2011-08-17 16:01 UTC (permalink / raw) To: linux-bluetooth; +Cc: Doron Keren, Ilia Kolominsky From: Doron Keren <doronkeren@ti.com> The patch fixes kernel panic which is due to race condition between the setup of incomming connection and clean-up of the dead one. Observed in the following case: attached HID device disconnects unexpectedly (without performing ACL disconnect ), the device tries to connect again before the ACL link time-out fires, this translates to the HCI_DISCONNECT, HCI_CONNECT_REQ events on the same handle, since HCI_DISCONNECT trigers the clean up of the HID device and handled in different context, the linking/unlinking connection object to sysfs, may mess up. Signed-off-by: Ilia Kolominsky <iliak@ti.com> --- net/bluetooth/hci_sysfs.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c index a6c3aa8..5967d63 100644 --- a/net/bluetooth/hci_sysfs.c +++ b/net/bluetooth/hci_sysfs.c @@ -9,6 +9,7 @@ #include <net/bluetooth/bluetooth.h> #include <net/bluetooth/hci_core.h> +static int acl_conn_index = 0; static struct class *bt_class; struct dentry *bt_debugfs; @@ -91,7 +92,8 @@ static void add_conn(struct work_struct *work) struct hci_conn *conn = container_of(work, struct hci_conn, work_add); struct hci_dev *hdev = conn->hdev; - dev_set_name(&conn->dev, "%s:%d", hdev->name, conn->handle); + acl_conn_index++; + dev_set_name(&conn->dev, "%s:%d:%d", hdev->name, conn->handle, acl_conn_index); dev_set_drvdata(&conn->dev, conn); -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Bluetooth-next: Add incremental indexing in sysfs HCI connection name. 2011-08-17 16:01 [PATCH] Bluetooth-next: Add incremental indexing in sysfs HCI connection name doron.keren.bluez @ 2011-08-17 21:42 ` Marcel Holtmann 2011-08-18 11:21 ` Keren, Doron 0 siblings, 1 reply; 9+ messages in thread From: Marcel Holtmann @ 2011-08-17 21:42 UTC (permalink / raw) To: doron.keren.bluez; +Cc: linux-bluetooth, Doron Keren, Ilia Kolominsky Hi Doron, > The patch fixes kernel panic which is due to race condition > between the setup of incomming connection and clean-up of the > dead one. Observed in the following case: attached HID device > disconnects unexpectedly (without performing ACL disconnect ), > the device tries to connect again before the ACL link time-out > fires, this translates to the HCI_DISCONNECT, HCI_CONNECT_REQ > events on the same handle, since HCI_DISCONNECT trigers the clean > up of the HID device and handled in different context, the > linking/unlinking connection object to sysfs, may mess up. > > Signed-off-by: Ilia Kolominsky <iliak@ti.com> > --- > net/bluetooth/hci_sysfs.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c > index a6c3aa8..5967d63 100644 > --- a/net/bluetooth/hci_sysfs.c > +++ b/net/bluetooth/hci_sysfs.c > @@ -9,6 +9,7 @@ > #include <net/bluetooth/bluetooth.h> > #include <net/bluetooth/hci_core.h> > > +static int acl_conn_index = 0; > static struct class *bt_class; > > struct dentry *bt_debugfs; > @@ -91,7 +92,8 @@ static void add_conn(struct work_struct *work) > struct hci_conn *conn = container_of(work, struct hci_conn, work_add); > struct hci_dev *hdev = conn->hdev; > > - dev_set_name(&conn->dev, "%s:%d", hdev->name, conn->handle); > + acl_conn_index++; > + dev_set_name(&conn->dev, "%s:%d:%d", hdev->name, conn->handle, acl_conn_index); > > dev_set_drvdata(&conn->dev, conn); can we get a bit more of details on what this is actually trying to solve. I do not like this way of solving it at all. I think it is trying to cover up symptoms and not fixing the real issue. Regards Marcel ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] Bluetooth-next: Add incremental indexing in sysfs HCI connection name. 2011-08-17 21:42 ` Marcel Holtmann @ 2011-08-18 11:21 ` Keren, Doron 2011-08-18 11:44 ` David Herrmann 2011-09-14 14:42 ` Marcel Holtmann 0 siblings, 2 replies; 9+ messages in thread From: Keren, Doron @ 2011-08-18 11:21 UTC (permalink / raw) To: Marcel Holtmann; +Cc: linux-bluetooth, Ilia, Kolominsky, Hadar, Amir Hi Marcel > -----Original Message----- > From: Marcel Holtmann [mailto:marcel@holtmann.org] > Sent: Thursday, August 18, 2011 12:42 AM > To: doron.keren.bluez@gmail.com > Cc: linux-bluetooth@vger.kernel.org; Keren, Doron; Ilia, Kolominsky > Subject: Re: [PATCH] Bluetooth-next: Add incremental indexing in sysfs HC= I > connection name. >=20 > Hi Doron, >=20 > > The patch fixes kernel panic which is due to race condition > > between the setup of incomming connection and clean-up of the > > dead one. Observed in the following case: attached HID device > > disconnects unexpectedly (without performing ACL disconnect ), > > the device tries to connect again before the ACL link time-out > > fires, this translates to the HCI_DISCONNECT, HCI_CONNECT_REQ > > events on the same handle, since HCI_DISCONNECT trigers the clean > > up of the HID device and handled in different context, the > > linking/unlinking connection object to sysfs, may mess up. > > > > Signed-off-by: Ilia Kolominsky <iliak@ti.com> > > --- > > net/bluetooth/hci_sysfs.c | 4 +++- > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c > > index a6c3aa8..5967d63 100644 > > --- a/net/bluetooth/hci_sysfs.c > > +++ b/net/bluetooth/hci_sysfs.c > > @@ -9,6 +9,7 @@ > > #include <net/bluetooth/bluetooth.h> > > #include <net/bluetooth/hci_core.h> > > > > +static int acl_conn_index =3D 0; > > static struct class *bt_class; > > > > struct dentry *bt_debugfs; > > @@ -91,7 +92,8 @@ static void add_conn(struct work_struct *work) > > struct hci_conn *conn =3D container_of(work, struct hci_conn, > work_add); > > struct hci_dev *hdev =3D conn->hdev; > > > > - dev_set_name(&conn->dev, "%s:%d", hdev->name, conn->handle); > > + acl_conn_index++; > > + dev_set_name(&conn->dev, "%s:%d:%d", hdev->name, conn->handle, > acl_conn_index); > > > > dev_set_drvdata(&conn->dev, conn); >=20 > can we get a bit more of details on what this is actually trying to > solve. I do not like this way of solving it at all. I think it is trying > to cover up symptoms and not fixing the real issue. >=20 > Regards >=20 > Marcel >=20 The scenario that causes the kernel panic Happens when HID device disconnec= t and connect fast. When HID device disconnects the name clean-up appears 1= 00-300msec after the "hci_disconn_complete_evt()", because the two L2CAP ch= annels need to be closed first. The problem is that the base-band has already cleaned the handle number whe= n the "hci_disconn_complete_evt()" sent. If another connection is initiatin= g right after, the base-band will send "hci_conn_request_evt()" with the sa= me handle number. During this time we have situation that two HCI connectio= ns has the same name, because the handle number from the base-band is the s= ame. There is no reason for the two HCI connections to share the same resou= rce, name. This duplicate name situation will cause Kernel panic. The real issue is that the HCI device connection name is saved in the SYSFS= =20 In the format: "/devices/virtual/bluetooth/hci0/hci0:1" The name in this format depends just on the base-band handle that received = in the "hci_conn_complete_evt()". The device name is cleaned just when the Variable conn->devref becomes 0. In the source code: if (atomic_dec_and_test(&conn->devref)) hci_conn_del_sysfs(conn); The incremental index in the name format: "/devices/virtual/bluetooth/hci0/= hci0:1:<Index>" Solves the problem of two HCI connections with the same name. Regards, Doron ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Bluetooth-next: Add incremental indexing in sysfs HCI connection name. 2011-08-18 11:21 ` Keren, Doron @ 2011-08-18 11:44 ` David Herrmann 2011-08-23 15:47 ` Peter Hurley 2011-09-14 14:42 ` Marcel Holtmann 1 sibling, 1 reply; 9+ messages in thread From: David Herrmann @ 2011-08-18 11:44 UTC (permalink / raw) To: Keren, Doron Cc: Marcel Holtmann, linux-bluetooth, Ilia, Kolominsky, Hadar, Amir On Thu, Aug 18, 2011 at 1:21 PM, Keren, Doron <doronkeren@ti.com> wrote: > Hi Marcel > >> -----Original Message----- >> From: Marcel Holtmann [mailto:marcel@holtmann.org] >> Sent: Thursday, August 18, 2011 12:42 AM >> To: doron.keren.bluez@gmail.com >> Cc: linux-bluetooth@vger.kernel.org; Keren, Doron; Ilia, Kolominsky >> Subject: Re: [PATCH] Bluetooth-next: Add incremental indexing in sysfs H= CI >> connection name. >> >> Hi Doron, >> >> > The patch fixes kernel panic which is due to race condition >> > between the setup of incomming connection and clean-up of the >> > dead one. Observed in the following case: attached HID device >> > disconnects unexpectedly (without performing ACL disconnect ), >> > the device tries to connect again before the ACL link time-out >> > fires, this translates to the HCI_DISCONNECT, HCI_CONNECT_REQ >> > events on the same handle, since HCI_DISCONNECT trigers the clean >> > up of the HID device and handled in different context, the >> > linking/unlinking connection object to sysfs, may mess up. >> > >> > Signed-off-by: Ilia Kolominsky <iliak@ti.com> >> > --- >> > =A0net/bluetooth/hci_sysfs.c | =A0 =A04 +++- >> > =A01 files changed, 3 insertions(+), 1 deletions(-) >> > >> > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c >> > index a6c3aa8..5967d63 100644 >> > --- a/net/bluetooth/hci_sysfs.c >> > +++ b/net/bluetooth/hci_sysfs.c >> > @@ -9,6 +9,7 @@ >> > =A0#include <net/bluetooth/bluetooth.h> >> > =A0#include <net/bluetooth/hci_core.h> >> > >> > +static int acl_conn_index =3D 0; >> > =A0static struct class *bt_class; >> > >> > =A0struct dentry *bt_debugfs; >> > @@ -91,7 +92,8 @@ static void add_conn(struct work_struct *work) >> > =A0 =A0 struct hci_conn *conn =3D container_of(work, struct hci_conn, >> work_add); >> > =A0 =A0 struct hci_dev *hdev =3D conn->hdev; >> > >> > - =A0 dev_set_name(&conn->dev, "%s:%d", hdev->name, conn->handle); >> > + =A0 acl_conn_index++; >> > + =A0 dev_set_name(&conn->dev, "%s:%d:%d", hdev->name, conn->handle, >> acl_conn_index); >> > >> > =A0 =A0 dev_set_drvdata(&conn->dev, conn); >> >> can we get a bit more of details on what this is actually trying to >> solve. I do not like this way of solving it at all. I think it is trying >> to cover up symptoms and not fixing the real issue. >> >> Regards >> >> Marcel >> > > The scenario that causes the kernel panic Happens when HID device disconn= ect and connect fast. When HID device disconnects the name clean-up appears= 100-300msec after the "hci_disconn_complete_evt()", because the two L2CAP = channels need to be closed first. > The problem is that the base-band has already cleaned the handle number w= hen the "hci_disconn_complete_evt()" sent. If another connection is initiat= ing right after, the base-band will send "hci_conn_request_evt()" with the = same handle number. During this time we have situation that two HCI connect= ions has the same name, because the handle number from the base-band is the= same. There is no reason for the two HCI connections to share the same res= ource, name. This duplicate name situation will cause Kernel panic. > > The real issue is that the HCI device connection name is saved in the SYS= FS > In the format: "/devices/virtual/bluetooth/hci0/hci0:1" > The name in this format depends just on the base-band handle that receive= d in the "hci_conn_complete_evt()". The device name is cleaned just when th= e > Variable conn->devref becomes 0. > In the source code: > =A0 =A0if (atomic_dec_and_test(&conn->devref)) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hci_conn_del_sysfs(conn); > > The incremental index in the name format: "/devices/virtual/bluetooth/hci= 0/hci0:1:<Index>" > Solves the problem of two HCI connections with the same name. > > Regards, > Doron > -- > 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 =A0http://vger.kernel.org/majordomo-info.html > I can confirm that there is a reconnection issue and I get the same oops (device "hci0:1" is tried to be added twice) on fast reconnections. However, I tested this fix and Marcel seems right. I no longer get the sysfs-oops, but I now get another oops in hidp_add_connection() so this doesn't fix the problem. After restarting the machine after this oops, my log contains only half of the oops-message so I can't post this here. I will try to get a full log and report it then. Regards David ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Bluetooth-next: Add incremental indexing in sysfs HCI connection name. 2011-08-18 11:44 ` David Herrmann @ 2011-08-23 15:47 ` Peter Hurley 2011-08-24 19:27 ` David Herrmann 0 siblings, 1 reply; 9+ messages in thread From: Peter Hurley @ 2011-08-23 15:47 UTC (permalink / raw) To: David Herrmann, Keren, Doron, Marcel Holtmann Cc: linux-bluetooth, Ilia, Kolominsky, Hadar, Amir SGkgRGF2aWQsIERvcm9uICYgTWFyY2VsLA0KDQpPbiBUaHUsIDIwMTEtMDgtMTggYXQgMDc6NDQg LTA0MDAsIERhdmlkIEhlcnJtYW5uIHdyb3RlOg0KPiBPbiBUaHUsIEF1ZyAxOCwgMjAxMSBhdCAx OjIxIFBNLCBLZXJlbiwgRG9yb24gPGRvcm9ua2VyZW5AdGkuY29tPiB3cm90ZToNCj4gPiBIaSBN YXJjZWwNCj4gPg0KPiA+PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiA+PiBGcm9tOiBN YXJjZWwgSG9sdG1hbm4gW21haWx0bzptYXJjZWxAaG9sdG1hbm4ub3JnXQ0KPiA+PiBTZW50OiBU aHVyc2RheSwgQXVndXN0IDE4LCAyMDExIDEyOjQyIEFNDQo+ID4+IFRvOiBkb3Jvbi5rZXJlbi5i bHVlekBnbWFpbC5jb20NCj4gPj4gQ2M6IGxpbnV4LWJsdWV0b290aEB2Z2VyLmtlcm5lbC5vcmc7 IEtlcmVuLCBEb3JvbjsgSWxpYSwgS29sb21pbnNreQ0KPiA+PiBTdWJqZWN0OiBSZTogW1BBVENI XSBCbHVldG9vdGgtbmV4dDogQWRkIGluY3JlbWVudGFsIGluZGV4aW5nIGluIHN5c2ZzIEhDSQ0K PiA+PiBjb25uZWN0aW9uIG5hbWUuDQo+ID4+DQo+ID4+IEhpIERvcm9uLA0KPiA+Pg0KPiA+PiA+ IFRoZSBwYXRjaCBmaXhlcyBrZXJuZWwgcGFuaWMgd2hpY2ggaXMgZHVlIHRvIHJhY2UgY29uZGl0 aW9uDQo+ID4+ID4gYmV0d2VlbiB0aGUgc2V0dXAgb2YgaW5jb21taW5nIGNvbm5lY3Rpb24gYW5k IGNsZWFuLXVwIG9mIHRoZQ0KPiA+PiA+IGRlYWQgb25lLiBPYnNlcnZlZCBpbiB0aGUgZm9sbG93 aW5nIGNhc2U6IGF0dGFjaGVkIEhJRCBkZXZpY2UNCj4gPj4gPiBkaXNjb25uZWN0cyB1bmV4cGVj dGVkbHkgKHdpdGhvdXQgcGVyZm9ybWluZyBBQ0wgZGlzY29ubmVjdCApLA0KPiA+PiA+IHRoZSBk ZXZpY2UgdHJpZXMgdG8gY29ubmVjdCBhZ2FpbiBiZWZvcmUgdGhlIEFDTCBsaW5rIHRpbWUtb3V0 DQo+ID4+ID4gZmlyZXMsIHRoaXMgdHJhbnNsYXRlcyB0byB0aGUgSENJX0RJU0NPTk5FQ1QsIEhD SV9DT05ORUNUX1JFUQ0KPiA+PiA+IGV2ZW50cyBvbiB0aGUgc2FtZSBoYW5kbGUsIHNpbmNlIEhD SV9ESVNDT05ORUNUIHRyaWdlcnMgdGhlIGNsZWFuDQo+ID4+ID4gdXAgb2YgdGhlIEhJRCBkZXZp Y2UgYW5kIGhhbmRsZWQgaW4gZGlmZmVyZW50IGNvbnRleHQsIHRoZQ0KPiA+PiA+IGxpbmtpbmcv dW5saW5raW5nIGNvbm5lY3Rpb24gb2JqZWN0IHRvIHN5c2ZzLCBtYXkgbWVzcyB1cC4NCj4gPj4g Pg0KPiA+PiA+IFNpZ25lZC1vZmYtYnk6IElsaWEgS29sb21pbnNreSA8aWxpYWtAdGkuY29tPg0K PiA+PiA+IC0tLQ0KPiA+PiA+ICBuZXQvYmx1ZXRvb3RoL2hjaV9zeXNmcy5jIHwgICAgNCArKyst DQo+ID4+ID4gIDEgZmlsZXMgY2hhbmdlZCwgMyBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9ucygt KQ0KPiA+PiA+DQo+ID4+ID4gZGlmZiAtLWdpdCBhL25ldC9ibHVldG9vdGgvaGNpX3N5c2ZzLmMg Yi9uZXQvYmx1ZXRvb3RoL2hjaV9zeXNmcy5jDQo+ID4+ID4gaW5kZXggYTZjM2FhOC4uNTk2N2Q2 MyAxMDA2NDQNCj4gPj4gPiAtLS0gYS9uZXQvYmx1ZXRvb3RoL2hjaV9zeXNmcy5jDQo+ID4+ID4g KysrIGIvbmV0L2JsdWV0b290aC9oY2lfc3lzZnMuYw0KPiA+PiA+IEBAIC05LDYgKzksNyBAQA0K PiA+PiA+ICAjaW5jbHVkZSA8bmV0L2JsdWV0b290aC9ibHVldG9vdGguaD4NCj4gPj4gPiAgI2lu Y2x1ZGUgPG5ldC9ibHVldG9vdGgvaGNpX2NvcmUuaD4NCj4gPj4gPg0KPiA+PiA+ICtzdGF0aWMg aW50IGFjbF9jb25uX2luZGV4ID0gMDsNCj4gPj4gPiAgc3RhdGljIHN0cnVjdCBjbGFzcyAqYnRf Y2xhc3M7DQo+ID4+ID4NCj4gPj4gPiAgc3RydWN0IGRlbnRyeSAqYnRfZGVidWdmczsNCj4gPj4g PiBAQCAtOTEsNyArOTIsOCBAQCBzdGF0aWMgdm9pZCBhZGRfY29ubihzdHJ1Y3Qgd29ya19zdHJ1 Y3QgKndvcmspDQo+ID4+ID4gICAgIHN0cnVjdCBoY2lfY29ubiAqY29ubiA9IGNvbnRhaW5lcl9v Zih3b3JrLCBzdHJ1Y3QgaGNpX2Nvbm4sDQo+ID4+IHdvcmtfYWRkKTsNCj4gPj4gPiAgICAgc3Ry dWN0IGhjaV9kZXYgKmhkZXYgPSBjb25uLT5oZGV2Ow0KPiA+PiA+DQo+ID4+ID4gLSAgIGRldl9z ZXRfbmFtZSgmY29ubi0+ZGV2LCAiJXM6JWQiLCBoZGV2LT5uYW1lLCBjb25uLT5oYW5kbGUpOw0K PiA+PiA+ICsgICBhY2xfY29ubl9pbmRleCsrOw0KPiA+PiA+ICsgICBkZXZfc2V0X25hbWUoJmNv bm4tPmRldiwgIiVzOiVkOiVkIiwgaGRldi0+bmFtZSwgY29ubi0+aGFuZGxlLA0KPiA+PiBhY2xf Y29ubl9pbmRleCk7DQo+ID4+ID4NCj4gPj4gPiAgICAgZGV2X3NldF9kcnZkYXRhKCZjb25uLT5k ZXYsIGNvbm4pOw0KPiA+Pg0KPiA+PiBjYW4gd2UgZ2V0IGEgYml0IG1vcmUgb2YgZGV0YWlscyBv biB3aGF0IHRoaXMgaXMgYWN0dWFsbHkgdHJ5aW5nIHRvDQo+ID4+IHNvbHZlLiBJIGRvIG5vdCBs aWtlIHRoaXMgd2F5IG9mIHNvbHZpbmcgaXQgYXQgYWxsLiBJIHRoaW5rIGl0IGlzIHRyeWluZw0K PiA+PiB0byBjb3ZlciB1cCBzeW1wdG9tcyBhbmQgbm90IGZpeGluZyB0aGUgcmVhbCBpc3N1ZS4N Cj4gPj4NCj4gPj4gUmVnYXJkcw0KPiA+Pg0KPiA+PiBNYXJjZWwNCj4gPj4NCj4gPg0KPiA+IFRo ZSBzY2VuYXJpbyB0aGF0IGNhdXNlcyB0aGUga2VybmVsIHBhbmljIEhhcHBlbnMgd2hlbiBISUQg ZGV2aWNlIGRpc2Nvbm5lY3QgYW5kIGNvbm5lY3QgZmFzdC4gV2hlbiBISUQgZGV2aWNlIGRpc2Nv bm5lY3RzIHRoZSBuYW1lIGNsZWFuLXVwIGFwcGVhcnMgMTAwLTMwMG1zZWMgYWZ0ZXIgdGhlICJo Y2lfZGlzY29ubl9jb21wbGV0ZV9ldnQoKSIsIGJlY2F1c2UgdGhlIHR3byBMMkNBUCBjaGFubmVs cyBuZWVkIHRvIGJlIGNsb3NlZCBmaXJzdC4NCg0KRG9yb24sDQpJdCdzIG5vdCBiZWNhdXNlIHRo ZSBMMkNBUCBjaGFubmVscyBuZWVkIGNsb3NpbmcgLSB0aGUgY2hhbm5lbHMgYXJlDQpjbG9zZWQg d2l0aGluIHRoZSBoY2lfZGlzY29ubl9jb21wbGV0ZV9ldnQoKSBjYWxsIHRyZWU6DQogICBoY2lf ZGlzY29ubl9jb21wbGV0ZV9ldnQNCiAgICAgIGhjaV9wcm90b19kaXNjb25uX2NmbQ0KICAgICAg ICAgbDJjYXBfZGlzY29ubl9jZm0NCiAgICAgICAgICAgIGwyY2FwX2Nvbm5fZGVsDQogICAgICAg ICAgICAgICBsMmNhcF9jaGFuX2RlbA0KVGhlIHNrLT5za19zdGF0ZV9jaGFuZ2UoKSBpbiBsMmNh cF9jaGFuX2RlbCB3YWtlcyB1cCB0aGUga2hpZCB0aHJlYWQNCndoaWNoIHRlc3RzIHRoZSBza19z dGF0ZSBhbmQgZXhpdHMgaXRzIHByb2Nlc3NpbmcgbG9vcCwgZXZlbnR1YWxseQ0KcmVhY2hpbmcg dGhlIGhjaV9jb25uX3B1dF9kZXZpY2UgYXMgdGhlIHRocmVhZCBzaHV0cyBkb3duLg0KDQpUaGUg cmVhc29uIGZvciB0aGUgZGVsYXkgaW4gZGVsZXRpbmcgdGhlIHN5c2ZzIGRldmljZSBpcyB0aGF0 IHRoZSBjaGlsZA0KSElEIGRldmljZSBuZWVkcyB0byBiZSBkZXN0cm95ZWQgZmlyc3QgdG8gcHJl c2VydmUgdGhlIHVldmVudCBvcmRlciB0bw0KdXNlci1zcGFjZSAod2hpY2ggaXMgdGhlIHB1cnBv c2Ugb2YgaGNpX2Nvbm5faG9sZF9kZXZpY2UvX3B1dF9kZXZpY2UpLg0KDQo+ID4gVGhlIHByb2Js ZW0gaXMgdGhhdCB0aGUgYmFzZS1iYW5kIGhhcyBhbHJlYWR5IGNsZWFuZWQgdGhlIGhhbmRsZSBu dW1iZXIgd2hlbiB0aGUgImhjaV9kaXNjb25uX2NvbXBsZXRlX2V2dCgpIiBzZW50LiBJZiBhbm90 aGVyIGNvbm5lY3Rpb24gaXMgaW5pdGlhdGluZyByaWdodCBhZnRlciwgdGhlIGJhc2UtYmFuZCB3 aWxsIHNlbmQgImhjaV9jb25uX3JlcXVlc3RfZXZ0KCkiIHdpdGggdGhlIHNhbWUgaGFuZGxlIG51 bWJlci4gRHVyaW5nIHRoaXMgdGltZSB3ZSBoYXZlIHNpdHVhdGlvbiB0aGF0IHR3byBIQ0kgY29u bmVjdGlvbnMgaGFzIHRoZSBzYW1lIG5hbWUsIGJlY2F1c2UgdGhlIGhhbmRsZSBudW1iZXIgZnJv bSB0aGUgYmFzZS1iYW5kIGlzIHRoZSBzYW1lLiBUaGVyZSBpcyBubyByZWFzb24gZm9yIHRoZSB0 d28gSENJIGNvbm5lY3Rpb25zIHRvIHNoYXJlIHRoZSBzYW1lIHJlc291cmNlLCBuYW1lLiBUaGlz IGR1cGxpY2F0ZSBuYW1lIHNpdHVhdGlvbiB3aWxsIGNhdXNlIEtlcm5lbCBwYW5pYy4NCj4gPg0K PiA+IFRoZSByZWFsIGlzc3VlIGlzIHRoYXQgdGhlIEhDSSBkZXZpY2UgY29ubmVjdGlvbiBuYW1l IGlzIHNhdmVkIGluIHRoZSBTWVNGUw0KPiA+IEluIHRoZSBmb3JtYXQ6ICIvZGV2aWNlcy92aXJ0 dWFsL2JsdWV0b290aC9oY2kwL2hjaTA6MSINCj4gPiBUaGUgbmFtZSBpbiB0aGlzIGZvcm1hdCBk ZXBlbmRzIGp1c3Qgb24gdGhlIGJhc2UtYmFuZCBoYW5kbGUgdGhhdCByZWNlaXZlZCBpbiB0aGUg ImhjaV9jb25uX2NvbXBsZXRlX2V2dCgpIi4gVGhlIGRldmljZSBuYW1lIGlzIGNsZWFuZWQganVz dCB3aGVuIHRoZQ0KPiA+IFZhcmlhYmxlIGNvbm4tPmRldnJlZiBiZWNvbWVzIDAuDQo+ID4gSW4g dGhlIHNvdXJjZSBjb2RlOg0KPiA+ICAgIGlmIChhdG9taWNfZGVjX2FuZF90ZXN0KCZjb25uLT5k ZXZyZWYpKQ0KPiA+ICAgICAgICAgICAgICAgIGhjaV9jb25uX2RlbF9zeXNmcyhjb25uKTsNCj4g Pg0KPiA+IFRoZSBpbmNyZW1lbnRhbCBpbmRleCBpbiB0aGUgbmFtZSBmb3JtYXQ6ICIvZGV2aWNl cy92aXJ0dWFsL2JsdWV0b290aC9oY2kwL2hjaTA6MTo8SW5kZXg+Ig0KPiA+IFNvbHZlcyB0aGUg cHJvYmxlbSBvZiB0d28gSENJIGNvbm5lY3Rpb25zIHdpdGggdGhlIHNhbWUgbmFtZS4NCg0KVGhp cyBpcyB0aGUgc2ltcGxlc3Qgc29sdXRpb24uIFRoZSBvdGhlciB3YXkgLSB3aGljaCBpcyBtb3Jl IGNvbXBsaWNhdGVkDQpidXQgcHJlc2VydmVzIG5hbWluZyAtIGlzIHRvIGRvIGF3YXkgd2l0aCB0 aGUNCmhjaV9jb25uX2hvbGRfZGV2aWNlL19wdXRfZGV2aWNlLCBhbmQgaW5zdGVhZCwgY29ycmVj dGx5IHNlcXVlbmNlIEhJRA0KY2hpbGQgZGV2aWNlIGRlc3RydWN0aW9uIHdpdGhpbiB0aGUgZGVs X2Nvbm4oKSB3b3JrIHF1ZXVlIGhhbmRsZXIgLQ0KYmFzaWNhbGx5IGltcG9zZSB3aGF0IHRoZSBk ZXZpY2UgbW9kZWwgc2hvdWxkIGFscmVhZHkgZG8uDQoNCj4gSSBjYW4gY29uZmlybSB0aGF0IHRo ZXJlIGlzIGEgcmVjb25uZWN0aW9uIGlzc3VlIGFuZCBJIGdldCB0aGUgc2FtZQ0KPiBvb3BzIChk ZXZpY2UgImhjaTA6MSIgaXMgdHJpZWQgdG8gYmUgYWRkZWQgdHdpY2UpIG9uIGZhc3QNCj4gcmVj b25uZWN0aW9ucy4NCj4gDQo+IEhvd2V2ZXIsIEkgdGVzdGVkIHRoaXMgZml4IGFuZCBNYXJjZWwg c2VlbXMgcmlnaHQuIEkgbm8gbG9uZ2VyIGdldCB0aGUNCj4gc3lzZnMtb29wcywgYnV0IEkgbm93 IGdldCBhbm90aGVyIG9vcHMgaW4gaGlkcF9hZGRfY29ubmVjdGlvbigpIHNvDQo+IHRoaXMgZG9l c24ndCBmaXggdGhlIHByb2JsZW0uDQo+IEFmdGVyIHJlc3RhcnRpbmcgdGhlIG1hY2hpbmUgYWZ0 ZXIgdGhpcyBvb3BzLCBteSBsb2cgY29udGFpbnMgb25seQ0KPiBoYWxmIG9mIHRoZSBvb3BzLW1l c3NhZ2Ugc28gSSBjYW4ndCBwb3N0IHRoaXMgaGVyZS4gSSB3aWxsIHRyeSB0byBnZXQNCj4gYSBm dWxsIGxvZyBhbmQgcmVwb3J0IGl0IHRoZW4uDQoNCkRhdmlkLA0KSSBrbm93IHlvdSBhbHJlYWR5 IHBvc3RlZCBhIHBhdGNoIGZvciB0aGlzICh3aGljaCBsb29rcyBvayBmb3IgZml4aW5nDQp0aGUg b29wcyBzcGVjaWZpY2FsbHkpLCBidXQgaXQnZCBzdGlsbCBiZSBoZWxwZnVsIHRvIHNlZSBhIGRl YnVnIGtlcm5lbA0KbG9nIHRoYXQgbGVkIHVwIHRvIHRoYXQuIFRoZXJlJ3Mgb25seSBhIGNvdXBs ZSBvZiByZWFzb25zIC0tIGFsbCBiYWQgLS0NCndoeSB0aGUgaGNpIGNvbm5lY3Rpb24gY291bGQg bm90IGJlIGZvdW5kIGluIHRoZSBjb25uZWN0aW9uIGxpc3Qgd2hpbGUNCnRoZSBjdHJsICYgaW50 ciBzb2NrZXRzIGFyZSBjb25uZWN0ZWQgKHdoaWNoIGFyZSB0ZXN0ZWQganVzdCBwcmlvciB0bw0K aGlkcF9hZGRfY29ubmVjdGlvbikuDQoNCk1hcmNlbCwNCkl0IHNlZW1zIHRvIG1lIHRoYXQgaGlk cF9nZXRfZGV2aWNlLCByZmNvbW1fZ2V0X2RldmljZSAmIGJuZXBfZ2V0X2RldmljZQ0KYXJlIHVu c2FmZS4gRWxzZXdoZXJlLCBjb25jdXJyZW50IGFjY2VzcyB0byB0aGUgY29ubl9oYXNoIGxpc3Qg aXMNCnByb3RlY3RlZCBieSBhY3F1aXJpbmcgdGhlIGRldmljZSBsb2NrIChoY2lfZGV2X2xvY2sv X3VubG9jayBhbmQgX2JoDQp2YXJpYW50cykuIENhbiB5b3UgY29uZmlybSBteSB1bmRlcnN0YW5k aW5nIGhlcmU/DQoNClJlZ2FyZHMsDQpQZXRlciBIdXJsZXkNCg== ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Bluetooth-next: Add incremental indexing in sysfs HCI connection name. 2011-08-23 15:47 ` Peter Hurley @ 2011-08-24 19:27 ` David Herrmann 2011-08-24 21:51 ` Peter Hurley 0 siblings, 1 reply; 9+ messages in thread From: David Herrmann @ 2011-08-24 19:27 UTC (permalink / raw) To: Peter Hurley Cc: Keren, Doron, Marcel Holtmann, linux-bluetooth, Ilia, Kolominsky, Hadar, Amir Hi Peter On Tue, Aug 23, 2011 at 5:47 PM, Peter Hurley <peter@hurleysoftware.com> wrote: > David, > I know you already posted a patch for this (which looks ok for fixing > the oops specifically), but it'd still be helpful to see a debug kernel > log that led up to that. There's only a couple of reasons -- all bad -- > why the hci connection could not be found in the connection list while > the ctrl & intr sockets are connected (which are tested just prior to > hidp_add_connection). Sorry for the delay. I currently have no other machine to run a debug kernel and I really don't like provoking oopses on my working machine. However, the next days I will setup a machine and try to send a full trace. Are there any useful config symbols for bluetooth debug? Despite the standard kernel-wide debug symbols. What are possible reasons why an l2cap connection can be available without an underlying ACL connection? My fix eliminates the oops but I still think that it is not fixing the real problem (anyway, I'd really like to see it upstream). I will try to send a debug log if I can reproduce it. > Regards, > Peter Hurley Cheers David ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Bluetooth-next: Add incremental indexing in sysfs HCI connection name. 2011-08-24 19:27 ` David Herrmann @ 2011-08-24 21:51 ` Peter Hurley 2011-08-25 17:11 ` David Herrmann 0 siblings, 1 reply; 9+ messages in thread From: Peter Hurley @ 2011-08-24 21:51 UTC (permalink / raw) To: David Herrmann; +Cc: Marcel Holtmann, linux-bluetooth Hi David, On Wed, 2011-08-24 at 15:27 -0400, David Herrmann wrote: > Hi Peter >=20 > On Tue, Aug 23, 2011 at 5:47 PM, Peter Hurley <peter@hurleysoftware.com> = wrote: > > David, > > I know you already posted a patch for this (which looks ok for fixing > > the oops specifically), but it'd still be helpful to see a debug kernel > > log that led up to that. There's only a couple of reasons -- all bad -- > > why the hci connection could not be found in the connection list while > > the ctrl & intr sockets are connected (which are tested just prior to > > hidp_add_connection). >=20 > Sorry for the delay. I currently have no other machine to run a debug > kernel and I really don't like provoking oopses on my working machine. > However, the next days I will setup a machine and try to send a full > trace. > Are there any useful config symbols for bluetooth debug? Despite the > standard kernel-wide debug symbols. I understand about trying to invoke oops. A one-line BT_DBG(""); in the NULL parent error pathway of hidp_setup_hid with your patch would be just as good (because it's the condition I'm trying to catch). Don't need (or want) kernel-wide output. If your kernel is configured for dynamic debug output, this is as easy as: $ sudo su $ echo -n 'module bluetooth +p' > /sys/kernel/debug/dynamic_debug/control $ echo -n 'module hidp +p' > /sys/kernel/debug/dynamic_debug/control Run test, then: $ echo -n 'module bluetooth -p' > /sys/kernel/debug/dynamic_debug/control $ echo -n 'module hidp -p' > /sys/kernel/debug/dynamic_debug/control $ exit Dynamic debug output is enabled in the Kconfig here: Kernel hacking / Enable dynamic printk() support =3D> Y > What are possible reasons why an l2cap connection can be available > without an underlying ACL connection? Well, if the ACL connection is gone, the l2cap channels should be gone as well, but the matching l2cap sockets stay around because of the references on them. But the sockets should be in state BT_CLOSED, which is tested just prior to calling hidp_add_connection. Possible reasons why the apparent contradiction: 1. Neither socket is locked by the hidp driver AFAICT. If true, then the sock state (and by extension, the l2cap channels and hci connection list could change at any time). l2cap_conn_del acquires the socket lock prior to l2cap_chan_del. 2. HCI connection list (conn_hash) is corrupted. hidp_get_device looks smp-unsafe to me. I think it needs to be acquiring device lock via hci_dev_lock_bh. 3. Some other unknown race. That's why a debug log would help. It would help establish when l2cap_conn_del and hci_conn_del are called relative to hidp_add_connection, and what the other pre-conditions are. > My fix eliminates the oops but I > still think that it is not fixing the real problem (anyway, I'd really > like to see it upstream). Not my place to say re: the patch. Gustavo Padovan and Marcel Holtmann are the gatekeepers here. (Looks ok to me though, at least as far as preventing NULL dereferences and handling the error exit properly). Regards, Peter Hurley ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Bluetooth-next: Add incremental indexing in sysfs HCI connection name. 2011-08-24 21:51 ` Peter Hurley @ 2011-08-25 17:11 ` David Herrmann 0 siblings, 0 replies; 9+ messages in thread From: David Herrmann @ 2011-08-25 17:11 UTC (permalink / raw) To: Peter Hurley; +Cc: Marcel Holtmann, linux-bluetooth On Wed, Aug 24, 2011 at 11:51 PM, Peter Hurley <peter@hurleysoftware.com> wrote: > Hi David, > >> What are possible reasons why an l2cap connection can be available >> without an underlying ACL connection? > > Well, if the ACL connection is gone, the l2cap channels should be gone > as well, but the matching l2cap sockets stay around because of the > references on them. But the sockets should be in state BT_CLOSED, which > is tested just prior to calling hidp_add_connection. If I disconnect my HID device, the l2cap sockets stay alive for 20 seconds! Even though the baseband connection is closed. I think this bug is not related to the hid/sysfs bug here, but may be also interesting. > Possible reasons why the apparent contradiction: > 1. Neither socket is locked by the hidp driver AFAICT. If true, then the > sock state (and by extension, the l2cap channels and hci connection list > could change at any time). l2cap_conn_del acquires the socket lock prior > to l2cap_chan_del. I guess the problem is that the l2cap channels are BT_CONNECTED when HIDP checks them but while hidp_add_conenction is called the l2cap channels close and the ACL connection is removed. hidp_add_connection then fails because it can't find the ACL connection. This is fixed by my patch but there is still a race condition. The ACL lookup does not increase reference count of the connection and hence it may disappear while we hold a pointer to it. > 2. HCI connection list (conn_hash) is corrupted. hidp_get_device looks > smp-unsafe to me. I think it needs to be acquiring device lock via > hci_dev_lock_bh. > 3. Some other unknown race. > > That's why a debug log would help. It would help establish when > l2cap_conn_del and hci_conn_del are called relative to > hidp_add_connection, and what the other pre-conditions are. It is quite hard to reproduce this bug, but I tried again a couple of times and got a full debug log now. However, after looking at it I noticed that I did not apply the patch from Doron Keren so this log is triggered by the sysfs-duplicate bug and not by the one I discovered. Last few lines with OOPS message from the kernel log: https://gist.github.com/1171138 Full kernel.log for the session (including the log above): http://dl.dropbox.com/u/1475019/kernel.txt > > Regards, > Peter Hurley > I will try again with this patch applied, but I think both bugs are related to each other. Regards David ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] Bluetooth-next: Add incremental indexing in sysfs HCI connection name. 2011-08-18 11:21 ` Keren, Doron 2011-08-18 11:44 ` David Herrmann @ 2011-09-14 14:42 ` Marcel Holtmann 1 sibling, 0 replies; 9+ messages in thread From: Marcel Holtmann @ 2011-09-14 14:42 UTC (permalink / raw) To: Keren, Doron; +Cc: linux-bluetooth, Ilia, Kolominsky, Hadar, Amir Hi Doron, > > > The patch fixes kernel panic which is due to race condition > > > between the setup of incomming connection and clean-up of the > > > dead one. Observed in the following case: attached HID device > > > disconnects unexpectedly (without performing ACL disconnect ), > > > the device tries to connect again before the ACL link time-out > > > fires, this translates to the HCI_DISCONNECT, HCI_CONNECT_REQ > > > events on the same handle, since HCI_DISCONNECT trigers the clean > > > up of the HID device and handled in different context, the > > > linking/unlinking connection object to sysfs, may mess up. > > > > > > Signed-off-by: Ilia Kolominsky <iliak@ti.com> > > > --- > > > net/bluetooth/hci_sysfs.c | 4 +++- > > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > > > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c > > > index a6c3aa8..5967d63 100644 > > > --- a/net/bluetooth/hci_sysfs.c > > > +++ b/net/bluetooth/hci_sysfs.c > > > @@ -9,6 +9,7 @@ > > > #include <net/bluetooth/bluetooth.h> > > > #include <net/bluetooth/hci_core.h> > > > > > > +static int acl_conn_index = 0; > > > static struct class *bt_class; > > > > > > struct dentry *bt_debugfs; > > > @@ -91,7 +92,8 @@ static void add_conn(struct work_struct *work) > > > struct hci_conn *conn = container_of(work, struct hci_conn, > > work_add); > > > struct hci_dev *hdev = conn->hdev; > > > > > > - dev_set_name(&conn->dev, "%s:%d", hdev->name, conn->handle); > > > + acl_conn_index++; > > > + dev_set_name(&conn->dev, "%s:%d:%d", hdev->name, conn->handle, > > acl_conn_index); > > > > > > dev_set_drvdata(&conn->dev, conn); > > > > can we get a bit more of details on what this is actually trying to > > solve. I do not like this way of solving it at all. I think it is trying > > to cover up symptoms and not fixing the real issue. > > > > Regards > > > > Marcel > > > > The scenario that causes the kernel panic Happens when HID device disconnect and connect fast. When HID device disconnects the name clean-up appears 100-300msec after the "hci_disconn_complete_evt()", because the two L2CAP channels need to be closed first. > The problem is that the base-band has already cleaned the handle number when the "hci_disconn_complete_evt()" sent. If another connection is initiating right after, the base-band will send "hci_conn_request_evt()" with the same handle number. During this time we have situation that two HCI connections has the same name, because the handle number from the base-band is the same. There is no reason for the two HCI connections to share the same resource, name. This duplicate name situation will cause Kernel panic. > > The real issue is that the HCI device connection name is saved in the SYSFS > In the format: "/devices/virtual/bluetooth/hci0/hci0:1" > The name in this format depends just on the base-band handle that received in the "hci_conn_complete_evt()". The device name is cleaned just when the > Variable conn->devref becomes 0. > In the source code: > if (atomic_dec_and_test(&conn->devref)) > hci_conn_del_sysfs(conn); > > The incremental index in the name format: "/devices/virtual/bluetooth/hci0/hci0:1:<Index>" > Solves the problem of two HCI connections with the same name. so I had another look at this and we are still trying to fix the symptoms and not the real bug. The real bug here is that we have basically sysfs file creation racing with the HCI event handling. One done inside a tasklet and the other done inside a workqueue. We have this problem since a long time and we really have to move HCI processing into a workqueue. That is the only way to finally be able to remove all races that might happen here. Regards Marcel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-09-14 14:42 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-08-17 16:01 [PATCH] Bluetooth-next: Add incremental indexing in sysfs HCI connection name doron.keren.bluez 2011-08-17 21:42 ` Marcel Holtmann 2011-08-18 11:21 ` Keren, Doron 2011-08-18 11:44 ` David Herrmann 2011-08-23 15:47 ` Peter Hurley 2011-08-24 19:27 ` David Herrmann 2011-08-24 21:51 ` Peter Hurley 2011-08-25 17:11 ` David Herrmann 2011-09-14 14:42 ` Marcel Holtmann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.