Resend RFC [PATCH] hci: Fix race between hidp_session and hci code
Subsystems (2)
From: Karl Relton <[email protected]>
Fix race between hidp_session and hci code that can lead to
hci device being removed from sysfs before its children devices. The removal is now delayed until the last reference to it in the hci code is removed.
--- include/net/bluetooth/hci_core.h | 10 +++------- net/bluetooth/hci_core.c | 15 +++++++++++++-- 2 files changed, 16 insertions(+), 9 deletions(-)
include/net/bluetooth/hci_core.h +3 -7
@@ -198,6 +198,7 @@ struct hci_dev {unsigned long quirks; +atomic_t ref_cnt;atomic_t cmd_cnt;unsigned int acl_cnt;unsigned int sco_cnt;
@@ -646,19 +647,14 @@ static inline void hci_conn_put(struct hci_conn *conn) }/* ----- HCI Devices ----- */ -static inline void hci_dev_put (struct hci_dev *d) -{ -BT_DBG ("%s orig refcnt %d" , d->name, -atomic_read (&d->dev.kobj.kref.refcount)); - -put_device (&d->dev); -} +void hci_dev_put (struct hci_dev *d);static inline struct hci_dev *hci_dev_hold (struct hci_dev *d) {BT_DBG ("%s orig refcnt %d" , d->name,atomic_read (&d->dev.kobj.kref.refcount)); +atomic_inc (&d->ref_cnt);get_device (&d->dev);return d; }
net/bluetooth/hci_core.c +13 -2
@@ -891,6 +891,19 @@ int hci_dev_close(__u16 dev)return err; } +void hci_dev_put (struct hci_dev *d) +{ +BT_DBG ("%s orig refcnt %d" , d->name, +atomic_read (&d->dev.kobj.kref.refcount)); + +if (atomic_dec_and_test (&d->ref_cnt)) { +hci_del_sysfs (d); + } +put_device (&d->dev); +} +EXPORT_SYMBOL (hci_dev_put); + +int hci_dev_reset (__u16 dev) {struct hci_dev *hdev;
@@ -1884,8 +1897,6 @@ void hci_unregister_dev(struct hci_dev *hdev)rfkill_destroy (hdev->rfkill); } -hci_del_sysfs (hdev); -destroy_workqueue (hdev->workqueue);destroy_workqueue (hdev->req_workqueue);
Signature · 1 line
1.7.9.5
Hi Karl
Quoted 18 lines · Karl Relton
From: Karl Relton <[email protected]>
Fix race between
hidp_sessionand hci code that can lead to
hci device being removed from sysfs before its children devices. The removal is now delayed until the last reference to it in the hci code is removed.Signed-off-by Karl Relton <[email protected]>--- include/net/bluetooth/hci_core.h | 10 +++------- net/bluetooth/hci_core.c | 15 +++++++++++++-- 2 files changed, 16 insertions(+), 9 deletions(-)include/net/bluetooth/hci_core.h +0
-0
include/net/bluetooth/hci_core.h struct hci_dev { · line 201
Quoted 4 lines
include/net/bluetooth/hci_core.h +1
-0@@ -198,6 +198,7 @@ struct hci_dev {unsigned long quirks; +atomic_t ref_cnt;
We used to have this field here in the past and I worked hard to remove it. Please don't reintroduce it.
The thing is, if you start adding a comment what this field represents, you would end up with:
/* reference count for hci_dev object */
However, how do you describe the refcnt inside of "hci_dev.dev"? Exactly! It does the same. It's the refcnt for an hci_dev object. Hence, please introduce this field only if you can add a proper comment.
Apart from that, see below:
Quoted 31 lines
include/net/bluetooth/hci_core.h +2
-7atomic_t cmd_cnt;unsigned int acl_cnt;unsigned int sco_cnt;@@ -646,19 +647,14 @@ static inline void hci_conn_put(struct hci_conn *conn) }/* ----- HCI Devices ----- */ -static inline void hci_dev_put (struct hci_dev *d) -{ -BT_DBG ("%s orig refcnt %d" , d->name, -atomic_read (&d->dev.kobj.kref.refcount)); - -put_device (&d->dev); -} +void hci_dev_put (struct hci_dev *d);static inline struct hci_dev *hci_dev_hold (struct hci_dev *d) {BT_DBG ("%s orig refcnt %d" , d->name,atomic_read (&d->dev.kobj.kref.refcount)); +atomic_inc (&d->ref_cnt);get_device (&d->dev);return d; }net/bluetooth/hci_core.c +0
-0@@ -891,6 +891,19 @@ int hci_dev_close(__u16 dev)return err;
net/bluetooth/hci_core.c int hci_dev_close(__u16 dev) · line 901
Quoted 10 lines
net/bluetooth/hci_core.c +8
-0} +void hci_dev_put (struct hci_dev *d) +{ +BT_DBG ("%s orig refcnt %d" , d->name, +atomic_read (&d->dev.kobj.kref.refcount)); + +if (atomic_dec_and_test (&d->ref_cnt)) { +hci_del_sysfs (d); + }
If you want to move hci_del_sysfs() to the place where the refcnt drops to zero, then move it to bt_host_release() in hci_sysfs.c. This is the place where hci_dev is destroyed.
However, apart from this being in the wrong place, the patch itself is wrong. We remove devices from sysfs exactly when the physical device disappears! This is just how we do it, and there are many reasons to do it this way.
So if stuff breaks because we remove the device from sysfs, then fix the stuff that breaks! In this case this means removing all sub-protocols from sysfs exactly at the same point when we remove the hci-dev from sysfs!
Besides, what do you think happens when hci_add_sysfs() itself takes a reference and drops it in hci_del_sysfs() (which is totally legal)? It would cause the device to never get deleted.
So please describe what exactly breaks so we can fix it properly.
Thanks
David
Quoted 26 lines
net/bluetooth/hci_core.c +5
-3+put_device (&d->dev); +} +EXPORT_SYMBOL (hci_dev_put); + +int hci_dev_reset (__u16 dev) {struct hci_dev *hdev;@@ -1884,8 +1897,6 @@ void hci_unregister_dev(struct hci_dev *hdev)rfkill_destroy (hdev->rfkill); } -hci_del_sysfs (hdev); -destroy_workqueue (hdev->workqueue);destroy_workqueue (hdev->req_workqueue); --1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html