Hi Marcel, On Sep 20, 2011, at 9:37 AM, Marcel Holtmann wrote: > Hi Andre, > >> If MGMT_OP_STOP_DISCOVERY command is issued before the kernel >> receives the HCI Inquiry Command Status Event from the controller >> then that command will fail and the discovery procedure won't be >> stopped. This situation may occur if a MGMT_OP_STOP_DISCOVERY >> command is issued just after a MGMT_OP_START_DISCOVERY. >> >> This race condition can be handled by checking for pending >> MGMT_OP_STOP_DISCOVERY command in inquiry command status event >> handler. If we have a pending MGMT_OP_STOP_DISCOVERY command we >> cancel the ongoing discovery. >> >> Signed-off-by: Andre Guedes <andre.guedes@xxxxxxxxxxxxx> >> --- >> include/net/bluetooth/hci_core.h | 1 + >> net/bluetooth/hci_event.c | 3 +++ >> net/bluetooth/mgmt.c | 14 +++++++++++++- >> 3 files changed, 17 insertions(+), 1 deletions(-) >> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h >> index 4a79a50..36e15cc 100644 >> --- a/include/net/bluetooth/hci_core.h >> +++ b/include/net/bluetooth/hci_core.h >> @@ -876,6 +876,7 @@ int mgmt_discovering(u16 index, u8 discovering); >> int mgmt_device_blocked(u16 index, bdaddr_t *bdaddr); >> int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr); >> int mgmt_discovery_complete(u16 index, u8 status); >> +int mgmt_has_pending_stop_discov(u16 index); >> >> /* HCI info for socket */ >> #define hci_pi(sk) ((struct hci_pinfo *) sk) >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c >> index b44f362..c9d641b 100644 >> --- a/net/bluetooth/hci_event.c >> +++ b/net/bluetooth/hci_event.c >> @@ -962,6 +962,9 @@ static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status) >> set_bit(HCI_INQUIRY, &hdev->flags); >> >> mgmt_discovering(hdev->id, 1); >> + >> + if (mgmt_has_pending_stop_discov(hdev->id)) >> + hci_cancel_inquiry(hdev); >> } >> >> static inline void hci_cs_create_conn(struct hci_dev *hdev, __u8 status) >> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c >> index d84f242..58cf33a 100644 >> --- a/net/bluetooth/mgmt.c >> +++ b/net/bluetooth/mgmt.c >> @@ -1674,7 +1674,11 @@ static int stop_discovery(struct sock *sk, u16 index) >> goto failed; >> } >> >> - err = hci_cancel_inquiry(hdev); >> + if (test_bit(HCI_INQUIRY, &hdev->flags)) >> + err = hci_cancel_inquiry(hdev); >> + else >> + err = 0; >> + > > do we really just wanna always return success here? Even if > stop_discovery is called for a none existing discovery. If stop_discovery() is called for a none existing discovery this code isn't even reached since stop_discovery() will return in MGMT_OP_START_DISCOVERY pending command check at the beginning of stop_discovery(). If we have an ongoing discovery but the HCI_INQUIRY flag isn't set, then we are in the race condition window. Only in that case, we return 0 and postpone the discovery cancel operation. > And btw. since you just changed the hci_cancel_inquiry() to return > -EPERM if the HCI_INQUIRY flag is not set you could do this simpler by > just checking the return value directly. No double check of the > HCI_INQUIRY flag. Yes, I could have done like this, but since in a further patch I'll add the HCI_LE_SCAN flag check I've done his way here (please see patch 13/14). The HCI_INQUIRY check in hci_cancel_inquiry() is to make sure no HCI_OP_CANCEL_INQUIRY command is issued unless the controller is in inquiry mode (spec recommendations). The HCI_INQUIRY (and HCI_LE_SCAN) check in stop_discovery() stands for sending the right command to cancel the ongoing discovery. Andre-- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
