Hi Marcel,
On Nov 11, 2011, at 8:13 PM, Marcel Holtmann wrote:
> Hi Andre,
>
>> This patch adds to hci_core the infra-structure to carry out the
>> LE scan. Functions were created to init the LE scan and cancel
>> an ongoing scanning (hci_do_le_scan and hci_cancel_le_scan).
>>
>> Signed-off-by: Andre Guedes <andre.guedes@xxxxxxxxxxxxx>
>> ---
>> include/net/bluetooth/hci_core.h | 5 +++
>> net/bluetooth/hci_core.c | 69 ++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 74 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index f6d5d90..69dda9b 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -232,6 +232,8 @@ struct hci_dev {
>> struct list_head adv_entries;
>> struct timer_list adv_timer;
>>
>> + struct timer_list le_scan_timer;
>> +
>> struct hci_dev_stats stat;
>>
>> struct sk_buff_head driver_init;
>> @@ -987,5 +989,8 @@ void hci_le_ltk_neg_reply(struct hci_conn *conn);
>>
>> int hci_do_inquiry(struct hci_dev *hdev, u8 length);
>> int hci_cancel_inquiry(struct hci_dev *hdev);
>> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
>> + int timeout);
>> +int hci_cancel_le_scan(struct hci_dev *hdev);
>>
>> #endif /* __HCI_CORE_H */
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 64cdafe..c07dc15 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1423,6 +1423,23 @@ int hci_add_adv_entry(struct hci_dev *hdev,
>> return 0;
>> }
>>
>> +static int le_scan(struct hci_dev *hdev, u8 enable)
>> +{
>> + struct hci_cp_le_set_scan_enable cp;
>> +
>> + memset(&cp, 0, sizeof(cp));
>> + cp.enable = enable;
>> +
>> + return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>> +}
>> +
>> +static void le_scan_timeout(unsigned long arg)
>> +{
>> + struct hci_dev *hdev = (void *) arg;
>> +
>> + le_scan(hdev, 0);
>> +}
>> +
>> /* Register HCI device */
>> int hci_register_dev(struct hci_dev *hdev)
>> {
>> @@ -1501,6 +1518,9 @@ int hci_register_dev(struct hci_dev *hdev)
>> setup_timer(&hdev->adv_timer, hci_clear_adv_cache,
>> (unsigned long) hdev);
>>
>> + setup_timer(&hdev->le_scan_timer, le_scan_timeout,
>> + (unsigned long) hdev);
>> +
>> INIT_WORK(&hdev->power_on, hci_power_on);
>> INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
>>
>> @@ -1587,6 +1607,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
>> hci_del_sysfs(hdev);
>>
>> del_timer(&hdev->adv_timer);
>> + del_timer(&hdev->le_scan_timer);
>>
>> destroy_workqueue(hdev->workqueue);
>>
>> @@ -2615,3 +2636,51 @@ int hci_cancel_inquiry(struct hci_dev *hdev)
>>
>> return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
>> }
>> +
>> +static int set_le_scan_param(struct hci_dev *hdev, u8 type, u16 interval,
>> + u16 window)
>> +{
>> + struct hci_cp_le_set_scan_param cp;
>> +
>> + memset(&cp, 0, sizeof(cp));
>> + cp.type = type;
>> + cp.interval = cpu_to_le16(interval);
>> + cp.window = cpu_to_le16(window);
>> +
>> + return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), &cp);
>> +}
>> +
>> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
>> + int timeout)
>> +{
>> + int err;
>> +
>> + if (test_bit(HCI_LE_SCAN, &hdev->hci_flags))
>> + return -EINPROGRESS;
>> +
>> + BT_DBG("%s", hdev->name);
>> +
>> + err = set_le_scan_param(hdev, type, interval, window);
>> + if (err < 0)
>> + return err;
>> +
>> + err = le_scan(hdev, 1);
>> + if (err < 0)
>> + return err;
>
> since you are using hci_send_cmd, you never check the error from the
> controller for set_le_scan_param. We should be doing exactly that before
> just going ahead with a scan.
Yes, you're right, there is no error checking at this point.
I took some time thinking about this problem and I concluded we
should not bother so much about it (at least for now). The reasons
are:
1. The spec says the Set LE Scan Parameters command fails if it
is issued when LE scan is enabled. We guarantee this doesn't happen
since we check the HCI_LE_SCAN flag before sending any command to
the controller.
2. Even if the Set LE Scan Parameters command fails for some other
unknown reason, we would do the LE scanning with the last parameters
set. This doesn't seem to be a big deal.
3. We've done lots of tests (with different dongles), but I've not
seen this error happening so far. It seems to be difficult to
reproduce it.
I also took some time thinking about a fix for that, but I didn't find
any easy/clean way to do it.
So I think we should just log if the Set LE Scan Parameters command
fails and, if somehow, this becomes often we come up with a fix for it.
> It is also not guaranteed that the controller queues up these commands,
> it might just return busy from le_scan() if it can have more than one
> outstanding commands (which many controller can do).
I didn't follow you here. We have a mechanism to keep track of how
many commands the host is allowed to send to the controller. The
tasklet hdev->cmd_task only issue a command if the controller is
able to handle it.
Besides, other parts of the code send more than one command in sequence
and it doesn't seem to be a problem (see set_fast_connectable() in
mgmt.c, hci_init_req() in hci_core.c and hci_setup() in hci_event.c).
But anyway, if the controller returns error from le_scan(), we do the
proper handling in hci_cc_le_set_scan_enable().
BR,
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
