[PATCH] Bluetooth: Fix registering hci with duplicate name

[PATCH] 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 |   43 ++++++++++++++++++++++++++++---------------
 1 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 52c7abf..7ea7a02 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1737,10 +1737,35 @@ int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
 	return 0;
 }
 
+/* Invoked locked */
+static int __hci_alloc_name(struct hci_dev *hdev)
+{
+	struct hci_dev *d;
+	unsigned long inuse = 0;
+	int i, id;
+
+	list_for_each_entry(d, &hci_dev_list, list) {
+		set_bit(d->id, &inuse);
+	}
+
+	i = find_first_zero_bit(&inuse, sizeof(inuse));
+
+	/* 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;
+
+	id = max_t(int, i, id);
+
+	sprintf(hdev->name, "hci%d", id);
+	hdev->id = id;
+
+	return id;
+}
+
 /* Register HCI device */
 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", hdev, hdev->name, hdev->bus);
@@ -1748,23 +1773,11 @@ int hci_register_dev(struct hci_dev *hdev)
 	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;
-
 	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++;
-	}
+	id = __hci_alloc_name(hdev);
 
-	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

Re: [PATCH] Bluetooth: Fix registering hci with duplicate name

[Marcel Holtmann]

Hi Andrei,

> 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 |   43 ++++++++++++++++++++++++++++---------------
>  1 files changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 52c7abf..7ea7a02 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1737,10 +1737,35 @@ int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
>  	return 0;
>  }
>  
> +/* Invoked locked */
> +static int __hci_alloc_name(struct hci_dev *hdev)
> +{

I think the name is pretty bad since it does not do any memory
allocation here.

> +	struct hci_dev *d;
> +	unsigned long inuse = 0;
> +	int i, id;
> +
> +	list_for_each_entry(d, &hci_dev_list, list) {
> +		set_bit(d->id, &inuse);
> +	}
> +
> +	i = find_first_zero_bit(&inuse, sizeof(inuse));
> +
> +	/* 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;
> +
> +	id = max_t(int, i, id);
> +
> +	sprintf(hdev->name, "hci%d", id);
> +	hdev->id = id;
> +
> +	return id;
> +}
> +
>  /* Register HCI device */
>  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", hdev, hdev->name, hdev->bus);
> @@ -1748,23 +1773,11 @@ int hci_register_dev(struct hci_dev *hdev)
>  	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;
> -
>  	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++;
> -	}
> +	id = __hci_alloc_name(hdev);
>  
> -	sprintf(hdev->name, "hci%d", id);
> -	hdev->id = id;
> -	list_add_tail(&hdev->list, head);
> +	list_add_tail(&hdev->list, &hci_dev_list);

Do we really need a separate function here. Can you just not fix this
inline?

Regards

Marcel

[PATCHv2] 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 |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 52c7abf..eaa6876 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1740,8 +1740,9 @@ 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 = 0;
 
 	BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
 
@@ -1755,16 +1756,17 @@ int hci_register_dev(struct hci_dev *hdev)
 
 	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++;
-	}
+	list_for_each_entry(d, &hci_dev_list, list)
+		set_bit(d->id, &inuse);
+
+	i = find_first_zero_bit(&inuse, sizeof(inuse));
+
+	id = max_t(int, i, id);
 
 	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

Re: [PATCH] Bluetooth: Fix registering hci with duplicate name

[Andrei Emeltchenko]

Hi Marcel,

On Wed, Apr 11, 2012 at 11:18:03AM +0200, Marcel Holtmann wrote:
> 
> Do we really need a separate function here. Can you just not fix this
> inline?

I have sent a new version, BTW is it OK to use unsigned long for bitmask? 
In netdev they use a page.

Best regards 
Andrei Emeltchenko 

Re: [PATCHv2] Bluetooth: Fix registering hci with duplicate name

[Marcel Holtmann]

Hi Andrei,

> 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 |   18 ++++++++++--------
>  1 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 52c7abf..eaa6876 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1740,8 +1740,9 @@ 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 = 0;
>  
>  	BT_DBG("%p name %s bus %d", hdev, hdev->name, hdev->bus);
>  
> @@ -1755,16 +1756,17 @@ int hci_register_dev(struct hci_dev *hdev)
>  
>  	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++;
> -	}
> +	list_for_each_entry(d, &hci_dev_list, list)
> +		set_bit(d->id, &inuse);
> +
> +	i = find_first_zero_bit(&inuse, sizeof(inuse));
> +
> +	id = max_t(int, i, id);

I am not 100% convinced that this is the best way to fix this. Not that
anybody will attach hundreds of controllers, but it is in theory
possible.

>  
>  	sprintf(hdev->name, "hci%d", id);
>  	hdev->id = id;
> -	list_add_tail(&hdev->list, head);
> +
> +	list_add_tail(&hdev->list, &hci_dev_list);

I am now a little bit confused. Is it not enough to just replace head
with &hci_dev_list to get this fixed? Or why is this failing in the
first place actually.

Also this change will of course break the fix that we added to ensure
that the HCI list comes sorted when calling hciconfig.

Regards

Marcel

Re: [PATCHv2] Bluetooth: Fix registering hci with duplicate name

[Andrei Emeltchenko]

On Wed, Apr 11, 2012 at 11:52:06AM +0200, Marcel Holtmann wrote:
> > @@ -1755,16 +1756,17 @@ int hci_register_dev(struct hci_dev *hdev)
> >  
> >  	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++;
> > -	}
> I am now a little bit confused. Is it not enough to just replace head
> with &hci_dev_list to get this fixed? Or why is this failing in the
> first place actually.

You can see actual code above. If you have hci0 and hci1 adding third AMP
will fail since it just checks 0!=1 => break and trying to create hci1
again.

> Also this change will of course break the fix that we added to ensure
> that the HCI list comes sorted when calling hciconfig.

Actually it still works. But if we want it to stay ordered after
deleting/adding then we need a small fix.

Best regards 
Andrei Emeltchenko 

Re: [PATCHv2] Bluetooth: Fix registering hci with duplicate name

[Marcel Holtmann]

Hi Andrei,

> > > @@ -1755,16 +1756,17 @@ int hci_register_dev(struct hci_dev *hdev)
> > >  
> > >  	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++;
> > > -	}
> > I am now a little bit confused. Is it not enough to just replace head
> > with &hci_dev_list to get this fixed? Or why is this failing in the
> > first place actually.
> 
> You can see actual code above. If you have hci0 and hci1 adding third AMP
> will fail since it just checks 0!=1 => break and trying to create hci1
> again.

I see a problem when you having only AMPs (no BR/EDR controller). Then
hci0 will be skipped and keep trying to create hci1 over and over again.

However in the case we have BR/EDR controller on hci0 and hci1 as AMP,
then this should just work.

So we need to fix the case where we have no BR/EDR controller on hci0
and trying to fix something else. Or did I get confused?

> > Also this change will of course break the fix that we added to ensure
> > that the HCI list comes sorted when calling hciconfig.
> 
> Actually it still works. But if we want it to stay ordered after
> deleting/adding then we need a small fix.

We added a fix for it to keep them sorted. So we might should retain
that behavior from now on.

Regards

Marcel

Re: [PATCHv2] Bluetooth: Fix registering hci with duplicate name

[Andrei Emeltchenko]

Hi Marcel,

On Wed, Apr 11, 2012 at 12:20:37PM +0200, Marcel Holtmann wrote:
> Hi Andrei,
> 
> > > > @@ -1755,16 +1756,17 @@ int hci_register_dev(struct hci_dev *hdev)
> > > >  
> > > >  	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++;
> > > > -	}
> > > I am now a little bit confused. Is it not enough to just replace head
> > > with &hci_dev_list to get this fixed? Or why is this failing in the
> > > first place actually.
> > 
> > You can see actual code above. If you have hci0 and hci1 adding third AMP
> > will fail since it just checks 0!=1 => break and trying to create hci1
> > again.
> 
> I see a problem when you having only AMPs (no BR/EDR controller). Then
> hci0 will be skipped and keep trying to create hci1 over and over again.
> 
> However in the case we have BR/EDR controller on hci0 and hci1 as AMP,
> then this should just work.
> 
> So we need to fix the case where we have no BR/EDR controller on hci0
> and trying to fix something else. Or did I get confused?

I think you got confused. 

We have hci0 (BREDR) and hci1 (AMP)

so list_for_each would give us first entry with id=0; Adding third (AMP)
would compare 1 (as given by id = (hdev->dev_type == HCI_BREDR) ? 0 : 1)
and 0 (id for BREDR) => break from the loop and tries to use id=1 again
for third controller.

Actually I do not know how the current code works at all, it is really
broken.

> 
> > > Also this change will of course break the fix that we added to ensure
> > > that the HCI list comes sorted when calling hciconfig.
> > 
> > Actually it still works. But if we want it to stay ordered after
> > deleting/adding then we need a small fix.
> 
> We added a fix for it to keep them sorted. So we might should retain
> that behavior from now on.

will do that.

Best regards 
Andrei Emeltchenko 

Re: [PATCHv2] Bluetooth: Fix registering hci with duplicate name

[Marcel Holtmann]

Hi Andrei,

> > > > > @@ -1755,16 +1756,17 @@ int hci_register_dev(struct hci_dev *hdev)
> > > > >  
> > > > >  	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++;
> > > > > -	}
> > > > I am now a little bit confused. Is it not enough to just replace head
> > > > with &hci_dev_list to get this fixed? Or why is this failing in the
> > > > first place actually.
> > > 
> > > You can see actual code above. If you have hci0 and hci1 adding third AMP
> > > will fail since it just checks 0!=1 => break and trying to create hci1
> > > again.
> > 
> > I see a problem when you having only AMPs (no BR/EDR controller). Then
> > hci0 will be skipped and keep trying to create hci1 over and over again.
> > 
> > However in the case we have BR/EDR controller on hci0 and hci1 as AMP,
> > then this should just work.
> > 
> > So we need to fix the case where we have no BR/EDR controller on hci0
> > and trying to fix something else. Or did I get confused?
> 
> I think you got confused. 
> 
> We have hci0 (BREDR) and hci1 (AMP)
> 
> so list_for_each would give us first entry with id=0; Adding third (AMP)
> would compare 1 (as given by id = (hdev->dev_type == HCI_BREDR) ? 0 : 1)
> and 0 (id for BREDR) => break from the loop and tries to use id=1 again
> for third controller.
> 
> Actually I do not know how the current code works at all, it is really
> broken.

so it is broken on two levels actually. The case where we have no
BR/EDRI controller as hci0 is also broken.

Can you try to come up with a different solution that is not using a bit
mask. That one might overflow actually. Something like this:

	min_id = (hdev->dev_type == HCI_BREDR) ? 0 : 1)

	for each (...) {
		if (id >= min_id && hdev->id != id)
			break;

		...
	}

Regards

Marcel

Re: [PATCHv2] Bluetooth: Fix registering hci with duplicate name

[Andrei Emeltchenko]

Hi Marcel,

On Wed, Apr 11, 2012 at 12:48:37PM +0200, Marcel Holtmann wrote:
> > > > > > @@ -1755,16 +1756,17 @@ int hci_register_dev(struct hci_dev *hdev)
> > > > > >  
> > > > > >  	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++;
> > > > > > -	}
> > > > > I am now a little bit confused. Is it not enough to just replace head
> > > > > with &hci_dev_list to get this fixed? Or why is this failing in the
> > > > > first place actually.
> > > > 
> > > > You can see actual code above. If you have hci0 and hci1 adding third AMP
> > > > will fail since it just checks 0!=1 => break and trying to create hci1
> > > > again.
> > > 
> > > I see a problem when you having only AMPs (no BR/EDR controller). Then
> > > hci0 will be skipped and keep trying to create hci1 over and over again.
> > > 
> > > However in the case we have BR/EDR controller on hci0 and hci1 as AMP,
> > > then this should just work.
> > > 
> > > So we need to fix the case where we have no BR/EDR controller on hci0
> > > and trying to fix something else. Or did I get confused?
> > 
> > I think you got confused. 
> > 
> > We have hci0 (BREDR) and hci1 (AMP)
> > 
> > so list_for_each would give us first entry with id=0; Adding third (AMP)
> > would compare 1 (as given by id = (hdev->dev_type == HCI_BREDR) ? 0 : 1)
> > and 0 (id for BREDR) => break from the loop and tries to use id=1 again
> > for third controller.
> > 
> > Actually I do not know how the current code works at all, it is really
> > broken.
> 
> so it is broken on two levels actually. The case where we have no
> BR/EDRI controller as hci0 is also broken.
> 
> Can you try to come up with a different solution that is not using a bit
> mask. That one might overflow actually. Something like this:
> 
> 	min_id = (hdev->dev_type == HCI_BREDR) ? 0 : 1)
> 
> 	for each (...) {
> 		if (id >= min_id && hdev->id != id)
> 			break;
> 
> 		...
> 	}

I think that would not work. If we have for example hci0,hci1,hci2 and
adding another one would choose hci2 again since (2 > 1) && (hdev->id=1 !=
2)

otherwise I think this is a bit complex when compared to bitmask which is
used in netdev in dev_alloc_name but they use a page:

/* Use one page as a bit array of possible slots */
inuse = (unsigned long *) get_zeroed_page(GFP_ATOMIC);

BTW: what is max number of controllers we support?

Best regards 
Andrei Emeltchenko 

[PATCHv3] 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 |   31 +++++++++++++++++++++++--------
 1 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 52c7abf..e08d822 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1740,14 +1740,20 @@ 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 list_head *head = &hci_dev_list;
+	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;
 
+	inuse = (unsigned long *) get_zeroed_page(GFP_KERNEL);
+	if (!inuse)
+		return -ENOMEM;
+
 	/* Do not allow HCI_AMP devices to register at index 0,
 	 * so the index can be used as the AMP controller ID.
 	 */
@@ -1755,16 +1761,25 @@ int hci_register_dev(struct hci_dev *hdev)
 
 	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++;
-	}
+	list_for_each_entry(d, &hci_dev_list, list)
+		set_bit(d->id, inuse);
+
+	i = find_first_zero_bit(inuse, sizeof(*inuse));
+	free_page((unsigned long) inuse);
+
+	id = max_t(int, i, id);
 
 	sprintf(hdev->name, "hci%d", id);
 	hdev->id = id;
-	list_add_tail(&hdev->list, head);
+
+	/* keep hci_dev_list ordered */
+	list_for_each_entry(d, &hci_dev_list, list)
+		if (d->id + 1 == id) {
+			head = &d->list;
+			break;
+		}
+
+	list_add(&hdev->list, head);
 
 	mutex_init(&hdev->lock);
 
-- 
1.7.9.1

Re: [PATCHv2] Bluetooth: Fix registering hci with duplicate name

[Marcel Holtmann]

Hi Andrei,

> > > > > > > @@ -1755,16 +1756,17 @@ int hci_register_dev(struct hci_dev *hdev)
> > > > > > >  
> > > > > > >  	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++;
> > > > > > > -	}
> > > > > > I am now a little bit confused. Is it not enough to just replace head
> > > > > > with &hci_dev_list to get this fixed? Or why is this failing in the
> > > > > > first place actually.
> > > > > 
> > > > > You can see actual code above. If you have hci0 and hci1 adding third AMP
> > > > > will fail since it just checks 0!=1 => break and trying to create hci1
> > > > > again.
> > > > 
> > > > I see a problem when you having only AMPs (no BR/EDR controller). Then
> > > > hci0 will be skipped and keep trying to create hci1 over and over again.
> > > > 
> > > > However in the case we have BR/EDR controller on hci0 and hci1 as AMP,
> > > > then this should just work.
> > > > 
> > > > So we need to fix the case where we have no BR/EDR controller on hci0
> > > > and trying to fix something else. Or did I get confused?
> > > 
> > > I think you got confused. 
> > > 
> > > We have hci0 (BREDR) and hci1 (AMP)
> > > 
> > > so list_for_each would give us first entry with id=0; Adding third (AMP)
> > > would compare 1 (as given by id = (hdev->dev_type == HCI_BREDR) ? 0 : 1)
> > > and 0 (id for BREDR) => break from the loop and tries to use id=1 again
> > > for third controller.
> > > 
> > > Actually I do not know how the current code works at all, it is really
> > > broken.
> > 
> > so it is broken on two levels actually. The case where we have no
> > BR/EDRI controller as hci0 is also broken.
> > 
> > Can you try to come up with a different solution that is not using a bit
> > mask. That one might overflow actually. Something like this:
> > 
> > 	min_id = (hdev->dev_type == HCI_BREDR) ? 0 : 1)
> > 
> > 	for each (...) {
> > 		if (id >= min_id && hdev->id != id)
> > 			break;
> > 
> > 		...
> > 	}
> 
> I think that would not work. If we have for example hci0,hci1,hci2 and
> adding another one would choose hci2 again since (2 > 1) && (hdev->id=1 !=
> 2)
> 
> otherwise I think this is a bit complex when compared to bitmask which is
> used in netdev in dev_alloc_name but they use a page:
> 
> /* Use one page as a bit array of possible slots */
> inuse = (unsigned long *) get_zeroed_page(GFP_ATOMIC);

I think we should not go over board with allocating a page.

> BTW: what is max number of controllers we support?

uint16 - 1 number of controllers can be handled.

Regards

Marcel