Checking the LE scanning state (HCI_LE_SCAN) before queuing a new "LE
Set Scan Enable (disable)" command is not enough to avoid sending
extra redundant commands. Since HCI_LE_SCAN is only updated upon
"Command Complete" confirmation, multiple "LE Set Scan Enable
(disable)" can get queued before the first one gets delivered.
I encountered this issue when unpairing (MGMT_OP_UNPAIR_DEVICE) and
immediately pairing (MGMT_OP_PAIR_DEVICE) the only auto-connected
device. MGMT_OP_UNPAIR_DEVICE stopped scanning since there were no
more auto-connected devices and MGMT_OP_PAIR_DEVICE also stopped
scanning because some controllers cannot scan while connecting.
I added a dedicated flag (le_scan_disable_queued) in hci_dev for this
purpose since dev_flags is already full.
Signed-off-by: Alfonso Acosta <[email protected]>
---
include/net/bluetooth/hci_core.h | 1 +
net/bluetooth/hci_core.c | 4 +++-
net/bluetooth/hci_event.c | 2 +-
3 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 37ff1ae..04c2f2f 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -292,6 +292,7 @@ struct hci_dev {
struct sk_buff_head rx_q;
struct sk_buff_head raw_q;
struct sk_buff_head cmd_q;
+ int le_scan_disable_queued;
struct sk_buff *recv_evt;
struct sk_buff *sent_cmd;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index cb05d7f..3628a9e 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -5441,10 +5441,12 @@ static void hci_cmd_work(struct work_struct *work)
void hci_req_add_le_scan_disable(struct hci_request *req)
{
struct hci_cp_le_set_scan_enable cp;
-
+ if (req->hdev->le_scan_disable_queued)
+ return;
memset(&cp, 0, sizeof(cp));
cp.enable = LE_SCAN_DISABLE;
hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
+ req->hdev->le_scan_disable_queued = 1;
}
static void add_to_white_list(struct hci_request *req,
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 8b0a2a6..ad81335 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1122,7 +1122,7 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
__u8 status = *((__u8 *) skb->data);
BT_DBG("%s status 0x%2.2x", hdev->name, status);
-
+ hdev->le_scan_disable_queued = 0;
if (status)
return;
--
1.9.1
> I can see the need for having some sort of fix for this. However, we
> already have several use cases where we'd need to check for pending HCI
> commands and adding a new variable to hci_dev for each case is not
> really a good way to solve it. If we really would want to do it and
> we're out of available hdev->dev_flags I'd consider making the hdev
> variable "unsigned long dev_flags[2]" which both test_bit and set_bit
> should be able to handle fine (and existing code wouldn't need
> changing).
Yep, I agree with extending the size of dev_flags, it's much cleaner.
It simply didn't occur to me.
> You also have the issue that your code only covers the HCI
> commands sent through hci_req_add_le_scan_disable() but not any other
> code paths that might send the same command (either from the kernel or
> from user space using a raw HCI socket, e.g. hcitool).
>
> So far we've been able to cover other similar cases adequately by
> looking into the mgmt pending command queue and so the HCI command queue
> hasn't required any special look-ups. Now it might be the time to
> consider something like that. I.e. if we really do need to check for a
> not-yet sent HCI command a helper to look-up pending commands in
> hdev->cmd_q should be added. OTOH, we'd still have a race when the HCI
> command has already been sent but not yet completed, in which case the
> helper might need to look at hdev->sent_cmd as well. Either way, solving
> this in a generic manner doesn't seem to be a completely trivial task.
I agree, this is the right way to fix it, but I am not sure I will
have the time to do in the short term (specially given my current lack
of familiarity with the code).
Maybe you can give me some pointers and I can see if I can find some
time? We can discuss this on IRC if you want.
> If I've understood correctly the background for this (which it really
> wouldn't hurt to mention in the commit message) is a paired device that
> looses its pairing information because if a reset and we want to repair
> with it? I think to make this less complex for user space we should
> consider simply making mgmt_pair_device() calls valid for already paired
> devices, in which case it'd simply trigger a re-pairing. That way you
> wouldn't need the initial mgmt_unpair_device() call. Thoughts?
I will extend the commit description to add more context.
Good point. Calling mgmt_unpair_device() alone before
mgmt_pair_device() is not much of a hassle. However, since
mgmt_unpair_device() also removes the autoconnect action and the
connection parameters, these need to be added again too (and kept
around for this purpose).
So, yes, I think it would simplify things.
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
Hi Alfonso,
On Fri, Sep 26, 2014, Alfonso Acosta wrote:
> Checking the LE scanning state (HCI_LE_SCAN) before queuing a new "LE
> Set Scan Enable (disable)" command is not enough to avoid sending
> extra redundant commands. Since HCI_LE_SCAN is only updated upon
> "Command Complete" confirmation, multiple "LE Set Scan Enable
> (disable)" can get queued before the first one gets delivered.
I can see the need for having some sort of fix for this. However, we
already have several use cases where we'd need to check for pending HCI
commands and adding a new variable to hci_dev for each case is not
really a good way to solve it. If we really would want to do it and
we're out of available hdev->dev_flags I'd consider making the hdev
variable "unsigned long dev_flags[2]" which both test_bit and set_bit
should be able to handle fine (and existing code wouldn't need
changing). You also have the issue that your code only covers the HCI
commands sent through hci_req_add_le_scan_disable() but not any other
code paths that might send the same command (either from the kernel or
from user space using a raw HCI socket, e.g. hcitool).
So far we've been able to cover other similar cases adequately by
looking into the mgmt pending command queue and so the HCI command queue
hasn't required any special look-ups. Now it might be the time to
consider something like that. I.e. if we really do need to check for a
not-yet sent HCI command a helper to look-up pending commands in
hdev->cmd_q should be added. OTOH, we'd still have a race when the HCI
command has already been sent but not yet completed, in which case the
helper might need to look at hdev->sent_cmd as well. Either way, solving
this in a generic manner doesn't seem to be a completely trivial task.
> I encountered this issue when unpairing (MGMT_OP_UNPAIR_DEVICE) and
> immediately pairing (MGMT_OP_PAIR_DEVICE) the only auto-connected
> device. MGMT_OP_UNPAIR_DEVICE stopped scanning since there were no
> more auto-connected devices and MGMT_OP_PAIR_DEVICE also stopped
> scanning because some controllers cannot scan while connecting.
>
> I added a dedicated flag (le_scan_disable_queued) in hci_dev for this
> purpose since dev_flags is already full.
If I've understood correctly the background for this (which it really
wouldn't hurt to mention in the commit message) is a paired device that
looses its pairing information because if a reset and we want to repair
with it? I think to make this less complex for user space we should
consider simply making mgmt_pair_device() calls valid for already paired
devices, in which case it'd simply trigger a re-pairing. That way you
wouldn't need the initial mgmt_unpair_device() call. Thoughts?
Johan