[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

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 

[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: [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 

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

[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

[PATCH] Bluetooth: Fix registering hci with duplicate name

[Ulisses Furquim]

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
...

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 device and 1 for AMP devices (thus AMP
devices will never receive register as index 0). Then every hdev->id in
the _ordered_ list <= to the id we want we increment id and move the
variable head. In the end we'll have id as the first available one and
head is where you need to add hdev after to keep the list ordered.

Reported-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Signed-off-by: Ulisses Furquim <ulisses@profusion.mobi>
---

Andrei, it'd be good if you could test it with AMP and add a tested-by as well, please.

Thanks.

---
 net/bluetooth/hci_core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index c4dc263..f24d3d8 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1754,7 +1754,7 @@ int hci_register_dev(struct hci_dev *hdev)
 
 	/* Find first available device id */
 	list_for_each(p, &hci_dev_list) {
-		if (list_entry(p, struct hci_dev, list)->id != id)
+		if (list_entry(p, struct hci_dev, list)->id > id)
 			break;
 		head = p; id++;
 	}
-- 
1.7.10

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

[Andrei Emeltchenko]

Hi Ulisses,

Ulisses Furquim <ulisses@profusion.mobi> написал(а):

>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
>...
>
>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 device and 1 for AMP devices (thus AMP
>devices will never receive register as index 0). Then every hdev->id in
>the _ordered_ list <= to the id we want we increment id and move the
>variable head. In the end we'll have id as the first available one and
>head is where you need to add hdev after to keep the list ordered.
>
>Reported-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>Signed-off-by: Ulisses Furquim <ulisses@profusion.mobi>
>---
>
>Andrei, it'd be good if you could test it with AMP and add a tested-by
>as well, please.
>
>Thanks.
>
>---
> net/bluetooth/hci_core.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>index c4dc263..f24d3d8 100644
>--- a/net/bluetooth/hci_core.c
>+++ b/net/bluetooth/hci_core.c
>@@ -1754,7 +1754,7 @@ int hci_register_dev(struct hci_dev *hdev)
> 
> 	/* Find first available device id */
> 	list_for_each(p, &hci_dev_list) {
>-		if (list_entry(p, struct hci_dev, list)->id != id)
>+		if (list_entry(p, struct hci_dev, list)->id > id)
> 			break;
> 		head = p; id++;

Without testing I feel that it would not work. If you register first AMP then bredr it would have the same id.

Regards, Andrei

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

[Ulisses Furquim]

Hi Andrei,

On Mon, Apr 16, 2012 at 6:44 PM, Andrei Emeltchenko
<andrei.emeltchenko.news@gmail.com> wrote:
> Hi Ulisses,
>
> Ulisses Furquim <ulisses@profusion.mobi> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=
=D0=B0=D0=BB(=D0=B0):
>
>>When adding HCI devices hci_register_dev assigns the same name
>>hci1 for subsequently added AMP devices.
>>
>>...
>>[ 6958.381886] sysfs: cannot create duplicate filename
>> =C2=A0 =C2=A0 =C2=A0 '/devices/virtual/bluetooth/hci1
>>...
>>
>>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 device and 1 for AMP devices (thus AMP
>>devices will never receive register as index 0). Then every hdev->id in
>>the _ordered_ list <=3D to the id we want we increment id and move the
>>variable head. In the end we'll have id as the first available one and
>>head is where you need to add hdev after to keep the list ordered.
>>
>>Reported-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>>Signed-off-by: Ulisses Furquim <ulisses@profusion.mobi>
>>---
>>
>>Andrei, it'd be good if you could test it with AMP and add a tested-by
>>as well, please.
>>
>>Thanks.
>>
>>---
>> net/bluetooth/hci_core.c | =C2=A0 =C2=A02 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>index c4dc263..f24d3d8 100644
>>--- a/net/bluetooth/hci_core.c
>>+++ b/net/bluetooth/hci_core.c
>>@@ -1754,7 +1754,7 @@ int hci_register_dev(struct hci_dev *hdev)
>>
>> =C2=A0 =C2=A0 =C2=A0 /* Find first available device id */
>> =C2=A0 =C2=A0 =C2=A0 list_for_each(p, &hci_dev_list) {
>>- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (list_entry(p, struc=
t hci_dev, list)->id !=3D id)
>>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (list_entry(p, struc=
t hci_dev, list)->id > id)
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 break;
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 head =3D p; id++;
>
> Without testing I feel that it would not work. If you register first AMP =
then bredr it would have the same id.

Hmm. First AMP will receive id 1, right? The first bredr will be tried
with id 0 and find the id 1 already there and break the loop and
insert hci0 before hci1 in the list, right? I still think it'll work.

Regards,

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

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

[Ulisses Furquim]

Hi Andrei,

On Mon, Apr 16, 2012 at 7:07 PM, Ulisses Furquim <ulisses@profusion.mobi> w=
rote:
> Hi Andrei,
>
> On Mon, Apr 16, 2012 at 6:44 PM, Andrei Emeltchenko
> <andrei.emeltchenko.news@gmail.com> wrote:
>> Hi Ulisses,
>>
>> Ulisses Furquim <ulisses@profusion.mobi> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=
=D0=B0=D0=BB(=D0=B0):
>>
>>>When adding HCI devices hci_register_dev assigns the same name
>>>hci1 for subsequently added AMP devices.
>>>
>>>...
>>>[ 6958.381886] sysfs: cannot create duplicate filename
>>> =C2=A0 =C2=A0 =C2=A0 '/devices/virtual/bluetooth/hci1
>>>...
>>>
>>>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 device and 1 for AMP devices (thus AMP
>>>devices will never receive register as index 0). Then every hdev->id in
>>>the _ordered_ list <=3D to the id we want we increment id and move the
>>>variable head. In the end we'll have id as the first available one and
>>>head is where you need to add hdev after to keep the list ordered.
>>>
>>>Reported-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>>>Signed-off-by: Ulisses Furquim <ulisses@profusion.mobi>
>>>---
>>>
>>>Andrei, it'd be good if you could test it with AMP and add a tested-by
>>>as well, please.
>>>
>>>Thanks.
>>>
>>>---
>>> net/bluetooth/hci_core.c | =C2=A0 =C2=A02 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>>diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>>index c4dc263..f24d3d8 100644
>>>--- a/net/bluetooth/hci_core.c
>>>+++ b/net/bluetooth/hci_core.c
>>>@@ -1754,7 +1754,7 @@ int hci_register_dev(struct hci_dev *hdev)
>>>
>>> =C2=A0 =C2=A0 =C2=A0 /* Find first available device id */
>>> =C2=A0 =C2=A0 =C2=A0 list_for_each(p, &hci_dev_list) {
>>>- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (list_entry(p, stru=
ct hci_dev, list)->id !=3D id)
>>>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (list_entry(p, stru=
ct hci_dev, list)->id > id)
>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 break;
>>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 head =3D p; id++;
>>
>> Without testing I feel that it would not work. If you register first AMP=
 then bredr it would have the same id.
>
> Hmm. First AMP will receive id 1, right? The first bredr will be tried
> with id 0 and find the id 1 already there and break the loop and
> insert hci0 before hci1 in the list, right? I still think it'll work.

Now that I think, I see a problem, but different than you said. It's
easily fixed, though.

 =C2=A0 =C2=A0 =C2=A0 list_for_each(p, &hci_dev_list) {
              int nid =3D list_entry(p, struct hci_dev, list)->id;
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (nid > id)
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 break;
              if (nid =3D=3D id)
                       id++;
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 head =3D p;
       }

Now we solve the problem of adding first bredr and then an AMP device
which had to result in hci0 and then hci1, but with the previous code
was resulting in hci0 and hci2. Please, if you could test it'd be
great.

Thanks,
Regards,

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