[PATCHv2 0/2] Fixing device registration bugs

[PATCHv2 0/2] Fixing device registration bugs

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

It does make sense to use bitmask marking used device ids.
This approach is used in device name allocation in many places
like:

..
drivers/infiniband/core/device.c <<alloc_name>>
net/core/dev.c <<__dev_alloc_name>>
net/core/sock.c <<assign_proto_idx>>
..

Andrei Emeltchenko (2):
  Bluetooth: Fix registering hci with duplicate name
  Bluetooth: Fix debug printing unallocated name

 net/bluetooth/hci_core.c |   32 ++++++++++++++++++--------------
 1 files changed, 18 insertions(+), 14 deletions(-)

-- 
1.7.9.1

[PATCHv2 1/2] Bluetooth: Fix registering hci with duplicate name

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

When adding HCI devices hci_register_dev assigns the same name
hci1 for subsequently added AMP devices. Find free device id
the same way as it is done in netdev.

...
[ 6958.381886] sysfs: cannot create duplicate filename
	'/devices/virtual/bluetooth/hci1
...

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/hci_core.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 52c7abf..9596a82 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1740,31 +1740,35 @@ int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
 /* Register HCI device */
 int hci_register_dev(struct hci_dev *hdev)
 {
-	struct list_head *head = &hci_dev_list, *p;
+	struct hci_dev *d;
 	int i, id, error;
+	unsigned long *inuse;
 
 	BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
 
 	if (!hdev->open || !hdev->close)
 		return -EINVAL;
 
-	/* Do not allow HCI_AMP devices to register at index 0,
-	 * so the index can be used as the AMP controller ID.
-	 */
-	id = (hdev->dev_type == HCI_BREDR) ? 0 : 1;
+	inuse = (unsigned long *) get_zeroed_page(GFP_KERNEL);
+	if (!inuse)
+		return -ENOMEM;
 
 	write_lock(&hci_dev_list_lock);
 
-	/* Find first available device id */
-	list_for_each(p, &hci_dev_list) {
-		if (list_entry(p, struct hci_dev, list)->id != id)
-			break;
-		head = p; id++;
-	}
+	/* Index HCI_BREDR_ID reserved for BR/EDR controllers */
+	if (hdev->dev_type != HCI_BREDR)
+		set_bit(HCI_BREDR_ID, inuse);
+
+	list_for_each_entry(d, &hci_dev_list, list)
+		set_bit(d->id, inuse);
+
+	id = find_first_zero_bit(inuse, PAGE_SIZE * 8);
+	free_page((unsigned long) inuse);
 
 	sprintf(hdev->name, "hci%d", id);
 	hdev->id = id;
-	list_add_tail(&hdev->list, head);
+
+	list_add_tail(&hdev->list, &hci_dev_list);
 
 	mutex_init(&hdev->lock);
 
-- 
1.7.9.1

[PATCHv2 2/2] Bluetooth: Fix debug printing unallocated name

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

It does make sense to print hdev name after allocation.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/hci_core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 9596a82..a3c955f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1744,8 +1744,6 @@ int hci_register_dev(struct hci_dev *hdev)
 	int i, id, error;
 	unsigned long *inuse;
 
-	BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
-
 	if (!hdev->open || !hdev->close)
 		return -EINVAL;
 
@@ -1768,6 +1766,8 @@ int hci_register_dev(struct hci_dev *hdev)
 	sprintf(hdev->name, "hci%d", id);
 	hdev->id = id;
 
+	BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
+
 	list_add_tail(&hdev->list, &hci_dev_list);
 
 	mutex_init(&hdev->lock);
-- 
1.7.9.1

[PATCHv3 0/2] Fixing device registration bugs

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Changes:
	* v3: Added missing HCI_BREDR_ID

It does make sense to use bitmask marking used device ids.
This approach is used in device name allocation in many places
like:
..
drivers/infiniband/core/device.c <<alloc_name>>
net/core/dev.c <<__dev_alloc_name>>
net/core/sock.c <<assign_proto_idx>>
..

Andrei Emeltchenko (2):
  Bluetooth: Fix registering hci with duplicate name
  Bluetooth: Fix debug printing unallocated name

 include/net/bluetooth/hci.h |    3 +++
 net/bluetooth/hci_core.c    |   32 ++++++++++++++++++--------------
 2 files changed, 21 insertions(+), 14 deletions(-)

-- 
1.7.9.5

[PATCHv3 1/2] Bluetooth: Fix registering hci with duplicate name

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

When adding HCI devices hci_register_dev may assigns the same name
hciX for subsequently added HCI devices. Use bitmask to find free
device ID the same way as it is done in other subsystems.

...
[ 6958.381886] sysfs: cannot create duplicate filename
	'/devices/virtual/bluetooth/hci1
...

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/hci.h |    3 +++
 net/bluetooth/hci_core.c    |   28 ++++++++++++++++------------
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 346f087..38b837b 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -56,6 +56,9 @@
 #define HCI_BREDR	0x00
 #define HCI_AMP		0x01
 
+/* First BR/EDR Controller shall have ID = 0 */
+#define HCI_BREDR_ID	0
+
 /* HCI device quirks */
 enum {
 	HCI_QUIRK_NO_RESET,
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 52c7abf..9596a82 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1740,31 +1740,35 @@ int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
 /* Register HCI device */
 int hci_register_dev(struct hci_dev *hdev)
 {
-	struct list_head *head = &hci_dev_list, *p;
+	struct hci_dev *d;
 	int i, id, error;
+	unsigned long *inuse;
 
 	BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
 
 	if (!hdev->open || !hdev->close)
 		return -EINVAL;
 
-	/* Do not allow HCI_AMP devices to register at index 0,
-	 * so the index can be used as the AMP controller ID.
-	 */
-	id = (hdev->dev_type == HCI_BREDR) ? 0 : 1;
+	inuse = (unsigned long *) get_zeroed_page(GFP_KERNEL);
+	if (!inuse)
+		return -ENOMEM;
 
 	write_lock(&hci_dev_list_lock);
 
-	/* Find first available device id */
-	list_for_each(p, &hci_dev_list) {
-		if (list_entry(p, struct hci_dev, list)->id != id)
-			break;
-		head = p; id++;
-	}
+	/* Index HCI_BREDR_ID reserved for BR/EDR controllers */
+	if (hdev->dev_type != HCI_BREDR)
+		set_bit(HCI_BREDR_ID, inuse);
+
+	list_for_each_entry(d, &hci_dev_list, list)
+		set_bit(d->id, inuse);
+
+	id = find_first_zero_bit(inuse, PAGE_SIZE * 8);
+	free_page((unsigned long) inuse);
 
 	sprintf(hdev->name, "hci%d", id);
 	hdev->id = id;
-	list_add_tail(&hdev->list, head);
+
+	list_add_tail(&hdev->list, &hci_dev_list);
 
 	mutex_init(&hdev->lock);
 
-- 
1.7.9.5

[PATCHv3 2/2] Bluetooth: Fix debug printing unallocated name

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

It does make sense to print hdev name after allocation.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/hci_core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 9596a82..a3c955f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1744,8 +1744,6 @@ int hci_register_dev(struct hci_dev *hdev)
 	int i, id, error;
 	unsigned long *inuse;
 
-	BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
-
 	if (!hdev->open || !hdev->close)
 		return -EINVAL;
 
@@ -1768,6 +1766,8 @@ int hci_register_dev(struct hci_dev *hdev)
 	sprintf(hdev->name, "hci%d", id);
 	hdev->id = id;
 
+	BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
+
 	list_add_tail(&hdev->list, &hci_dev_list);
 
 	mutex_init(&hdev->lock);
-- 
1.7.9.5

Re: [PATCHv3 1/2] Bluetooth: Fix registering hci with duplicate name

[Ulisses Furquim]

Hi Andrei,

On Fri, Apr 13, 2012 at 8:21 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
>
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> When adding HCI devices hci_register_dev may assigns the same name
> hciX for subsequently added HCI devices. Use bitmask to find free
> device ID the same way as it is done in other subsystems.
>
> ...
> [ 6958.381886] sysfs: cannot create duplicate filename
>        '/devices/virtual/bluetooth/hci1
> ...
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/hci.h |    3 +++
>  net/bluetooth/hci_core.c    |   28 ++++++++++++++++------------
>  2 files changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 346f087..38b837b 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -56,6 +56,9 @@
>  #define HCI_BREDR      0x00
>  #define HCI_AMP                0x01
>
> +/* First BR/EDR Controller shall have ID = 0 */
> +#define HCI_BREDR_ID   0
> +
>  /* HCI device quirks */
>  enum {
>        HCI_QUIRK_NO_RESET,
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 52c7abf..9596a82 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1740,31 +1740,35 @@ int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
>  /* Register HCI device */
>  int hci_register_dev(struct hci_dev *hdev)
>  {
> -       struct list_head *head = &hci_dev_list, *p;
> +       struct hci_dev *d;
>        int i, id, error;
> +       unsigned long *inuse;
>
>        BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
>
>        if (!hdev->open || !hdev->close)
>                return -EINVAL;
>
> -       /* Do not allow HCI_AMP devices to register at index 0,
> -        * so the index can be used as the AMP controller ID.
> -        */
> -       id = (hdev->dev_type == HCI_BREDR) ? 0 : 1;
> +       inuse = (unsigned long *) get_zeroed_page(GFP_KERNEL);
> +       if (!inuse)
> +               return -ENOMEM;
>
>        write_lock(&hci_dev_list_lock);
>
> -       /* Find first available device id */
> -       list_for_each(p, &hci_dev_list) {
> -               if (list_entry(p, struct hci_dev, list)->id != id)
> -                       break;
> -               head = p; id++;
> -       }
> +       /* Index HCI_BREDR_ID reserved for BR/EDR controllers */
> +       if (hdev->dev_type != HCI_BREDR)
> +               set_bit(HCI_BREDR_ID, inuse);
> +
> +       list_for_each_entry(d, &hci_dev_list, list)
> +               set_bit(d->id, inuse);
> +
> +       id = find_first_zero_bit(inuse, PAGE_SIZE * 8);
> +       free_page((unsigned long) inuse);
>
>        sprintf(hdev->name, "hci%d", id);
>        hdev->id = id;
> -       list_add_tail(&hdev->list, head);
> +
> +       list_add_tail(&hdev->list, &hci_dev_list);

Well, I don't like this bitmask approach, really. We used to have this
list ordered and the code was doing that before we added the
restriction of index 0 for AMP devices, I think. Using a bitmask looks
overkill to me when we can probably modify the loop to find the first
available ID. Marcel even suggested an approach with a pseudo code
that seems to be the best way to go. Something along the lines of:

       list_for_each(p, &hci_dev_list) {
               if (list_entry(p, struct hci_dev, list)->id > id)
/* just replacing != with > */
                       break;
               head = p; id++;
       }

We assume id starts with the number we'll try to add the new device
and keep iterating until we find the proper place. The only difference
is we start with 0 for BR/EDR and 1 for AMP devices. Then every
hdev->id in the _ordered_ list <= to the id we want we increment id
and move head. In the end we'll have id as the first available one and
head is where you need to add hdev (before head) to the list to keep
it ordered. Right? Does that work for you? Am I missing anything here?

Best regards,

--
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [PATCHv3 1/2] Bluetooth: Fix registering hci with duplicate name

[Andrei Emeltchenko]

Hi Ulisses,

On Fri, Apr 13, 2012 at 7:25 PM, Ulisses Furquim <ulisses@profusion.mobi> wrote:

> On Fri, Apr 13, 2012 at 8:21 AM, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
>>
>> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>>
>> When adding HCI devices hci_register_dev may assigns the same name
>> hciX for subsequently added HCI devices. Use bitmask to find free
>> device ID the same way as it is done in other subsystems.
>>
>> ...
>> [ 6958.381886] sysfs: cannot create duplicate filename
>>        '/devices/virtual/bluetooth/hci1
> Well, I don't like this bitmask approach, really. We used to have this
> list ordered and the code was doing that before we added the
> restriction of index 0 for AMP devices, I think.

I think it just was not tested enough.

> Using a bitmask looks
> overkill to me when we can probably modify the loop to find the first

This approach is used in many places when choosing device id, specifically
in network device code, so I just took it as I do not like to reinvent
the wheel.

> available ID. Marcel even suggested an approach with a pseudo code
> that seems to be the best way to go. Something along the lines of:
>
>        list_for_each(p, &hci_dev_list) {
>                if (list_entry(p, struct hci_dev, list)->id > id)
> /* just replacing != with > */
>                        break;
>                head = p; id++;
>        }
>
> We assume id starts with the number we'll try to add the new device
> and keep iterating until we find the proper place. The only difference
> is we start with 0 for BR/EDR and 1 for AMP devices. Then every
> hdev->id in the _ordered_ list <= to the id we want we increment id
> and move head. In the end we'll have id as the first available one and
> head is where you need to add hdev (before head) to the list to keep
> it ordered. Right? Does that work for you? Am I missing anything here?

Please better send a patch ;) Either way is OK for me.

Regards,
Andrei

Re: [PATCHv3 1/2] Bluetooth: Fix registering hci with duplicate name

[Ulisses Furquim]

Hi Andrei,

On Fri, Apr 13, 2012 at 3:39 PM, Andrei Emeltchenko
<andrei.emeltchenko.news@gmail.com> wrote:
> Hi Ulisses,
>
> On Fri, Apr 13, 2012 at 7:25 PM, Ulisses Furquim <ulisses@profusion.mobi> wrote:
>
>> On Fri, Apr 13, 2012 at 8:21 AM, Andrei Emeltchenko
>> <Andrei.Emeltchenko.news@gmail.com> wrote:
>>>
>>> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>>>
>>> When adding HCI devices hci_register_dev may assigns the same name
>>> hciX for subsequently added HCI devices. Use bitmask to find free
>>> device ID the same way as it is done in other subsystems.
>>>
>>> ...
>>> [ 6958.381886] sysfs: cannot create duplicate filename
>>>        '/devices/virtual/bluetooth/hci1
>> Well, I don't like this bitmask approach, really. We used to have this
>> list ordered and the code was doing that before we added the
>> restriction of index 0 for AMP devices, I think.
>
> I think it just was not tested enough.

Probably.

>> Using a bitmask looks
>> overkill to me when we can probably modify the loop to find the first
>
> This approach is used in many places when choosing device id, specifically
> in network device code, so I just took it as I do not like to reinvent
> the wheel.

I know and I agree not reinventing the wheel but AFAIK they use
bitmask when they don't have the list and thus mark everything and
then extract the information they need afterwards. Our case is very
simple, we do have the list already and the code doesn't run often so
it looks overkill to me the bitmask approach.

>> available ID. Marcel even suggested an approach with a pseudo code
>> that seems to be the best way to go. Something along the lines of:
>>
>>        list_for_each(p, &hci_dev_list) {
>>                if (list_entry(p, struct hci_dev, list)->id > id)
>> /* just replacing != with > */
>>                        break;
>>                head = p; id++;
>>        }
>>
>> We assume id starts with the number we'll try to add the new device
>> and keep iterating until we find the proper place. The only difference
>> is we start with 0 for BR/EDR and 1 for AMP devices. Then every
>> hdev->id in the _ordered_ list <= to the id we want we increment id
>> and move head. In the end we'll have id as the first available one and
>> head is where you need to add hdev (before head) to the list to keep
>> it ordered. Right? Does that work for you? Am I missing anything here?
>
> Please better send a patch ;) Either way is OK for me.

Did it work for you? Sorry, couldn't test it here and that's why I
just sent comments and asked if I wasn't missing anything. :-) But I
can try to send a patch later today, just in case.

Best regards,


-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs