Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1531301495-14479-1-git-send-email-jaganathx.kanakkassery@intel.com> <1531301495-14479-9-git-send-email-jaganathx.kanakkassery@intel.com> From: Jaganath K Date: Mon, 16 Jul 2018 13:42:45 +0530 Message-ID: Subject: Re: [PATCH v4 08/13] Bluetooth: Impmlement extended adv enable To: Marcel Holtmann Cc: "open list:BLUETOOTH DRIVERS" , Jaganath Kanakkassery Content-Type: text/plain; charset="UTF-8" List-ID: Hi Marcel, On Sun, Jul 15, 2018 at 12:08 AM, Marcel Holtmann wro= te: > Hi Jaganath, > >> This patch basically replaces legacy adv with extended adv >> based on the controller support. Currently there is no >> design change. ie only one adv set will be enabled at a time. >> >> < HCI Command: LE Set Extended Advertising Parameters (0x08|0x0036) plen= 25 >> Handle: 0x00 >> Properties: 0x0010 >> Use legacy advertising PDUs: ADV_NONCONN_IND >> Min advertising interval: 1280.000 msec (0x0800) >> Max advertising interval: 1280.000 msec (0x0800) >> Channel map: 37, 38, 39 (0x07) >> Own address type: Random (0x01) >> Peer address type: Public (0x00) >> Peer address: 00:00:00:00:00:00 (OUI 00-00-00) >> Filter policy: Allow Scan Request from Any, Allow Connect Request= from Any (0x00) >> TX power: 127 dbm (0x7f) >> Primary PHY: LE 1M (0x01) >> Secondary max skip: 0x00 >> Secondary PHY: LE 1M (0x01) >> SID: 0x00 >> Scan request notifications: Disabled (0x00) >>> HCI Event: Command Complete (0x0e) plen 5 >> LE Set Extended Advertising Parameters (0x08|0x0036) ncmd 1 >> Status: Success (0x00) >> TX power (selected): 7 dbm (0x07) >> < HCI Command: LE Set Extended Advertising Enable (0x08|0x0039) plen 6 >> Extended advertising: Enabled (0x01) >> Number of sets: 1 (0x01) >> Entry 0 >> Handle: 0x00 >> Duration: 0 ms (0x00) >> Max ext adv events: 0 >>> HCI Event: Command Complete (0x0e) plen 4 >> LE Set Extended Advertising Enable (0x08|0x0039) ncmd 2 >> Status: Success (0x00) >> >> Signed-off-by: Jaganath Kanakkassery >> --- >> include/net/bluetooth/hci.h | 39 ++++++++++++ >> net/bluetooth/hci_event.c | 63 +++++++++++++++++++ >> net/bluetooth/hci_request.c | 144 ++++++++++++++++++++++++++++++++++++++= ++---- >> net/bluetooth/hci_request.h | 3 + >> net/bluetooth/mgmt.c | 19 ++++-- >> 5 files changed, 251 insertions(+), 17 deletions(-) >> >> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h >> index e584863..c0c542d 100644 >> --- a/include/net/bluetooth/hci.h >> +++ b/include/net/bluetooth/hci.h >> @@ -1587,6 +1587,45 @@ struct hci_rp_le_read_num_supported_adv_sets { >> __u8 num_of_sets; >> } __packed; >> >> +#define HCI_OP_LE_SET_EXT_ADV_PARAMS 0x2036 >> +struct hci_cp_le_set_ext_adv_params { >> + __u8 handle; >> + __le16 evt_properties; >> + __u8 min_interval[3]; >> + __u8 max_interval[3]; >> + __u8 channel_map; >> + __u8 own_addr_type; >> + __u8 peer_addr_type; >> + bdaddr_t peer_addr; >> + __u8 filter_policy; >> + __u8 tx_power; >> + __u8 primary_phy; >> + __u8 secondary_max_skip; >> + __u8 secondary_phy; >> + __u8 sid; >> + __u8 notif_enable; >> +} __packed; > > please do proper alignment here and follow the style we currently have in= hci.h. > >> + >> +#define HCI_ADV_PHY_1M 0X01 >> + >> +struct hci_rp_le_set_ext_adv_params { >> + __u8 status; >> + __u8 tx_power; >> +} __attribute__ ((packed)); >> + >> +#define HCI_OP_LE_SET_EXT_ADV_ENABLE 0x2039 >> +struct hci_cp_le_set_ext_adv_enable { >> + __u8 enable; >> + __u8 num_of_sets; >> + __u8 data[0]; >> +} __packed; >> + >> +struct hci_cp_ext_adv_set { >> + __u8 handle; >> + __le16 duration; >> + __u8 max_events; >> +} __packed; >> + >> /* ---- HCI Events ---- */ >> #define HCI_EV_INQUIRY_COMPLETE 0x01 >> >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c >> index 90e7235..e17002f 100644 >> --- a/net/bluetooth/hci_event.c >> +++ b/net/bluetooth/hci_event.c >> @@ -1099,6 +1099,41 @@ static void hci_cc_le_set_adv_enable(struct hci_d= ev *hdev, struct sk_buff *skb) >> hci_dev_unlock(hdev); >> } >> >> +static void hci_cc_le_set_ext_adv_enable(struct hci_dev *hdev, >> + struct sk_buff *skb) >> +{ >> + struct hci_cp_le_set_ext_adv_enable *cp; >> + struct hci_cp_ext_adv_set *adv_set; >> + __u8 status =3D *((__u8 *) skb->data); >> + >> + BT_DBG("%s status 0x%2.2x", hdev->name, status); >> + >> + if (status) >> + return; >> + >> + cp =3D hci_sent_cmd_data(hdev, HCI_OP_LE_SET_EXT_ADV_ENABLE); >> + if (!cp) >> + return; >> + >> + adv_set =3D (void *) cp->data; >> + >> + hci_dev_lock(hdev); >> + >> + if (cp->enable) { >> + struct hci_conn *conn; >> + >> + hci_dev_set_flag(hdev, HCI_LE_ADV); >> + >> + conn =3D hci_lookup_le_connect(hdev); >> + if (conn) >> + queue_delayed_work(hdev->workqueue, >> + &conn->le_conn_timeout, >> + conn->conn_timeout); >> + } >> + >> + hci_dev_unlock(hdev); >> +} >> + >> static void hci_cc_le_set_scan_param(struct hci_dev *hdev, struct sk_buf= f *skb) >> { >> struct hci_cp_le_set_scan_param *cp; >> @@ -1486,6 +1521,26 @@ static void hci_cc_set_adv_param(struct hci_dev *= hdev, struct sk_buff *skb) >> hci_dev_unlock(hdev); >> } >> >> +static void hci_cc_set_ext_adv_param(struct hci_dev *hdev, struct sk_bu= ff *skb) >> +{ >> + struct hci_rp_le_set_ext_adv_params *rp =3D (void *) skb->data; >> + struct hci_cp_le_set_ext_adv_params *cp; >> + >> + BT_DBG("%s status 0x%2.2x", hdev->name, rp->status); >> + >> + if (rp->status) >> + return; >> + >> + cp =3D hci_sent_cmd_data(hdev, HCI_OP_LE_SET_EXT_ADV_PARAMS); >> + if (!cp) >> + return; >> + >> + hci_dev_lock(hdev); >> + hdev->adv_addr_type =3D cp->own_addr_type; >> + hdev->adv_tx_power =3D rp->tx_power; > > Don't do this. The adv_tx_power is meant for the default adv TX power of = the controller. This can not be overwritten. Leave this alone for now. I re= alize that you want this to support the TX power data type, but that needs = to be stored in the adv instance then. And then used from there. > > So create a pre-change that has the tx_power information in the adv insta= nce. And on instance populate it using hdev->adv_tx_power. Use that also fo= r ext adv parameters and when the parameter setting comes back then update = the value with the controller chosen TX power. So that adv data has the cor= rect value in there. > I will add tx_power in the instance and initialize with INVALID as no point in using hdev->adv_tx_power since it will be INVALID anyway. > Please note that in case of not using legacy advertising and actually tra= nsmitting TX power via ext adv, then we need to make sure it is not include= d in the adv data. > I did not get the comment "actually transmitting TX power via ext adv". As far as I understood, in ext adv, tx_power will be always valid which is returned from set_param and if it is set in the flags by user it has to = be always included in adv data. >> + hci_dev_unlock(hdev); >> +} >> + >> static void hci_cc_read_rssi(struct hci_dev *hdev, struct sk_buff *skb) >> { >> struct hci_rp_read_rssi *rp =3D (void *) skb->data; >> @@ -3207,6 +3262,14 @@ static void hci_cmd_complete_evt(struct hci_dev *= hdev, struct sk_buff *skb, >> hci_cc_le_read_num_adv_sets(hdev, skb); >> break; >> >> + case HCI_OP_LE_SET_EXT_ADV_PARAMS: >> + hci_cc_set_ext_adv_param(hdev, skb); >> + break; >> + >> + case HCI_OP_LE_SET_EXT_ADV_ENABLE: >> + hci_cc_le_set_ext_adv_enable(hdev, skb); >> + break; >> + >> default: >> BT_DBG("%s opcode 0x%4.4x", hdev->name, *opcode); >> break; >> diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c >> index 7f5950e..a2091b5 100644 >> --- a/net/bluetooth/hci_request.c >> +++ b/net/bluetooth/hci_request.c >> @@ -894,6 +894,24 @@ void hci_req_add_le_passive_scan(struct hci_request= *req) >> hdev->le_scan_window, own_addr_type, filter_pol= icy); >> } >> >> +static u8 get_adv_instance_scan_rsp_len(struct hci_dev *hdev, u8 instan= ce) >> +{ >> + struct adv_info *adv_instance; >> + >> + /* Ignore instance 0 */ >> + if (instance =3D=3D 0x00) >> + return 0; >> + >> + adv_instance =3D hci_find_adv_instance(hdev, instance); >> + if (!adv_instance) >> + return 0; >> + >> + /* TODO: Take into account the "appearance" and "local-name" flags= here. >> + * These are currently being ignored as they are not supported. >> + */ >> + return adv_instance->scan_rsp_len; >> +} >> + >> static u8 get_cur_adv_instance_scan_rsp_len(struct hci_dev *hdev) >> { >> u8 instance =3D hdev->cur_adv_instance; >> @@ -1303,9 +1321,13 @@ void hci_req_reenable_advertising(struct hci_dev = *hdev) >> __hci_req_schedule_adv_instance(&req, hdev->cur_adv_instan= ce, >> true); >> } else { >> - __hci_req_update_adv_data(&req, 0x00); >> - __hci_req_update_scan_rsp_data(&req, 0x00); >> - __hci_req_enable_advertising(&req); >> + if (ext_adv_capable(hdev)) { >> + __hci_req_start_ext_adv(&req, 0x00); >> + } else { >> + __hci_req_update_adv_data(&req, 0x00); >> + __hci_req_update_scan_rsp_data(&req, 0x00); >> + __hci_req_enable_advertising(&req); >> + } >> } >> >> hci_req_run(&req, adv_enable_complete); >> @@ -1342,6 +1364,88 @@ static void adv_timeout_expire(struct work_struct= *work) >> hci_dev_unlock(hdev); >> } >> >> +static int __hci_req_setup_ext_adv_instance(struct hci_request *req, >> + u8 instance) >> +{ >> + struct hci_cp_le_set_ext_adv_params cp; >> + struct hci_dev *hdev =3D req->hdev; >> + bool connectable; >> + u32 flags; >> + /* In ext adv set param interval is 3 octets */ >> + const u8 adv_interval[3] =3D { 0x00, 0x08, 0x00 }; >> + >> + flags =3D get_adv_instance_flags(hdev, instance); >> + >> + /* If the "connectable" instance flag was not set, then choose bet= ween >> + * ADV_IND and ADV_NONCONN_IND based on the global connectable set= ting. >> + */ >> + connectable =3D (flags & MGMT_ADV_FLAG_CONNECTABLE) || >> + mgmt_get_connectable(hdev); >> + >> + if (!is_advertising_allowed(hdev, connectable)) >> + return -EPERM; >> + >> + memset(&cp, 0, sizeof(cp)); >> + >> + memcpy(cp.min_interval, adv_interval, sizeof(cp.min_interval)); >> + memcpy(cp.max_interval, adv_interval, sizeof(cp.max_interval)); >> + >> + if (connectable) >> + cp.evt_properties =3D cpu_to_le16(LE_LEGACY_ADV_IND); >> + else if (get_adv_instance_scan_rsp_len(hdev, instance)) >> + cp.evt_properties =3D cpu_to_le16(LE_LEGACY_ADV_SCAN_IND); >> + else >> + cp.evt_properties =3D cpu_to_le16(LE_LEGACY_NONCONN_IND); >> + >> + cp.own_addr_type =3D BDADDR_LE_PUBLIC; >> + cp.channel_map =3D hdev->le_adv_channel_map; >> + cp.tx_power =3D 127; >> + cp.primary_phy =3D HCI_ADV_PHY_1M; >> + cp.secondary_phy =3D HCI_ADV_PHY_1M; >> + cp.handle =3D 0; >> + >> + hci_req_add(req, HCI_OP_LE_SET_EXT_ADV_PARAMS, sizeof(cp), &cp); >> + >> + return 0; >> +} >> + >> +void __hci_req_enable_ext_advertising(struct hci_request *req) >> +{ >> + struct hci_cp_le_set_ext_adv_enable *cp; >> + struct hci_cp_ext_adv_set *adv_set; >> + u8 data[sizeof(*cp) + sizeof(*adv_set) * 1]; >> + >> + cp =3D (void *) data; >> + adv_set =3D (void *) cp->data; >> + >> + memset(cp, 0, sizeof(*cp)); >> + >> + cp->enable =3D 0x01; >> + cp->num_of_sets =3D 0x01; >> + >> + memset(adv_set, 0, sizeof(*adv_set)); >> + >> + adv_set->handle =3D 0; >> + >> + hci_req_add(req, HCI_OP_LE_SET_EXT_ADV_ENABLE, >> + sizeof(*cp) + sizeof(*adv_set) * cp->num_of_sets, >> + data); >> +} >> + >> +int __hci_req_start_ext_adv(struct hci_request *req, u8 instance) >> +{ >> + struct hci_dev *hdev =3D req->hdev; >> + int err; >> + >> + err =3D __hci_req_setup_ext_adv_instance(req, instance); >> + if (err < 0) >> + return err; >> + >> + __hci_req_enable_ext_advertising(req); >> + >> + return 0; >> +} >> + >> int __hci_req_schedule_adv_instance(struct hci_request *req, u8 instance= , >> bool force) >> { >> @@ -1395,9 +1499,13 @@ int __hci_req_schedule_adv_instance(struct hci_re= quest *req, u8 instance, >> return 0; >> >> hdev->cur_adv_instance =3D instance; >> - __hci_req_update_adv_data(req, instance); >> - __hci_req_update_scan_rsp_data(req, instance); >> - __hci_req_enable_advertising(req); >> + if (ext_adv_capable(hdev)) { >> + __hci_req_start_ext_adv(req, instance); >> + } else { >> + __hci_req_update_adv_data(req, instance); >> + __hci_req_update_scan_rsp_data(req, instance); >> + __hci_req_enable_advertising(req); >> + } >> >> return 0; >> } >> @@ -1668,8 +1776,12 @@ static int connectable_update(struct hci_request = *req, unsigned long opt) >> >> /* Update the advertising parameters if necessary */ >> if (hci_dev_test_flag(hdev, HCI_ADVERTISING) || >> - !list_empty(&hdev->adv_instances)) >> - __hci_req_enable_advertising(req); >> + !list_empty(&hdev->adv_instances)) { >> + if (ext_adv_capable(hdev)) >> + __hci_req_start_ext_adv(req, hdev->cur_adv_instanc= e); >> + else >> + __hci_req_enable_advertising(req); >> + } >> >> __hci_update_background_scan(req); >> >> @@ -1778,8 +1890,12 @@ static int discoverable_update(struct hci_request= *req, unsigned long opt) >> /* Discoverable mode affects the local advertising >> * address in limited privacy mode. >> */ >> - if (hci_dev_test_flag(hdev, HCI_LIMITED_PRIVACY)) >> - __hci_req_enable_advertising(req); >> + if (hci_dev_test_flag(hdev, HCI_LIMITED_PRIVACY)) { >> + if (ext_adv_capable(hdev)) >> + __hci_req_start_ext_adv(req, 0x00); >> + else >> + __hci_req_enable_advertising(req); >> + } >> } >> >> hci_dev_unlock(hdev); >> @@ -2375,8 +2491,12 @@ static int powered_update_hci(struct hci_request = *req, unsigned long opt) >> __hci_req_update_adv_data(req, 0x00); >> __hci_req_update_scan_rsp_data(req, 0x00); >> >> - if (hci_dev_test_flag(hdev, HCI_ADVERTISING)) >> - __hci_req_enable_advertising(req); >> + if (hci_dev_test_flag(hdev, HCI_ADVERTISING)) { >> + if (ext_adv_capable(hdev)) >> + __hci_req_start_ext_adv(req, 0x00)= ; >> + else >> + __hci_req_enable_advertising(req); >> + } >> } else if (!list_empty(&hdev->adv_instances)) { >> struct adv_info *adv_instance; >> >> diff --git a/net/bluetooth/hci_request.h b/net/bluetooth/hci_request.h >> index 702beb1..9b8c74d 100644 >> --- a/net/bluetooth/hci_request.h >> +++ b/net/bluetooth/hci_request.h >> @@ -80,6 +80,9 @@ void hci_req_clear_adv_instance(struct hci_dev *hdev, = struct sock *sk, >> struct hci_request *req, u8 instance, >> bool force); >> >> +int __hci_req_start_ext_adv(struct hci_request *req, u8 instance); >> +void __hci_req_enable_ext_advertising(struct hci_request *req); >> + >> void __hci_req_update_class(struct hci_request *req); >> >> /* Returns true if HCI commands were queued */ >> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c >> index 1c2ed42..22997f8 100644 >> --- a/net/bluetooth/mgmt.c >> +++ b/net/bluetooth/mgmt.c >> @@ -961,7 +961,10 @@ static void rpa_expired(struct work_struct *work) >> * function. >> */ >> hci_req_init(&req, hdev); >> - __hci_req_enable_advertising(&req); >> + if (ext_adv_capable(hdev)) >> + __hci_req_start_ext_adv(&req, hdev->cur_adv_instance); >> + elsey >> + __hci_req_enable_advertising(&req); >> hci_req_run(&req, NULL); >> } >> >> @@ -4405,9 +4408,14 @@ static int set_advertising(struct sock *sk, struc= t hci_dev *hdev, void *data, >> * HCI_ADVERTISING flag is not yet set. >> */ >> hdev->cur_adv_instance =3D 0x00; >> - __hci_req_update_adv_data(&req, 0x00); >> - __hci_req_update_scan_rsp_data(&req, 0x00); >> - __hci_req_enable_advertising(&req); >> + >> + if (ext_adv_capable(hdev)) { >> + __hci_req_start_ext_adv(&req, 0x00); >> + } else { >> + __hci_req_update_adv_data(&req, 0x00); >> + __hci_req_update_scan_rsp_data(&req, 0x00); >> + __hci_req_enable_advertising(&req); >> + } >> } else { >> __hci_req_disable_advertising(&req); >> } >> @@ -6335,7 +6343,8 @@ static u32 get_supported_adv_flags(struct hci_dev = *hdev) >> flags |=3D MGMT_ADV_FLAG_APPEARANCE; >> flags |=3D MGMT_ADV_FLAG_LOCAL_NAME; >> >> - if (hdev->adv_tx_power !=3D HCI_TX_POWER_INVALID) >> + if ((hdev->adv_tx_power !=3D HCI_TX_POWER_INVALID) || >> + ext_adv_capable(hdev)) >> flags |=3D MGMT_ADV_FLAG_TX_POWER; > > See comments above on how this needs to be handled differently. As mentioned above, in ext adv, tx_power will be always valid, so this code looks fine for me. Plz let me know if i am missing something. Thanks, Jaganath