[PATCH] Bluetooth: Fix registering HCI devices with duplicate name

[PATCH] Bluetooth: Fix registering HCI devices with duplicate name

[Marcel Holtmann]

When adding HCI devices hci_register_dev assigns the same name
hci1 for subsequently added AMP devices.

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

Reported-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/hci_core.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 52c7abf..4becd28 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1741,7 +1741,7 @@ int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
 int hci_register_dev(struct hci_dev *hdev)
 {
 	struct list_head *head = &hci_dev_list, *p;
-	int i, id, error;
+	int i, error, min_id, id = 0;
 
 	BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
 
@@ -1751,13 +1751,14 @@ int hci_register_dev(struct hci_dev *hdev)
 	/* 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;
+	min_id = (hdev->dev_type == HCI_BREDR) ? 0 : 1;
 
 	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)
+		if (min_id >= id &&
+		    list_entry(p, struct hci_dev, list)->id != id)
 			break;
 		head = p; id++;
 	}
-- 
1.7.7.6

Re: [PATCH] Bluetooth: Fix registering HCI devices with duplicate name

[Andrei Emeltchenko]

Hi Marcel,

On Wed, Apr 11, 2012 at 05:32:08PM +0200, Marcel Holtmann wrote:
> When adding HCI devices hci_register_dev assigns the same name
> hci1 for subsequently added AMP devices.
> 
> ...
> [ 6958.381886] sysfs: cannot create duplicate filename
>         '/devices/virtual/bluetooth/hci1
> ...
> 
> Reported-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
>  net/bluetooth/hci_core.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 52c7abf..4becd28 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1741,7 +1741,7 @@ int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
>  int hci_register_dev(struct hci_dev *hdev)
>  {
>  	struct list_head *head = &hci_dev_list, *p;
> -	int i, id, error;
> +	int i, error, min_id, id = 0;
>  
>  	BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
>  
> @@ -1751,13 +1751,14 @@ int hci_register_dev(struct hci_dev *hdev)
>  	/* 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;
> +	min_id = (hdev->dev_type == HCI_BREDR) ? 0 : 1;
>  
>  	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)
> +		if (min_id >= id &&
> +		    list_entry(p, struct hci_dev, list)->id != id)
>  			break;

I have tested a bit and sometimes this does not work, apparently you need
to change

<------8<------------------------------------
|
|@@ -1813,7 +1813,7 @@ int hci_register_dev(struct hci_dev *hdev)
| 
|          sprintf(hdev->name, "hci%d", id);
|          hdev->id = id;
|  -       list_add_tail(&hdev->list, head);
|  +       list_add(&hdev->list, head);
|
|          mutex_init(&hdev->lock);
|
<------8<------------------------------------

Otherwise it does not allow to create second AMP:

[   97.055459] Create virtual AMP device
[   97.083333] Bluetooth: min_id 1 id 1
[  180.063159] Create virtual AMP device
[  180.140567] Bluetooth: min_id 1 id 0
[  180.182268] ------------[ cut here ]------------
[  180.200463] WARNING: at fs/sysfs/dir.c:508 sysfs_add_one+0x9b/0xd0()
[  180.253814] sysfs: cannot create duplicate filename
'/class/bluetooth/hci0'

But even with this I still get sometimes errors below after
adding/removing virtual AMPs.

[  219.358959] Bluetooth: min_id 1 id 4
[  219.367534] ------------[ cut here ]------------
[  219.373130] WARNING: at fs/sysfs/dir.c:508 sysfs_add_one+0x9b/0xd0()
[  219.386933] sysfs: cannot create duplicate filename
'/devices/virtual/bluetooth/hci4'


BTW: Have you checked my patch version 3.

Best regards 
Andrei Emeltchenko 

Re: [PATCH] Bluetooth: Fix registering HCI devices with duplicate name

[Marcel Holtmann]

Hi Andrei,

> > When adding HCI devices hci_register_dev assigns the same name
> > hci1 for subsequently added AMP devices.
> > 
> > ...
> > [ 6958.381886] sysfs: cannot create duplicate filename
> >         '/devices/virtual/bluetooth/hci1
> > ...
> > 
> > Reported-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> > ---
> >  net/bluetooth/hci_core.c |    7 ++++---
> >  1 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 52c7abf..4becd28 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -1741,7 +1741,7 @@ int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
> >  int hci_register_dev(struct hci_dev *hdev)
> >  {
> >  	struct list_head *head = &hci_dev_list, *p;
> > -	int i, id, error;
> > +	int i, error, min_id, id = 0;
> >  
> >  	BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
> >  
> > @@ -1751,13 +1751,14 @@ int hci_register_dev(struct hci_dev *hdev)
> >  	/* 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;
> > +	min_id = (hdev->dev_type == HCI_BREDR) ? 0 : 1;
> >  
> >  	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)
> > +		if (min_id >= id &&
> > +		    list_entry(p, struct hci_dev, list)->id != id)
> >  			break;
> 
> I have tested a bit and sometimes this does not work, apparently you need
> to change
> 
> <------8<------------------------------------
> |
> |@@ -1813,7 +1813,7 @@ int hci_register_dev(struct hci_dev *hdev)
> | 
> |          sprintf(hdev->name, "hci%d", id);
> |          hdev->id = id;
> |  -       list_add_tail(&hdev->list, head);
> |  +       list_add(&hdev->list, head);
> |
> |          mutex_init(&hdev->lock);
> |
> <------8<------------------------------------
>
> Otherwise it does not allow to create second AMP:
> 
> [   97.055459] Create virtual AMP device
> [   97.083333] Bluetooth: min_id 1 id 1
> [  180.063159] Create virtual AMP device
> [  180.140567] Bluetooth: min_id 1 id 0
> [  180.182268] ------------[ cut here ]------------
> [  180.200463] WARNING: at fs/sysfs/dir.c:508 sysfs_add_one+0x9b/0xd0()
> [  180.253814] sysfs: cannot create duplicate filename
> '/class/bluetooth/hci0'

that makes sense. Since if the list is not ordered, we are obviously
failing here to pick the right id. So it is important that we keep our
list ordered.

> But even with this I still get sometimes errors below after
> adding/removing virtual AMPs.
> 
> [  219.358959] Bluetooth: min_id 1 id 4
> [  219.367534] ------------[ cut here ]------------
> [  219.373130] WARNING: at fs/sysfs/dir.c:508 sysfs_add_one+0x9b/0xd0()
> [  219.386933] sysfs: cannot create duplicate filename
> '/devices/virtual/bluetooth/hci4'

Can you print out the order of the list when this happens. Something is
fishy here. And I bet it is broken for a while now. Just that nobody has
really noticed yet.

> BTW: Have you checked my patch version 3.

If we have to run through that list twice, then we are doing something
wrong. Also having to run through the whole list seems pointless. We
just need to make sure that the list stays ordered.

Regards

Marcel

Re: [PATCH] Bluetooth: Fix registering HCI devices with duplicate name

[Andrei Emeltchenko]

Hi Marcel

On Thu, Apr 12, 2012 at 11:50:08AM +0200, Marcel Holtmann wrote:
> > > @@ -1751,13 +1751,14 @@ int hci_register_dev(struct hci_dev *hdev)
> > >  	/* 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;
> > > +	min_id = (hdev->dev_type == HCI_BREDR) ? 0 : 1;
> > >  
> > >  	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)
> > > +		if (min_id >= id &&
> > > +		    list_entry(p, struct hci_dev, list)->id != id)
> > >  			break;
> > 
> > I have tested a bit and sometimes this does not work, apparently you need
> > to change
> > 
> > <------8<------------------------------------
> > |
> > |@@ -1813,7 +1813,7 @@ int hci_register_dev(struct hci_dev *hdev)
> > | 
> > |          sprintf(hdev->name, "hci%d", id);
> > |          hdev->id = id;
> > |  -       list_add_tail(&hdev->list, head);
> > |  +       list_add(&hdev->list, head);
> > |
> > |          mutex_init(&hdev->lock);
> > |
> > <------8<------------------------------------
> >
> > Otherwise it does not allow to create second AMP:
> > 
> > [   97.055459] Create virtual AMP device
> > [   97.083333] Bluetooth: min_id 1 id 1
> > [  180.063159] Create virtual AMP device
> > [  180.140567] Bluetooth: min_id 1 id 0
> > [  180.182268] ------------[ cut here ]------------
> > [  180.200463] WARNING: at fs/sysfs/dir.c:508 sysfs_add_one+0x9b/0xd0()
> > [  180.253814] sysfs: cannot create duplicate filename
> > '/class/bluetooth/hci0'
> 
> that makes sense. Since if the list is not ordered, we are obviously
> failing here to pick the right id. So it is important that we keep our
> list ordered.
> 
> > But even with this I still get sometimes errors below after
> > adding/removing virtual AMPs.
> > 
> > [  219.358959] Bluetooth: min_id 1 id 4
> > [  219.367534] ------------[ cut here ]------------
> > [  219.373130] WARNING: at fs/sysfs/dir.c:508 sysfs_add_one+0x9b/0xd0()
> > [  219.386933] sysfs: cannot create duplicate filename
> > '/devices/virtual/bluetooth/hci4'
> 
> Can you print out the order of the list when this happens. Something is
> fishy here. And I bet it is broken for a while now. Just that nobody has
> really noticed yet.

I had hci0...hci4 then removed hci3 and created again and it chooses hci4.
This is because logic is not smart and just checks hdev->id != id

[  162.994073] Create virtual AMP device
[  163.018114] Bluetooth: min_id 1 id 3
[  166.452585] Create virtual AMP device
[  166.508589] Bluetooth: min_id 1 id 4
[  197.062710] Clean up virtual AMP device => hci3
[  202.391750] Create virtual AMP device
[  202.425382] Bluetooth: min_id 1 id 4
[  202.428386] ------------[ cut here ]------------
[  202.431338] WARNING: at fs/sysfs/dir.c:508 sysfs_add_one+0x9b/0xd0()
[  202.541644] sysfs: cannot create duplicate filename
'/devices/virtual/bluetooth/hci4'

> > BTW: Have you checked my patch version 3.
> 
> If we have to run through that list twice, then we are doing something
> wrong. Also having to run through the whole list seems pointless. We
> just need to make sure that the list stays ordered.

We are not running through the whole list, just until found free id.

Best regards 
Andrei Emeltchenko 

Re: [PATCH] Bluetooth: Fix registering HCI devices with duplicate name

[Marcel Holtmann]

Hi Andrei,

> > > > @@ -1751,13 +1751,14 @@ int hci_register_dev(struct hci_dev *hdev)
> > > >  	/* 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;
> > > > +	min_id = (hdev->dev_type == HCI_BREDR) ? 0 : 1;
> > > >  
> > > >  	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)
> > > > +		if (min_id >= id &&
> > > > +		    list_entry(p, struct hci_dev, list)->id != id)
> > > >  			break;
> > > 
> > > I have tested a bit and sometimes this does not work, apparently you need
> > > to change
> > > 
> > > <------8<------------------------------------
> > > |
> > > |@@ -1813,7 +1813,7 @@ int hci_register_dev(struct hci_dev *hdev)
> > > | 
> > > |          sprintf(hdev->name, "hci%d", id);
> > > |          hdev->id = id;
> > > |  -       list_add_tail(&hdev->list, head);
> > > |  +       list_add(&hdev->list, head);
> > > |
> > > |          mutex_init(&hdev->lock);
> > > |
> > > <------8<------------------------------------
> > >
> > > Otherwise it does not allow to create second AMP:
> > > 
> > > [   97.055459] Create virtual AMP device
> > > [   97.083333] Bluetooth: min_id 1 id 1
> > > [  180.063159] Create virtual AMP device
> > > [  180.140567] Bluetooth: min_id 1 id 0
> > > [  180.182268] ------------[ cut here ]------------
> > > [  180.200463] WARNING: at fs/sysfs/dir.c:508 sysfs_add_one+0x9b/0xd0()
> > > [  180.253814] sysfs: cannot create duplicate filename
> > > '/class/bluetooth/hci0'
> > 
> > that makes sense. Since if the list is not ordered, we are obviously
> > failing here to pick the right id. So it is important that we keep our
> > list ordered.
> > 
> > > But even with this I still get sometimes errors below after
> > > adding/removing virtual AMPs.
> > > 
> > > [  219.358959] Bluetooth: min_id 1 id 4
> > > [  219.367534] ------------[ cut here ]------------
> > > [  219.373130] WARNING: at fs/sysfs/dir.c:508 sysfs_add_one+0x9b/0xd0()
> > > [  219.386933] sysfs: cannot create duplicate filename
> > > '/devices/virtual/bluetooth/hci4'
> > 
> > Can you print out the order of the list when this happens. Something is
> > fishy here. And I bet it is broken for a while now. Just that nobody has
> > really noticed yet.
> 
> I had hci0...hci4 then removed hci3 and created again and it chooses hci4.
> This is because logic is not smart and just checks hdev->id != id
> 
> [  162.994073] Create virtual AMP device
> [  163.018114] Bluetooth: min_id 1 id 3
> [  166.452585] Create virtual AMP device
> [  166.508589] Bluetooth: min_id 1 id 4
> [  197.062710] Clean up virtual AMP device => hci3
> [  202.391750] Create virtual AMP device
> [  202.425382] Bluetooth: min_id 1 id 4
> [  202.428386] ------------[ cut here ]------------
> [  202.431338] WARNING: at fs/sysfs/dir.c:508 sysfs_add_one+0x9b/0xd0()
> [  202.541644] sysfs: cannot create duplicate filename
> '/devices/virtual/bluetooth/hci4'

then lets fix the logic we have right now. It can not be that hard to
have this behave correctly. Find the proper next free id and find the
right location in the list to add it to.

> > > BTW: Have you checked my patch version 3.
> > 
> > If we have to run through that list twice, then we are doing something
> > wrong. Also having to run through the whole list seems pointless. We
> > just need to make sure that the list stays ordered.
> 
> We are not running through the whole list, just until found free id.

You are running the whole list for the bitmask creation and then a
second time for finding the right location in the list. Fair enough that
the second time you only run until you found a free spot, but
nevertheless, you run it again.

Regards

Marcel

Re: [PATCH] Bluetooth: Fix registering HCI devices with duplicate name

[Andrei Emeltchenko]

Hi Marcel,

On Thu, Apr 12, 2012 at 12:15:14PM +0200, Marcel Holtmann wrote:
> then lets fix the logic we have right now. It can not be that hard to
> have this behave correctly. Find the proper next free id and find the
> right location in the list to add it to.

We are highly dependent here on ordered list which is not good IMO,
especially given that BR/EDR shall be first element with id=0. This of
course works well in default situation when we have only 2 controllers.

> > > > BTW: Have you checked my patch version 3.
> > > 
> > > If we have to run through that list twice, then we are doing something
> > > wrong. Also having to run through the whole list seems pointless. We
> > > just need to make sure that the list stays ordered.
> > 
> > We are not running through the whole list, just until found free id.
> 
> You are running the whole list for the bitmask creation and then a
> second time for finding the right location in the list. Fair enough that

Second run is not needed, we can just add to the tail of the list. In most
cases list will still be ordered and this does not matter since we do not
depend on ordered list.

Best regards 
Andrei Emeltchenko 

> the second time you only run until you found a free spot, but
> nevertheless, you run it again.
> 
> Regards
> 
> Marcel
> 
> 

Re: [PATCH] Bluetooth: Fix registering HCI devices with duplicate name

[Marcel Holtmann]

Hi Andrei,

> > then lets fix the logic we have right now. It can not be that hard to
> > have this behave correctly. Find the proper next free id and find the
> > right location in the list to add it to.
> 
> We are highly dependent here on ordered list which is not good IMO,
> especially given that BR/EDR shall be first element with id=0. This of
> course works well in default situation when we have only 2 controllers.
> 
> > > > > BTW: Have you checked my patch version 3.
> > > > 
> > > > If we have to run through that list twice, then we are doing something
> > > > wrong. Also having to run through the whole list seems pointless. We
> > > > just need to make sure that the list stays ordered.
> > > 
> > > We are not running through the whole list, just until found free id.
> > 
> > You are running the whole list for the bitmask creation and then a
> > second time for finding the right location in the list. Fair enough that
> 
> Second run is not needed, we can just add to the tail of the list. In most
> cases list will still be ordered and this does not matter since we do not
> depend on ordered list.

can we just ensure we have an ordered list and fix this.

Regards

Marcel

[PATCH] Bluetooth: Fix registering HCI devices 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. This is modified version
of Marcel Holtmann patch.

...
[ 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 |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 52c7abf..99e93cd 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1741,7 +1741,7 @@ int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
 int hci_register_dev(struct hci_dev *hdev)
 {
 	struct list_head *head = &hci_dev_list, *p;
-	int i, id, error;
+	int i, error, min_id, id = 0;
 
 	BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
 
@@ -1751,20 +1751,22 @@ int hci_register_dev(struct hci_dev *hdev)
 	/* 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;
+	min_id = (hdev->dev_type == HCI_BREDR) ? 0 : 1;
 
 	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)
+		struct hci_dev *hdev = list_entry(p, struct hci_dev, list);
+		if (min_id <= id && hdev->id != id)
 			break;
+
 		head = p; id++;
 	}
 
 	sprintf(hdev->name, "hci%d", id);
 	hdev->id = id;
-	list_add_tail(&hdev->list, head);
+	list_add(&hdev->list, head);
 
 	mutex_init(&hdev->lock);
 
-- 
1.7.9.1

Re: [PATCH] Bluetooth: Fix registering HCI devices with duplicate name

[Andrei Emeltchenko]

On Thu, Apr 12, 2012 at 02:39:57PM +0300, Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> When adding HCI devices hci_register_dev assigns the same name
> hci1 for subsequently added AMP devices. This is modified version
> of Marcel Holtmann patch.
> 
> ...
> [ 6958.381886] sysfs: cannot create duplicate filename
>         '/devices/virtual/bluetooth/hci1

Please do not consider this patch. I think bitmask approach is better.

Best regards 
Andrei Emeltchenko