[bluetooth-next V2] Bluetooth: handle device reset event

[bluetooth-next V2] Bluetooth: handle device reset event

[Tomas Winkler]

From: Gregory Paskar <gregory.paskar@intel.com>

A Bluetooth device experiencing hardware failure may issue
a HARDWARE_ERROR hci event. The reaction to this event is device
reset flow implemented in following sequence.

1. Notify: HCI_DEV_DOWN
2. Reinitialize internal structures.
3. Call driver flush function
4. Send HCI reset request to the device.
5. Send HCI init sequence reset to the device.
6. Notify HCI_DEV_UP.

Signed-off-by: Gregory Paskar <gregory.paskar@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Reviewed-by: Ron Rindjunsky <ron.rindjunsky@intel.com>
---
V2:
1. Fix email typo
2. Fix style issues

 include/net/bluetooth/hci.h      |    5 ++++
 include/net/bluetooth/hci_core.h |    2 +
 net/bluetooth/hci_core.c         |   47 +++++++++++++++++++++++++++++++++++++-
 net/bluetooth/hci_event.c        |    3 ++
 4 files changed, 56 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index ed3aea1..cd23eb4 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -691,6 +691,11 @@ struct hci_ev_cmd_status {
 	__le16   opcode;
 } __attribute__ ((packed));
 
+#define HCI_EV_HARDWARE_ERROR 0x10
+struct hci_ev_hw_error {
+	__u8     hw_code;
+} __attribute__ ((packed));
+
 #define HCI_EV_ROLE_CHANGE		0x12
 struct hci_ev_role_change {
 	__u8     status;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 7b86094..d2b1cba 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -110,6 +110,8 @@ struct hci_dev {
 	struct tasklet_struct	rx_task;
 	struct tasklet_struct	tx_task;
 
+	struct work_struct 	hw_err_work;
+
 	struct sk_buff_head	rx_q;
 	struct sk_buff_head	raw_q;
 	struct sk_buff_head	cmd_q;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 94ba349..b7c0fbf 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -834,6 +834,51 @@ static int hci_rfkill_set_block(void *data, bool blocked)
 	return 0;
 }
 
+static void hci_device_hw_error(struct hci_dev *hdev)
+{
+	hci_req_lock(hdev);
+
+	/* notify: going down */
+	clear_bit(HCI_UP, &hdev->flags);
+	hci_notify(hdev, HCI_DEV_DOWN);
+
+	tasklet_disable(&hdev->tx_task);
+	skb_queue_purge(&hdev->rx_q);
+	skb_queue_purge(&hdev->cmd_q);
+
+	hci_dev_lock_bh(hdev);
+	inquiry_cache_flush(hdev);
+	hci_conn_hash_flush(hdev);
+	hci_dev_unlock_bh(hdev);
+
+	if (hdev->flush)
+		hdev->flush(hdev);
+
+	/* reset device */
+	atomic_set(&hdev->cmd_cnt, 1);
+	hdev->acl_cnt = 0;
+	hdev->sco_cnt = 0;
+	hci_reset_req(hdev, 0);
+
+	/* init device */
+	atomic_set(&hdev->cmd_cnt, 1);
+	hci_init_req(hdev, 0);
+
+	tasklet_enable(&hdev->tx_task);
+
+	/* notify: going back up */
+	set_bit(HCI_UP, &hdev->flags);
+	hci_notify(hdev, HCI_DEV_UP);
+
+	hci_req_unlock(hdev);
+}
+
+static void hci_hw_err_work(struct work_struct *ws)
+{
+	struct hci_dev *hdev = container_of(ws, struct hci_dev, hw_err_work);
+	hci_device_hw_error(hdev);
+}
+
 static const struct rfkill_ops hci_rfkill_ops = {
 	.set_block = hci_rfkill_set_block,
 };
@@ -903,7 +948,7 @@ int hci_register_dev(struct hci_dev *hdev)
 	tasklet_init(&hdev->cmd_task, hci_cmd_task,(unsigned long) hdev);
 	tasklet_init(&hdev->rx_task, hci_rx_task, (unsigned long) hdev);
 	tasklet_init(&hdev->tx_task, hci_tx_task, (unsigned long) hdev);
-
+	INIT_WORK(&hdev->hw_err_work, hci_hw_err_work);
 	skb_queue_head_init(&hdev->rx_q);
 	skb_queue_head_init(&hdev->cmd_q);
 	skb_queue_head_init(&hdev->raw_q);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 28517ba..e8f48cc 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1953,6 +1953,9 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
 	case HCI_EV_REMOTE_HOST_FEATURES:
 		hci_remote_host_features_evt(hdev, skb);
 		break;
+	case HCI_EV_HARDWARE_ERROR:
+		schedule_work(&hdev->hw_err_work);
+		break;
 
 	default:
 		BT_DBG("%s event 0x%x", hdev->name, event);
-- 
1.6.0.6

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[bluetooth-next] bluetooth: hci_sysfs: use strict_strtoul instead of simple_strtoul

[Tomas Winkler]

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 net/bluetooth/hci_sysfs.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 2bc6f6a..e4bfce5 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -286,11 +286,11 @@ static ssize_t show_idle_timeout(struct device *dev, struct device_attribute *at
 static ssize_t store_idle_timeout(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct hci_dev *hdev = dev_get_drvdata(dev);
-	char *ptr;
-	__u32 val;
+	unsigned long val;
+	int ret;
 
-	val = simple_strtoul(buf, &ptr, 10);
-	if (ptr == buf)
+	ret = strict_strtoul(buf, 0, &val);
+	if (ret)
 		return -EINVAL;
 
 	if (val != 0 && (val < 500 || val > 3600000))
@@ -310,11 +310,11 @@ static ssize_t show_sniff_max_interval(struct device *dev, struct device_attribu
 static ssize_t store_sniff_max_interval(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct hci_dev *hdev = dev_get_drvdata(dev);
-	char *ptr;
-	__u16 val;
+	unsigned long val;
+	int ret;
 
-	val = simple_strtoul(buf, &ptr, 10);
-	if (ptr == buf)
+	ret = strict_strtoul(buf, 0, &val);
+	if (ret)
 		return -EINVAL;
 
 	if (val < 0x0002 || val > 0xFFFE || val % 2)
@@ -337,11 +337,11 @@ static ssize_t show_sniff_min_interval(struct device *dev, struct device_attribu
 static ssize_t store_sniff_min_interval(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct hci_dev *hdev = dev_get_drvdata(dev);
-	char *ptr;
-	__u16 val;
+	unsigned long val;
+	int ret;
 
-	val = simple_strtoul(buf, &ptr, 10);
-	if (ptr == buf)
+	ret = strict_strtoul(buf, 0, &val);
+	if (ret)
 		return -EINVAL;
 
 	if (val < 0x0002 || val > 0xFFFE || val % 2)
-- 
1.6.0.6

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [bluetooth-next V2] Bluetooth: handle device reset event

[Marcel Holtmann]

Hi Tomas,

> A Bluetooth device experiencing hardware failure may issue
> a HARDWARE_ERROR hci event. The reaction to this event is device
> reset flow implemented in following sequence.
> 
> 1. Notify: HCI_DEV_DOWN
> 2. Reinitialize internal structures.
> 3. Call driver flush function
> 4. Send HCI reset request to the device.
> 5. Send HCI init sequence reset to the device.
> 6. Notify HCI_DEV_UP.

I prefer if we create a generic per controller workqueue first before
having a workqueue for every task. Something similar to what the
mac80211 layer offers right now.

Also in a second step we might wanna move the HCI event processing
completely into a workqueue. If we get no performance hit with that,
then sysfs handling and device reset becomes a lot simpler and less
prone to race conditions with device removal.

Regards

Marcel

Re: [bluetooth-next] bluetooth: hci_sysfs: use strict_strtoul instead of simple_strtoul

[Marcel Holtmann]

Hi Tomas,

> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  net/bluetooth/hci_sysfs.c |   24 ++++++++++++------------
>  1 files changed, 12 insertions(+), 12 deletions(-)

can you please explain the rational behind this change. What is the
benefit? I just fail to see it right away.

Regards

Marcel

RE: [bluetooth-next] bluetooth: hci_sysfs: use strict_strtoul instead of simple_strtoul

[Winkler, Tomas]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogTWFyY2VsIEhvbHRtYW5u
IFttYWlsdG86bWFyY2VsQGhvbHRtYW5uLm9yZ10NCj4gU2VudDogVHVlc2RheSwgTWFyY2ggMDks
IDIwMTAgMzowNCBBTQ0KPiBUbzogV2lua2xlciwgVG9tYXMNCj4gQ2M6IGxpbnV4LWJsdWV0b290
aEB2Z2VyLmtlcm5lbC5vcmc7IENvaGVuLCBHdXk7IFJpbmRqdW5za3ksIFJvbg0KPiBTdWJqZWN0
OiBSZTogW2JsdWV0b290aC1uZXh0XSBibHVldG9vdGg6IGhjaV9zeXNmczogdXNlIHN0cmljdF9z
dHJ0b3VsDQo+IGluc3RlYWQgb2Ygc2ltcGxlX3N0cnRvdWwNCj4gDQo+IEhpIFRvbWFzLA0KPiAN
Cj4gPiBTaWduZWQtb2ZmLWJ5OiBUb21hcyBXaW5rbGVyIDx0b21hcy53aW5rbGVyQGludGVsLmNv
bT4NCj4gPiAtLS0NCj4gPiAgbmV0L2JsdWV0b290aC9oY2lfc3lzZnMuYyB8ICAgMjQgKysrKysr
KysrKysrLS0tLS0tLS0tLS0tDQo+ID4gIDEgZmlsZXMgY2hhbmdlZCwgMTIgaW5zZXJ0aW9ucygr
KSwgMTIgZGVsZXRpb25zKC0pDQo+IA0KPiBjYW4geW91IHBsZWFzZSBleHBsYWluIHRoZSByYXRp
b25hbCBiZWhpbmQgdGhpcyBjaGFuZ2UuIFdoYXQgaXMgdGhlDQo+IGJlbmVmaXQ/IEkganVzdCBm
YWlsIHRvIHNlZSBpdCByaWdodCBhd2F5Lg0KDQpUaGUgcmVhbCByZWFzb24gdXNpbmcgc3RyaWN0
IGluc3RlYWQgb2Ygc2ltcGxlIHN0cnRvdWwgaXMgZXhwbGFpbmVkIGhlcmUgdGh0dHA6Ly93d3cu
a2VybmVsLm9yZy9kb2MvaHRtbGRvY3Mva2VybmVsLWFwaS9yZTQyLmh0bWwNCkluIHRoZSBib3R0
b20gbGluZSBpdCBqdXN0IHNvbWV0aGluZyBhIGNoYWNrcGF0Y2ggaXMgY29tcGxhaW4gYWJvdXQu
IEkndmUgdG91Y2hlZCB0aGUgZmlsZSB0byBpbnNlcnQgc29tZSB0ZXN0IGhvb2sgZm9yIHRoZSBI
Q0kgcmVzZXQgc28gSSBmaXhlZCB0aGF0IG9uIHRoZSB3YXkuIA0KDQpUaGFua3MNClRvbWFzDQoN
Ci0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLQpJbnRlbCBJc3JhZWwgKDc0KSBMaW1pdGVkCgpUaGlzIGUtbWFpbCBhbmQg
YW55IGF0dGFjaG1lbnRzIG1heSBjb250YWluIGNvbmZpZGVudGlhbCBtYXRlcmlhbCBmb3IKdGhl
IHNvbGUgdXNlIG9mIHRoZSBpbnRlbmRlZCByZWNpcGllbnQocykuIEFueSByZXZpZXcgb3IgZGlz
dHJpYnV0aW9uCmJ5IG90aGVycyBpcyBzdHJpY3RseSBwcm9oaWJpdGVkLiBJZiB5b3UgYXJlIG5v
dCB0aGUgaW50ZW5kZWQKcmVjaXBpZW50LCBwbGVhc2UgY29udGFjdCB0aGUgc2VuZGVyIGFuZCBk
ZWxldGUgYWxsIGNvcGllcy4K

RE: [bluetooth-next] bluetooth: hci_sysfs: use strict_strtoul instead of simple_strtoul

[Marcel Holtmann]

Hi Tomas,

> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > ---
> > >  net/bluetooth/hci_sysfs.c |   24 ++++++++++++------------
> > >  1 files changed, 12 insertions(+), 12 deletions(-)
> > 
> > can you please explain the rational behind this change. What is the
> > benefit? I just fail to see it right away.
> 
> The real reason using strict instead of simple strtoul is explained here thttp://www.kernel.org/doc/htmldocs/kernel-api/re42.html
> In the bottom line it just something a chackpatch is complain about. I've touched the file to insert some test hook for the HCI reset so I fixed that on the way. 

I am fine with that. However please use the following constructs:

	if (strict_strtoul(...) < 0)
		return -EINVAL;

There is no point in having ret variable if you don't use it.

Also I like to have a commit body and not only a subject line. It
doesn't have to be a novel, but only the subject is not good enough for
me.

Regards

Marcel

RE: [bluetooth-next V2] Bluetooth: handle device reset event

[Winkler, Tomas]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogTWFyY2VsIEhvbHRtYW5u
IFttYWlsdG86bWFyY2VsQGhvbHRtYW5uLm9yZ10NCj4gU2VudDogVHVlc2RheSwgTWFyY2ggMDks
IDIwMTAgMzowMyBBTQ0KPiBUbzogV2lua2xlciwgVG9tYXMNCj4gQ2M6IGxpbnV4LWJsdWV0b290
aEB2Z2VyLmtlcm5lbC5vcmc7IENvaGVuLCBHdXk7IFJpbmRqdW5za3ksIFJvbjsgUGFza2FyLA0K
PiBHcmVnb3J5DQo+IFN1YmplY3Q6IFJlOiBbYmx1ZXRvb3RoLW5leHQgVjJdIEJsdWV0b290aDog
aGFuZGxlIGRldmljZSByZXNldCBldmVudA0KPiANCj4gSGkgVG9tYXMsDQo+IA0KPiA+IEEgQmx1
ZXRvb3RoIGRldmljZSBleHBlcmllbmNpbmcgaGFyZHdhcmUgZmFpbHVyZSBtYXkgaXNzdWUNCj4g
PiBhIEhBUkRXQVJFX0VSUk9SIGhjaSBldmVudC4gVGhlIHJlYWN0aW9uIHRvIHRoaXMgZXZlbnQg
aXMgZGV2aWNlDQo+ID4gcmVzZXQgZmxvdyBpbXBsZW1lbnRlZCBpbiBmb2xsb3dpbmcgc2VxdWVu
Y2UuDQo+ID4NCj4gPiAxLiBOb3RpZnk6IEhDSV9ERVZfRE9XTg0KPiA+IDIuIFJlaW5pdGlhbGl6
ZSBpbnRlcm5hbCBzdHJ1Y3R1cmVzLg0KPiA+IDMuIENhbGwgZHJpdmVyIGZsdXNoIGZ1bmN0aW9u
DQo+ID4gNC4gU2VuZCBIQ0kgcmVzZXQgcmVxdWVzdCB0byB0aGUgZGV2aWNlLg0KPiA+IDUuIFNl
bmQgSENJIGluaXQgc2VxdWVuY2UgcmVzZXQgdG8gdGhlIGRldmljZS4NCj4gPiA2LiBOb3RpZnkg
SENJX0RFVl9VUC4NCj4gDQo+IEkgcHJlZmVyIGlmIHdlIGNyZWF0ZSBhIGdlbmVyaWMgcGVyIGNv
bnRyb2xsZXIgd29ya3F1ZXVlIGZpcnN0IGJlZm9yZQ0KPiBoYXZpbmcgYSB3b3JrcXVldWUgZm9y
IGV2ZXJ5IHRhc2suIFNvbWV0aGluZyBzaW1pbGFyIHRvIHdoYXQgdGhlDQo+IG1hYzgwMjExIGxh
eWVyIG9mZmVycyByaWdodCBub3cuDQoNClRoYXQgd291bGQgYmUgZ29vZCBhcHByb2FjaCBidXQg
d2UgYXJlIHVzaW5nIGRlZmF1bHQga2VybmVsIHdvcmtxdWV1ZSBpbiB0aGlzIHNvbHV0aW9uIHNv
IHRoZXJlIGlzIG5vIHdvcmtxdWV1ZSBmb3IgZXZlcnkgdGFzay4gDQpJJ20gbm90IHN1cmUgaWYg
dGhpcyBlZmZvcnQgc2hvdWxkIGJsb2NrIHRoaXMgcGF0Y2guIA0KDQo+IEFsc28gaW4gYSBzZWNv
bmQgc3RlcCB3ZSBtaWdodCB3YW5uYSBtb3ZlIHRoZSBIQ0kgZXZlbnQgcHJvY2Vzc2luZw0KPiBj
b21wbGV0ZWx5IGludG8gYSB3b3JrcXVldWUuIElmIHdlIGdldCBubyBwZXJmb3JtYW5jZSBoaXQg
d2l0aCB0aGF0LA0KPiB0aGVuIHN5c2ZzIGhhbmRsaW5nIGFuZCBkZXZpY2UgcmVzZXQgYmVjb21l
cyBhIGxvdCBzaW1wbGVyIGFuZCBsZXNzDQo+IHByb25lIHRvIHJhY2UgY29uZGl0aW9ucyB3aXRo
IGRldmljZSByZW1vdmFsLg0KDQpMb29rcyBnb29kIHRvIG1lLiBTbyB3aGVuIHRoaXMgaXMgcmVh
ZHkgd2UgY2FuIG1vdmUgYWxzbyB0aGUgcmVzZXQgYWxzbyB0byBwZXIgY29udHJvbGxlciBxdWV1
ZS4NCg0KVGhhbmtzDQpUb21hcyANCg0KLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tCkludGVsIElzcmFlbCAoNzQpIExp
bWl0ZWQKClRoaXMgZS1tYWlsIGFuZCBhbnkgYXR0YWNobWVudHMgbWF5IGNvbnRhaW4gY29uZmlk
ZW50aWFsIG1hdGVyaWFsIGZvcgp0aGUgc29sZSB1c2Ugb2YgdGhlIGludGVuZGVkIHJlY2lwaWVu
dChzKS4gQW55IHJldmlldyBvciBkaXN0cmlidXRpb24KYnkgb3RoZXJzIGlzIHN0cmljdGx5IHBy
b2hpYml0ZWQuIElmIHlvdSBhcmUgbm90IHRoZSBpbnRlbmRlZApyZWNpcGllbnQsIHBsZWFzZSBj
b250YWN0IHRoZSBzZW5kZXIgYW5kIGRlbGV0ZSBhbGwgY29waWVzLgo=

RE: [bluetooth-next V2] Bluetooth: handle device reset event

[Marcel Holtmann]

Hi Tomas,

> > > A Bluetooth device experiencing hardware failure may issue
> > > a HARDWARE_ERROR hci event. The reaction to this event is device
> > > reset flow implemented in following sequence.
> > >
> > > 1. Notify: HCI_DEV_DOWN
> > > 2. Reinitialize internal structures.
> > > 3. Call driver flush function
> > > 4. Send HCI reset request to the device.
> > > 5. Send HCI init sequence reset to the device.
> > > 6. Notify HCI_DEV_UP.
> > 
> > I prefer if we create a generic per controller workqueue first before
> > having a workqueue for every task. Something similar to what the
> > mac80211 layer offers right now.
> 
> That would be good approach but we are using default kernel workqueue in this solution so there is no workqueue for every task. 
> I'm not sure if this effort should block this patch. 

I have an initial patch that I have to dig out. It is actually not that
complicated. And I would prefer to get that one first before we apply
this patch.

The reason why I really wanna solve this is because of potential race
conditions between the workqueue and a device removal. I have a bad
feeling that even with the current sysfs workqueue we have a race
condition hiding somewhere. So if going forward we have to shift more
and more code into workqueues we need a single point where this can be
fixed.

> > Also in a second step we might wanna move the HCI event processing
> > completely into a workqueue. If we get no performance hit with that,
> > then sysfs handling and device reset becomes a lot simpler and less
> > prone to race conditions with device removal.
> 
> Looks good to me. So when this is ready we can move also the reset also to per controller queue.

I haven't done any performance testing with this approach. So I hope we
are not shooting ourselves in the foot with it.

Regards

Marcel

RE: [bluetooth-next V2] Bluetooth: handle device reset event

[Winkler, Tomas]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogTWFyY2VsIEhvbHRtYW5u
IFttYWlsdG86bWFyY2VsQGhvbHRtYW5uLm9yZ10NCj4gU2VudDogV2VkbmVzZGF5LCBNYXJjaCAx
MCwgMjAxMCA2OjQ5IFBNDQo+IFRvOiBXaW5rbGVyLCBUb21hcw0KPiBDYzogbGludXgtYmx1ZXRv
b3RoQHZnZXIua2VybmVsLm9yZzsgQ29oZW4sIEd1eTsgUmluZGp1bnNreSwgUm9uOyBQYXNrYXIs
DQo+IEdyZWdvcnkNCj4gU3ViamVjdDogUkU6IFtibHVldG9vdGgtbmV4dCBWMl0gQmx1ZXRvb3Ro
OiBoYW5kbGUgZGV2aWNlIHJlc2V0IGV2ZW50DQo+IA0KPiBIaSBUb21hcywNCj4gDQo+ID4gPiA+
IEEgQmx1ZXRvb3RoIGRldmljZSBleHBlcmllbmNpbmcgaGFyZHdhcmUgZmFpbHVyZSBtYXkgaXNz
dWUNCj4gPiA+ID4gYSBIQVJEV0FSRV9FUlJPUiBoY2kgZXZlbnQuIFRoZSByZWFjdGlvbiB0byB0
aGlzIGV2ZW50IGlzIGRldmljZQ0KPiA+ID4gPiByZXNldCBmbG93IGltcGxlbWVudGVkIGluIGZv
bGxvd2luZyBzZXF1ZW5jZS4NCj4gPiA+ID4NCj4gPiA+ID4gMS4gTm90aWZ5OiBIQ0lfREVWX0RP
V04NCj4gPiA+ID4gMi4gUmVpbml0aWFsaXplIGludGVybmFsIHN0cnVjdHVyZXMuDQo+ID4gPiA+
IDMuIENhbGwgZHJpdmVyIGZsdXNoIGZ1bmN0aW9uDQo+ID4gPiA+IDQuIFNlbmQgSENJIHJlc2V0
IHJlcXVlc3QgdG8gdGhlIGRldmljZS4NCj4gPiA+ID4gNS4gU2VuZCBIQ0kgaW5pdCBzZXF1ZW5j
ZSByZXNldCB0byB0aGUgZGV2aWNlLg0KPiA+ID4gPiA2LiBOb3RpZnkgSENJX0RFVl9VUC4NCj4g
PiA+DQo+ID4gPiBJIHByZWZlciBpZiB3ZSBjcmVhdGUgYSBnZW5lcmljIHBlciBjb250cm9sbGVy
IHdvcmtxdWV1ZSBmaXJzdCBiZWZvcmUNCj4gPiA+IGhhdmluZyBhIHdvcmtxdWV1ZSBmb3IgZXZl
cnkgdGFzay4gU29tZXRoaW5nIHNpbWlsYXIgdG8gd2hhdCB0aGUNCj4gPiA+IG1hYzgwMjExIGxh
eWVyIG9mZmVycyByaWdodCBub3cuDQo+ID4NCj4gPiBUaGF0IHdvdWxkIGJlIGdvb2QgYXBwcm9h
Y2ggYnV0IHdlIGFyZSB1c2luZyBkZWZhdWx0IGtlcm5lbCB3b3JrcXVldWUgaW4NCj4gdGhpcyBz
b2x1dGlvbiBzbyB0aGVyZSBpcyBubyB3b3JrcXVldWUgZm9yIGV2ZXJ5IHRhc2suDQo+ID4gSSdt
IG5vdCBzdXJlIGlmIHRoaXMgZWZmb3J0IHNob3VsZCBibG9jayB0aGlzIHBhdGNoLg0KPiANCj4g
SSBoYXZlIGFuIGluaXRpYWwgcGF0Y2ggdGhhdCBJIGhhdmUgdG8gZGlnIG91dC4gSXQgaXMgYWN0
dWFsbHkgbm90IHRoYXQNCj4gY29tcGxpY2F0ZWQuIEFuZCBJIHdvdWxkIHByZWZlciB0byBnZXQg
dGhhdCBvbmUgZmlyc3QgYmVmb3JlIHdlIGFwcGx5DQo+IHRoaXMgcGF0Y2guDQoNCk9rYXksIEkg
Y2FuIHRlc3QgdGhpcyBwYXJ0aWN1bGFyIGZsb3cgd2l0aCB5b3VyIHBhdGNoIGlmIHlvdSBzZW5k
IGl0IHRvIG1lLg0KDQo+IFRoZSByZWFzb24gd2h5IEkgcmVhbGx5IHdhbm5hIHNvbHZlIHRoaXMg
aXMgYmVjYXVzZSBvZiBwb3RlbnRpYWwgcmFjZQ0KPiBjb25kaXRpb25zIGJldHdlZW4gdGhlIHdv
cmtxdWV1ZSBhbmQgYSBkZXZpY2UgcmVtb3ZhbC4gSSBoYXZlIGEgYmFkDQo+IGZlZWxpbmcgdGhh
dCBldmVuIHdpdGggdGhlIGN1cnJlbnQgc3lzZnMgd29ya3F1ZXVlIHdlIGhhdmUgYSByYWNlDQo+
IGNvbmRpdGlvbiBoaWRpbmcgc29tZXdoZXJlLiBTbyBpZiBnb2luZyBmb3J3YXJkIHdlIGhhdmUg
dG8gc2hpZnQgbW9yZQ0KPiBhbmQgbW9yZSBjb2RlIGludG8gd29ya3F1ZXVlcyB3ZSBuZWVkIGEg
c2luZ2xlIHBvaW50IHdoZXJlIHRoaXMgY2FuIGJlDQo+IGZpeGVkLg0KDQpVbmRlcnN0b29kLg0K
DQpJbiB0aGF0IGNvbnRleHQgSSB3b3VsZCBuZWVkIHRvIGRvIHNvbWUgbW9yZSBpbnN0cnVtZW50
YXRpb24gdG8gdGVzdCB0aGlzIGtpbmQgb2YgcmFjZSBmb3IgdGhpcyBmZWF0dXJlLiANCg0KPiA+
ID4gQWxzbyBpbiBhIHNlY29uZCBzdGVwIHdlIG1pZ2h0IHdhbm5hIG1vdmUgdGhlIEhDSSBldmVu
dCBwcm9jZXNzaW5nDQo+ID4gPiBjb21wbGV0ZWx5IGludG8gYSB3b3JrcXVldWUuIElmIHdlIGdl
dCBubyBwZXJmb3JtYW5jZSBoaXQgd2l0aCB0aGF0LA0KPiA+ID4gdGhlbiBzeXNmcyBoYW5kbGlu
ZyBhbmQgZGV2aWNlIHJlc2V0IGJlY29tZXMgYSBsb3Qgc2ltcGxlciBhbmQgbGVzcw0KPiA+ID4g
cHJvbmUgdG8gcmFjZSBjb25kaXRpb25zIHdpdGggZGV2aWNlIHJlbW92YWwuDQo+ID4NCj4gPiBM
b29rcyBnb29kIHRvIG1lLiBTbyB3aGVuIHRoaXMgaXMgcmVhZHkgd2UgY2FuIG1vdmUgYWxzbyB0
aGUgcmVzZXQgYWxzbyB0bw0KPiBwZXIgY29udHJvbGxlciBxdWV1ZS4NCj4gDQo+IEkgaGF2ZW4n
dCBkb25lIGFueSBwZXJmb3JtYW5jZSB0ZXN0aW5nIHdpdGggdGhpcyBhcHByb2FjaC4gU28gSSBo
b3BlIHdlDQo+IGFyZSBub3Qgc2hvb3Rpbmcgb3Vyc2VsdmVzIGluIHRoZSBmb290IHdpdGggaXQu
DQoNClRoYW5rcw0KVG9tYXMNCi0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQpJbnRlbCBJc3JhZWwgKDc0KSBMaW1pdGVk
CgpUaGlzIGUtbWFpbCBhbmQgYW55IGF0dGFjaG1lbnRzIG1heSBjb250YWluIGNvbmZpZGVudGlh
bCBtYXRlcmlhbCBmb3IKdGhlIHNvbGUgdXNlIG9mIHRoZSBpbnRlbmRlZCByZWNpcGllbnQocyku
IEFueSByZXZpZXcgb3IgZGlzdHJpYnV0aW9uCmJ5IG90aGVycyBpcyBzdHJpY3RseSBwcm9oaWJp
dGVkLiBJZiB5b3UgYXJlIG5vdCB0aGUgaW50ZW5kZWQKcmVjaXBpZW50LCBwbGVhc2UgY29udGFj
dCB0aGUgc2VuZGVyIGFuZCBkZWxldGUgYWxsIGNvcGllcy4K

RE: [bluetooth-next V2] Bluetooth: handle device reset event

[Marcel Holtmann]

Hi Tomas,

> > > > > A Bluetooth device experiencing hardware failure may issue
> > > > > a HARDWARE_ERROR hci event. The reaction to this event is device
> > > > > reset flow implemented in following sequence.
> > > > >
> > > > > 1. Notify: HCI_DEV_DOWN
> > > > > 2. Reinitialize internal structures.
> > > > > 3. Call driver flush function
> > > > > 4. Send HCI reset request to the device.
> > > > > 5. Send HCI init sequence reset to the device.
> > > > > 6. Notify HCI_DEV_UP.
> > > >
> > > > I prefer if we create a generic per controller workqueue first before
> > > > having a workqueue for every task. Something similar to what the
> > > > mac80211 layer offers right now.
> > >
> > > That would be good approach but we are using default kernel workqueue in
> > this solution so there is no workqueue for every task.
> > > I'm not sure if this effort should block this patch.
> > 
> > I have an initial patch that I have to dig out. It is actually not that
> > complicated. And I would prefer to get that one first before we apply
> > this patch.
> 
> Okay, I can test this particular flow with your patch if you send it to me.

I pushed the per controller workqueue patch into bluetooth-testing.git
tree. Please adapt your patch to that and test it.

> > The reason why I really wanna solve this is because of potential race
> > conditions between the workqueue and a device removal. I have a bad
> > feeling that even with the current sysfs workqueue we have a race
> > condition hiding somewhere. So if going forward we have to shift more
> > and more code into workqueues we need a single point where this can be
> > fixed.
> 
> Understood.
> 
> In that context I would need to do some more instrumentation to test this kind of race for this feature. 

I could be very well that we are protected against such a race. However
with moving from tasklet into workqueue I am always cautious. And I
wanna make sure that we keep the complexity simple. I want one place to
fix if it breaks and not twenty.

Regards

Marcel