[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-bluetooth
Subject:    Re: [PATCH v2 6/9] Bluetooth: Add LE scan functions to hci_core
From:       Andre Guedes <andre.guedes () openbossa ! org>
Date:       2011-11-30 18:11:19
Message-ID: D717F826-12B0-4441-91CD-D75CE7609D5C () openbossa ! org
[Download RAW message or body]

Hi Marcel,

On Nov 28, 2011, at 1:28 PM, Marcel Holtmann wrote:

> Hi Andre,
> 
>> This patch adds to hci_core functions to init LE scan and cancel
>> an ongoing scanning (hci_do_le_scan and hci_cancel_le_scan).
>> 
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci_core.h |    3 +++
>> net/bluetooth/hci_core.c         |   32 ++++++++++++++++++++++++++++++++
>> 2 files changed, 35 insertions(+), 0 deletions(-)
>> 
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index a48c699..db137ca 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -998,5 +998,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 8e96e3b..1e5d9db 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -2678,5 +2678,37 @@ int hci_cancel_inquiry(struct hci_dev *hdev)
>> 	return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
>> }
>> 
>> +int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
>> +								int timeout)
>> +{
>> +	struct le_scan_params *params = &hdev->le_scan_params;
>> +
>> +	if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
>> +		return -EINPROGRESS;
>> +
>> +	BT_DBG("%s", hdev->name);
>> +
>> +	params->type = type;
>> +	params->interval = interval;
>> +	params->window = window;
>> +	params->timeout = timeout;
>> +
>> +	queue_work(hdev->workqueue, &hdev->le_scan);
> 
> so you are using the controller workqueue already. That is good. However
> if the send command are already processed in a workqueue, we could just
> sleep for their results. No need for hci_req_complete handling that we
> are using for ioctl based triggers. We could have a lot simpler
> hci_request handling from within the workqueue.

Ok, I'll replace hci_request by another simpler mechanism.

>> +
>> +	return 0;
>> +}
>> +
>> +int hci_cancel_le_scan(struct hci_dev *hdev)
>> +{
>> +	if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
>> +		return -EPERM;
>> +
>> +	BT_DBG("%s", hdev->name);
>> +
>> +	del_timer(&hdev->le_scan_timer);
>> +
>> +	return send_le_scan_enable_cmd(hdev, 0);
>> +}
>> +
> 
> Don't you need to clear out the work struct as well? In case that one is
> still running? Meaning cancel gets called quickly after starting the
> scan. The window might be small, but this is a race condition.


If we want to be able to cancel le scan during this small window we
should cancel it (if it didn't start yet) or wait until "le scan" work
finishes. To achieve that we can use cancel_work_sync(), but it
blocks. So, we'll need another work to handle this.

This is a bit tricky actually. Since hdev->workqueue is single thread,
the "cancel le scan" work will always run after "le scan" work. So, if
we enqueue "cancel le scan" work in hdev->workqueue we won't be able
to cancel the "le scan" work if it is not started yet.

Do you think we should enqueue "cancel le scan" work in the system-wide
workqueue so we have the chance to cancel "le scan" work before it
starts?

BR,

Andre

--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]