Hi all,
This v2 implements Marcel's comments about the previous patch set.
The changes are:
- set_le_scan_param and set_le_scan_enable helpers were removed and their
code put into hci_le_scan function.
- Decimal values were replaced by hexadecimal in HCI parameters.
I also removed Patch "Bluetooth: Add hci_req_cleanup function" from this
patch set since all request errors are now handled by the HCI request
framework.
Regards,
Andre
Andre Guedes (6):
Bluetooth: Update hci_le_scan to use HCI request
Bluetooth: Merge LE-only and interleaved cases
Bluetooth: Remove timeout handling from hci_cancel_le_scan
Bluetooth: Remove LE scan work
Bluetooth: Change LE scanning timeout macros
Bluetooth: Add LE scan type macros
include/net/bluetooth/hci_core.h | 17 ++----
net/bluetooth/hci_core.c | 113 ++++++++-------------------------------
net/bluetooth/mgmt.c | 62 ++++++++++++++++-----
3 files changed, 76 insertions(+), 116 deletions(-)
--
1.8.1.2
Hi Marcel,
On Fri, Mar 8, 2013 at 10:29 PM, Marcel Holtmann <[email protected]> wrote:
> Hi Andre,
>
>> Define LE scanning timeout macros in jiffies just like we do for
>> others timeout macros.
>>
>> Signed-off-by: Andre Guedes <[email protected]>
>> ---
>> net/bluetooth/mgmt.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 3fec412..8567bec 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -109,8 +109,8 @@ static const u16 mgmt_events[] = {
>> #define LE_SCAN_TYPE 0x01
>> #define LE_SCAN_WIN 0x12
>> #define LE_SCAN_INT 0x12
>> -#define LE_SCAN_TIMEOUT_LE_ONLY 10240 /* TGAP(gen_disc_scan_min) */
>> -#define LE_SCAN_TIMEOUT_BREDR_LE 5120 /* TGAP(100)/2 */
>> +#define LE_SCAN_TIMEOUT_LE_ONLY msecs_to_jiffies(10240)
>> +#define LE_SCAN_TIMEOUT_BREDR_LE msecs_to_jiffies(5120)
>>
>> #define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
>> #define INQUIRY_LEN_BREDR_LE 0x04 /* TGAP(100)/2 */
>> @@ -2438,12 +2438,12 @@ static void enable_le_scan_complete(struct hci_dev *hdev, u8 status)
>> switch (hdev->discovery.type) {
>> case DISCOV_TYPE_LE:
>> queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
>> - msecs_to_jiffies(LE_SCAN_TIMEOUT_LE_ONLY));
>> + LE_SCAN_TIMEOUT_LE_ONLY);
>> break;
>>
>> case DISCOV_TYPE_INTERLEAVED:
>> queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
>> - msecs_to_jiffies(LE_SCAN_TIMEOUT_BREDR_LE));
>> + LE_SCAN_TIMEOUT_BREDR_LE);
>> break;
>
> why is this not done before any other patch. These things can go in as cleanups pretty quickly.
I'll put this at the beginning of v3 patch set.
Regards,
Andre
Hi Marcel,
On Fri, Mar 8, 2013 at 10:28 PM, Marcel Holtmann <[email protected]> wrote:
> Hi Andre,
>
>> This patch moves the LE scanning timeout handling from hci_cancel_
>> le_scan helper to stop_discovery.
>>
>> Since we want discovery code handling LE scanning timeout, we should
>> handle it in stop_discovery instead of hci_cancel_le_scan.
>>
>> Signed-off-by: Andre Guedes <[email protected]>
>> ---
>> net/bluetooth/hci_core.c | 14 +++++---------
>> net/bluetooth/mgmt.c | 15 ++++++++++++---
>> 2 files changed, 17 insertions(+), 12 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 59f583b..200ec5a 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1905,20 +1905,16 @@ static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
>>
>> int hci_cancel_le_scan(struct hci_dev *hdev)
>> {
>> + struct hci_cp_le_set_scan_enable cp;
>> +
>> BT_DBG("%s", hdev->name);
>>
>> if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
>> return -EALREADY;
>>
>> - if (cancel_delayed_work(&hdev->le_scan_disable)) {
>> - struct hci_cp_le_set_scan_enable cp;
>> -
>> - /* Send HCI command to disable LE Scan */
>> - memset(&cp, 0, sizeof(cp));
>> - hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>> - }
>> -
>> - return 0;
>> + /* Send HCI command to disable LE Scan */
>> + memset(&cp, 0, sizeof(cp));
>> + return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>> }
>
> so we have HCI request framework now and we keep using hci_send_cmd without checking if it fails or succeeds. I don't really think so. This needs to be done as a HCI request as well.
>
> If you want a convenience helper for a single command with a complete callback, then please add that one, but sending commands where the error is not handling from hci_event.c is not acceptable.
I'll fix this and use hci request here instead.
Regards,
Andre
Hi Marcel,
On Fri, Mar 8, 2013 at 10:24 PM, Marcel Holtmann <[email protected]> wrot=
e:
> Hi Andre,
>
>> This patch changes hci_le_scan helper so it uses the HCI request
>> framework to enable LE scanning.
>>
>> Also, the LE scanning disable timeout was removed from the helper
>> and it is now handled in mgmt start_discovery code.
>
> just a heads up, these need to become way more verbose. Explain what you =
are doing.
Ok, I'll be more verbose here.
>> Signed-off-by: Andre Guedes <[email protected]>
>> ---
>> include/net/bluetooth/hci_core.h | 3 +--
>> net/bluetooth/hci_core.c | 29 +++++++++++++++++----------
>> net/bluetooth/mgmt.c | 43 ++++++++++++++++++++++++++++++++++=
++----
>> 3 files changed, 59 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hc=
i_core.h
>> index d6c3256..964b270 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -1173,8 +1173,7 @@ void hci_le_start_enc(struct hci_conn *conn, __le1=
6 ediv, __u8 rand[8],
>> __u8 ltk[16]);
>> int hci_do_inquiry(struct hci_dev *hdev, u8 length);
>> int hci_cancel_inquiry(struct hci_dev *hdev);
>> -int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window=
,
>> - int timeout);
>> +int hci_le_scan(struct hci_request *req, u8 type, u16 interval, u16 win=
dow);
>> int hci_cancel_le_scan(struct hci_dev *hdev);
>>
>> u8 bdaddr_to_le(u8 bdaddr_type);
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index a49c445..59f583b 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1945,25 +1945,34 @@ static void le_scan_work(struct work_struct *wor=
k)
>> param->timeout);
>> }
>>
>> -int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window=
,
>> - int timeout)
>> +int hci_le_scan(struct hci_request *req, u8 type, u16 interval, u16 win=
dow)
>> {
>> - struct le_scan_params *param =3D &hdev->le_scan_params;
>> + struct hci_dev *hdev =3D req->hdev;
>> + struct hci_cp_le_set_scan_param param_cp;
>> + struct hci_cp_le_set_scan_enable enable_cp;
>>
>> BT_DBG("%s", hdev->name);
>>
>> if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
>> return -ENOTSUPP;
>>
>> - if (work_busy(&hdev->le_scan))
>> - return -EINPROGRESS;
>> + if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
>> + return -EALREADY;
>
> This check in here is silly. Why bother initializing a request and then a=
bort it later. You know it ahead of time. This whole thing of hci_req_init =
in on part and the hci_req_add somewhere else is not a good idea. See comme=
nt below.
>
> I am serious, that I want to get rid of this spaghetti type of coding. Th=
e HCI transaction should be simple and easy to understand. That is why I in=
sisted on that Johan having to redo his patch set many times. It needs to r=
ead like this:
>
> start transaction
>
> add something
> add something more
>
> run it
>
> And that should be all in one function so that people can actually follow=
what is happening when we do something like enabling LE scanning. I do not=
want to look into 3-4 different functions to see what is going on.
>
>> +
>> + memset(¶m_cp, 0, sizeof(param_cp));
>> + param_cp.type =3D type;
>> + param_cp.interval =3D cpu_to_le16(interval);
>> + param_cp.window =3D cpu_to_le16(window);
>> +
>> + hci_req_add(req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(param_cp),
>> + ¶m_cp);
>>
>> - param->type =3D type;
>> - param->interval =3D interval;
>> - param->window =3D window;
>> - param->timeout =3D timeout;
>> + memset(&enable_cp, 0, sizeof(enable_cp));
>> + enable_cp.enable =3D 0x01;
>> + enable_cp.filter_dup =3D 0x01;
>>
>> - queue_work(system_long_wq, &hdev->le_scan);
>> + hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
>> + &enable_cp);
>>
>> return 0;
>> }
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 39395c7..fbf3265 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -2428,11 +2428,34 @@ int mgmt_interleaved_discovery(struct hci_dev *h=
dev)
>> return err;
>> }
>>
>> +static void enable_le_scan_complete(struct hci_dev *hdev, u8 status)
>> +{
>> + BT_DBG("status %d", status);
>> +
>> + if (status)
>> + return;
>> +
>> + switch (hdev->discovery.type) {
>> + case DISCOV_TYPE_LE:
>> + queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable=
,
>> + msecs_to_jiffies(LE_SCAN_TIMEOUT_LE_ONL=
Y));
>> + break;
>> +
>> + case DISCOV_TYPE_INTERLEAVED:
>> + queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable=
,
>> + msecs_to_jiffies(LE_SCAN_TIMEOUT_BREDR_=
LE));
>> + break;
>> + default:
>> + BT_ERR("Invalid discovery type %d", hdev->discovery.type);
>> + }
>> +}
>> +
>
> Why don't we have one complete handler for LE only and one for interleave=
d scanning. Seriously, it gets all smashed together for no apparent reason.=
I fully understand that before this was needed, but with the request frame=
work, the core takes care of everything for you. It makes sure that the rig=
ht complete handler gets call. So lets stop being silly here.
Fair enough. I'll separate them and we'll have a callback for LE-only
and another for interleaved.
> On a different note, can we please get rid of this msecs_to_jiffies calls=
in place. They should be made part of the constant definition. It is a was=
te of space and they just clutter up the code.
This is done in Patch 5. I'll move that patch to the beginning of v3 patch =
set.
>> static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>> void *data, u16 len)
>> {
>> struct mgmt_cp_start_discovery *cp =3D data;
>> struct pending_cmd *cmd;
>> + struct hci_request req;
>> int err;
>>
>> BT_DBG("%s", hdev->name);
>> @@ -2485,8 +2508,14 @@ static int start_discovery(struct sock *sk, struc=
t hci_dev *hdev,
>> goto failed;
>> }
>>
>> - err =3D hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
>> - LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
>> + hci_req_init(&req, hdev);
>> +
>> + err =3D hci_le_scan(&req, LE_SCAN_TYPE, LE_SCAN_INT,
>> + LE_SCAN_WIN);
>> + if (err)
>> + break;
>> +
>> + err =3D hci_req_run(&req, enable_le_scan_complete);
>> break;
>
> So this is one thing I do not want to see. I want hci_req_init, hci_req_a=
dd and hci_req_run in the same function. No go off and call some magic help=
er that does 4 other things. Put the HCI opcodes and parameters right where=
they are suppose to be.
Ok, I'll put hci_req_init, hci_req_add and hci_req_run in start_discovery.
> If you need to put the transaction into a separate function because it is=
shared or more complex, then also hci_req_init and hci_req_run should go i=
nto that function.
This is not the case for hci_le_scan for now. So, I'll also remove
hci_le_scan helper.
We might need a separate function to enable LE scanning in future
since this feature will be used by device discovery and LE connection.
However, if this is the case, I'll add the function in LE connection
patch set.
>> case DISCOV_TYPE_INTERLEAVED:
>> @@ -2497,8 +2526,14 @@ static int start_discovery(struct sock *sk, struc=
t hci_dev *hdev,
>> goto failed;
>> }
>>
>> - err =3D hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT, LE_SC=
AN_WIN,
>> - LE_SCAN_TIMEOUT_BREDR_LE);
>> + hci_req_init(&req, hdev);
>> +
>> + err =3D hci_le_scan(&req, LE_SCAN_TYPE, LE_SCAN_INT,
>> + LE_SCAN_WIN);
>> + if (err)
>> + break;
>> +
>> + err =3D hci_req_run(&req, enable_le_scan_complete);
>> break;
>
> What is the difference between the code statement above and the one be be=
low now.
>
> So you smash the different handling into the complete callback to save sp=
ace, but then the calling side is duplicated. You can do better than this.
To keep this patch simpler, I did the smashing in the subsequent
patch. However, since we'll have a separate callback for LE-only and
interleaved, this cases won't be the same anymore.
Regards,
Andre
Hi Andre,
> Define LE scanning timeout macros in jiffies just like we do for
> others timeout macros.
>
> Signed-off-by: Andre Guedes <[email protected]>
> ---
> net/bluetooth/mgmt.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 3fec412..8567bec 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -109,8 +109,8 @@ static const u16 mgmt_events[] = {
> #define LE_SCAN_TYPE 0x01
> #define LE_SCAN_WIN 0x12
> #define LE_SCAN_INT 0x12
> -#define LE_SCAN_TIMEOUT_LE_ONLY 10240 /* TGAP(gen_disc_scan_min) */
> -#define LE_SCAN_TIMEOUT_BREDR_LE 5120 /* TGAP(100)/2 */
> +#define LE_SCAN_TIMEOUT_LE_ONLY msecs_to_jiffies(10240)
> +#define LE_SCAN_TIMEOUT_BREDR_LE msecs_to_jiffies(5120)
>
> #define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
> #define INQUIRY_LEN_BREDR_LE 0x04 /* TGAP(100)/2 */
> @@ -2438,12 +2438,12 @@ static void enable_le_scan_complete(struct hci_dev *hdev, u8 status)
> switch (hdev->discovery.type) {
> case DISCOV_TYPE_LE:
> queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
> - msecs_to_jiffies(LE_SCAN_TIMEOUT_LE_ONLY));
> + LE_SCAN_TIMEOUT_LE_ONLY);
> break;
>
> case DISCOV_TYPE_INTERLEAVED:
> queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
> - msecs_to_jiffies(LE_SCAN_TIMEOUT_BREDR_LE));
> + LE_SCAN_TIMEOUT_BREDR_LE);
> break;
why is this not done before any other patch. These things can go in as cleanups pretty quickly.
I am fine if you want to keep rebasing and me complaining about these, your choice ;)
Regards
Marcel
Hi Andre,
> This patch moves the LE scanning timeout handling from hci_cancel_
> le_scan helper to stop_discovery.
>
> Since we want discovery code handling LE scanning timeout, we should
> handle it in stop_discovery instead of hci_cancel_le_scan.
>
> Signed-off-by: Andre Guedes <[email protected]>
> ---
> net/bluetooth/hci_core.c | 14 +++++---------
> net/bluetooth/mgmt.c | 15 ++++++++++++---
> 2 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 59f583b..200ec5a 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1905,20 +1905,16 @@ static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
>
> int hci_cancel_le_scan(struct hci_dev *hdev)
> {
> + struct hci_cp_le_set_scan_enable cp;
> +
> BT_DBG("%s", hdev->name);
>
> if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
> return -EALREADY;
>
> - if (cancel_delayed_work(&hdev->le_scan_disable)) {
> - struct hci_cp_le_set_scan_enable cp;
> -
> - /* Send HCI command to disable LE Scan */
> - memset(&cp, 0, sizeof(cp));
> - hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
> - }
> -
> - return 0;
> + /* Send HCI command to disable LE Scan */
> + memset(&cp, 0, sizeof(cp));
> + return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
> }
so we have HCI request framework now and we keep using hci_send_cmd without checking if it fails or succeeds. I don't really think so. This needs to be done as a HCI request as well.
If you want a convenience helper for a single command with a complete callback, then please add that one, but sending commands where the error is not handling from hci_event.c is not acceptable.
Regards
Marcel
Hi Andre,
> This patch changes hci_le_scan helper so it uses the HCI request
> framework to enable LE scanning.
>
> Also, the LE scanning disable timeout was removed from the helper
> and it is now handled in mgmt start_discovery code.
just a heads up, these need to become way more verbose. Explain what you are doing.
> Signed-off-by: Andre Guedes <[email protected]>
> ---
> include/net/bluetooth/hci_core.h | 3 +--
> net/bluetooth/hci_core.c | 29 +++++++++++++++++----------
> net/bluetooth/mgmt.c | 43 ++++++++++++++++++++++++++++++++++++----
> 3 files changed, 59 insertions(+), 16 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index d6c3256..964b270 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1173,8 +1173,7 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
> __u8 ltk[16]);
> int hci_do_inquiry(struct hci_dev *hdev, u8 length);
> int hci_cancel_inquiry(struct hci_dev *hdev);
> -int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
> - int timeout);
> +int hci_le_scan(struct hci_request *req, u8 type, u16 interval, u16 window);
> int hci_cancel_le_scan(struct hci_dev *hdev);
>
> u8 bdaddr_to_le(u8 bdaddr_type);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index a49c445..59f583b 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1945,25 +1945,34 @@ static void le_scan_work(struct work_struct *work)
> param->timeout);
> }
>
> -int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
> - int timeout)
> +int hci_le_scan(struct hci_request *req, u8 type, u16 interval, u16 window)
> {
> - struct le_scan_params *param = &hdev->le_scan_params;
> + struct hci_dev *hdev = req->hdev;
> + struct hci_cp_le_set_scan_param param_cp;
> + struct hci_cp_le_set_scan_enable enable_cp;
>
> BT_DBG("%s", hdev->name);
>
> if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
> return -ENOTSUPP;
>
> - if (work_busy(&hdev->le_scan))
> - return -EINPROGRESS;
> + if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
> + return -EALREADY;
This check in here is silly. Why bother initializing a request and then abort it later. You know it ahead of time. This whole thing of hci_req_init in on part and the hci_req_add somewhere else is not a good idea. See comment below.
I am serious, that I want to get rid of this spaghetti type of coding. The HCI transaction should be simple and easy to understand. That is why I insisted on that Johan having to redo his patch set many times. It needs to read like this:
start transaction
add something
add something more
run it
And that should be all in one function so that people can actually follow what is happening when we do something like enabling LE scanning. I do not want to look into 3-4 different functions to see what is going on.
> +
> + memset(¶m_cp, 0, sizeof(param_cp));
> + param_cp.type = type;
> + param_cp.interval = cpu_to_le16(interval);
> + param_cp.window = cpu_to_le16(window);
> +
> + hci_req_add(req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(param_cp),
> + ¶m_cp);
>
> - param->type = type;
> - param->interval = interval;
> - param->window = window;
> - param->timeout = timeout;
> + memset(&enable_cp, 0, sizeof(enable_cp));
> + enable_cp.enable = 0x01;
> + enable_cp.filter_dup = 0x01;
>
> - queue_work(system_long_wq, &hdev->le_scan);
> + hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
> + &enable_cp);
>
> return 0;
> }
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 39395c7..fbf3265 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2428,11 +2428,34 @@ int mgmt_interleaved_discovery(struct hci_dev *hdev)
> return err;
> }
>
> +static void enable_le_scan_complete(struct hci_dev *hdev, u8 status)
> +{
> + BT_DBG("status %d", status);
> +
> + if (status)
> + return;
> +
> + switch (hdev->discovery.type) {
> + case DISCOV_TYPE_LE:
> + queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
> + msecs_to_jiffies(LE_SCAN_TIMEOUT_LE_ONLY));
> + break;
> +
> + case DISCOV_TYPE_INTERLEAVED:
> + queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
> + msecs_to_jiffies(LE_SCAN_TIMEOUT_BREDR_LE));
> + break;
> + default:
> + BT_ERR("Invalid discovery type %d", hdev->discovery.type);
> + }
> +}
> +
Why don't we have one complete handler for LE only and one for interleaved scanning. Seriously, it gets all smashed together for no apparent reason. I fully understand that before this was needed, but with the request framework, the core takes care of everything for you. It makes sure that the right complete handler gets call. So lets stop being silly here.
If you want to look into examples from userspace, then look into what the changes to a proper mgmt framework did to make this simpler and cleaner. No crazy state tracking to remember who was calling what. Just can rely on that the caller gives a proper complete function.
On a different note, can we please get rid of this msecs_to_jiffies calls in place. They should be made part of the constant definition. It is a waste of space and they just clutter up the code.
> static int start_discovery(struct sock *sk, struct hci_dev *hdev,
> void *data, u16 len)
> {
> struct mgmt_cp_start_discovery *cp = data;
> struct pending_cmd *cmd;
> + struct hci_request req;
> int err;
>
> BT_DBG("%s", hdev->name);
> @@ -2485,8 +2508,14 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
> goto failed;
> }
>
> - err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
> - LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
> + hci_req_init(&req, hdev);
> +
> + err = hci_le_scan(&req, LE_SCAN_TYPE, LE_SCAN_INT,
> + LE_SCAN_WIN);
> + if (err)
> + break;
> +
> + err = hci_req_run(&req, enable_le_scan_complete);
> break;
So this is one thing I do not want to see. I want hci_req_init, hci_req_add and hci_req_run in the same function. No go off and call some magic helper that does 4 other things. Put the HCI opcodes and parameters right where they are suppose to be.
If you need to put the transaction into a separate function because it is shared or more complex, then also hci_req_init and hci_req_run should go into that function.
> case DISCOV_TYPE_INTERLEAVED:
> @@ -2497,8 +2526,14 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
> goto failed;
> }
>
> - err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT, LE_SCAN_WIN,
> - LE_SCAN_TIMEOUT_BREDR_LE);
> + hci_req_init(&req, hdev);
> +
> + err = hci_le_scan(&req, LE_SCAN_TYPE, LE_SCAN_INT,
> + LE_SCAN_WIN);
> + if (err)
> + break;
> +
> + err = hci_req_run(&req, enable_le_scan_complete);
> break;
What is the difference between the code statement above and the one be below now.
So you smash the different handling into the complete callback to save space, but then the calling side is duplicated. You can do better than this.
Regards
Marcel
This patch adds macros for active and passive LE scan type values. It
also removes the LE_SCAN_TYPE macro since it is not used anymore.
Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci_core.h | 4 ++++
net/bluetooth/mgmt.c | 3 +--
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 0e4d893..8ce48ee 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1163,6 +1163,10 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
__u8 ltk[16]);
int hci_do_inquiry(struct hci_dev *hdev, u8 length);
int hci_cancel_inquiry(struct hci_dev *hdev);
+
+#define LE_SCAN_PASSIVE 0x00
+#define LE_SCAN_ACTIVE 0x01
+
int hci_le_scan(struct hci_request *req, u8 type, u16 interval, u16 window);
int hci_cancel_le_scan(struct hci_dev *hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 8567bec..e6dde64 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -106,7 +106,6 @@ static const u16 mgmt_events[] = {
* These LE scan and inquiry parameters were chosen according to LE General
* Discovery Procedure specification.
*/
-#define LE_SCAN_TYPE 0x01
#define LE_SCAN_WIN 0x12
#define LE_SCAN_INT 0x12
#define LE_SCAN_TIMEOUT_LE_ONLY msecs_to_jiffies(10240)
@@ -2519,7 +2518,7 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
hci_req_init(&req, hdev);
- err = hci_le_scan(&req, LE_SCAN_TYPE, LE_SCAN_INT,
+ err = hci_le_scan(&req, LE_SCAN_ACTIVE, LE_SCAN_INT,
LE_SCAN_WIN);
if (err)
break;
--
1.8.1.2
Define LE scanning timeout macros in jiffies just like we do for
others timeout macros.
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/mgmt.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3fec412..8567bec 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -109,8 +109,8 @@ static const u16 mgmt_events[] = {
#define LE_SCAN_TYPE 0x01
#define LE_SCAN_WIN 0x12
#define LE_SCAN_INT 0x12
-#define LE_SCAN_TIMEOUT_LE_ONLY 10240 /* TGAP(gen_disc_scan_min) */
-#define LE_SCAN_TIMEOUT_BREDR_LE 5120 /* TGAP(100)/2 */
+#define LE_SCAN_TIMEOUT_LE_ONLY msecs_to_jiffies(10240)
+#define LE_SCAN_TIMEOUT_BREDR_LE msecs_to_jiffies(5120)
#define INQUIRY_LEN_BREDR 0x08 /* TGAP(100) */
#define INQUIRY_LEN_BREDR_LE 0x04 /* TGAP(100)/2 */
@@ -2438,12 +2438,12 @@ static void enable_le_scan_complete(struct hci_dev *hdev, u8 status)
switch (hdev->discovery.type) {
case DISCOV_TYPE_LE:
queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
- msecs_to_jiffies(LE_SCAN_TIMEOUT_LE_ONLY));
+ LE_SCAN_TIMEOUT_LE_ONLY);
break;
case DISCOV_TYPE_INTERLEAVED:
queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
- msecs_to_jiffies(LE_SCAN_TIMEOUT_BREDR_LE));
+ LE_SCAN_TIMEOUT_BREDR_LE);
break;
default:
BT_ERR("Invalid discovery type %d", hdev->discovery.type);
--
1.8.1.2
This patch removes the LE scan work and all code related to it. We
now use the HCI request framework to enable LE scanning so the LE
scan work is no longer needed.
Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci_core.h | 10 ------
net/bluetooth/hci_core.c | 72 ----------------------------------------
2 files changed, 82 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 964b270..0e4d893 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -117,13 +117,6 @@ struct oob_data {
u8 randomizer[16];
};
-struct le_scan_params {
- u8 type;
- u16 interval;
- u16 window;
- int timeout;
-};
-
#define HCI_MAX_SHORT_NAME_LENGTH 10
struct amp_assoc {
@@ -278,9 +271,6 @@ struct hci_dev {
struct delayed_work le_scan_disable;
- struct work_struct le_scan;
- struct le_scan_params le_scan_params;
-
__s8 adv_tx_power;
__u8 adv_data[HCI_MAX_AD_LENGTH];
__u8 adv_data_len;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 200ec5a..45fe8f3 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1063,8 +1063,6 @@ static int hci_dev_do_close(struct hci_dev *hdev)
{
BT_DBG("%s %p", hdev->name, hdev);
- cancel_work_sync(&hdev->le_scan);
-
cancel_delayed_work(&hdev->power_off);
hci_req_cancel(hdev, ENODEV);
@@ -1845,64 +1843,6 @@ int hci_blacklist_del(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type)
return mgmt_device_unblocked(hdev, bdaddr, type);
}
-static void le_scan_param_req(struct hci_request *req, unsigned long opt)
-{
- struct le_scan_params *param = (struct le_scan_params *) opt;
- struct hci_cp_le_set_scan_param cp;
-
- memset(&cp, 0, sizeof(cp));
- cp.type = param->type;
- cp.interval = cpu_to_le16(param->interval);
- cp.window = cpu_to_le16(param->window);
-
- hci_req_add(req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), &cp);
-}
-
-static void le_scan_enable_req(struct hci_request *req, unsigned long opt)
-{
- struct hci_cp_le_set_scan_enable cp;
-
- memset(&cp, 0, sizeof(cp));
- cp.enable = 1;
- cp.filter_dup = 1;
-
- hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
-}
-
-static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
- u16 window, int timeout)
-{
- long timeo = msecs_to_jiffies(3000);
- struct le_scan_params param;
- int err;
-
- BT_DBG("%s", hdev->name);
-
- if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
- return -EINPROGRESS;
-
- param.type = type;
- param.interval = interval;
- param.window = window;
-
- hci_req_lock(hdev);
-
- err = __hci_req_sync(hdev, le_scan_param_req, (unsigned long) ¶m,
- timeo);
- if (!err)
- err = __hci_req_sync(hdev, le_scan_enable_req, 0, timeo);
-
- hci_req_unlock(hdev);
-
- if (err < 0)
- return err;
-
- queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
- msecs_to_jiffies(timeout));
-
- return 0;
-}
-
int hci_cancel_le_scan(struct hci_dev *hdev)
{
struct hci_cp_le_set_scan_enable cp;
@@ -1930,17 +1870,6 @@ static void le_scan_disable_work(struct work_struct *work)
hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
}
-static void le_scan_work(struct work_struct *work)
-{
- struct hci_dev *hdev = container_of(work, struct hci_dev, le_scan);
- struct le_scan_params *param = &hdev->le_scan_params;
-
- BT_DBG("%s", hdev->name);
-
- hci_do_le_scan(hdev, param->type, param->interval, param->window,
- param->timeout);
-}
-
int hci_le_scan(struct hci_request *req, u8 type, u16 interval, u16 window)
{
struct hci_dev *hdev = req->hdev;
@@ -2007,7 +1936,6 @@ struct hci_dev *hci_alloc_dev(void)
INIT_WORK(&hdev->cmd_work, hci_cmd_work);
INIT_WORK(&hdev->tx_work, hci_tx_work);
INIT_WORK(&hdev->power_on, hci_power_on);
- INIT_WORK(&hdev->le_scan, le_scan_work);
INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);
--
1.8.1.2
This patch moves the LE scanning timeout handling from hci_cancel_
le_scan helper to stop_discovery.
Since we want discovery code handling LE scanning timeout, we should
handle it in stop_discovery instead of hci_cancel_le_scan.
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/hci_core.c | 14 +++++---------
net/bluetooth/mgmt.c | 15 ++++++++++++---
2 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 59f583b..200ec5a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1905,20 +1905,16 @@ static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
int hci_cancel_le_scan(struct hci_dev *hdev)
{
+ struct hci_cp_le_set_scan_enable cp;
+
BT_DBG("%s", hdev->name);
if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
return -EALREADY;
- if (cancel_delayed_work(&hdev->le_scan_disable)) {
- struct hci_cp_le_set_scan_enable cp;
-
- /* Send HCI command to disable LE Scan */
- memset(&cp, 0, sizeof(cp));
- hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
- }
-
- return 0;
+ /* Send HCI command to disable LE Scan */
+ memset(&cp, 0, sizeof(cp));
+ return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
}
static void le_scan_disable_work(struct work_struct *work)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 0fe7a87..3fec412 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2579,10 +2579,19 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
switch (hdev->discovery.state) {
case DISCOVERY_FINDING:
- if (test_bit(HCI_INQUIRY, &hdev->flags))
+ if (test_bit(HCI_INQUIRY, &hdev->flags)) {
err = hci_cancel_inquiry(hdev);
- else
- err = hci_cancel_le_scan(hdev);
+ } else {
+ /*
+ * If delayed work could not be canceled, it means the
+ * work is running. Therefore, there is no need to send
+ * another HCI command to disable LE scanning.
+ */
+ if (!cancel_delayed_work(&hdev->le_scan_disable))
+ err = 0;
+ else
+ err = hci_cancel_le_scan(hdev);
+ }
break;
--
1.8.1.2
This patch merges the LE-only and interleaved discovery type cases.
Since hci_le_scan helper uses the HCI request framework, those cases
are pretty much the same now.
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/mgmt.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index fbf3265..0fe7a87 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2501,6 +2501,7 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
break;
case DISCOV_TYPE_LE:
+ case DISCOV_TYPE_INTERLEAVED:
if (!lmp_host_le_capable(hdev)) {
err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
MGMT_STATUS_NOT_SUPPORTED);
@@ -2508,18 +2509,8 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
goto failed;
}
- hci_req_init(&req, hdev);
-
- err = hci_le_scan(&req, LE_SCAN_TYPE, LE_SCAN_INT,
- LE_SCAN_WIN);
- if (err)
- break;
-
- err = hci_req_run(&req, enable_le_scan_complete);
- break;
-
- case DISCOV_TYPE_INTERLEAVED:
- if (!lmp_host_le_capable(hdev) || !lmp_bredr_capable(hdev)) {
+ if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
+ !lmp_bredr_capable(hdev)) {
err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
MGMT_STATUS_NOT_SUPPORTED);
mgmt_pending_remove(cmd);
--
1.8.1.2
This patch changes hci_le_scan helper so it uses the HCI request
framework to enable LE scanning.
Also, the LE scanning disable timeout was removed from the helper
and it is now handled in mgmt start_discovery code.
Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci_core.h | 3 +--
net/bluetooth/hci_core.c | 29 +++++++++++++++++----------
net/bluetooth/mgmt.c | 43 ++++++++++++++++++++++++++++++++++++----
3 files changed, 59 insertions(+), 16 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index d6c3256..964b270 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1173,8 +1173,7 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
__u8 ltk[16]);
int hci_do_inquiry(struct hci_dev *hdev, u8 length);
int hci_cancel_inquiry(struct hci_dev *hdev);
-int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
- int timeout);
+int hci_le_scan(struct hci_request *req, u8 type, u16 interval, u16 window);
int hci_cancel_le_scan(struct hci_dev *hdev);
u8 bdaddr_to_le(u8 bdaddr_type);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index a49c445..59f583b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1945,25 +1945,34 @@ static void le_scan_work(struct work_struct *work)
param->timeout);
}
-int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
- int timeout)
+int hci_le_scan(struct hci_request *req, u8 type, u16 interval, u16 window)
{
- struct le_scan_params *param = &hdev->le_scan_params;
+ struct hci_dev *hdev = req->hdev;
+ struct hci_cp_le_set_scan_param param_cp;
+ struct hci_cp_le_set_scan_enable enable_cp;
BT_DBG("%s", hdev->name);
if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
return -ENOTSUPP;
- if (work_busy(&hdev->le_scan))
- return -EINPROGRESS;
+ if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
+ return -EALREADY;
+
+ memset(¶m_cp, 0, sizeof(param_cp));
+ param_cp.type = type;
+ param_cp.interval = cpu_to_le16(interval);
+ param_cp.window = cpu_to_le16(window);
+
+ hci_req_add(req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(param_cp),
+ ¶m_cp);
- param->type = type;
- param->interval = interval;
- param->window = window;
- param->timeout = timeout;
+ memset(&enable_cp, 0, sizeof(enable_cp));
+ enable_cp.enable = 0x01;
+ enable_cp.filter_dup = 0x01;
- queue_work(system_long_wq, &hdev->le_scan);
+ hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
+ &enable_cp);
return 0;
}
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 39395c7..fbf3265 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2428,11 +2428,34 @@ int mgmt_interleaved_discovery(struct hci_dev *hdev)
return err;
}
+static void enable_le_scan_complete(struct hci_dev *hdev, u8 status)
+{
+ BT_DBG("status %d", status);
+
+ if (status)
+ return;
+
+ switch (hdev->discovery.type) {
+ case DISCOV_TYPE_LE:
+ queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
+ msecs_to_jiffies(LE_SCAN_TIMEOUT_LE_ONLY));
+ break;
+
+ case DISCOV_TYPE_INTERLEAVED:
+ queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
+ msecs_to_jiffies(LE_SCAN_TIMEOUT_BREDR_LE));
+ break;
+ default:
+ BT_ERR("Invalid discovery type %d", hdev->discovery.type);
+ }
+}
+
static int start_discovery(struct sock *sk, struct hci_dev *hdev,
void *data, u16 len)
{
struct mgmt_cp_start_discovery *cp = data;
struct pending_cmd *cmd;
+ struct hci_request req;
int err;
BT_DBG("%s", hdev->name);
@@ -2485,8 +2508,14 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
goto failed;
}
- err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
- LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
+ hci_req_init(&req, hdev);
+
+ err = hci_le_scan(&req, LE_SCAN_TYPE, LE_SCAN_INT,
+ LE_SCAN_WIN);
+ if (err)
+ break;
+
+ err = hci_req_run(&req, enable_le_scan_complete);
break;
case DISCOV_TYPE_INTERLEAVED:
@@ -2497,8 +2526,14 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
goto failed;
}
- err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT, LE_SCAN_WIN,
- LE_SCAN_TIMEOUT_BREDR_LE);
+ hci_req_init(&req, hdev);
+
+ err = hci_le_scan(&req, LE_SCAN_TYPE, LE_SCAN_INT,
+ LE_SCAN_WIN);
+ if (err)
+ break;
+
+ err = hci_req_run(&req, enable_le_scan_complete);
break;
default:
--
1.8.1.2