Hey all,
I just had a discussion with Johan Hedberg on IRC about problems I am experiencing with
the hci-tester tool.
Basically what I do is:
$ sudo stop bluetooth
$ sudo hciconfig hci0 down
Verifying hci0 device is down
$ sudo hciconfig -a
Now running the hci-tester and getting
$ sudo ./hci-tester
Reset - init
Reset - setup
Reset - setup complete
Reset - run
Reset - test passed
Reset - teardown
Reset - teardown complete
Reset - done
Read Local Version Information - init
Failed to setup upper tester user channel
Read Local Version Information - pre setup failed
Read Local Version Information - done
Read Local Supported Commands - init
Failed to setup upper tester user channel
Read Local Supported Commands - pre setup failed
Read Local Supported Commands - done
Read Local Supported Features - init
Failed to setup upper tester user channel
Read Local Supported Features - pre setup failed
Read Local Supported Features - done
Read Local Extended Features - init
Failed to setup upper tester user channel
Read Local Extended Features - pre setup failed
Read Local Extended Features - done
Read Buffer Size - init
Failed to setup upper tester user channel
Read Buffer Size - pre setup failed
Read Buffer Size - done
[...]
I stripped all the last lines of the output. More detailed is at
http://paste.ubuntu.com/12213127/. The problem here seems to be
that after running the first test the HCI device stays up even
when the HCI_CHANNEL_USER socket gets closed.
Johan pointed out that the problem seems to be that the
HCI_CHANNEL_USER flag doesn't get cleared before calling hci_dev_close
which actively checks for this one being set and if it is it fails
right away to bring down the device.
I am proposing the attached patch here with keeping in mind that
this might still introduce problems somewhere else but wanted to
put this out for discussion.
regards,
Simon
Hi Johan,
>> When a HCI_CHANNEL_USER socket is open and should be closed we first
>> try to close the device which will fail as hci_dev_close checks for
>> HCI_CHANNEL_USER being set and if it is it just fails to close the
>> device. Clearing the HCI_CHANNEL_USER flag first before trying to
>> close the device fixes this.
>>
>> Signed-off-by: Simon Fels <[email protected]>
>> ---
>> backports/net/bluetooth/hci_sock.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/backports/net/bluetooth/hci_sock.c b/backports/net/bluetooth/hci_sock.c
>> index 9a2732f..c84c13e 100644
>> --- a/backports/net/bluetooth/hci_sock.c
>> +++ b/backports/net/bluetooth/hci_sock.c
>> @@ -503,8 +503,8 @@ static int hci_sock_release(struct socket *sock)
>>
>> if (hdev) {
>> if (hci_pi(sk)->channel == HCI_CHANNEL_USER) {
>> - hci_dev_close(hdev->id);
>> hci_dev_clear_flag(hdev, HCI_USER_CHANNEL);
>> + hci_dev_close(hdev->id);
>> mgmt_index_added(hdev);
>> }
>
> Thanks for catching this and coming up with a patch proposal!
>
> My main concern is that there's code within the hci_dev_close() path
> that assumes HCI_USER_CHANNEL may be set. E.g. this in
> hci_dev_do_close() (which hci_dev_close calls):
>
> if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) &&
> !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
> test_bit(HCI_UP, &hdev->flags)) {
> /* Execute vendor specific shutdown routine */
> if (hdev->shutdown)
> hdev->shutdown(hdev);
> }
>
> With your change the hdev->shutdown() callback would get called which
> seems to be what the above if-statement tries to protect against.
commit 9380f9eacfbbee701daa416edd6625efcd3e29e1 flipped them around. And I was worried about that at that time, but since I forgot to add a comment, I could not remember why I ordered them in that way in the first place. I just had a hunch that I did it for a reason.
So we might need to look at this one again and make sure this time we get this right and comment on it.
Regards
Marcel
Hi Simon,
On Fri, Aug 28, 2015, Simon Fels wrote:
> When a HCI_CHANNEL_USER socket is open and should be closed we first
> try to close the device which will fail as hci_dev_close checks for
> HCI_CHANNEL_USER being set and if it is it just fails to close the
> device. Clearing the HCI_CHANNEL_USER flag first before trying to
> close the device fixes this.
>
> Signed-off-by: Simon Fels <[email protected]>
> ---
> backports/net/bluetooth/hci_sock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/backports/net/bluetooth/hci_sock.c b/backports/net/bluetooth/hci_sock.c
> index 9a2732f..c84c13e 100644
> --- a/backports/net/bluetooth/hci_sock.c
> +++ b/backports/net/bluetooth/hci_sock.c
> @@ -503,8 +503,8 @@ static int hci_sock_release(struct socket *sock)
>
> if (hdev) {
> if (hci_pi(sk)->channel == HCI_CHANNEL_USER) {
> - hci_dev_close(hdev->id);
> hci_dev_clear_flag(hdev, HCI_USER_CHANNEL);
> + hci_dev_close(hdev->id);
> mgmt_index_added(hdev);
> }
Thanks for catching this and coming up with a patch proposal!
My main concern is that there's code within the hci_dev_close() path
that assumes HCI_USER_CHANNEL may be set. E.g. this in
hci_dev_do_close() (which hci_dev_close calls):
if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) &&
!hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
test_bit(HCI_UP, &hdev->flags)) {
/* Execute vendor specific shutdown routine */
if (hdev->shutdown)
hdev->shutdown(hdev);
}
With your change the hdev->shutdown() callback would get called which
seems to be what the above if-statement tries to protect against.
Johan
When a HCI_CHANNEL_USER socket is open and should be closed we first try to close the
device which will fail as hci_dev_close checks for HCI_CHANNEL_USER being set and if it is
it just fails to close the device. Clearing the HCI_CHANNEL_USER flag first before trying
to close the device fixes this.
Signed-off-by: Simon Fels <[email protected]>
---
backports/net/bluetooth/hci_sock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/backports/net/bluetooth/hci_sock.c b/backports/net/bluetooth/hci_sock.c
index 9a2732f..c84c13e 100644
--- a/backports/net/bluetooth/hci_sock.c
+++ b/backports/net/bluetooth/hci_sock.c
@@ -503,8 +503,8 @@ static int hci_sock_release(struct socket *sock)
if (hdev) {
if (hci_pi(sk)->channel == HCI_CHANNEL_USER) {
- hci_dev_close(hdev->id);
hci_dev_clear_flag(hdev, HCI_USER_CHANNEL);
+ hci_dev_close(hdev->id);
mgmt_index_added(hdev);
}
--
2.1.4