[PATCH] Bluetooth: Fix device reset

[PATCH] Bluetooth: Fix device reset

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>


Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/hci_core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index e77cc79..3f30714 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -822,7 +822,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
 	skb_queue_purge(&hdev->cmd_q);
 	atomic_set(&hdev->cmd_cnt, 1);
 	if (!test_bit(HCI_RAW, &hdev->flags) &&
-				test_bit(HCI_QUIRK_NO_RESET, &hdev->quirks)) {
+			!test_bit(HCI_QUIRK_NO_RESET, &hdev->quirks)) {
 		set_bit(HCI_INIT, &hdev->flags);
 		__hci_request(hdev, hci_reset_req, 0,
 					msecs_to_jiffies(250));
-- 
1.7.8.3

Re: [PATCH] Bluetooth: Fix device reset

[Johan Hedberg]

Hi Andrei,

On Mon, Feb 06, 2012, Emeltchenko Andrei wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/hci_core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index e77cc79..3f30714 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -822,7 +822,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
>  	skb_queue_purge(&hdev->cmd_q);
>  	atomic_set(&hdev->cmd_cnt, 1);
>  	if (!test_bit(HCI_RAW, &hdev->flags) &&
> -				test_bit(HCI_QUIRK_NO_RESET, &hdev->quirks)) {
> +			!test_bit(HCI_QUIRK_NO_RESET, &hdev->quirks)) {
>  		set_bit(HCI_INIT, &hdev->flags);
>  		__hci_request(hdev, hci_reset_req, 0,
>  					msecs_to_jiffies(250));

Nack. The NO_RESET quirk is supposed to prevent reset during the device
init sequence. The purpose of the test in hci_dev_do_close() is to let
the reset be done there instead.

Johan

Re: [PATCH] Bluetooth: Fix device reset

[Marcel Holtmann]

Hi Johan,

> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > 
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  net/bluetooth/hci_core.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index e77cc79..3f30714 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -822,7 +822,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> >  	skb_queue_purge(&hdev->cmd_q);
> >  	atomic_set(&hdev->cmd_cnt, 1);
> >  	if (!test_bit(HCI_RAW, &hdev->flags) &&
> > -				test_bit(HCI_QUIRK_NO_RESET, &hdev->quirks)) {
> > +			!test_bit(HCI_QUIRK_NO_RESET, &hdev->quirks)) {
> >  		set_bit(HCI_INIT, &hdev->flags);
> >  		__hci_request(hdev, hci_reset_req, 0,
> >  					msecs_to_jiffies(250));
> 
> Nack. The NO_RESET quirk is supposed to prevent reset during the device
> init sequence. The purpose of the test in hci_dev_do_close() is to let
> the reset be done there instead.

we might wanna rename this into HCI_QUIRK_RESET_ON_CLOSE. Otherwise
people keep in getting confused.

Regards

Marcel

Re: [PATCH] Bluetooth: Fix device reset

[Andrei Emeltchenko]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

Hi,

06.02.2012 21:04 пользователь "Marcel Holtmann" <marcel@holtmann.org>
написал:
>
> Hi Johan,
>
> > > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > >
> > >
> > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > ---
> > >  net/bluetooth/hci_core.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > index e77cc79..3f30714 100644
> > > --- a/net/bluetooth/hci_core.c
> > > +++ b/net/bluetooth/hci_core.c
> > > @@ -822,7 +822,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> > >     skb_queue_purge(&hdev->cmd_q);
> > >     atomic_set(&hdev->cmd_cnt, 1);
> > >     if (!test_bit(HCI_RAW, &hdev->flags) &&
> > > -                           test_bit(HCI_QUIRK_NO_RESET,
&hdev->quirks)) {
> > > +                   !test_bit(HCI_QUIRK_NO_RESET, &hdev->quirks)) {
> > >             set_bit(HCI_INIT, &hdev->flags);
> > >             __hci_request(hdev, hci_reset_req, 0,
> > >                                     msecs_to_jiffies(250));
> >
> > Nack. The NO_RESET quirk is supposed to prevent reset during the device
> > init sequence. The purpose of the test in hci_dev_do_close() is to let
> > the reset be done there instead.
>
> we might wanna rename this into HCI_QUIRK_RESET_ON_CLOSE. Otherwise
> people keep in getting confused.
>

Moreover without my patch my USB dongle gets stuck after reset.

Regards
Andrei

[-- Attachment #2: Type: text/html, Size: 2109 bytes --]

Re: [PATCH] Bluetooth: Fix device reset

[Marcel Holtmann]

Hi Andrei,

> > > > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > >
> > > >
> > > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > > ---
> > > >  net/bluetooth/hci_core.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > > index e77cc79..3f30714 100644
> > > > --- a/net/bluetooth/hci_core.c
> > > > +++ b/net/bluetooth/hci_core.c
> > > > @@ -822,7 +822,7 @@ static int hci_dev_do_close(struct hci_dev
> *hdev)
> > > >     skb_queue_purge(&hdev->cmd_q);
> > > >     atomic_set(&hdev->cmd_cnt, 1);
> > > >     if (!test_bit(HCI_RAW, &hdev->flags) &&
> > > > -                           test_bit(HCI_QUIRK_NO_RESET,
> &hdev->quirks)) {
> > > > +                   !test_bit(HCI_QUIRK_NO_RESET,
> &hdev->quirks)) {
> > > >             set_bit(HCI_INIT, &hdev->flags);
> > > >             __hci_request(hdev, hci_reset_req, 0,
> > > >                                     msecs_to_jiffies(250));
> > >
> > > Nack. The NO_RESET quirk is supposed to prevent reset during the
> device
> > > init sequence. The purpose of the test in hci_dev_do_close() is to
> let
> > > the reset be done there instead.
> >
> > we might wanna rename this into HCI_QUIRK_RESET_ON_CLOSE. Otherwise
> > people keep in getting confused.
> >
> 
> Moreover without my patch my USB dongle gets stuck after reset.

please send hcidump traces for this. And you mean hciconfig hci0 reset?

Regards

Marcel

Re: [PATCH] Bluetooth: Fix device reset

[Andrei Emeltchenko]

Hi Marcel,

On Mon, Feb 06, 2012 at 11:38:51AM -0800, Marcel Holtmann wrote:
> Hi Andrei,
> 
> > > > > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > > >
> > > > >
> > > > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > > > ---
> > > > >  net/bluetooth/hci_core.c |    2 +-
> > > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > >
> > > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > > > index e77cc79..3f30714 100644
> > > > > --- a/net/bluetooth/hci_core.c
> > > > > +++ b/net/bluetooth/hci_core.c
> > > > > @@ -822,7 +822,7 @@ static int hci_dev_do_close(struct hci_dev
> > *hdev)
> > > > >     skb_queue_purge(&hdev->cmd_q);
> > > > >     atomic_set(&hdev->cmd_cnt, 1);
> > > > >     if (!test_bit(HCI_RAW, &hdev->flags) &&
> > > > > -                           test_bit(HCI_QUIRK_NO_RESET,
> > &hdev->quirks)) {
> > > > > +                   !test_bit(HCI_QUIRK_NO_RESET,
> > &hdev->quirks)) {
> > > > >             set_bit(HCI_INIT, &hdev->flags);
> > > > >             __hci_request(hdev, hci_reset_req, 0,
> > > > >                                     msecs_to_jiffies(250));
> > > >
> > > > Nack. The NO_RESET quirk is supposed to prevent reset during the
> > device
> > > > init sequence. The purpose of the test in hci_dev_do_close() is to
> > let
> > > > the reset be done there instead.
> > >
> > > we might wanna rename this into HCI_QUIRK_RESET_ON_CLOSE. Otherwise
> > > people keep in getting confused.
> > >
> > 
> > Moreover without my patch my USB dongle gets stuck after reset.
> 
> please send hcidump traces for this. And you mean hciconfig hci0 reset?

In the trace there is only one line:

< HCI Command: Reset (0x03|0x0003) plen 0

Then chip stuck (I am testing this inside virtual machine).

I have A-Link BCM USB dongle.

Best regards 
Andrei Emeltchenko 
> 

Re: [PATCH] Bluetooth: Fix device reset

[Marcel Holtmann]

Hi Andrei,

> > > > > > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > > > >
> > > > > >
> > > > > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > > > > ---
> > > > > >  net/bluetooth/hci_core.c |    2 +-
> > > > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > > >
> > > > > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > > > > > index e77cc79..3f30714 100644
> > > > > > --- a/net/bluetooth/hci_core.c
> > > > > > +++ b/net/bluetooth/hci_core.c
> > > > > > @@ -822,7 +822,7 @@ static int hci_dev_do_close(struct hci_dev
> > > *hdev)
> > > > > >     skb_queue_purge(&hdev->cmd_q);
> > > > > >     atomic_set(&hdev->cmd_cnt, 1);
> > > > > >     if (!test_bit(HCI_RAW, &hdev->flags) &&
> > > > > > -                           test_bit(HCI_QUIRK_NO_RESET,
> > > &hdev->quirks)) {
> > > > > > +                   !test_bit(HCI_QUIRK_NO_RESET,
> > > &hdev->quirks)) {
> > > > > >             set_bit(HCI_INIT, &hdev->flags);
> > > > > >             __hci_request(hdev, hci_reset_req, 0,
> > > > > >                                     msecs_to_jiffies(250));
> > > > >
> > > > > Nack. The NO_RESET quirk is supposed to prevent reset during the
> > > device
> > > > > init sequence. The purpose of the test in hci_dev_do_close() is to
> > > let
> > > > > the reset be done there instead.
> > > >
> > > > we might wanna rename this into HCI_QUIRK_RESET_ON_CLOSE. Otherwise
> > > > people keep in getting confused.
> > > >
> > > 
> > > Moreover without my patch my USB dongle gets stuck after reset.
> > 
> > please send hcidump traces for this. And you mean hciconfig hci0 reset?
> 
> In the trace there is only one line:
> 
> < HCI Command: Reset (0x03|0x0003) plen 0
> 
> Then chip stuck (I am testing this inside virtual machine).
> 
> I have A-Link BCM USB dongle.

the patch in general is correct, but it seems that it has some side
effects on some dongles. Can you try doing hciconfig hci0 reset versus a
hciconfig hci0 down + hciconfig hci0 up and if it makes any difference.

Regards

Marcel

Re: [PATCH] Bluetooth: Fix device reset

[Andrei Emeltchenko]

Hi Marcel,

On Thu, Feb 09, 2012 at 02:51:11PM +0100, Marcel Holtmann wrote:
> > > > > > >     if (!test_bit(HCI_RAW, &hdev->flags) &&
> > > > > > > -                           test_bit(HCI_QUIRK_NO_RESET,
> > > > &hdev->quirks)) {
> > > > > > > +                   !test_bit(HCI_QUIRK_NO_RESET,
> > > > &hdev->quirks)) {
> > > > > > Nack. The NO_RESET quirk is supposed to prevent reset during the
> > > > device
> > > > > > init sequence. The purpose of the test in hci_dev_do_close() is to
> > > > let
> > > > > > the reset be done there instead.
> > > > >
> > > > > we might wanna rename this into HCI_QUIRK_RESET_ON_CLOSE. Otherwise
> > > > > people keep in getting confused.
> > > > >
> > > > 
> > > > Moreover without my patch my USB dongle gets stuck after reset.
> > > 
> > > please send hcidump traces for this. And you mean hciconfig hci0 reset?
> > 
> > In the trace there is only one line:
> > 
> > < HCI Command: Reset (0x03|0x0003) plen 0
> > 
> > Then chip stuck (I am testing this inside virtual machine).
> > 
> > I have A-Link BCM USB dongle.
> 
> the patch in general is correct, but it seems that it has some side
> effects on some dongles. Can you try doing hciconfig hci0 reset versus a
> hciconfig hci0 down + hciconfig hci0 up and if it makes any difference.

I was testing for some time and I think now with Johan's patch reverted it
only a little bit better. I was testing with hciconfig hci0 down +
hciconfig hci0 up and with hciconfig hci0 reset and at some point dongle
gets stuck in my virtualbox either way. This is very unstable and probably
related to timing.

Has anybody else experienced the same problem?

Best regards 
Andrei Emeltchenko