Hi
This is the same as my previous patchset but split into sequential patches. The
first patch makes the destruct-cb optional so the patches 2-7 can remove the
empty callbacks.
Patches 8-14 make the remaining drivers drop the destruct-callback and directly
free the driver data.
Patches 15-18 then remove the (now unused!) destruct callback and then properly
fixes the reference counts (same patches as already posted).
I hope the first 14 patches make clear that half of our drivers already work
without the destruct callback and the other half can be safely converted into
not using it. In fact, the drivers that use it introduce some memleaks which are
fixed by simply dropping the callback and freeing everything right away.
I've added detailed commit messages to each driver. If a single step is unclear,
please point me to it.
By the way, this fixes several bugs we currently have, including some mem-leaks
as described in the commits and the fact that hci_free_dev() currently
*directly* destroys the hci object regardless of any previous calls to
hci_hold_dev/hci_put_dev(). This is because the hci->refcnt counter is in no way
linked to the lifetime of the hci object. See hci_sysfs the release_host
callback which actually frees the hci object. It is in no way triggered by
hci->refcnt.
This patchset fixes all this and introduces proper reference counting.
Cheers
David
David Herrmann (18):
Bluetooth: Make hci-destruct callback optional
Bluetooth: bluecard-cs: Remove empty destruct cb
Bluetooth: bt3c-cs: Remove empty destruct cb
Bluetooth: btmrvl: Remove empty destruct cb
Bluetooth: btuart-cs: Remove empty destruct cb
Bluetooth: btwilink: Remove empty destruct cb
Bluetooth: dtl1-cs: Remove empty destruct cb
Bluetooth: vhci: Free driver_data on file release
Bluetooth: bfusb: Free driver_data on USB shutdown
Bluetooth: btusb: Free driver data on USB shutdown
Bluetooth: bpa10x: Free private driver data on usb shutdown
Bluetooth: btsdio: Free driver data on SDIO shutdown
Bluetooth: uart-ldisc: Fix memory leak and remove destruct cb
Bluetooth: Remove unused hci-destruct cb
Bluetooth: Correctly acquire module ref
Bluetooth: Remove HCI-owner field
Bluetooth: Correctly take hci_dev->dev refcount
Bluetooth: Remove __hci_dev_put/hold
drivers/bluetooth/bfusb.c | 13 +------------
drivers/bluetooth/bluecard_cs.c | 8 --------
drivers/bluetooth/bpa10x.c | 17 +++--------------
drivers/bluetooth/bt3c_cs.c | 8 --------
drivers/bluetooth/btmrvl_main.c | 6 ------
drivers/bluetooth/btsdio.c | 13 +------------
drivers/bluetooth/btuart_cs.c | 8 --------
drivers/bluetooth/btusb.c | 17 +++--------------
drivers/bluetooth/btwilink.c | 10 ----------
drivers/bluetooth/dtl1_cs.c | 8 --------
drivers/bluetooth/hci_ldisc.c | 14 ++------------
drivers/bluetooth/hci_vhci.c | 9 +--------
include/net/bluetooth/hci_core.h | 28 ++++------------------------
net/bluetooth/hci_core.c | 9 ++++-----
net/bluetooth/hci_sysfs.c | 2 ++
15 files changed, 21 insertions(+), 149 deletions(-)
--
1.7.8.1
Hi David,
On Sat, Jan 07, 2012, David Herrmann wrote:
> David Herrmann (18):
> Bluetooth: Make hci-destruct callback optional
> Bluetooth: bluecard-cs: Remove empty destruct cb
> Bluetooth: bt3c-cs: Remove empty destruct cb
> Bluetooth: btmrvl: Remove empty destruct cb
> Bluetooth: btuart-cs: Remove empty destruct cb
> Bluetooth: btwilink: Remove empty destruct cb
> Bluetooth: dtl1-cs: Remove empty destruct cb
> Bluetooth: vhci: Free driver_data on file release
> Bluetooth: bfusb: Free driver_data on USB shutdown
> Bluetooth: btusb: Free driver data on USB shutdown
> Bluetooth: bpa10x: Free private driver data on usb shutdown
> Bluetooth: btsdio: Free driver data on SDIO shutdown
> Bluetooth: uart-ldisc: Fix memory leak and remove destruct cb
> Bluetooth: Remove unused hci-destruct cb
> Bluetooth: Correctly acquire module ref
> Bluetooth: Remove HCI-owner field
> Bluetooth: Correctly take hci_dev->dev refcount
> Bluetooth: Remove __hci_dev_put/hold
>
> drivers/bluetooth/bfusb.c | 13 +------------
> drivers/bluetooth/bluecard_cs.c | 8 --------
> drivers/bluetooth/bpa10x.c | 17 +++--------------
> drivers/bluetooth/bt3c_cs.c | 8 --------
> drivers/bluetooth/btmrvl_main.c | 6 ------
> drivers/bluetooth/btsdio.c | 13 +------------
> drivers/bluetooth/btuart_cs.c | 8 --------
> drivers/bluetooth/btusb.c | 17 +++--------------
> drivers/bluetooth/btwilink.c | 10 ----------
> drivers/bluetooth/dtl1_cs.c | 8 --------
> drivers/bluetooth/hci_ldisc.c | 14 ++------------
> drivers/bluetooth/hci_vhci.c | 9 +--------
> include/net/bluetooth/hci_core.h | 28 ++++------------------------
> net/bluetooth/hci_core.c | 9 ++++-----
> net/bluetooth/hci_sysfs.c | 2 ++
> 15 files changed, 21 insertions(+), 149 deletions(-)
All of these patches have been applied to my bluetooth-next tree. In the
future please adjust your editor settings so that your commit message
lines aren't longer than 72 characters. The message should be fully
visible with git log (which indents the messages) on a 80-wide terminal
I ended up fixing the messages manually this time.
Johan
Hi David,
> This is the same as my previous patchset but split into sequential patches. The
> first patch makes the destruct-cb optional so the patches 2-7 can remove the
> empty callbacks.
> Patches 8-14 make the remaining drivers drop the destruct-callback and directly
> free the driver data.
> Patches 15-18 then remove the (now unused!) destruct callback and then properly
> fixes the reference counts (same patches as already posted).
>
> I hope the first 14 patches make clear that half of our drivers already work
> without the destruct callback and the other half can be safely converted into
> not using it. In fact, the drivers that use it introduce some memleaks which are
> fixed by simply dropping the callback and freeing everything right away.
>
> I've added detailed commit messages to each driver. If a single step is unclear,
> please point me to it.
>
>
> By the way, this fixes several bugs we currently have, including some mem-leaks
> as described in the commits and the fact that hci_free_dev() currently
> *directly* destroys the hci object regardless of any previous calls to
> hci_hold_dev/hci_put_dev(). This is because the hci->refcnt counter is in no way
> linked to the lifetime of the hci object. See hci_sysfs the release_host
> callback which actually frees the hci object. It is in no way triggered by
> hci->refcnt.
> This patchset fixes all this and introduces proper reference counting.
I went through the patches again and after thinking about it for a bit,
this all looks good.
Acked-by: Marcel Holtmann <[email protected]>
Johan, feel free to include this patchset into your tree.
Regards
Marcel
Since we remove the owner field of hci_dev hci_dev_put and __hci_dev_put do the
same so we can merge them into one function.
Same for hci_dev_hold and __hci_dev_hold.
Signed-off-by: David Herrmann <[email protected]>
---
drivers/bluetooth/btusb.c | 4 ++--
include/net/bluetooth/hci_core.h | 12 ++----------
net/bluetooth/hci_core.c | 4 ++--
3 files changed, 6 insertions(+), 14 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 246944e..2dbecfa 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1082,7 +1082,7 @@ static void btusb_disconnect(struct usb_interface *intf)
hdev = data->hdev;
- __hci_dev_hold(hdev);
+ hci_dev_hold(hdev);
usb_set_intfdata(data->intf, NULL);
@@ -1096,7 +1096,7 @@ static void btusb_disconnect(struct usb_interface *intf)
else if (data->isoc)
usb_driver_release_interface(&btusb_driver, data->isoc);
- __hci_dev_put(hdev);
+ hci_dev_put(hdev);
hci_free_dev(hdev);
kfree(data);
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 00da2e1..d0ed6ee 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -562,25 +562,17 @@ static inline void hci_conn_put(struct hci_conn *conn)
}
/* ----- HCI Devices ----- */
-static inline void __hci_dev_put(struct hci_dev *d)
+static inline void hci_dev_put(struct hci_dev *d)
{
put_device(&d->dev);
}
-/*
- * hci_dev_put and hci_dev_hold are macros to avoid dragging all the
- * overhead of all the modular infrastructure into this header.
- */
-#define hci_dev_put(d) __hci_dev_put(d)
-
-static inline struct hci_dev *__hci_dev_hold(struct hci_dev *d)
+static inline struct hci_dev *hci_dev_hold(struct hci_dev *d)
{
get_device(&d->dev);
return d;
}
-#define hci_dev_hold(d) __hci_dev_hold(d)
-
#define hci_dev_lock(d) mutex_lock(&d->lock)
#define hci_dev_unlock(d) mutex_unlock(&d->lock)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 3ee109f..80175ab 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1553,7 +1553,7 @@ int hci_register_dev(struct hci_dev *hdev)
schedule_work(&hdev->power_on);
hci_notify(hdev, HCI_DEV_REG);
- __hci_dev_hold(hdev);
+ hci_dev_hold(hdev);
return id;
@@ -1616,7 +1616,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
hci_adv_entries_clear(hdev);
hci_dev_unlock(hdev);
- __hci_dev_put(hdev);
+ hci_dev_put(hdev);
}
EXPORT_SYMBOL(hci_unregister_dev);
--
1.7.8.1
The hci_dev->dev device structure has an internal refcount. This refcount is
used to protect the whole hci_dev object. However, we currently do not use it.
Therefore, if someone calls hci_free_dev() we currently immediately destroy the
hci_dev object because we never took the device refcount.
This even happens if the hci_dev->refcnt is not 0. In fact, the hci_dev->refcnt
is totally useless in its current state. Therefore, we simply remove
hci_dev->refcnt and instead use hci_dev->dev refcnt.
This fixes all the symptoms and also correctly integrates the device structure
into our bluetooth bus system.
Signed-off-by: David Herrmann <[email protected]>
---
include/net/bluetooth/hci_core.h | 5 ++---
net/bluetooth/hci_core.c | 2 +-
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 3d41b2e..00da2e1 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -113,7 +113,6 @@ struct adv_entry {
struct hci_dev {
struct list_head list;
struct mutex lock;
- atomic_t refcnt;
char name[8];
unsigned long flags;
@@ -565,7 +564,7 @@ static inline void hci_conn_put(struct hci_conn *conn)
/* ----- HCI Devices ----- */
static inline void __hci_dev_put(struct hci_dev *d)
{
- atomic_dec(&d->refcnt);
+ put_device(&d->dev);
}
/*
@@ -576,7 +575,7 @@ static inline void __hci_dev_put(struct hci_dev *d)
static inline struct hci_dev *__hci_dev_hold(struct hci_dev *d)
{
- atomic_inc(&d->refcnt);
+ get_device(&d->dev);
return d;
}
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index b37db46..3ee109f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1470,7 +1470,6 @@ int hci_register_dev(struct hci_dev *hdev)
hdev->id = id;
list_add_tail(&hdev->list, head);
- atomic_set(&hdev->refcnt, 1);
mutex_init(&hdev->lock);
hdev->flags = 0;
@@ -1554,6 +1553,7 @@ int hci_register_dev(struct hci_dev *hdev)
schedule_work(&hdev->power_on);
hci_notify(hdev, HCI_DEV_REG);
+ __hci_dev_hold(hdev);
return id;
--
1.7.8.1
After unregistering an hci_dev object a bluetooth driver does not have any
callbacks in the hci_dev structure left over. Therefore, there is no need to
keep a reference to the module.
Previously, we needed this to protect the hci-destruct callback. However, this
callback is no longer available so we do not need this owner field, anymore.
Drivers now call hci_unregister_dev() and they are done with the object.
Signed-off-by: David Herrmann <[email protected]>
---
drivers/bluetooth/bfusb.c | 2 --
drivers/bluetooth/bluecard_cs.c | 2 --
drivers/bluetooth/bpa10x.c | 2 --
drivers/bluetooth/bt3c_cs.c | 2 --
drivers/bluetooth/btmrvl_main.c | 1 -
drivers/bluetooth/btsdio.c | 2 --
drivers/bluetooth/btuart_cs.c | 2 --
drivers/bluetooth/btusb.c | 2 --
drivers/bluetooth/btwilink.c | 1 -
drivers/bluetooth/dtl1_cs.c | 2 --
drivers/bluetooth/hci_ldisc.c | 2 --
drivers/bluetooth/hci_vhci.c | 2 --
include/net/bluetooth/hci_core.h | 13 ++-----------
net/bluetooth/hci_core.c | 3 +--
14 files changed, 3 insertions(+), 35 deletions(-)
diff --git a/drivers/bluetooth/bfusb.c b/drivers/bluetooth/bfusb.c
index 857a951..e97f42a 100644
--- a/drivers/bluetooth/bfusb.c
+++ b/drivers/bluetooth/bfusb.c
@@ -705,8 +705,6 @@ static int bfusb_probe(struct usb_interface *intf, const struct usb_device_id *i
hdev->send = bfusb_send_frame;
hdev->ioctl = bfusb_ioctl;
- hdev->owner = THIS_MODULE;
-
if (hci_register_dev(hdev) < 0) {
BT_ERR("Can't register HCI device");
hci_free_dev(hdev);
diff --git a/drivers/bluetooth/bluecard_cs.c b/drivers/bluetooth/bluecard_cs.c
index 5cb325a..6b1261f 100644
--- a/drivers/bluetooth/bluecard_cs.c
+++ b/drivers/bluetooth/bluecard_cs.c
@@ -738,8 +738,6 @@ static int bluecard_open(bluecard_info_t *info)
hdev->send = bluecard_hci_send_frame;
hdev->ioctl = bluecard_hci_ioctl;
- hdev->owner = THIS_MODULE;
-
id = inb(iobase + 0x30);
if ((id & 0x0f) == 0x02)
diff --git a/drivers/bluetooth/bpa10x.c b/drivers/bluetooth/bpa10x.c
index 92c2424..229bdc9 100644
--- a/drivers/bluetooth/bpa10x.c
+++ b/drivers/bluetooth/bpa10x.c
@@ -470,8 +470,6 @@ static int bpa10x_probe(struct usb_interface *intf, const struct usb_device_id *
hdev->flush = bpa10x_flush;
hdev->send = bpa10x_send_frame;
- hdev->owner = THIS_MODULE;
-
set_bit(HCI_QUIRK_NO_RESET, &hdev->quirks);
err = hci_register_dev(hdev);
diff --git a/drivers/bluetooth/bt3c_cs.c b/drivers/bluetooth/bt3c_cs.c
index e74334d..0e304cb 100644
--- a/drivers/bluetooth/bt3c_cs.c
+++ b/drivers/bluetooth/bt3c_cs.c
@@ -584,8 +584,6 @@ static int bt3c_open(bt3c_info_t *info)
hdev->send = bt3c_hci_send_frame;
hdev->ioctl = bt3c_hci_ioctl;
- hdev->owner = THIS_MODULE;
-
/* Load firmware */
err = request_firmware(&firmware, "BT3CPCC.bin", &info->p_dev->dev);
if (err < 0) {
diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index d62fc0d..d69c095 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -552,7 +552,6 @@ int btmrvl_register_hdev(struct btmrvl_private *priv)
hdev->flush = btmrvl_flush;
hdev->send = btmrvl_send_frame;
hdev->ioctl = btmrvl_ioctl;
- hdev->owner = THIS_MODULE;
btmrvl_send_module_cfg_cmd(priv, MODULE_BRINGUP_REQ);
diff --git a/drivers/bluetooth/btsdio.c b/drivers/bluetooth/btsdio.c
index d38945c..2d6e4ed 100644
--- a/drivers/bluetooth/btsdio.c
+++ b/drivers/bluetooth/btsdio.c
@@ -337,8 +337,6 @@ static int btsdio_probe(struct sdio_func *func,
hdev->flush = btsdio_flush;
hdev->send = btsdio_send_frame;
- hdev->owner = THIS_MODULE;
-
err = hci_register_dev(hdev);
if (err < 0) {
hci_free_dev(hdev);
diff --git a/drivers/bluetooth/btuart_cs.c b/drivers/bluetooth/btuart_cs.c
index 84e02f1..80ad2b9 100644
--- a/drivers/bluetooth/btuart_cs.c
+++ b/drivers/bluetooth/btuart_cs.c
@@ -502,8 +502,6 @@ static int btuart_open(btuart_info_t *info)
hdev->send = btuart_hci_send_frame;
hdev->ioctl = btuart_hci_ioctl;
- hdev->owner = THIS_MODULE;
-
spin_lock_irqsave(&(info->lock), flags);
/* Reset UART */
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 85bb17d..246944e 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1000,8 +1000,6 @@ static int btusb_probe(struct usb_interface *intf,
hdev->send = btusb_send_frame;
hdev->notify = btusb_notify;
- hdev->owner = THIS_MODULE;
-
/* Interface numbers are hardcoded in the specification */
data->isoc = usb_ifnum_to_if(data->udev, 1);
diff --git a/drivers/bluetooth/btwilink.c b/drivers/bluetooth/btwilink.c
index da9cf6a..b81b32e 100644
--- a/drivers/bluetooth/btwilink.c
+++ b/drivers/bluetooth/btwilink.c
@@ -317,7 +317,6 @@ static int bt_ti_probe(struct platform_device *pdev)
hdev->close = ti_st_close;
hdev->flush = NULL;
hdev->send = ti_st_send_frame;
- hdev->owner = THIS_MODULE;
err = hci_register_dev(hdev);
if (err < 0) {
diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
index aae40ca..295cf1b 100644
--- a/drivers/bluetooth/dtl1_cs.c
+++ b/drivers/bluetooth/dtl1_cs.c
@@ -484,8 +484,6 @@ static int dtl1_open(dtl1_info_t *info)
hdev->send = dtl1_hci_send_frame;
hdev->ioctl = dtl1_hci_ioctl;
- hdev->owner = THIS_MODULE;
-
spin_lock_irqsave(&(info->lock), flags);
/* Reset UART */
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 5ea49df..459ff0b 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -392,8 +392,6 @@ static int hci_uart_register_dev(struct hci_uart *hu)
hdev->send = hci_uart_send_frame;
hdev->parent = hu->tty->dev;
- hdev->owner = THIS_MODULE;
-
if (!reset)
set_bit(HCI_QUIRK_NO_RESET, &hdev->quirks);
diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
index 44a8012..5f305c1 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -244,8 +244,6 @@ static int vhci_open(struct inode *inode, struct file *file)
hdev->flush = vhci_flush;
hdev->send = vhci_send_frame;
- hdev->owner = THIS_MODULE;
-
if (hci_register_dev(hdev) < 0) {
BT_ERR("Can't register HCI device");
kfree(data);
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index ab97ad3..3d41b2e 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -246,8 +246,6 @@ struct hci_dev {
struct rfkill *rfkill;
- struct module *owner;
-
unsigned long dev_flags;
int (*open)(struct hci_dev *hdev);
@@ -574,11 +572,7 @@ static inline void __hci_dev_put(struct hci_dev *d)
* hci_dev_put and hci_dev_hold are macros to avoid dragging all the
* overhead of all the modular infrastructure into this header.
*/
-#define hci_dev_put(d) \
-do { \
- __hci_dev_put(d); \
- module_put(d->owner); \
-} while (0)
+#define hci_dev_put(d) __hci_dev_put(d)
static inline struct hci_dev *__hci_dev_hold(struct hci_dev *d)
{
@@ -586,10 +580,7 @@ static inline struct hci_dev *__hci_dev_hold(struct hci_dev *d)
return d;
}
-#define hci_dev_hold(d) \
-({ \
- try_module_get(d->owner) ? __hci_dev_hold(d) : NULL; \
-})
+#define hci_dev_hold(d) __hci_dev_hold(d)
#define hci_dev_lock(d) mutex_lock(&d->lock)
#define hci_dev_unlock(d) mutex_unlock(&d->lock)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 6fdd6e5..b37db46 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1447,8 +1447,7 @@ int hci_register_dev(struct hci_dev *hdev)
struct list_head *head = &hci_dev_list, *p;
int i, id, error;
- BT_DBG("%p name %s bus %d owner %p", hdev, hdev->name,
- hdev->bus, hdev->owner);
+ BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
if (!hdev->open || !hdev->close)
return -EINVAL;
--
1.7.8.1
We provide a device-object to other subsystems and we provide our own
release-function. Therefore, the device-object must own a reference to our
module, otherwise the release-function may get deleted before the device-object
does.
Signed-off-by: David Herrmann <[email protected]>
---
net/bluetooth/hci_sysfs.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 5210956..e3bdee0 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -372,6 +372,7 @@ static void bt_host_release(struct device *dev)
{
void *data = dev_get_drvdata(dev);
kfree(data);
+ module_put(THIS_MODULE);
}
static struct device_type bt_host = {
@@ -523,6 +524,7 @@ void hci_init_sysfs(struct hci_dev *hdev)
dev->type = &bt_host;
dev->class = bt_class;
+ __module_get(THIS_MODULE);
dev_set_drvdata(dev, hdev);
device_initialize(dev);
}
--
1.7.8.1
The hci-destruct callback is not used by any driver so we can remove it. There
is no reason to keep it alive, anymore. Drivers can free their internal data on
driver-release and we do not need to provide a public destruct callback.
Internally, we still use a destruct callback inside of hci_sysfs.c. This one is
used to correctly free our hci_dev data structure if no more users have a
reference to it.
Signed-off-by: David Herrmann <[email protected]>
---
include/net/bluetooth/hci_core.h | 6 +-----
1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index b2c23e6..ab97ad3 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -254,7 +254,6 @@ struct hci_dev {
int (*close)(struct hci_dev *hdev);
int (*flush)(struct hci_dev *hdev);
int (*send)(struct sk_buff *skb);
- void (*destruct)(struct hci_dev *hdev);
void (*notify)(struct hci_dev *hdev, unsigned int evt);
int (*ioctl)(struct hci_dev *hdev, unsigned int cmd, unsigned long arg);
};
@@ -568,10 +567,7 @@ static inline void hci_conn_put(struct hci_conn *conn)
/* ----- HCI Devices ----- */
static inline void __hci_dev_put(struct hci_dev *d)
{
- if (atomic_dec_and_test(&d->refcnt)) {
- if (d->destruct)
- d->destruct(d);
- }
+ atomic_dec(&d->refcnt);
}
/*
--
1.7.8.1
We currently leak the hci_uart object if HCI_UART_PROTO_SET is never set because
the hci-destruct callback will then never be called.
This fix removes the hci-destruct callback and frees the driver internal private
hci_uart object directly on tty-close. We call hci_unregister_dev() here so the
hci-core will never call our callbacks again (except destruct). Therefore, we
can safely free the driver internal data right away and set the destruct
callback to NULL.
Signed-off-by: David Herrmann <[email protected]>
---
drivers/bluetooth/hci_ldisc.c | 12 ++----------
1 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 48ad2a7..5ea49df 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -231,15 +231,6 @@ static int hci_uart_send_frame(struct sk_buff *skb)
return 0;
}
-static void hci_uart_destruct(struct hci_dev *hdev)
-{
- if (!hdev)
- return;
-
- BT_DBG("%s", hdev->name);
- kfree(hdev->driver_data);
-}
-
/* ------ LDISC part ------ */
/* hci_uart_tty_open
*
@@ -316,6 +307,8 @@ static void hci_uart_tty_close(struct tty_struct *tty)
hci_free_dev(hdev);
}
}
+
+ kfree(hu);
}
}
@@ -397,7 +390,6 @@ static int hci_uart_register_dev(struct hci_uart *hu)
hdev->close = hci_uart_close;
hdev->flush = hci_uart_flush;
hdev->send = hci_uart_send_frame;
- hdev->destruct = hci_uart_destruct;
hdev->parent = hu->tty->dev;
hdev->owner = THIS_MODULE;
--
1.7.8.1
Instead of waiting for the hdev object to get freed we now free the private
driver-internal data on SDIO shutdown. This allows us to remove the obsolete
hci-destruct callback and free our data object right away after calling
hci_unregister_dev(). The HCI-core does not call any callbacks after this so we
are never called again and can safely exit the module.
Signed-off-by: David Herrmann <[email protected]>
---
drivers/bluetooth/btsdio.c | 11 +----------
1 files changed, 1 insertions(+), 10 deletions(-)
diff --git a/drivers/bluetooth/btsdio.c b/drivers/bluetooth/btsdio.c
index 792e32d..d38945c 100644
--- a/drivers/bluetooth/btsdio.c
+++ b/drivers/bluetooth/btsdio.c
@@ -289,15 +289,6 @@ static int btsdio_send_frame(struct sk_buff *skb)
return 0;
}
-static void btsdio_destruct(struct hci_dev *hdev)
-{
- struct btsdio_data *data = hdev->driver_data;
-
- BT_DBG("%s", hdev->name);
-
- kfree(data);
-}
-
static int btsdio_probe(struct sdio_func *func,
const struct sdio_device_id *id)
{
@@ -345,7 +336,6 @@ static int btsdio_probe(struct sdio_func *func,
hdev->close = btsdio_close;
hdev->flush = btsdio_flush;
hdev->send = btsdio_send_frame;
- hdev->destruct = btsdio_destruct;
hdev->owner = THIS_MODULE;
@@ -378,6 +368,7 @@ static void btsdio_remove(struct sdio_func *func)
hci_unregister_dev(hdev);
hci_free_dev(hdev);
+ kfree(data);
}
static struct sdio_driver btsdio_driver = {
--
1.7.8.1
Instead of waiting for the hci-device to be destroyed we now free the private
driver data on driver shutdown right away. We call hci_unregister_dev() on
driver shutdown, that means, the hci-core will never ever call our callbacks
again except the destruct callback. It also does not access hdev->driver_data so
there is no reason to keep that alive.
We simply set the destruct cb to NULL to avoid getting called again.
Signed-off-by: David Herrmann <[email protected]>
---
drivers/bluetooth/bpa10x.c | 15 +++------------
1 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/drivers/bluetooth/bpa10x.c b/drivers/bluetooth/bpa10x.c
index 751b338..92c2424 100644
--- a/drivers/bluetooth/bpa10x.c
+++ b/drivers/bluetooth/bpa10x.c
@@ -432,17 +432,6 @@ static int bpa10x_send_frame(struct sk_buff *skb)
return 0;
}
-static void bpa10x_destruct(struct hci_dev *hdev)
-{
- struct bpa10x_data *data = hdev->driver_data;
-
- BT_DBG("%s", hdev->name);
-
- kfree_skb(data->rx_skb[0]);
- kfree_skb(data->rx_skb[1]);
- kfree(data);
-}
-
static int bpa10x_probe(struct usb_interface *intf, const struct usb_device_id *id)
{
struct bpa10x_data *data;
@@ -480,7 +469,6 @@ static int bpa10x_probe(struct usb_interface *intf, const struct usb_device_id *
hdev->close = bpa10x_close;
hdev->flush = bpa10x_flush;
hdev->send = bpa10x_send_frame;
- hdev->destruct = bpa10x_destruct;
hdev->owner = THIS_MODULE;
@@ -512,6 +500,9 @@ static void bpa10x_disconnect(struct usb_interface *intf)
hci_unregister_dev(data->hdev);
hci_free_dev(data->hdev);
+ kfree_skb(data->rx_skb[0]);
+ kfree_skb(data->rx_skb[1]);
+ kfree(data);
}
static struct usb_driver bpa10x_driver = {
--
1.7.8.1
Instead of using the hci-destruct callback we free our private driver data on
USB shutdown. We already called hci_unregister_dev() here so the hci core will
never ever call our callbacks again except the destruct callback.
However, there is no reason to keep our *private* driver data alive if we get
never called again and the hci-core does never touch it the data. So we simply
free it right away and set the destruct callback to NULL.
Signed-off-by: David Herrmann <[email protected]>
---
drivers/bluetooth/btusb.c | 11 +----------
1 files changed, 1 insertions(+), 10 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index fbfba80..85bb17d 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -786,15 +786,6 @@ done:
return err;
}
-static void btusb_destruct(struct hci_dev *hdev)
-{
- struct btusb_data *data = hdev->driver_data;
-
- BT_DBG("%s", hdev->name);
-
- kfree(data);
-}
-
static void btusb_notify(struct hci_dev *hdev, unsigned int evt)
{
struct btusb_data *data = hdev->driver_data;
@@ -1007,7 +998,6 @@ static int btusb_probe(struct usb_interface *intf,
hdev->close = btusb_close;
hdev->flush = btusb_flush;
hdev->send = btusb_send_frame;
- hdev->destruct = btusb_destruct;
hdev->notify = btusb_notify;
hdev->owner = THIS_MODULE;
@@ -1111,6 +1101,7 @@ static void btusb_disconnect(struct usb_interface *intf)
__hci_dev_put(hdev);
hci_free_dev(hdev);
+ kfree(data);
}
#ifdef CONFIG_PM
--
1.7.8.1
This frees the private driver data on USB shutdown instead of using the
hci-destruct callback. We already call usb_set_intfdata(intf, NULL) but we do
not do the same with the hci object. This would be totally safe, though.
After calling hci_unregister_dev()/hci_free_dev() the hdev object will never
call any callback of us again except the destruct callback. Therefore, we can
safely set the destruct callback to NULL and free the driver data right away.
This allows to unload the module without waiting for the hdev device to be
released.
Signed-off-by: David Herrmann <[email protected]>
---
drivers/bluetooth/bfusb.c | 11 +----------
1 files changed, 1 insertions(+), 10 deletions(-)
diff --git a/drivers/bluetooth/bfusb.c b/drivers/bluetooth/bfusb.c
index a936763..857a951 100644
--- a/drivers/bluetooth/bfusb.c
+++ b/drivers/bluetooth/bfusb.c
@@ -544,15 +544,6 @@ static int bfusb_send_frame(struct sk_buff *skb)
return 0;
}
-static void bfusb_destruct(struct hci_dev *hdev)
-{
- struct bfusb_data *data = hdev->driver_data;
-
- BT_DBG("hdev %p bfusb %p", hdev, data);
-
- kfree(data);
-}
-
static int bfusb_ioctl(struct hci_dev *hdev, unsigned int cmd, unsigned long arg)
{
return -ENOIOCTLCMD;
@@ -712,7 +703,6 @@ static int bfusb_probe(struct usb_interface *intf, const struct usb_device_id *i
hdev->close = bfusb_close;
hdev->flush = bfusb_flush;
hdev->send = bfusb_send_frame;
- hdev->destruct = bfusb_destruct;
hdev->ioctl = bfusb_ioctl;
hdev->owner = THIS_MODULE;
@@ -753,6 +743,7 @@ static void bfusb_disconnect(struct usb_interface *intf)
hci_unregister_dev(hdev);
hci_free_dev(hdev);
+ kfree(data);
}
static struct usb_driver bfusb_driver = {
--
1.7.8.1
This removes the hci-destruct callback and instead frees the private driver data
in the vhci_release file release function. There is no reason to keep private
driver data available if the driver has already shut down.
After vhci_release is called our module can be unloaded. The only reason it is
kept alive is the hci-core having a module-ref on us because of our destruct
callback. However, this callback only frees hdev->driver_data. That is, we wait
for the hdev-device to get destroyed to free our internal driver-data. In fact,
the hci-core does never touch hdev->driver_data so it doesn't care if it is
NULL. Therefore, we simply free it when unloading the driver.
Another important fact is that the hdev core does not call any callbacks other
than the destruct-cb after hci_unregister_dev() has been called. So there is no
function of our module that will be called nor does the hci-core touch
hdev->driver_data. Hence, no other code can touch hdev->driver_data after our
cleanup so the destruct callback is definitely unnecessary here.
Signed-off-by: David Herrmann <[email protected]>
---
drivers/bluetooth/hci_vhci.c | 7 +------
1 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
index 2ed6ab1..44a8012 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -103,11 +103,6 @@ static int vhci_send_frame(struct sk_buff *skb)
return 0;
}
-static void vhci_destruct(struct hci_dev *hdev)
-{
- kfree(hdev->driver_data);
-}
-
static inline ssize_t vhci_get_user(struct vhci_data *data,
const char __user *buf, size_t count)
{
@@ -248,7 +243,6 @@ static int vhci_open(struct inode *inode, struct file *file)
hdev->close = vhci_close_dev;
hdev->flush = vhci_flush;
hdev->send = vhci_send_frame;
- hdev->destruct = vhci_destruct;
hdev->owner = THIS_MODULE;
@@ -273,6 +267,7 @@ static int vhci_release(struct inode *inode, struct file *file)
hci_free_dev(hdev);
file->private_data = NULL;
+ kfree(data);
return 0;
}
--
1.7.8.1
The destruct cb is optional so we can safely remove our dummy cb.
Signed-off-by: David Herrmann <[email protected]>
---
drivers/bluetooth/dtl1_cs.c | 6 ------
1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
index b2db5e9..aae40ca 100644
--- a/drivers/bluetooth/dtl1_cs.c
+++ b/drivers/bluetooth/dtl1_cs.c
@@ -439,11 +439,6 @@ static int dtl1_hci_send_frame(struct sk_buff *skb)
}
-static void dtl1_hci_destruct(struct hci_dev *hdev)
-{
-}
-
-
static int dtl1_hci_ioctl(struct hci_dev *hdev, unsigned int cmd, unsigned long arg)
{
return -ENOIOCTLCMD;
@@ -487,7 +482,6 @@ static int dtl1_open(dtl1_info_t *info)
hdev->close = dtl1_hci_close;
hdev->flush = dtl1_hci_flush;
hdev->send = dtl1_hci_send_frame;
- hdev->destruct = dtl1_hci_destruct;
hdev->ioctl = dtl1_hci_ioctl;
hdev->owner = THIS_MODULE;
--
1.7.8.1
The destruct cb is optional so remove our empty dummy cb.
Signed-off-by: David Herrmann <[email protected]>
---
drivers/bluetooth/btwilink.c | 9 ---------
1 files changed, 0 insertions(+), 9 deletions(-)
diff --git a/drivers/bluetooth/btwilink.c b/drivers/bluetooth/btwilink.c
index b5f83b4..da9cf6a 100644
--- a/drivers/bluetooth/btwilink.c
+++ b/drivers/bluetooth/btwilink.c
@@ -291,14 +291,6 @@ static int ti_st_send_frame(struct sk_buff *skb)
return 0;
}
-static void ti_st_destruct(struct hci_dev *hdev)
-{
- BT_DBG("%s", hdev->name);
- /* do nothing here, since platform remove
- * would free the hdev->driver_data
- */
-}
-
static int bt_ti_probe(struct platform_device *pdev)
{
static struct ti_st *hst;
@@ -325,7 +317,6 @@ static int bt_ti_probe(struct platform_device *pdev)
hdev->close = ti_st_close;
hdev->flush = NULL;
hdev->send = ti_st_send_frame;
- hdev->destruct = ti_st_destruct;
hdev->owner = THIS_MODULE;
err = hci_register_dev(hdev);
--
1.7.8.1
The destruct callback is optional and we provide an empty callback so remove it.
Signed-off-by: David Herrmann <[email protected]>
---
drivers/bluetooth/btuart_cs.c | 6 ------
1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/drivers/bluetooth/btuart_cs.c b/drivers/bluetooth/btuart_cs.c
index 200b3a2..84e02f1 100644
--- a/drivers/bluetooth/btuart_cs.c
+++ b/drivers/bluetooth/btuart_cs.c
@@ -459,11 +459,6 @@ static int btuart_hci_send_frame(struct sk_buff *skb)
}
-static void btuart_hci_destruct(struct hci_dev *hdev)
-{
-}
-
-
static int btuart_hci_ioctl(struct hci_dev *hdev, unsigned int cmd, unsigned long arg)
{
return -ENOIOCTLCMD;
@@ -505,7 +500,6 @@ static int btuart_open(btuart_info_t *info)
hdev->close = btuart_hci_close;
hdev->flush = btuart_hci_flush;
hdev->send = btuart_hci_send_frame;
- hdev->destruct = btuart_hci_destruct;
hdev->ioctl = btuart_hci_ioctl;
hdev->owner = THIS_MODULE;
--
1.7.8.1
The callback is optional and we provide an empty callback so remove it entirely.
Signed-off-by: David Herrmann <[email protected]>
---
drivers/bluetooth/btmrvl_main.c | 5 -----
1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index a88a78c..d62fc0d 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -387,10 +387,6 @@ static int btmrvl_ioctl(struct hci_dev *hdev,
return -ENOIOCTLCMD;
}
-static void btmrvl_destruct(struct hci_dev *hdev)
-{
-}
-
static int btmrvl_send_frame(struct sk_buff *skb)
{
struct hci_dev *hdev = (struct hci_dev *) skb->dev;
@@ -555,7 +551,6 @@ int btmrvl_register_hdev(struct btmrvl_private *priv)
hdev->close = btmrvl_close;
hdev->flush = btmrvl_flush;
hdev->send = btmrvl_send_frame;
- hdev->destruct = btmrvl_destruct;
hdev->ioctl = btmrvl_ioctl;
hdev->owner = THIS_MODULE;
--
1.7.8.1
The callback is optional and we provide an empty callback so remove it.
Signed-off-by: David Herrmann <[email protected]>
---
drivers/bluetooth/bt3c_cs.c | 6 ------
1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/drivers/bluetooth/bt3c_cs.c b/drivers/bluetooth/bt3c_cs.c
index 0c97e5d..e74334d 100644
--- a/drivers/bluetooth/bt3c_cs.c
+++ b/drivers/bluetooth/bt3c_cs.c
@@ -456,11 +456,6 @@ static int bt3c_hci_send_frame(struct sk_buff *skb)
}
-static void bt3c_hci_destruct(struct hci_dev *hdev)
-{
-}
-
-
static int bt3c_hci_ioctl(struct hci_dev *hdev, unsigned int cmd, unsigned long arg)
{
return -ENOIOCTLCMD;
@@ -587,7 +582,6 @@ static int bt3c_open(bt3c_info_t *info)
hdev->close = bt3c_hci_close;
hdev->flush = bt3c_hci_flush;
hdev->send = bt3c_hci_send_frame;
- hdev->destruct = bt3c_hci_destruct;
hdev->ioctl = bt3c_hci_ioctl;
hdev->owner = THIS_MODULE;
--
1.7.8.1
The destruct callback is optional and we provide an empty callback so remove it
entirely to avoid unnecessary code.
Signed-off-by: David Herrmann <[email protected]>
---
drivers/bluetooth/bluecard_cs.c | 6 ------
1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/drivers/bluetooth/bluecard_cs.c b/drivers/bluetooth/bluecard_cs.c
index c6a0c61..5cb325a 100644
--- a/drivers/bluetooth/bluecard_cs.c
+++ b/drivers/bluetooth/bluecard_cs.c
@@ -691,11 +691,6 @@ static int bluecard_hci_send_frame(struct sk_buff *skb)
}
-static void bluecard_hci_destruct(struct hci_dev *hdev)
-{
-}
-
-
static int bluecard_hci_ioctl(struct hci_dev *hdev, unsigned int cmd, unsigned long arg)
{
return -ENOIOCTLCMD;
@@ -741,7 +736,6 @@ static int bluecard_open(bluecard_info_t *info)
hdev->close = bluecard_hci_close;
hdev->flush = bluecard_hci_flush;
hdev->send = bluecard_hci_send_frame;
- hdev->destruct = bluecard_hci_destruct;
hdev->ioctl = bluecard_hci_ioctl;
hdev->owner = THIS_MODULE;
--
1.7.8.1
Several drivers already provide an empty callback so we can actually make this
optional and then remove all those empty callbacks in the drivers.
This callback isn't needed at all by most drivers as they can remove their
allocated structures on device disconnect and not on hci destruction.
Signed-off-by: David Herrmann <[email protected]>
---
include/net/bluetooth/hci_core.h | 6 ++++--
net/bluetooth/hci_core.c | 2 +-
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 5e2e984..b2c23e6 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -568,8 +568,10 @@ static inline void hci_conn_put(struct hci_conn *conn)
/* ----- HCI Devices ----- */
static inline void __hci_dev_put(struct hci_dev *d)
{
- if (atomic_dec_and_test(&d->refcnt))
- d->destruct(d);
+ if (atomic_dec_and_test(&d->refcnt)) {
+ if (d->destruct)
+ d->destruct(d);
+ }
}
/*
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 6d38d80..6fdd6e5 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1450,7 +1450,7 @@ int hci_register_dev(struct hci_dev *hdev)
BT_DBG("%p name %s bus %d owner %p", hdev, hdev->name,
hdev->bus, hdev->owner);
- if (!hdev->open || !hdev->close || !hdev->destruct)
+ if (!hdev->open || !hdev->close)
return -EINVAL;
/* Do not allow HCI_AMP devices to register at index 0,
--
1.7.8.1