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 <[email protected]>
---
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
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 <[email protected]>
> ---
> net/bluetooth/hci_core.c | 1 -
> 1 file changed, 1 deletion(-)
The patch has been applied to my tree now. Thanks.
Johan
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 <[email protected]>
> >> ---
> >> ?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
Hi Johan,
On Thu, Mar 1, 2012 at 7:32 PM, Johan Hedberg <[email protected]> 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 <[email protected]>
>> ---
>> ?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
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 <[email protected]>
> ---
> 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