* [RFC 1/4] Bluetooth: Remove obsolete hci-destruct callback
2011-10-28 13:00 [RFC 0/4] Fix hci_dev ref-counts David Herrmann
@ 2011-10-28 13:00 ` David Herrmann
2011-10-28 13:00 ` [RFC 2/4] Bluetooth: Fix hci-sysfs ref-counts David Herrmann
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: David Herrmann @ 2011-10-28 13:00 UTC (permalink / raw)
To: linux-bluetooth; +Cc: marcel, padovan, David Herrmann
The destruct-callback is used by *all* drivers to remove their platform
data only. However, all drivers call hci_unregister_dev() before this
callback is executed. Therefore, there is no reason to keep platform
data alive after the driver was unregistered.
Some drivers already depend on this behaviour, since they erroneously
already free the data after calling hci_free_dev(). This patch makes all
drivers behave that way and removes the callback entirely.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
drivers/bluetooth/bfusb.c | 11 +----------
drivers/bluetooth/bluecard_cs.c | 6 ------
drivers/bluetooth/bpa10x.c | 15 +++------------
drivers/bluetooth/bt3c_cs.c | 6 ------
drivers/bluetooth/btmrvl_main.c | 5 -----
drivers/bluetooth/btsdio.c | 11 +----------
drivers/bluetooth/btuart_cs.c | 6 ------
drivers/bluetooth/btusb.c | 11 +----------
drivers/bluetooth/btwilink.c | 9 ---------
drivers/bluetooth/dtl1_cs.c | 6 ------
drivers/bluetooth/hci_ldisc.c | 11 +----------
drivers/bluetooth/hci_vhci.c | 7 +------
include/net/bluetooth/hci_core.h | 4 +---
net/bluetooth/hci_core.c | 2 +-
14 files changed, 10 insertions(+), 100 deletions(-)
diff --git a/drivers/bluetooth/bfusb.c b/drivers/bluetooth/bfusb.c
index 005919a..876d081 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;
@@ -711,7 +702,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;
@@ -754,6 +744,7 @@ static void bfusb_disconnect(struct usb_interface *intf)
BT_ERR("Can't unregister HCI device %s", hdev->name);
hci_free_dev(hdev);
+ kfree(data);
}
static struct usb_driver bfusb_driver = {
diff --git a/drivers/bluetooth/bluecard_cs.c b/drivers/bluetooth/bluecard_cs.c
index aed1904..c395937 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;
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 = {
diff --git a/drivers/bluetooth/bt3c_cs.c b/drivers/bluetooth/bt3c_cs.c
index 4fc0194..8c031a1 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;
diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index 548d1d9..efb8d34 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -385,10 +385,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;
@@ -553,7 +549,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;
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 = {
diff --git a/drivers/bluetooth/btuart_cs.c b/drivers/bluetooth/btuart_cs.c
index 526b618..fed50c7 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;
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 9cbac6b..7f50838 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -778,15 +778,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;
@@ -999,7 +990,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;
@@ -1103,6 +1093,7 @@ static void btusb_disconnect(struct usb_interface *intf)
__hci_dev_put(hdev);
hci_free_dev(hdev);
+ kfree(data);
}
#ifdef CONFIG_PM
diff --git a/drivers/bluetooth/btwilink.c b/drivers/bluetooth/btwilink.c
index 04d353f..22a1f01 100644
--- a/drivers/bluetooth/btwilink.c
+++ b/drivers/bluetooth/btwilink.c
@@ -290,14 +290,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;
@@ -324,7 +316,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);
diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
index 5e4c2de..002e891 100644
--- a/drivers/bluetooth/dtl1_cs.c
+++ b/drivers/bluetooth/dtl1_cs.c
@@ -442,11 +442,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;
@@ -490,7 +485,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;
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 48ad2a7..d88dcff 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,7 @@ static void hci_uart_tty_close(struct tty_struct *tty)
hci_free_dev(hdev);
}
}
+ kfree(hu);
}
}
@@ -397,7 +389,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;
diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
index 67c180c..ba77d00 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -101,11 +101,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)
{
@@ -243,7 +238,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;
@@ -269,6 +263,7 @@ static int vhci_release(struct inode *inode, struct file *file)
}
hci_free_dev(hdev);
+ kfree(data);
file->private_data = NULL;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 9ef26fa..13a62ad 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -232,7 +232,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);
};
@@ -494,8 +493,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))
- d->destruct(d);
+ atomic_dec(&d->refcnt);
}
static inline void hci_dev_put(struct hci_dev *d)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index f0e0f8e..daa948f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1449,7 +1449,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;
write_lock_bh(&hci_dev_list_lock);
--
1.7.7.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [RFC 2/4] Bluetooth: Fix hci-sysfs ref-counts
2011-10-28 13:00 [RFC 0/4] Fix hci_dev ref-counts David Herrmann
2011-10-28 13:00 ` [RFC 1/4] Bluetooth: Remove obsolete hci-destruct callback David Herrmann
@ 2011-10-28 13:00 ` David Herrmann
2011-10-29 16:40 ` David Herrmann
2011-10-28 13:00 ` [RFC 3/4] Bluetooth: Remove hci owner field David Herrmann
` (2 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: David Herrmann @ 2011-10-28 13:00 UTC (permalink / raw)
To: linux-bluetooth; +Cc: marcel, padovan, David Herrmann
When registering a device object we need to take a module reference so
the device-destruct function is available when the object is freed.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
include/net/bluetooth/hci_core.h | 2 +-
net/bluetooth/hci_core.c | 6 +++++-
net/bluetooth/hci_sysfs.c | 8 +++++++-
3 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 13a62ad..c8368a5 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -580,7 +580,7 @@ int hci_recv_frame(struct sk_buff *skb);
int hci_recv_fragment(struct hci_dev *hdev, int type, void *data, int count);
int hci_recv_stream_fragment(struct hci_dev *hdev, void *data, int count);
-void hci_init_sysfs(struct hci_dev *hdev);
+int hci_init_sysfs(struct hci_dev *hdev);
int hci_add_sysfs(struct hci_dev *hdev);
void hci_del_sysfs(struct hci_dev *hdev);
void hci_conn_init_sysfs(struct hci_conn *conn);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index daa948f..20c3c97 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -912,7 +912,11 @@ struct hci_dev *hci_alloc_dev(void)
if (!hdev)
return NULL;
- hci_init_sysfs(hdev);
+ if (hci_init_sysfs(hdev)) {
+ kfree(hdev);
+ return NULL;
+ }
+
skb_queue_head_init(&hdev->driver_init);
return hdev;
diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
index 116401a..605cb2b 100644
--- a/net/bluetooth/hci_sysfs.c
+++ b/net/bluetooth/hci_sysfs.c
@@ -78,6 +78,7 @@ static void bt_link_release(struct device *dev)
{
void *data = dev_get_drvdata(dev);
kfree(data);
+ module_put(THIS_MODULE);
}
static struct device_type bt_link = {
@@ -540,15 +541,20 @@ static int auto_accept_delay_get(void *data, u64 *val)
DEFINE_SIMPLE_ATTRIBUTE(auto_accept_delay_fops, auto_accept_delay_get,
auto_accept_delay_set, "%llu\n");
-void hci_init_sysfs(struct hci_dev *hdev)
+int hci_init_sysfs(struct hci_dev *hdev)
{
struct device *dev = &hdev->dev;
+ if (!try_module_get(THIS_MODULE))
+ return -EIO;
+
dev->type = &bt_host;
dev->class = bt_class;
dev_set_drvdata(dev, hdev);
device_initialize(dev);
+
+ return 0;
}
int hci_add_sysfs(struct hci_dev *hdev)
--
1.7.7.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [RFC 2/4] Bluetooth: Fix hci-sysfs ref-counts
2011-10-28 13:00 ` [RFC 2/4] Bluetooth: Fix hci-sysfs ref-counts David Herrmann
@ 2011-10-29 16:40 ` David Herrmann
0 siblings, 0 replies; 8+ messages in thread
From: David Herrmann @ 2011-10-29 16:40 UTC (permalink / raw)
To: linux-bluetooth; +Cc: marcel, padovan, David Herrmann
On 10/28/11, David Herrmann <dh.herrmann@googlemail.com> wrote:
> When registering a device object we need to take a module reference so
> the device-destruct function is available when the object is freed.
>
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> ---
> include/net/bluetooth/hci_core.h | 2 +-
> net/bluetooth/hci_core.c | 6 +++++-
> net/bluetooth/hci_sysfs.c | 8 +++++++-
> 3 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h
> b/include/net/bluetooth/hci_core.h
> index 13a62ad..c8368a5 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -580,7 +580,7 @@ int hci_recv_frame(struct sk_buff *skb);
> int hci_recv_fragment(struct hci_dev *hdev, int type, void *data, int
> count);
> int hci_recv_stream_fragment(struct hci_dev *hdev, void *data, int count);
>
> -void hci_init_sysfs(struct hci_dev *hdev);
> +int hci_init_sysfs(struct hci_dev *hdev);
> int hci_add_sysfs(struct hci_dev *hdev);
> void hci_del_sysfs(struct hci_dev *hdev);
> void hci_conn_init_sysfs(struct hci_conn *conn);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index daa948f..20c3c97 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -912,7 +912,11 @@ struct hci_dev *hci_alloc_dev(void)
> if (!hdev)
> return NULL;
>
> - hci_init_sysfs(hdev);
> + if (hci_init_sysfs(hdev)) {
> + kfree(hdev);
> + return NULL;
> + }
> +
> skb_queue_head_init(&hdev->driver_init);
>
> return hdev;
> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 116401a..605cb2b 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -78,6 +78,7 @@ static void bt_link_release(struct device *dev)
> {
> void *data = dev_get_drvdata(dev);
> kfree(data);
> + module_put(THIS_MODULE);
> }
Ok I messed up. This needs to be inside bt_host_release and not
bt_link_release, of course. Then everything works fine.
> static struct device_type bt_link = {
> @@ -540,15 +541,20 @@ static int auto_accept_delay_get(void *data, u64 *val)
> DEFINE_SIMPLE_ATTRIBUTE(auto_accept_delay_fops, auto_accept_delay_get,
> auto_accept_delay_set, "%llu\n");
>
> -void hci_init_sysfs(struct hci_dev *hdev)
> +int hci_init_sysfs(struct hci_dev *hdev)
> {
> struct device *dev = &hdev->dev;
>
> + if (!try_module_get(THIS_MODULE))
> + return -EIO;
> +
> dev->type = &bt_host;
> dev->class = bt_class;
>
> dev_set_drvdata(dev, hdev);
> device_initialize(dev);
> +
> + return 0;
> }
>
> int hci_add_sysfs(struct hci_dev *hdev)
> --
> 1.7.7.1
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC 3/4] Bluetooth: Remove hci owner field
2011-10-28 13:00 [RFC 0/4] Fix hci_dev ref-counts David Herrmann
2011-10-28 13:00 ` [RFC 1/4] Bluetooth: Remove obsolete hci-destruct callback David Herrmann
2011-10-28 13:00 ` [RFC 2/4] Bluetooth: Fix hci-sysfs ref-counts David Herrmann
@ 2011-10-28 13:00 ` David Herrmann
2011-10-28 13:00 ` [RFC 4/4] Bluetooth: Correctly take device refcounts David Herrmann
2011-10-31 11:58 ` [RFC 0/4] Fix hci_dev ref-counts Marcel Holtmann
4 siblings, 0 replies; 8+ messages in thread
From: David Herrmann @ 2011-10-28 13:00 UTC (permalink / raw)
To: linux-bluetooth; +Cc: marcel, padovan, David Herrmann
There is no reason to keep a reference to the owner module anymore.
Modules call hci_unregister_dev() when being unloaded and this removes
all callbacks from the hci_dev structure. Hence, we do not need to make
sure that the module stays alive until the structure is freed.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
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 | 7 +------
net/bluetooth/hci_core.c | 3 +--
14 files changed, 2 insertions(+), 30 deletions(-)
diff --git a/drivers/bluetooth/bfusb.c b/drivers/bluetooth/bfusb.c
index 876d081..9c046ef 100644
--- a/drivers/bluetooth/bfusb.c
+++ b/drivers/bluetooth/bfusb.c
@@ -704,8 +704,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 c395937..dca64b84 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 8c031a1..d5bb8ee 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 efb8d34..3eefd64 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -550,7 +550,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 fed50c7..d7c2de4 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 7f50838..9bf3a7b 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -992,8 +992,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 22a1f01..bf96280 100644
--- a/drivers/bluetooth/btwilink.c
+++ b/drivers/bluetooth/btwilink.c
@@ -316,7 +316,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 002e891..6be4869 100644
--- a/drivers/bluetooth/dtl1_cs.c
+++ b/drivers/bluetooth/dtl1_cs.c
@@ -487,8 +487,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 d88dcff..2542b3f 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -391,8 +391,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 ba77d00..b490c097 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -239,8 +239,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 c8368a5..e5dc220 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -226,8 +226,6 @@ struct hci_dev {
struct rfkill *rfkill;
- struct module *owner;
-
int (*open)(struct hci_dev *hdev);
int (*close)(struct hci_dev *hdev);
int (*flush)(struct hci_dev *hdev);
@@ -499,7 +497,6 @@ static inline void __hci_dev_put(struct hci_dev *d)
static inline void hci_dev_put(struct hci_dev *d)
{
__hci_dev_put(d);
- module_put(d->owner);
}
static inline struct hci_dev *__hci_dev_hold(struct hci_dev *d)
@@ -510,9 +507,7 @@ static inline struct hci_dev *__hci_dev_hold(struct hci_dev *d)
static inline struct hci_dev *hci_dev_hold(struct hci_dev *d)
{
- if (try_module_get(d->owner))
- return __hci_dev_hold(d);
- return NULL;
+ return __hci_dev_hold(d);
}
#define hci_dev_lock(d) spin_lock(&d->lock)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 20c3c97..fb981bd 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1450,8 +1450,7 @@ int hci_register_dev(struct hci_dev *hdev)
struct list_head *head = &hci_dev_list, *p;
int i, id = 0, 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.7.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [RFC 4/4] Bluetooth: Correctly take device refcounts
2011-10-28 13:00 [RFC 0/4] Fix hci_dev ref-counts David Herrmann
` (2 preceding siblings ...)
2011-10-28 13:00 ` [RFC 3/4] Bluetooth: Remove hci owner field David Herrmann
@ 2011-10-28 13:00 ` David Herrmann
2011-10-31 11:58 ` [RFC 0/4] Fix hci_dev ref-counts Marcel Holtmann
4 siblings, 0 replies; 8+ messages in thread
From: David Herrmann @ 2011-10-28 13:00 UTC (permalink / raw)
To: linux-bluetooth; +Cc: marcel, padovan, David Herrmann
The hci_dev->dev internal ref-count is used by hci core to protect the
hci_dev structure. We need to make sure to take this refcount when
calling hci_dev_hold/put(), otherwise we might end up with a valid
reference but a freed hci_dev structure.
We also entirely remove the second hci_dev->refcnt reference counter as
it is not used for anything right now.
Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
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 e5dc220..7fa1ab8 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -116,7 +116,6 @@ struct adv_entry {
struct hci_dev {
struct list_head list;
spinlock_t lock;
- atomic_t refcnt;
char name[8];
unsigned long flags;
@@ -491,7 +490,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);
}
static inline void hci_dev_put(struct hci_dev *d)
@@ -501,7 +500,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 fb981bd..53e3585 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1468,7 +1468,6 @@ int hci_register_dev(struct hci_dev *hdev)
hdev->id = id;
list_add(&hdev->list, head);
- atomic_set(&hdev->refcnt, 1);
spin_lock_init(&hdev->lock);
hdev->flags = 0;
@@ -1552,6 +1551,7 @@ int hci_register_dev(struct hci_dev *hdev)
queue_work(hdev->workqueue, &hdev->power_on);
hci_notify(hdev, HCI_DEV_REG);
+ __hci_dev_hold(hdev);
return id;
--
1.7.7.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [RFC 0/4] Fix hci_dev ref-counts
2011-10-28 13:00 [RFC 0/4] Fix hci_dev ref-counts David Herrmann
` (3 preceding siblings ...)
2011-10-28 13:00 ` [RFC 4/4] Bluetooth: Correctly take device refcounts David Herrmann
@ 2011-10-31 11:58 ` Marcel Holtmann
2011-11-01 19:59 ` David Herrmann
4 siblings, 1 reply; 8+ messages in thread
From: Marcel Holtmann @ 2011-10-31 11:58 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-bluetooth, padovan
Hi David,
> We currently have two reference counters for each hci_dev. This patchset tries
> to reduce this to one ref-count and fix several bugs with them. The two
> available ref-counts currently are:
>
> 1) hci_dev->dev internal
> This ref-count is increased by hci_alloc_dev() and decreased inside
> hci_free_dev(). No other code currently uses this refcount. It can be set with
> get_device() and put_device().
> When this ref-count drops zero, then the hci_dev structure is deallocated (see
> bt_link_release inside hci_sysfs.c), hence, this ref-count is the most important
> one. However, we currently do not use it correctly.
>
> 2) hci_dev->refcnt
> This ref-count is used inside hci_dev_hold/put() and practically used as if it
> would protect the hci_dev structure. However, if this ref-count drops zero, then
> the ->destruct callback is called which is used by all drivers *exclusively* to
> deallocate driver structures and *not* to deallocate or destroy the hci_dev.
actually I think we should convert the tasklets into one workqueue first
and then integrate deeper into sysfs before trying to hack around this
problem.
Our core driver registration code is from the 2.4 series and needs major
updating. The sysfs code is glued on top of it and not really well
integrated. So fixing that one properly might make some of these race
conditions go away.
Regards
Marcel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [RFC 0/4] Fix hci_dev ref-counts
2011-10-31 11:58 ` [RFC 0/4] Fix hci_dev ref-counts Marcel Holtmann
@ 2011-11-01 19:59 ` David Herrmann
0 siblings, 0 replies; 8+ messages in thread
From: David Herrmann @ 2011-11-01 19:59 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, padovan
On Mon, Oct 31, 2011 at 12:58 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi David,
>
>> We currently have two reference counters for each hci_dev. This patchset tries
>> to reduce this to one ref-count and fix several bugs with them. The two
>> available ref-counts currently are:
>>
>> 1) hci_dev->dev internal
>> This ref-count is increased by hci_alloc_dev() and decreased inside
>> hci_free_dev(). No other code currently uses this refcount. It can be set with
>> get_device() and put_device().
>> When this ref-count drops zero, then the hci_dev structure is deallocated (see
>> bt_link_release inside hci_sysfs.c), hence, this ref-count is the most important
>> one. However, we currently do not use it correctly.
>>
>> 2) hci_dev->refcnt
>> This ref-count is used inside hci_dev_hold/put() and practically used as if it
>> would protect the hci_dev structure. However, if this ref-count drops zero, then
>> the ->destruct callback is called which is used by all drivers *exclusively* to
>> deallocate driver structures and *not* to deallocate or destroy the hci_dev.
>
> actually I think we should convert the tasklets into one workqueue first
> and then integrate deeper into sysfs before trying to hack around this
> problem.
I am currently playing around with several patches for that but I
still need to get away from that single-threaded workqueue as it is
quite ugly.
> Our core driver registration code is from the 2.4 series and needs major
> updating. The sysfs code is glued on top of it and not really well
> integrated. So fixing that one properly might make some of these race
> conditions go away.
Ok. I'll will reconsider sending a new RFC when we fixed the workqueue
stuff. As our system is a simple bus-system, it shouldn't be too hard
to do that.
> Regards
>
> Marcel
Regards
David
^ permalink raw reply [flat|nested] 8+ messages in thread