[PATCH 0/5] Fix driver core registration

[PATCH 0/5] Fix driver core registration

[David Herrmann]

This patchset correctly integrates the "struct device" in a hci_dev. It removes
the refcnt field and instead relies on the internal refcount of device objects.
It also removes the "destruct" callback as it is really not needed. The bus is
responsible of freeing devices after a driver unregistered itself.

This also fixes several bugs in the current implementation. Each patch should be
pretty straightforward. It all works well on my machine.

Regards
David

David Herrmann (5):
  Bluetooth: Remove obsolete hci-destruct callback
  Bluetooth: Correctly acquire module reference
  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    |   13 +------------
 drivers/bluetooth/hci_vhci.c     |    9 +--------
 include/net/bluetooth/hci_core.h |   30 +++++-------------------------
 net/bluetooth/hci_core.c         |   15 +++++++++------
 net/bluetooth/hci_sysfs.c        |    8 +++++++-
 15 files changed, 31 insertions(+), 152 deletions(-)

-- 
1.7.8.1

[PATCH 1/5] Bluetooth: Remove obsolete hci-destruct callback

[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. Hence, we can free platform data
directly and then remove the hci-destruct callback entirely.

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 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 = {
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;
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 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;
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;
 
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 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;
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
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);
diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
index 969bb22..9cdce11 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 2ed6ab1..6ef00b7 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;
 
@@ -271,6 +265,7 @@ static int vhci_release(struct inode *inode, struct file *file)
 
 	hci_unregister_dev(hdev);
 	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 5e2e984..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,8 +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))
-		d->destruct(d);
+	atomic_dec(&d->refcnt);
 }
 
 /*
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

Re: [PATCH 1/5] Bluetooth: Remove obsolete hci-destruct callback

[Marcel Holtmann]

Hi David,

> 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. Hence, we can free platform data
> directly and then remove the hci-destruct callback entirely.
> 
> 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.

it is important to not end up with a race condition between sysfs and
hci_dev here. For example a cat /sys/class/bluetooth/hci0/name could
still have sysfs file open and we need to keep the memory around until
it gets destructed.

Regards

Marcel

Re: [PATCH 1/5] Bluetooth: Remove obsolete hci-destruct callback

[David Herrmann]

On Thu, Dec 29, 2011 at 6:48 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi David,
>
>> 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. Hence, we can free platform data
>> directly and then remove the hci-destruct callback entirely.
>>
>> 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.
>
> it is important to not end up with a race condition between sysfs and
> hci_dev here. For example a cat /sys/class/bluetooth/hci0/name could
> still have sysfs file open and we need to keep the memory around until
> it gets destructed.

Ok, I am a bit confused. The destruct callback I am talking about here
is about driver state, not hci_dev state.
Also, this does not remove the hci_dev device-destruct callback. This
only removes the driver-state destruct callback.
Driver-data should be accessed by drivers only! Therefore, I don't
understand why we keep this driver-data alive until hci_dev is
destroyed.

I haven't found any other subsystem which behaves like this. I think
it's a bit confusing what this destruct callback actually does. It is
used as if it has something to do with hci_dev but it doesn't. I don't
understand why we use it and I think it's some left-over from some old
interface. However, look into the drivers/bluetooth/* files and see
what all these callbacks do. They only remove *driver-internal* state.
They do not remove any hci internal state. And driver-internal state
can be removed when the driver is unregistered like every other
bus-system does.
As an example, look at the kfree() calls I moved. The data which is
freed by them is never accessed outside of the
drivers/bluetooth/<driver>.c file so I don't think there is any
race-condition here.

> Regards
>
> Marcel

Thanks for reviewing
David

Re: [PATCH 1/5] Bluetooth: Remove obsolete hci-destruct callback

[David Herrmann]

Hi Marcel

On Thu, Dec 29, 2011 at 7:04 PM, David Herrmann
<dh.herrmann@googlemail.com> wrote:
> On Thu, Dec 29, 2011 at 6:48 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>> Hi David,
>>
>>> 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. Hence, we can free platform data
>>> directly and then remove the hci-destruct callback entirely.
>>>
>>> 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.
>>
>> it is important to not end up with a race condition between sysfs and
>> hci_dev here. For example a cat /sys/class/bluetooth/hci0/name could
>> still have sysfs file open and we need to keep the memory around until
>> it gets destructed.

I will resend this as sequential patches which thorough explanations
so it might be more clear why this callback is obsolete.

Cheers
David

[PATCH 2/5] Bluetooth: Correctly acquire module reference

[David Herrmann]

When creating hci_dev objects we may end up with dangling devices that some
other subsystem has a ref-count of. If the bluetooth module is unloaded while
there are still hci_dev objects around, we might end up with calling invalid
callbacks on hci_dev destruction.

This patch correctly takes a module reference for every hci_dev object so when
destroying the object we can be sure that the bluetooth module is still
available.

This is the normal procedure of all bus systems as the device core does not
provide owner-tracking.

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 ab97ad3..720d81d 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -654,7 +654,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 6fdd6e5..fb08b01 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -940,7 +940,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 5210956..67f75e9 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 = {
@@ -516,15 +517,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.8.1

[PATCH 3/5] Bluetooth: Remove HCI-owner field

[David Herrmann]

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 <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 |   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 9cdce11..fb50b0d 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 6ef00b7..89b5ac5 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 720d81d..13f7c06 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 fb08b01..0c7e2b2 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1451,8 +1451,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

[PATCH 4/5] Bluetooth: Correctly take hci_dev->dev refcount

[David Herrmann]

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 out bluetooth bus system.

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 13f7c06..8b17d07 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 0c7e2b2..4dbbfbc 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1474,7 +1474,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;
@@ -1558,6 +1557,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

[PATCH 5/5] Bluetooth: Remove __hci_dev_put/hold

[David Herrmann]

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 <dh.herrmann@googlemail.com>
---
 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 8b17d07..afab7d3 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 4dbbfbc..ac0b795 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1557,7 +1557,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;
 
@@ -1620,7 +1620,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