[PATCH] Bluetooth: Fix registering hci with duplicate name

[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

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

[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