Hi Marcel,
On 8 May 2014 17:26, Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
> Hi Andrzej,
>
>> This patch adds support for Get Connection Information mgmt command
>> which can be used to query for information about connection, i.e. RSSI
>> or local TX power level.
>>
>> RSSI is returned as response parameter while any other information is
>> returned in standard EIR format.
>>
>> Signed-off-by: Andrzej Kaczmarek <andrzej.kaczmarek@xxxxxxxxx>
>> ---
>> include/net/bluetooth/hci_core.h | 2 +
>> include/net/bluetooth/mgmt.h | 16 ++++
>> net/bluetooth/mgmt.c | 176 +++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 194 insertions(+)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 30d245f..8d7d48b 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -378,6 +378,8 @@ struct hci_conn {
>> __s8 tx_power;
>> unsigned long flags;
>>
>> + unsigned long last_info_read;
>> +
>
> I will clearly mark this as timestamp. Similar to what we do with the inquiry cache.
>
>> __u8 remote_cap;
>> __u8 remote_auth;
>> __u8 remote_id;
>> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
>> index d4b571c..c26b7e2 100644
>> --- a/include/net/bluetooth/mgmt.h
>> +++ b/include/net/bluetooth/mgmt.h
>> @@ -409,6 +409,22 @@ struct mgmt_cp_load_irks {
>> } __packed;
>> #define MGMT_LOAD_IRKS_SIZE 2
>>
>> +#define MGMT_CONN_INFO_DATA_TX_POWER 0x00000001
>> +
>> +#define MGMT_OP_GET_CONN_INFORMATION 0x0031
>> +struct mgmt_cp_get_conn_information {
>> + struct mgmt_addr_info addr;
>> + __u32 data_type;
>> +} __packed;
>> +#define MGMT_GET_CONN_INFORMATION_SIZE (MGMT_ADDR_INFO_SIZE + 4)
>> +struct mgmt_rp_get_conn_information {
>> + struct mgmt_addr_info addr;
>> + __s8 rssi;
>> + __u32 flags;
>> + __le16 eir_len;
>> + __u8 eir[0];
>> +} __packed;
>
> Short this to GET_CONN_INFO and get_conn_info please. We never use the full “information” name.
>
>> +
>> #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 bb3188f..58074c8 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -83,6 +83,7 @@ static const u16 mgmt_commands[] = {
>> MGMT_OP_SET_DEBUG_KEYS,
>> MGMT_OP_SET_PRIVACY,
>> MGMT_OP_LOAD_IRKS,
>> + MGMT_OP_GET_CONN_INFORMATION,
>
> Short this to GET_CONN_INFO.
>
>> };
>>
>> static const u16 mgmt_events[] = {
>> @@ -113,6 +114,8 @@ static const u16 mgmt_events[] = {
>>
>> #define CACHE_TIMEOUT msecs_to_jiffies(2 * 1000)
>>
>> +#define CONN_INFO_INTERVAL msecs_to_jiffies(2 * 1000)
>> +
>
> With the inquiry cache we are calling this MAX_AGE. So we should follow this here as well. INTERVAL is the wrong name here.
>
>> #define hdev_is_powered(hdev) (test_bit(HCI_UP, &hdev->flags) && \
>> !test_bit(HCI_AUTO_OFF, &hdev->dev_flags))
>>
>> @@ -4574,6 +4577,178 @@ static int load_long_term_keys(struct sock *sk, struct hci_dev *hdev,
>> return err;
>> }
>>
>> +struct cmd_conn_lookup {
>> + struct hci_conn *conn;
>> + u8 mgmt_status;
>> +};
>> +
>> +static void conn_info_complete(struct pending_cmd *cmd, void *data)
>> +{
>> + struct cmd_conn_lookup *match = data;
>> + struct mgmt_cp_get_conn_information *cp;
>> + struct mgmt_rp_get_conn_information *rp;
>> + char buf[sizeof(*rp) + 3];
>> + struct hci_conn *conn = cmd->user_data;
>> + __u16 eir_len = 0;
>> +
>> + if (conn != match->conn)
>> + return;
>> +
>> + cp = (struct mgmt_cp_get_conn_information *) cmd->param;
>> + rp = (struct mgmt_rp_get_conn_information *) buf;
>> +
>> + memset(buf, 0, sizeof(buf));
>> + memcpy(&rp->addr, &cp->addr, sizeof(rp->addr));
>> +
>> + if (match->mgmt_status) {
>> + cmd_complete(cmd->sk, cmd->index, MGMT_OP_GET_CONN_INFORMATION,
>> + match->mgmt_status, rp, sizeof(*rp));
>> + goto done;
>> + }
>> +
>> + rp->rssi = conn->rssi;
>> +
>> + if (cp->data_type & MGMT_CONN_INFO_DATA_TX_POWER) {
>> + eir_len = eir_append_data(rp->eir, 0, EIR_TX_POWER,
>> + &conn->tx_power,
>> + sizeof(conn->tx_power));
>> +
>> + rp->eir_len = cpu_to_le16(eir_len);
>> + }
>> +
>> + cmd_complete(cmd->sk, cmd->index, MGMT_OP_GET_CONN_INFORMATION,
>> + MGMT_STATUS_SUCCESS, rp, sizeof(*rp) + eir_len);
>> +
>> +done:
>> + hci_conn_drop(conn);
>> +
>> + mgmt_pending_remove(cmd);
>> +}
>> +
>> +static void conn_info_query_complete(struct hci_dev *hdev, u8 status,
>> + void *data)
>> +{
>> + struct hci_conn *conn = data;
>> + struct cmd_conn_lookup match = { conn, mgmt_status(status) };
>> +
>> + BT_DBG("status 0x%02x", status);
>> +
>> + if (!status)
>> + conn->last_info_read = jiffies;
>> +
>> + hci_dev_lock(hdev);
>> +
>> + mgmt_pending_foreach(MGMT_OP_GET_CONN_INFORMATION, hdev,
>> + conn_info_complete, &match);
>> +
>> + hci_dev_unlock(hdev);
>> +}
>
> This whole thing is a bit convoluted. Can we get this done simpler or add some extra comments for people follow through on how this works.
I'll see if this can be made any simpler and add some extra comments at least.
> We do need a mgmt-tester test case for this of course.
Sure, I'll create one once we have final specification for this command.
>> +
>> +static bool check_pending_conn_info(struct hci_dev *hdev, struct hci_conn *conn)
>> +{
>> + struct pending_cmd *cmd;
>> +
>> + list_for_each_entry(cmd, &hdev->mgmt_pending, list) {
>> + if (cmd->opcode == MGMT_OP_GET_CONN_INFORMATION &&
>> + cmd->user_data == conn)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static int get_conn_info(struct sock *sk, struct hci_dev *hdev, void *data,
>> + u16 len)
>> +{
>> + struct mgmt_cp_get_conn_information *cp = data;
>> + struct mgmt_rp_get_conn_information rp;
>> + struct hci_conn *conn;
>> + struct pending_cmd *cmd;
>> + struct hci_request req;
>> + bool is_pending;
>> + int err = 0;
>> +
>> + BT_DBG("%s", hdev->name);
>> +
>> + memset(&rp, 0, sizeof(rp));
>> + memcpy(&rp.addr, &cp->addr, sizeof(rp.addr));
>
> Other locations in the code are doing this:
>
> memset(&rp, 0, sizeof(rp));
> bacpy(&rp.addr.bdaddr, &cp->addr.bdaddr);
> rp.addr.type = cp->addr.type;
>
>> +
>> + if (!bdaddr_type_is_valid(cp->addr.type))
>> + return cmd_complete(sk, hdev->id, MGMT_OP_GET_CONN_INFORMATION,
>> + MGMT_STATUS_INVALID_PARAMS,
>> + &rp, sizeof(rp));
>
>> +
>> + hci_dev_lock(hdev);
>> +
>> + if (!hdev_is_powered(hdev)) {
>> + err = cmd_complete(sk, hdev->id, MGMT_OP_GET_CONN_INFORMATION,
>> + MGMT_STATUS_NOT_POWERED, &rp, sizeof(rp));
>> + goto unlock;
>> + }
>> +
>> + if (cp->addr.type == BDADDR_BREDR)
>> + conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
>> + &cp->addr.bdaddr);
>> + else
>> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->addr.bdaddr);
>> +
>> + if (!conn || conn->state == BT_OPEN || conn->state == BT_CLOSED) {
>
> Lets check for != BT_CONNECTED here.
>
>> + err = cmd_complete(sk, hdev->id, MGMT_OP_GET_CONN_INFORMATION,
>> + MGMT_STATUS_NOT_CONNECTED, &rp, sizeof(rp));
>> + goto unlock;
>> + }
>> +
>> + is_pending = check_pending_conn_info(hdev, conn);
>
> Don’t we have a mgmt_pending_find for this.
We have but there's similar problem as with hci_request in other patch
- we want pending command for given hci_conn. So this actually does
something like mgmt_pending_find_if (with cmp callback). Do you want
to have such generic function instead of dedicated helper?
BR,
Andrzej
--
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
