[PATCH] Bluetooth: Don't force DISCOVERY_STOPPED state in inquiry_cache_flush

[PATCH] Bluetooth: Don't force DISCOVERY_STOPPED state in inquiry_cache_flush

[Andre Guedes]

We are not supposed to force DISCOVERY_STOPPED in inquiry_cache_flush
because we may break the discovery state machine. For instance,
during interleaved discovery, when we are about to start inquiry,
the state machine forcibly goes to DISCOVERY_STOPPED while it
should stay in DISCOVERY_FINDING state.

This problem results in unexpected behaviors such as sending two
mgmt_discovering events to userspace (when only one event is
expected) and Stop Discovery failures.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_core.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d3ddc0b..661d65f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -413,7 +413,6 @@ static void inquiry_cache_flush(struct hci_dev *hdev)
 
 	INIT_LIST_HEAD(&cache->unknown);
 	INIT_LIST_HEAD(&cache->resolve);
-	cache->state = DISCOVERY_STOPPED;
 }
 
 struct inquiry_entry *hci_inquiry_cache_lookup(struct hci_dev *hdev, bdaddr_t *bdaddr)
-- 
1.7.9.2

Re: [PATCH] Bluetooth: Don't force DISCOVERY_STOPPED state in inquiry_cache_flush

[Johan Hedberg]

Hi Andre,

On Thu, Mar 01, 2012, Andre Guedes wrote:
> We are not supposed to force DISCOVERY_STOPPED in inquiry_cache_flush
> because we may break the discovery state machine. For instance,
> during interleaved discovery, when we are about to start inquiry,
> the state machine forcibly goes to DISCOVERY_STOPPED while it
> should stay in DISCOVERY_FINDING state.
> 
> This problem results in unexpected behaviors such as sending two
> mgmt_discovering events to userspace (when only one event is
> expected) and Stop Discovery failures.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  net/bluetooth/hci_core.c |    1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index d3ddc0b..661d65f 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -413,7 +413,6 @@ static void inquiry_cache_flush(struct hci_dev *hdev)
>  
>  	INIT_LIST_HEAD(&cache->unknown);
>  	INIT_LIST_HEAD(&cache->resolve);
> -	cache->state = DISCOVERY_STOPPED;
>  }

Nack.

The reason why this was there is hci_dev_do_close() and hci_dev_reset()
which call inquiry_cache_flush(). If the discovery state is not set
correctly through these code paths you might get into a situation where
you can't start discovery again after doing "hciconfig hci0 reset" or
"hciconfig hci0 down" while discovery was active. So I agree that some
fix is needed but you need to ensure that you don't break these use
cases.

Johan

Re: [PATCH] Bluetooth: Don't force DISCOVERY_STOPPED state in inquiry_cache_flush

[Andre Guedes]

Hi Johan,

On Thu, Mar 1, 2012 at 7:32 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Andre,
>
> On Thu, Mar 01, 2012, Andre Guedes wrote:
>> We are not supposed to force DISCOVERY_STOPPED in inquiry_cache_flush
>> because we may break the discovery state machine. For instance,
>> during interleaved discovery, when we are about to start inquiry,
>> the state machine forcibly goes to DISCOVERY_STOPPED while it
>> should stay in DISCOVERY_FINDING state.
>>
>> This problem results in unexpected behaviors such as sending two
>> mgmt_discovering events to userspace (when only one event is
>> expected) and Stop Discovery failures.
>>
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>>  net/bluetooth/hci_core.c |    1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index d3ddc0b..661d65f 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -413,7 +413,6 @@ static void inquiry_cache_flush(struct hci_dev *hdev)
>>
>>       INIT_LIST_HEAD(&cache->unknown);
>>       INIT_LIST_HEAD(&cache->resolve);
>> -     cache->state = DISCOVERY_STOPPED;
>>  }
>
> Nack.
>
> The reason why this was there is hci_dev_do_close() and hci_dev_reset()
> which call inquiry_cache_flush(). If the discovery state is not set
> correctly through these code paths you might get into a situation where
> you can't start discovery again after doing "hciconfig hci0 reset" or
> "hciconfig hci0 down" while discovery was active. So I agree that some
> fix is needed but you need to ensure that you don't break these use
> cases.

This issue is already address by upstream patch "Bluetooth: Set
DISCOVERY_STOPPED if controller resets". So, the discovery state machine
will be right even if we run "hciconfig hci0 reset" or "hciconfig hci0
down" while discovery is active.

BR,

Andre

Re: [PATCH] Bluetooth: Don't force DISCOVERY_STOPPED state in inquiry_cache_flush

[Johan Hedberg]

Hi Andre,

On Fri, Mar 02, 2012, Andre Guedes wrote:
> > On Thu, Mar 01, 2012, Andre Guedes wrote:
> >> We are not supposed to force DISCOVERY_STOPPED in inquiry_cache_flush
> >> because we may break the discovery state machine. For instance,
> >> during interleaved discovery, when we are about to start inquiry,
> >> the state machine forcibly goes to DISCOVERY_STOPPED while it
> >> should stay in DISCOVERY_FINDING state.
> >>
> >> This problem results in unexpected behaviors such as sending two
> >> mgmt_discovering events to userspace (when only one event is
> >> expected) and Stop Discovery failures.
> >>
> >> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> >> ---
> >>  net/bluetooth/hci_core.c |    1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> >> index d3ddc0b..661d65f 100644
> >> --- a/net/bluetooth/hci_core.c
> >> +++ b/net/bluetooth/hci_core.c
> >> @@ -413,7 +413,6 @@ static void inquiry_cache_flush(struct hci_dev *hdev)
> >>
> >>       INIT_LIST_HEAD(&cache->unknown);
> >>       INIT_LIST_HEAD(&cache->resolve);
> >> -     cache->state = DISCOVERY_STOPPED;
> >>  }
> >
> > Nack.
> >
> > The reason why this was there is hci_dev_do_close() and hci_dev_reset()
> > which call inquiry_cache_flush(). If the discovery state is not set
> > correctly through these code paths you might get into a situation where
> > you can't start discovery again after doing "hciconfig hci0 reset" or
> > "hciconfig hci0 down" while discovery was active. So I agree that some
> > fix is needed but you need to ensure that you don't break these use
> > cases.
> 
> This issue is already address by upstream patch "Bluetooth: Set
> DISCOVERY_STOPPED if controller resets". So, the discovery state machine
> will be right even if we run "hciconfig hci0 reset" or "hciconfig hci0
> down" while discovery is active.

I didn't realize this has been added to the cc_reset function. In that
case I think both use cases should be fine with this patch, i.e. I
change my nack to an ack :)

Johan

Re: [PATCH] Bluetooth: Don't force DISCOVERY_STOPPED state in inquiry_cache_flush

[Johan Hedberg]

Hi Andre,

On Thu, Mar 01, 2012, Andre Guedes wrote:
> We are not supposed to force DISCOVERY_STOPPED in inquiry_cache_flush
> because we may break the discovery state machine. For instance,
> during interleaved discovery, when we are about to start inquiry,
> the state machine forcibly goes to DISCOVERY_STOPPED while it
> should stay in DISCOVERY_FINDING state.
> 
> This problem results in unexpected behaviors such as sending two
> mgmt_discovering events to userspace (when only one event is
> expected) and Stop Discovery failures.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  net/bluetooth/hci_core.c |    1 -
>  1 file changed, 1 deletion(-)

The patch has been applied to my tree now. Thanks.

Johan