Hi Jakub,
> This patch refactor BREDR inquiry and LE scan triggering logic into
> separate methods.
please write it as BR/EDR which makes it generally more consistent.
>
> Signed-off-by: Jakub Pawlowski <jpawlowski@xxxxxxxxxx>
> ---
> net/bluetooth/mgmt.c | 145 +++++++++++++++++++++++++++++----------------------
> 1 file changed, 82 insertions(+), 63 deletions(-)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 32c2c75..92fa8e88 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -3913,93 +3913,112 @@ done:
> return err;
> }
>
> -static bool trigger_discovery(struct hci_request *req, u8 *status)
> +static bool trigger_bredr_inquiry(struct hci_request *req, u8 *status)
> {
> struct hci_dev *hdev = req->hdev;
> - struct hci_cp_le_set_scan_param param_cp;
> - struct hci_cp_le_set_scan_enable enable_cp;
> struct hci_cp_inquiry inq_cp;
I think you can rename this into just cp now. There is only now.
> /* General inquiry access code (GIAC) */
> u8 lap[3] = { 0x33, 0x8b, 0x9e };
> +
> + *status = mgmt_bredr_support(hdev);
> + if (*status)
> + return false;
> +
> + if (hci_dev_test_flag(hdev, HCI_INQUIRY)) {
> + *status = MGMT_STATUS_BUSY;
> + return false;
> + }
> +
> + hci_inquiry_cache_flush(hdev);
> +
> + memset(&inq_cp, 0, sizeof(inq_cp));
> + memcpy(&inq_cp.lap, lap, sizeof(inq_cp.lap));
> + inq_cp.length = DISCOV_BREDR_INQUIRY_LEN;
I think we normally have an empty line between filling the structs and then calling hci_req_add.
> + hci_req_add(req, HCI_OP_INQUIRY, sizeof(inq_cp), &inq_cp);
Extra empty line here.
> + return true;
> +}
> +
> +static bool trigger_le_scan(struct hci_request *req, u8 *status,
> + __le16 interval)
> +{
Lets make the status the last parameter. Having a return value in the middle is odd.
> + struct hci_dev *hdev = req->hdev;
> + struct hci_cp_le_set_scan_param param_cp;
> + struct hci_cp_le_set_scan_enable enable_cp;
> u8 own_addr_type;
> int err;
>
> - switch (hdev->discovery.type) {
> - case DISCOV_TYPE_BREDR:
> - *status = mgmt_bredr_support(hdev);
> - if (*status)
> - return false;
> + *status = mgmt_le_support(hdev);
> + if (*status)
> + return false;
>
> - if (test_bit(HCI_INQUIRY, &hdev->flags)) {
> - *status = MGMT_STATUS_BUSY;
> + if (hci_dev_test_flag(hdev, HCI_LE_ADV)) {
> + /* Don't let discovery abort an outgoing connection attempt
> + * that's using directed advertising.
> + */
> + if (hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT)) {
> + *status = MGMT_STATUS_REJECTED;
> return false;
> }
>
> - hci_inquiry_cache_flush(hdev);
> + disable_advertising(req);
> + }
> +
> + /* If controller is scanning, it means the background scanning is
> + * running. Thus, we should temporarily stop it in order to set the
> + * discovery scanning parameters.
> + */
> + if (hci_dev_test_flag(hdev, HCI_LE_SCAN))
> + hci_req_add_le_scan_disable(req);
>
> - memset(&inq_cp, 0, sizeof(inq_cp));
> - memcpy(&inq_cp.lap, lap, sizeof(inq_cp.lap));
> - inq_cp.length = DISCOV_BREDR_INQUIRY_LEN;
> - hci_req_add(req, HCI_OP_INQUIRY, sizeof(inq_cp), &inq_cp);
> + memset(¶m_cp, 0, sizeof(param_cp));
This memset should go above the code that fills the param_cp.
> +
> + /* All active scans will be done with either a resolvable private
> + * address (when privacy feature has been enabled) or non-resolvable
> + * private address.
> + */
> + err = hci_update_random_address(req, true, &own_addr_type);
> + if (err < 0) {
> + *status = MGMT_STATUS_FAILED;
> + return false;
> + }
> +
Put the memset from above right here.
> + param_cp.type = LE_SCAN_ACTIVE;
> + param_cp.interval = interval;
> + param_cp.window = cpu_to_le16(DISCOV_LE_SCAN_WIN);
> + param_cp.own_address_type = own_addr_type;
> + hci_req_add(req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(param_cp),
> + ¶m_cp);
> +
> + memset(&enable_cp, 0, sizeof(enable_cp));
> + enable_cp.enable = LE_SCAN_ENABLE;
> + enable_cp.filter_dup = LE_SCAN_FILTER_DUP_ENABLE;
> + hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
> + &enable_cp);
> +
> + return true;
> +}
> +
> +static bool trigger_discovery(struct hci_request *req, u8 *status)
> +{
> + struct hci_dev *hdev = req->hdev;
> +
> + switch (hdev->discovery.type) {
> + case DISCOV_TYPE_BREDR:
> + if (!trigger_bredr_inquiry(req, status))
> + return false;
> break;
>
> case DISCOV_TYPE_LE:
> case DISCOV_TYPE_INTERLEAVED:
> - *status = mgmt_le_support(hdev);
> - if (*status)
> - return false;
> -
> if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
> !hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) {
> *status = MGMT_STATUS_NOT_SUPPORTED;
> return false;
> }
>
> - if (hci_dev_test_flag(hdev, HCI_LE_ADV)) {
> - /* Don't let discovery abort an outgoing
> - * connection attempt that's using directed
> - * advertising.
> - */
> - if (hci_conn_hash_lookup_state(hdev, LE_LINK,
> - BT_CONNECT)) {
> - *status = MGMT_STATUS_REJECTED;
> - return false;
> - }
> -
> - disable_advertising(req);
> - }
> -
> - /* If controller is scanning, it means the background scanning
> - * is running. Thus, we should temporarily stop it in order to
> - * set the discovery scanning parameters.
> - */
> - if (hci_dev_test_flag(hdev, HCI_LE_SCAN))
> - hci_req_add_le_scan_disable(req);
> -
> - memset(¶m_cp, 0, sizeof(param_cp));
> -
> - /* All active scans will be done with either a resolvable
> - * private address (when privacy feature has been enabled)
> - * or non-resolvable private address.
> - */
> - err = hci_update_random_address(req, true, &own_addr_type);
> - if (err < 0) {
> - *status = MGMT_STATUS_FAILED;
> + if (!trigger_le_scan(req, status,
> + cpu_to_le16(DISCOV_LE_SCAN_INT)))
> return false;
I had to apply the patch see the resulting code, but I prefer if we turn this actually into nice code while we are at it. I pretty much dislike entering a case block and then checking the same value again.
switch (hdev->discovery.type) {
case DISCOV_TYPE_BREDR:
if (!trigger_bredr_inquiry(req, status))
return false;
break;
case DISCOV_TYPE_INTERLEAVED:
if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) {
*status = MGMT_STATUS_NOT_SUPPORTED;
return false;
}
/* fall through */
case DISCOV_TYPE_LE:
if (!trigger_le_scan(req, status,
cpu_to_le16(DISCOV_LE_SCAN_INT)))
return false;
break;
default:
*status = MGMT_STATUS_INVALID_PARAMS;
return false;
}
Regards
Marcel
--
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
