Re: [RFC][PATCH v4 BlueZ 4/4] Bluetooth: mgmt: Added new MGMT_OP_ADD_CONN_PARAM command

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Luiz,

On 17/03/17 17:44, Luiz Augusto von Dentz wrote:
> Hi Felipe,
> 
> On Tue, Mar 14, 2017 at 8:30 PM, Felipe F. Tonello <eu@xxxxxxxxxxxxxxxxx> wrote:
>> This command is similar to MGMT_OP_LOAD_CONN_PARAM with the exception
>> that it only updates one connection parameter and it doesn't cleanup
>> previous parameters set.
>>
>> This is useful for applications to update one specific connection
>> parameter and not caring about other cached connection parameters.
>>
>> Signed-off-by: Felipe F. Tonello <eu@xxxxxxxxxxxxxxxxx>
>> ---
>>  include/net/bluetooth/mgmt.h |  10 +++++
>>  net/bluetooth/mgmt.c         | 103 ++++++++++++++++++++++++++++---------------
>>  2 files changed, 77 insertions(+), 36 deletions(-)
>>
>> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
>> index 72a456bbbcd5..34e5fae8c9d5 100644
>> --- a/include/net/bluetooth/mgmt.h
>> +++ b/include/net/bluetooth/mgmt.h
>> @@ -604,6 +604,16 @@ struct mgmt_cp_set_appearance {
>>  } __packed;
>>  #define MGMT_SET_APPEARANCE_SIZE       2
>>
>> +#define MGMT_OP_ADD_CONN_PARAM         0x0044
>> +struct mgmt_cp_add_conn_param {
>> +       struct mgmt_addr_info addr;
>> +       __le16 min_interval;
>> +       __le16 max_interval;
>> +       __le16 latency;
>> +       __le16 timeout;
>> +} __packed;
>> +#define MGMT_ADD_CONN_PARAM_SIZE       (MGMT_ADDR_INFO_SIZE + 8)
> 
> I do remember mentioning that this command shall accept multiple
> setting at time, just like load.

Yes, that is correct. But as I mentioned on the previous email I sent, I
think this is not the correct approach, since this command effectively
would be only used in the case of peripheral using a Slave Connection
Interval AD. In this case, to avoid multiple calls to this command, I
believe we should parse this particular AD in the Kernel, and just
update the connection parameters accordingly. There is no need to let
user-space know about this, since it is in the AD anyway (it will be
persistent by the slave peripheral wishes).

> 
>> +
>>  #define MGMT_EV_CMD_COMPLETE           0x0001
>>  struct mgmt_ev_cmd_complete {
>>         __le16  opcode;
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 1fba2a03f8ae..b8e86bf1fe1b 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -106,6 +106,7 @@ static const u16 mgmt_commands[] = {
>>         MGMT_OP_START_LIMITED_DISCOVERY,
>>         MGMT_OP_READ_EXT_INFO,
>>         MGMT_OP_SET_APPEARANCE,
>> +       MGMT_OP_ADD_CONN_PARAM,
>>  };
>>
>>  static const u16 mgmt_events[] = {
>> @@ -5462,6 +5463,69 @@ static int remove_device(struct sock *sk, struct hci_dev *hdev,
>>         return err;
>>  }
>>
>> +/* This function requires the caller holds hdev->lock */
>> +static u8 really_add_conn_param(struct hci_dev *hdev,
>> +                               struct mgmt_cp_add_conn_param *param)
>> +{
>> +       struct hci_conn_params *hci_param;
>> +       u16 min, max, latency, timeout;
>> +       u8 addr_type;
>> +
>> +       if (param->addr.type == BDADDR_LE_PUBLIC)
>> +               addr_type = ADDR_LE_DEV_PUBLIC;
>> +       else if (param->addr.type == BDADDR_LE_RANDOM)
>> +               addr_type = ADDR_LE_DEV_RANDOM;
>> +       else
>> +               return MGMT_STATUS_INVALID_PARAMS;
>> +
>> +       min = le16_to_cpu(param->min_interval);
>> +       max = le16_to_cpu(param->max_interval);
>> +       latency = le16_to_cpu(param->latency);
>> +       timeout = le16_to_cpu(param->timeout);
>> +
>> +       BT_DBG("min 0x%04x max 0x%04x latency 0x%04x timeout 0x%04x",
>> +              min, max, latency, timeout);
>> +
>> +       if (hci_check_conn_params(min, max, latency, timeout) < 0) {
>> +               BT_ERR("Ignoring invalid connection parameters");
>> +               return MGMT_STATUS_INVALID_PARAMS;
>> +       }
>> +
>> +       hci_param = hci_conn_params_add(hdev, &param->addr.bdaddr,
>> +                                       addr_type);
>> +       if (!hci_param) {
>> +               BT_ERR("Failed to add connection parameters");
>> +               return MGMT_STATUS_FAILED;
>> +       }
>> +
>> +       hci_param->conn_min_interval = min;
>> +       hci_param->conn_max_interval = max;
>> +       hci_param->conn_latency = latency;
>> +       hci_param->supervision_timeout = timeout;
>> +
>> +       return 0;
>> +}
>> +
>> +static int add_conn_param(struct sock *sk, struct hci_dev *hdev, void *data,
>> +                            u16 len)
>> +{
>> +       struct mgmt_cp_add_conn_param *cp = data;
>> +       u8 cmd_status;
>> +
>> +       if (!lmp_le_capable(hdev))
>> +               return mgmt_cmd_status(sk, hdev->id, MGMT_OP_ADD_CONN_PARAM,
>> +                                      MGMT_STATUS_NOT_SUPPORTED);
>> +
>> +       hci_dev_lock(hdev);
>> +
>> +       cmd_status = really_add_conn_param(hdev, cp);
>> +
>> +       hci_dev_unlock(hdev);
>> +
>> +       return mgmt_cmd_complete(sk, hdev->id, MGMT_OP_ADD_CONN_PARAM,
>> +                                cmd_status, NULL, 0);
>> +}
>> +
>>  static int load_conn_param(struct sock *sk, struct hci_dev *hdev, void *data,
>>                            u16 len)
>>  {
>> @@ -5500,46 +5564,12 @@ static int load_conn_param(struct sock *sk, struct hci_dev *hdev, void *data,
>>
>>         for (i = 0; i < param_count; i++) {
>>                 struct mgmt_conn_param *param = &cp->params[i];
>> -               struct hci_conn_params *hci_param;
>> -               u16 min, max, latency, timeout;
>> -               u8 addr_type;
>>
>>                 BT_DBG("Adding %pMR (type %u)", &param->addr.bdaddr,
>>                        param->addr.type);
>>
>> -               if (param->addr.type == BDADDR_LE_PUBLIC) {
>> -                       addr_type = ADDR_LE_DEV_PUBLIC;
>> -               } else if (param->addr.type == BDADDR_LE_RANDOM) {
>> -                       addr_type = ADDR_LE_DEV_RANDOM;
>> -               } else {
>> -                       BT_ERR("Ignoring invalid connection parameters");
>> -                       continue;
>> -               }
>> -
>> -               min = le16_to_cpu(param->min_interval);
>> -               max = le16_to_cpu(param->max_interval);
>> -               latency = le16_to_cpu(param->latency);
>> -               timeout = le16_to_cpu(param->timeout);
>> -
>> -               BT_DBG("min 0x%04x max 0x%04x latency 0x%04x timeout 0x%04x",
>> -                      min, max, latency, timeout);
>> -
>> -               if (hci_check_conn_params(min, max, latency, timeout) < 0) {
>> -                       BT_ERR("Ignoring invalid connection parameters");
>> -                       continue;
>> -               }
>> -
>> -               hci_param = hci_conn_params_add(hdev, &param->addr.bdaddr,
>> -                                               addr_type);
>> -               if (!hci_param) {
>> -                       BT_ERR("Failed to add connection parameters");
>> -                       continue;
>> -               }
>> -
>> -               hci_param->conn_min_interval = min;
>> -               hci_param->conn_max_interval = max;
>> -               hci_param->conn_latency = latency;
>> -               hci_param->supervision_timeout = timeout;
>> +               really_add_conn_param(hdev,
>> +                       (struct mgmt_cp_add_conn_param *)param);
> 
> Once Add support more than one setting the only real difference from
> Load should be that Load does reset the settings before adding,
> everything else remains the same.
> 
>>         }
>>
>>         hci_dev_unlock(hdev);
>> @@ -6538,6 +6568,7 @@ static const struct hci_mgmt_handler mgmt_handlers[] = {
>>         { read_ext_controller_info,MGMT_READ_EXT_INFO_SIZE,
>>                                                 HCI_MGMT_UNTRUSTED },
>>         { set_appearance,          MGMT_SET_APPEARANCE_SIZE },
>> +       { add_conn_param,          MGMT_ADD_CONN_PARAM_SIZE },
>>  };
>>
>>  void mgmt_index_added(struct hci_dev *hdev)
>> --
>> 2.12.0
>>
> 

-- 
Felipe

Attachment: 0x92698E6A.asc
Description: application/pgp-keys


[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux