Re: [PATCH v4 08/13] Bluetooth: Impmlement extended adv enable

[Jaganath K <jaganath.k.os@gmail.com>]

Hi Marcel,

On Sun, Jul 15, 2018 at 12:08 AM, Marcel Holtmann <marcel@holtmann.org> 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 <jaganathx.kanakkassery@intel.com>
>> ---
>> 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