| Message ID | 20210520164213.1381518-1-luiz.dentz@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] Bluetooth: Add helper for serialized HCI command execution | expand |
Hi Luiz, > This make use of hci_cmd_sync_queue for the following MGMT commands: > > Set Device Class > Set Device ID > Add UUID > Remove UUID > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > include/net/bluetooth/hci_core.h | 2 + > net/bluetooth/Makefile | 2 +- > net/bluetooth/hci_request.c | 18 +++ > net/bluetooth/hci_sync.c | 241 +++++++++++++++++++++++++++++++ > net/bluetooth/hci_sync.h | 9 ++ > net/bluetooth/mgmt.c | 123 ++++++++-------- > 6 files changed, 328 insertions(+), 67 deletions(-) > create mode 100644 net/bluetooth/hci_sync.c > create mode 100644 net/bluetooth/hci_sync.h I was thinking to put things into hci_bredr.c and hci_le.c. > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index aecbdf99c216..2494c547a622 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -1699,6 +1699,8 @@ struct sk_buff *__hci_cmd_sync(struct hci_dev *hdev, u16 opcode, u32 plen, > const void *param, u32 timeout); > struct sk_buff *__hci_cmd_sync_ev(struct hci_dev *hdev, u16 opcode, u32 plen, > const void *param, u8 event, u32 timeout); > +int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, > + const void *param, u32 timeout); > int __hci_cmd_send(struct hci_dev *hdev, u16 opcode, u32 plen, > const void *param); > > diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile > index cc0995301f93..ab46b2b4f3cb 100644 > --- a/net/bluetooth/Makefile > +++ b/net/bluetooth/Makefile > @@ -14,7 +14,7 @@ bluetooth_6lowpan-y := 6lowpan.o > > bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \ > hci_sock.o hci_sysfs.o l2cap_core.o l2cap_sock.o smp.o lib.o \ > - ecdh_helper.o hci_request.o mgmt_util.o mgmt_config.o > + ecdh_helper.o hci_request.o hci_sync.o mgmt_util.o mgmt_config.o > > bluetooth-$(CONFIG_BT_BREDR) += sco.o > bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o > diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c > index fa9125b782f8..f390ac33ced9 100644 > --- a/net/bluetooth/hci_request.c > +++ b/net/bluetooth/hci_request.c > @@ -189,6 +189,24 @@ struct sk_buff *__hci_cmd_sync(struct hci_dev *hdev, u16 opcode, u32 plen, > } > EXPORT_SYMBOL(__hci_cmd_sync); > > +int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, > + const void *param, u32 timeout) > +{ > + struct sk_buff *skb; > + uint8_t status; Use u8 here since that is not user space. And drop the timeout parameter. We always use the same anyway. > + > + skb = __hci_cmd_sync(hdev, opcode, plen, param, timeout); > + if (IS_ERR(skb)) > + return PTR_ERR(skb); > + > + status = skb->data[0]; > + > + kfree_skb(skb); > + > + return status; > +} > +EXPORT_SYMBOL(__hci_cmd_sync_status); Either we use u8 as return value or we convert it to the matching errno value. > + > /* Execute request and wait for completion. */ > int __hci_req_sync(struct hci_dev *hdev, int (*func)(struct hci_request *req, > unsigned long opt), > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > new file mode 100644 > index 000000000000..5b73fcf09c18 > --- /dev/null > +++ b/net/bluetooth/hci_sync.c > @@ -0,0 +1,241 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * BlueZ - Bluetooth protocol stack for Linux > + * > + * Copyright (C) 2021 Intel Corporation > + */ > + > +#include <net/bluetooth/bluetooth.h> > +#include <net/bluetooth/hci_core.h> > + > +#include "hci_request.h" > +#include "hci_sync.h" > + > +#define PNP_INFO_SVCLASS_ID 0x1200 > + > +static u8 *create_uuid16_list(struct hci_dev *hdev, u8 *data, ptrdiff_t len) > +{ > + u8 *ptr = data, *uuids_start = NULL; > + struct bt_uuid *uuid; > + > + if (len < 4) > + return ptr; > + > + list_for_each_entry(uuid, &hdev->uuids, list) { > + u16 uuid16; > + > + if (uuid->size != 16) > + continue; > + > + uuid16 = get_unaligned_le16(&uuid->uuid[12]); > + if (uuid16 < 0x1100) > + continue; > + > + if (uuid16 == PNP_INFO_SVCLASS_ID) > + continue; > + > + if (!uuids_start) { > + uuids_start = ptr; > + uuids_start[0] = 1; > + uuids_start[1] = EIR_UUID16_ALL; > + ptr += 2; > + } > + > + /* Stop if not enough space to put next UUID */ > + if ((ptr - data) + sizeof(u16) > len) { > + uuids_start[1] = EIR_UUID16_SOME; > + break; > + } > + > + *ptr++ = (uuid16 & 0x00ff); > + *ptr++ = (uuid16 & 0xff00) >> 8; > + uuids_start[0] += sizeof(uuid16); > + } > + > + return ptr; > +} > + > +static u8 *create_uuid32_list(struct hci_dev *hdev, u8 *data, ptrdiff_t len) > +{ > + u8 *ptr = data, *uuids_start = NULL; > + struct bt_uuid *uuid; > + > + if (len < 6) > + return ptr; > + > + list_for_each_entry(uuid, &hdev->uuids, list) { > + if (uuid->size != 32) > + continue; > + > + if (!uuids_start) { > + uuids_start = ptr; > + uuids_start[0] = 1; > + uuids_start[1] = EIR_UUID32_ALL; > + ptr += 2; > + } > + > + /* Stop if not enough space to put next UUID */ > + if ((ptr - data) + sizeof(u32) > len) { > + uuids_start[1] = EIR_UUID32_SOME; > + break; > + } > + > + memcpy(ptr, &uuid->uuid[12], sizeof(u32)); > + ptr += sizeof(u32); > + uuids_start[0] += sizeof(u32); > + } > + > + return ptr; > +} > + > +static u8 *create_uuid128_list(struct hci_dev *hdev, u8 *data, ptrdiff_t len) > +{ > + u8 *ptr = data, *uuids_start = NULL; > + struct bt_uuid *uuid; > + > + if (len < 18) > + return ptr; > + > + list_for_each_entry(uuid, &hdev->uuids, list) { > + if (uuid->size != 128) > + continue; > + > + if (!uuids_start) { > + uuids_start = ptr; > + uuids_start[0] = 1; > + uuids_start[1] = EIR_UUID128_ALL; > + ptr += 2; > + } > + > + /* Stop if not enough space to put next UUID */ > + if ((ptr - data) + 16 > len) { > + uuids_start[1] = EIR_UUID128_SOME; > + break; > + } > + > + memcpy(ptr, uuid->uuid, 16); > + ptr += 16; > + uuids_start[0] += 16; > + } > + > + return ptr; > +} > + > +static void create_eir(struct hci_dev *hdev, u8 *data) > +{ > + u8 *ptr = data; > + size_t name_len; > + > + name_len = strlen(hdev->dev_name); > + > + if (name_len > 0) { > + /* EIR Data type */ > + if (name_len > 48) { > + name_len = 48; > + ptr[1] = EIR_NAME_SHORT; > + } else > + ptr[1] = EIR_NAME_COMPLETE; > + > + /* EIR Data length */ > + ptr[0] = name_len + 1; > + > + memcpy(ptr + 2, hdev->dev_name, name_len); > + > + ptr += (name_len + 2); > + } > + > + if (hdev->inq_tx_power != HCI_TX_POWER_INVALID) { > + ptr[0] = 2; > + ptr[1] = EIR_TX_POWER; > + ptr[2] = (u8) hdev->inq_tx_power; > + > + ptr += 3; > + } > + > + if (hdev->devid_source > 0) { > + ptr[0] = 9; > + ptr[1] = EIR_DEVICE_ID; > + > + put_unaligned_le16(hdev->devid_source, ptr + 2); > + put_unaligned_le16(hdev->devid_vendor, ptr + 4); > + put_unaligned_le16(hdev->devid_product, ptr + 6); > + put_unaligned_le16(hdev->devid_version, ptr + 8); > + > + ptr += 10; > + } > + > + ptr = create_uuid16_list(hdev, ptr, HCI_MAX_EIR_LENGTH - (ptr - data)); > + ptr = create_uuid32_list(hdev, ptr, HCI_MAX_EIR_LENGTH - (ptr - data)); > + ptr = create_uuid128_list(hdev, ptr, HCI_MAX_EIR_LENGTH - (ptr - data)); > +} > + > +int __hci_update_eir_sync(struct hci_dev *hdev) > +{ > + struct hci_cp_write_eir cp; > + > + bt_dev_dbg(hdev, ""); > + > + if (!hdev_is_powered(hdev)) > + return 0; > + > + if (!lmp_ext_inq_capable(hdev)) > + return 0; > + > + if (!hci_dev_test_flag(hdev, HCI_SSP_ENABLED)) > + return 0; > + > + if (hci_dev_test_flag(hdev, HCI_SERVICE_CACHE)) > + return 0; > + > + memset(&cp, 0, sizeof(cp)); > + > + create_eir(hdev, cp.data); > + > + if (memcmp(cp.data, hdev->eir, sizeof(cp.data)) == 0) > + return 0; > + > + memcpy(hdev->eir, cp.data, sizeof(cp.data)); > + > + return __hci_cmd_sync_status(hdev, HCI_OP_WRITE_EIR, sizeof(cp), &cp, > + HCI_CMD_TIMEOUT); > +} > + > +static u8 get_service_classes(struct hci_dev *hdev) > +{ > + struct bt_uuid *uuid; > + u8 val = 0; > + > + list_for_each_entry(uuid, &hdev->uuids, list) > + val |= uuid->svc_hint; > + > + return val; > +} > + > +int __hci_update_class_sync(struct hci_dev *hdev) > +{ > + u8 cod[3]; > + > + bt_dev_dbg(hdev, ""); > + > + if (!hdev_is_powered(hdev)) > + return 0; > + > + if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) > + return 0; > + > + if (hci_dev_test_flag(hdev, HCI_SERVICE_CACHE)) > + return 0; > + > + cod[0] = hdev->minor_class; > + cod[1] = hdev->major_class; > + cod[2] = get_service_classes(hdev); > + > + if (hci_dev_test_flag(hdev, HCI_LIMITED_DISCOVERABLE)) > + cod[1] |= 0x20; > + > + if (memcmp(cod, hdev->dev_class, 3) == 0) > + return 0; > + > + return __hci_cmd_sync_status(hdev, HCI_OP_WRITE_CLASS_OF_DEV, > + sizeof(cod), cod, HCI_CMD_TIMEOUT); > +} > diff --git a/net/bluetooth/hci_sync.h b/net/bluetooth/hci_sync.h > new file mode 100644 > index 000000000000..735ff4b46e86 > --- /dev/null > +++ b/net/bluetooth/hci_sync.h > @@ -0,0 +1,9 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * BlueZ - Bluetooth protocol stack for Linux > + * > + * Copyright (C) 2021 Intel Corporation > + */ > + > +int __hci_update_eir_sync(struct hci_dev *hdev); > +int __hci_update_class_sync(struct hci_dev *hdev); > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c > index b44e19c69c44..96759c166678 100644 > --- a/net/bluetooth/mgmt.c > +++ b/net/bluetooth/mgmt.c > @@ -34,6 +34,7 @@ > #include <net/bluetooth/mgmt.h> > > #include "hci_request.h" > +#include "hci_sync.h" > #include "smp.h" > #include "mgmt_util.h" > #include "mgmt_config.h" > @@ -950,25 +951,21 @@ bool mgmt_get_connectable(struct hci_dev *hdev) > return hci_dev_test_flag(hdev, HCI_CONNECTABLE); > } > > +static void __service_cache_sync(struct hci_dev *hdev, void *data) > +{ > + __hci_update_eir_sync(hdev); > + __hci_update_class_sync(hdev); > +} > + > static void service_cache_off(struct work_struct *work) > { > struct hci_dev *hdev = container_of(work, struct hci_dev, > service_cache.work); > - struct hci_request req; > > if (!hci_dev_test_and_clear_flag(hdev, HCI_SERVICE_CACHE)) > return; > > - hci_req_init(&req, hdev); > - > - hci_dev_lock(hdev); > - > - __hci_req_update_eir(&req); > - __hci_req_update_class(&req); > - > - hci_dev_unlock(hdev); > - > - hci_req_run(&req, NULL); > + hci_cmd_sync_queue(hdev, __service_cache_sync, NULL); > } > > static void rpa_expired(struct work_struct *work) > @@ -2093,8 +2090,17 @@ static void mgmt_class_complete(struct hci_dev *hdev, u16 mgmt_op, u8 status) > hci_dev_unlock(hdev); > } > > -static void add_uuid_complete(struct hci_dev *hdev, u8 status, u16 opcode) > +static void __add_uuid_sync(struct hci_dev *hdev, void *data) > { > + uint8_t status; > + > + status = __hci_update_class_sync(hdev); > + if (status) > + goto done; > + > + status = __hci_update_eir_sync(hdev); > + > +done: > bt_dev_dbg(hdev, "status 0x%02x", status); > > mgmt_class_complete(hdev, MGMT_OP_ADD_UUID, status); > @@ -2104,7 +2110,6 @@ static int add_uuid(struct sock *sk, struct hci_dev *hdev, void *data, u16 len) > { > struct mgmt_cp_add_uuid *cp = data; > struct mgmt_pending_cmd *cmd; > - struct hci_request req; > struct bt_uuid *uuid; > int err; > > @@ -2130,20 +2135,9 @@ static int add_uuid(struct sock *sk, struct hci_dev *hdev, void *data, u16 len) > > list_add_tail(&uuid->list, &hdev->uuids); > > - hci_req_init(&req, hdev); > - > - __hci_req_update_class(&req); > - __hci_req_update_eir(&req); > - > - err = hci_req_run(&req, add_uuid_complete); > - if (err < 0) { > - if (err != -ENODATA) > - goto failed; > - > - err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_ADD_UUID, 0, > - hdev->dev_class, 3); > + err = hci_cmd_sync_queue(hdev, __add_uuid_sync, NULL); > + if (err < 0) > goto failed; > - } > > cmd = mgmt_pending_add(sk, MGMT_OP_ADD_UUID, hdev, data, len); > if (!cmd) { > @@ -2172,8 +2166,17 @@ static bool enable_service_cache(struct hci_dev *hdev) > return false; > } > > -static void remove_uuid_complete(struct hci_dev *hdev, u8 status, u16 opcode) > +static void __remove_uuid_sync(struct hci_dev *hdev, void *data) > { > + uint8_t status; > + > + status = __hci_update_class_sync(hdev); > + if (status) > + goto done; > + > + status = __hci_update_eir_sync(hdev); > + > +done: > bt_dev_dbg(hdev, "status 0x%02x", status); > > mgmt_class_complete(hdev, MGMT_OP_REMOVE_UUID, status); > @@ -2186,7 +2189,6 @@ static int remove_uuid(struct sock *sk, struct hci_dev *hdev, void *data, > struct mgmt_pending_cmd *cmd; > struct bt_uuid *match, *tmp; > u8 bt_uuid_any[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }; > - struct hci_request req; > int err, found; > > bt_dev_dbg(hdev, "sock %p", sk); > @@ -2230,20 +2232,9 @@ static int remove_uuid(struct sock *sk, struct hci_dev *hdev, void *data, > } > > update_class: > - hci_req_init(&req, hdev); > - > - __hci_req_update_class(&req); > - __hci_req_update_eir(&req); > - > - err = hci_req_run(&req, remove_uuid_complete); > - if (err < 0) { > - if (err != -ENODATA) > - goto unlock; > - > - err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_REMOVE_UUID, 0, > - hdev->dev_class, 3); > + err = hci_cmd_sync_queue(hdev, __remove_uuid_sync, NULL); > + if (err < 0) > goto unlock; > - } > > cmd = mgmt_pending_add(sk, MGMT_OP_REMOVE_UUID, hdev, data, len); > if (!cmd) { > @@ -2258,8 +2249,24 @@ static int remove_uuid(struct sock *sk, struct hci_dev *hdev, void *data, > return err; > } > > -static void set_class_complete(struct hci_dev *hdev, u8 status, u16 opcode) > +static void __set_class_sync(struct hci_dev *hdev, void *data) > { > + uint8_t status = 0; > + > + hci_dev_lock(hdev); > + > + if (hci_dev_test_and_clear_flag(hdev, HCI_SERVICE_CACHE)) { > + hci_dev_unlock(hdev); > + cancel_delayed_work_sync(&hdev->service_cache); > + hci_dev_lock(hdev); > + status = __hci_update_eir_sync(hdev); > + } > + > + hci_dev_unlock(hdev); > + > + if (!status) > + status = __hci_update_class_sync(hdev); > + > bt_dev_dbg(hdev, "status 0x%02x", status); > > mgmt_class_complete(hdev, MGMT_OP_SET_DEV_CLASS, status); > @@ -2270,7 +2277,6 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data, > { > struct mgmt_cp_set_dev_class *cp = data; > struct mgmt_pending_cmd *cmd; > - struct hci_request req; > int err; > > bt_dev_dbg(hdev, "sock %p", sk); > @@ -2302,26 +2308,9 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data, > goto unlock; > } > > - hci_req_init(&req, hdev); > - > - if (hci_dev_test_and_clear_flag(hdev, HCI_SERVICE_CACHE)) { > - hci_dev_unlock(hdev); > - cancel_delayed_work_sync(&hdev->service_cache); > - hci_dev_lock(hdev); > - __hci_req_update_eir(&req); > - } > - > - __hci_req_update_class(&req); > - > - err = hci_req_run(&req, set_class_complete); > - if (err < 0) { > - if (err != -ENODATA) > - goto unlock; > - > - err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_SET_DEV_CLASS, 0, > - hdev->dev_class, 3); > + err = hci_cmd_sync_queue(hdev, __set_class_sync, NULL); > + if (err < 0) > goto unlock; > - } > > cmd = mgmt_pending_add(sk, MGMT_OP_SET_DEV_CLASS, hdev, data, len); > if (!cmd) { > @@ -5263,11 +5252,15 @@ static int unblock_device(struct sock *sk, struct hci_dev *hdev, void *data, > return err; > } > > +static void __set_device_id_sync(struct hci_dev *hdev, void *data) > +{ > + __hci_update_eir_sync(hdev); > +} > + > static int set_device_id(struct sock *sk, struct hci_dev *hdev, void *data, > u16 len) > { > struct mgmt_cp_set_device_id *cp = data; > - struct hci_request req; > int err; > __u16 source; > > @@ -5289,9 +5282,7 @@ static int set_device_id(struct sock *sk, struct hci_dev *hdev, void *data, > err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_SET_DEVICE_ID, 0, > NULL, 0); > > - hci_req_init(&req, hdev); > - __hci_req_update_eir(&req); > - hci_req_run(&req, NULL); > + hci_cmd_sync_queue(hdev, __set_device_id_sync, NULL); > > hci_dev_unlock(hdev); I dislike the prefix __sync. They are in a separate anyway. So keep is simple and also scrap the __ prefix. Regards Marcel
Hi Marcel, On Thu, May 20, 2021 at 1:17 PM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Luiz, > > > This make use of hci_cmd_sync_queue for the following MGMT commands: > > > > Set Device Class > > Set Device ID > > Add UUID > > Remove UUID > > > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > --- > > include/net/bluetooth/hci_core.h | 2 + > > net/bluetooth/Makefile | 2 +- > > net/bluetooth/hci_request.c | 18 +++ > > net/bluetooth/hci_sync.c | 241 +++++++++++++++++++++++++++++++ > > net/bluetooth/hci_sync.h | 9 ++ > > net/bluetooth/mgmt.c | 123 ++++++++-------- > > 6 files changed, 328 insertions(+), 67 deletions(-) > > create mode 100644 net/bluetooth/hci_sync.c > > create mode 100644 net/bluetooth/hci_sync.h > > I was thinking to put things into hci_bredr.c and hci_le.c. Initially I thought about that but I was afraid of things that could affect states in both bearers so I went with what I considered to be simpler. > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > index aecbdf99c216..2494c547a622 100644 > > --- a/include/net/bluetooth/hci_core.h > > +++ b/include/net/bluetooth/hci_core.h > > @@ -1699,6 +1699,8 @@ struct sk_buff *__hci_cmd_sync(struct hci_dev *hdev, u16 opcode, u32 plen, > > const void *param, u32 timeout); > > struct sk_buff *__hci_cmd_sync_ev(struct hci_dev *hdev, u16 opcode, u32 plen, > > const void *param, u8 event, u32 timeout); > > +int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, > > + const void *param, u32 timeout); > > int __hci_cmd_send(struct hci_dev *hdev, u16 opcode, u32 plen, > > const void *param); > > > > diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile > > index cc0995301f93..ab46b2b4f3cb 100644 > > --- a/net/bluetooth/Makefile > > +++ b/net/bluetooth/Makefile > > @@ -14,7 +14,7 @@ bluetooth_6lowpan-y := 6lowpan.o > > > > bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \ > > hci_sock.o hci_sysfs.o l2cap_core.o l2cap_sock.o smp.o lib.o \ > > - ecdh_helper.o hci_request.o mgmt_util.o mgmt_config.o > > + ecdh_helper.o hci_request.o hci_sync.o mgmt_util.o mgmt_config.o > > > > bluetooth-$(CONFIG_BT_BREDR) += sco.o > > bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o > > diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c > > index fa9125b782f8..f390ac33ced9 100644 > > --- a/net/bluetooth/hci_request.c > > +++ b/net/bluetooth/hci_request.c > > @@ -189,6 +189,24 @@ struct sk_buff *__hci_cmd_sync(struct hci_dev *hdev, u16 opcode, u32 plen, > > } > > EXPORT_SYMBOL(__hci_cmd_sync); > > > > +int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, > > + const void *param, u32 timeout) > > +{ > > + struct sk_buff *skb; > > + uint8_t status; > > Use u8 here since that is not user space. And drop the timeout parameter. We always use the same anyway. Yep, I fixed that already. > > + > > + skb = __hci_cmd_sync(hdev, opcode, plen, param, timeout); > > + if (IS_ERR(skb)) > > + return PTR_ERR(skb); > > + > > + status = skb->data[0]; > > + > > + kfree_skb(skb); > > + > > + return status; > > +} > > +EXPORT_SYMBOL(__hci_cmd_sync_status); > > Either we use u8 as return value or we convert it to the matching errno value. Ok, I was thinking of converting but it looks like the mgmt can actually report the status, that said we may need to convert the PTR_ERR then. > > + > > /* Execute request and wait for completion. */ > > int __hci_req_sync(struct hci_dev *hdev, int (*func)(struct hci_request *req, > > unsigned long opt), > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > > new file mode 100644 > > index 000000000000..5b73fcf09c18 > > --- /dev/null > > +++ b/net/bluetooth/hci_sync.c > > @@ -0,0 +1,241 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * BlueZ - Bluetooth protocol stack for Linux > > + * > > + * Copyright (C) 2021 Intel Corporation > > + */ > > + > > +#include <net/bluetooth/bluetooth.h> > > +#include <net/bluetooth/hci_core.h> > > + > > +#include "hci_request.h" > > +#include "hci_sync.h" > > + > > +#define PNP_INFO_SVCLASS_ID 0x1200 > > + > > +static u8 *create_uuid16_list(struct hci_dev *hdev, u8 *data, ptrdiff_t len) > > +{ > > + u8 *ptr = data, *uuids_start = NULL; > > + struct bt_uuid *uuid; > > + > > + if (len < 4) > > + return ptr; > > + > > + list_for_each_entry(uuid, &hdev->uuids, list) { > > + u16 uuid16; > > + > > + if (uuid->size != 16) > > + continue; > > + > > + uuid16 = get_unaligned_le16(&uuid->uuid[12]); > > + if (uuid16 < 0x1100) > > + continue; > > + > > + if (uuid16 == PNP_INFO_SVCLASS_ID) > > + continue; > > + > > + if (!uuids_start) { > > + uuids_start = ptr; > > + uuids_start[0] = 1; > > + uuids_start[1] = EIR_UUID16_ALL; > > + ptr += 2; > > + } > > + > > + /* Stop if not enough space to put next UUID */ > > + if ((ptr - data) + sizeof(u16) > len) { > > + uuids_start[1] = EIR_UUID16_SOME; > > + break; > > + } > > + > > + *ptr++ = (uuid16 & 0x00ff); > > + *ptr++ = (uuid16 & 0xff00) >> 8; > > + uuids_start[0] += sizeof(uuid16); > > + } > > + > > + return ptr; > > +} > > + > > +static u8 *create_uuid32_list(struct hci_dev *hdev, u8 *data, ptrdiff_t len) > > +{ > > + u8 *ptr = data, *uuids_start = NULL; > > + struct bt_uuid *uuid; > > + > > + if (len < 6) > > + return ptr; > > + > > + list_for_each_entry(uuid, &hdev->uuids, list) { > > + if (uuid->size != 32) > > + continue; > > + > > + if (!uuids_start) { > > + uuids_start = ptr; > > + uuids_start[0] = 1; > > + uuids_start[1] = EIR_UUID32_ALL; > > + ptr += 2; > > + } > > + > > + /* Stop if not enough space to put next UUID */ > > + if ((ptr - data) + sizeof(u32) > len) { > > + uuids_start[1] = EIR_UUID32_SOME; > > + break; > > + } > > + > > + memcpy(ptr, &uuid->uuid[12], sizeof(u32)); > > + ptr += sizeof(u32); > > + uuids_start[0] += sizeof(u32); > > + } > > + > > + return ptr; > > +} > > + > > +static u8 *create_uuid128_list(struct hci_dev *hdev, u8 *data, ptrdiff_t len) > > +{ > > + u8 *ptr = data, *uuids_start = NULL; > > + struct bt_uuid *uuid; > > + > > + if (len < 18) > > + return ptr; > > + > > + list_for_each_entry(uuid, &hdev->uuids, list) { > > + if (uuid->size != 128) > > + continue; > > + > > + if (!uuids_start) { > > + uuids_start = ptr; > > + uuids_start[0] = 1; > > + uuids_start[1] = EIR_UUID128_ALL; > > + ptr += 2; > > + } > > + > > + /* Stop if not enough space to put next UUID */ > > + if ((ptr - data) + 16 > len) { > > + uuids_start[1] = EIR_UUID128_SOME; > > + break; > > + } > > + > > + memcpy(ptr, uuid->uuid, 16); > > + ptr += 16; > > + uuids_start[0] += 16; > > + } > > + > > + return ptr; > > +} > > + > > +static void create_eir(struct hci_dev *hdev, u8 *data) > > +{ > > + u8 *ptr = data; > > + size_t name_len; > > + > > + name_len = strlen(hdev->dev_name); > > + > > + if (name_len > 0) { > > + /* EIR Data type */ > > + if (name_len > 48) { > > + name_len = 48; > > + ptr[1] = EIR_NAME_SHORT; > > + } else > > + ptr[1] = EIR_NAME_COMPLETE; > > + > > + /* EIR Data length */ > > + ptr[0] = name_len + 1; > > + > > + memcpy(ptr + 2, hdev->dev_name, name_len); > > + > > + ptr += (name_len + 2); > > + } > > + > > + if (hdev->inq_tx_power != HCI_TX_POWER_INVALID) { > > + ptr[0] = 2; > > + ptr[1] = EIR_TX_POWER; > > + ptr[2] = (u8) hdev->inq_tx_power; > > + > > + ptr += 3; > > + } > > + > > + if (hdev->devid_source > 0) { > > + ptr[0] = 9; > > + ptr[1] = EIR_DEVICE_ID; > > + > > + put_unaligned_le16(hdev->devid_source, ptr + 2); > > + put_unaligned_le16(hdev->devid_vendor, ptr + 4); > > + put_unaligned_le16(hdev->devid_product, ptr + 6); > > + put_unaligned_le16(hdev->devid_version, ptr + 8); > > + > > + ptr += 10; > > + } > > + > > + ptr = create_uuid16_list(hdev, ptr, HCI_MAX_EIR_LENGTH - (ptr - data)); > > + ptr = create_uuid32_list(hdev, ptr, HCI_MAX_EIR_LENGTH - (ptr - data)); > > + ptr = create_uuid128_list(hdev, ptr, HCI_MAX_EIR_LENGTH - (ptr - data)); > > +} > > + > > +int __hci_update_eir_sync(struct hci_dev *hdev) > > +{ > > + struct hci_cp_write_eir cp; > > + > > + bt_dev_dbg(hdev, ""); > > + > > + if (!hdev_is_powered(hdev)) > > + return 0; > > + > > + if (!lmp_ext_inq_capable(hdev)) > > + return 0; > > + > > + if (!hci_dev_test_flag(hdev, HCI_SSP_ENABLED)) > > + return 0; > > + > > + if (hci_dev_test_flag(hdev, HCI_SERVICE_CACHE)) > > + return 0; > > + > > + memset(&cp, 0, sizeof(cp)); > > + > > + create_eir(hdev, cp.data); > > + > > + if (memcmp(cp.data, hdev->eir, sizeof(cp.data)) == 0) > > + return 0; > > + > > + memcpy(hdev->eir, cp.data, sizeof(cp.data)); > > + > > + return __hci_cmd_sync_status(hdev, HCI_OP_WRITE_EIR, sizeof(cp), &cp, > > + HCI_CMD_TIMEOUT); > > +} > > + > > +static u8 get_service_classes(struct hci_dev *hdev) > > +{ > > + struct bt_uuid *uuid; > > + u8 val = 0; > > + > > + list_for_each_entry(uuid, &hdev->uuids, list) > > + val |= uuid->svc_hint; > > + > > + return val; > > +} > > + > > +int __hci_update_class_sync(struct hci_dev *hdev) > > +{ > > + u8 cod[3]; > > + > > + bt_dev_dbg(hdev, ""); > > + > > + if (!hdev_is_powered(hdev)) > > + return 0; > > + > > + if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) > > + return 0; > > + > > + if (hci_dev_test_flag(hdev, HCI_SERVICE_CACHE)) > > + return 0; > > + > > + cod[0] = hdev->minor_class; > > + cod[1] = hdev->major_class; > > + cod[2] = get_service_classes(hdev); > > + > > + if (hci_dev_test_flag(hdev, HCI_LIMITED_DISCOVERABLE)) > > + cod[1] |= 0x20; > > + > > + if (memcmp(cod, hdev->dev_class, 3) == 0) > > + return 0; > > + > > + return __hci_cmd_sync_status(hdev, HCI_OP_WRITE_CLASS_OF_DEV, > > + sizeof(cod), cod, HCI_CMD_TIMEOUT); > > +} > > diff --git a/net/bluetooth/hci_sync.h b/net/bluetooth/hci_sync.h > > new file mode 100644 > > index 000000000000..735ff4b46e86 > > --- /dev/null > > +++ b/net/bluetooth/hci_sync.h > > @@ -0,0 +1,9 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * BlueZ - Bluetooth protocol stack for Linux > > + * > > + * Copyright (C) 2021 Intel Corporation > > + */ > > + > > +int __hci_update_eir_sync(struct hci_dev *hdev); > > +int __hci_update_class_sync(struct hci_dev *hdev); > > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c > > index b44e19c69c44..96759c166678 100644 > > --- a/net/bluetooth/mgmt.c > > +++ b/net/bluetooth/mgmt.c > > @@ -34,6 +34,7 @@ > > #include <net/bluetooth/mgmt.h> > > > > #include "hci_request.h" > > +#include "hci_sync.h" > > #include "smp.h" > > #include "mgmt_util.h" > > #include "mgmt_config.h" > > @@ -950,25 +951,21 @@ bool mgmt_get_connectable(struct hci_dev *hdev) > > return hci_dev_test_flag(hdev, HCI_CONNECTABLE); > > } > > > > +static void __service_cache_sync(struct hci_dev *hdev, void *data) > > +{ > > + __hci_update_eir_sync(hdev); > > + __hci_update_class_sync(hdev); > > +} > > + > > static void service_cache_off(struct work_struct *work) > > { > > struct hci_dev *hdev = container_of(work, struct hci_dev, > > service_cache.work); > > - struct hci_request req; > > > > if (!hci_dev_test_and_clear_flag(hdev, HCI_SERVICE_CACHE)) > > return; > > > > - hci_req_init(&req, hdev); > > - > > - hci_dev_lock(hdev); > > - > > - __hci_req_update_eir(&req); > > - __hci_req_update_class(&req); > > - > > - hci_dev_unlock(hdev); > > - > > - hci_req_run(&req, NULL); > > + hci_cmd_sync_queue(hdev, __service_cache_sync, NULL); > > } > > > > static void rpa_expired(struct work_struct *work) > > @@ -2093,8 +2090,17 @@ static void mgmt_class_complete(struct hci_dev *hdev, u16 mgmt_op, u8 status) > > hci_dev_unlock(hdev); > > } > > > > -static void add_uuid_complete(struct hci_dev *hdev, u8 status, u16 opcode) > > +static void __add_uuid_sync(struct hci_dev *hdev, void *data) > > { > > + uint8_t status; > > + > > + status = __hci_update_class_sync(hdev); > > + if (status) > > + goto done; > > + > > + status = __hci_update_eir_sync(hdev); > > + > > +done: > > bt_dev_dbg(hdev, "status 0x%02x", status); > > > > mgmt_class_complete(hdev, MGMT_OP_ADD_UUID, status); > > @@ -2104,7 +2110,6 @@ static int add_uuid(struct sock *sk, struct hci_dev *hdev, void *data, u16 len) > > { > > struct mgmt_cp_add_uuid *cp = data; > > struct mgmt_pending_cmd *cmd; > > - struct hci_request req; > > struct bt_uuid *uuid; > > int err; > > > > @@ -2130,20 +2135,9 @@ static int add_uuid(struct sock *sk, struct hci_dev *hdev, void *data, u16 len) > > > > list_add_tail(&uuid->list, &hdev->uuids); > > > > - hci_req_init(&req, hdev); > > - > > - __hci_req_update_class(&req); > > - __hci_req_update_eir(&req); > > - > > - err = hci_req_run(&req, add_uuid_complete); > > - if (err < 0) { > > - if (err != -ENODATA) > > - goto failed; > > - > > - err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_ADD_UUID, 0, > > - hdev->dev_class, 3); > > + err = hci_cmd_sync_queue(hdev, __add_uuid_sync, NULL); > > + if (err < 0) > > goto failed; > > - } > > > > cmd = mgmt_pending_add(sk, MGMT_OP_ADD_UUID, hdev, data, len); > > if (!cmd) { > > @@ -2172,8 +2166,17 @@ static bool enable_service_cache(struct hci_dev *hdev) > > return false; > > } > > > > -static void remove_uuid_complete(struct hci_dev *hdev, u8 status, u16 opcode) > > +static void __remove_uuid_sync(struct hci_dev *hdev, void *data) > > { > > + uint8_t status; > > + > > + status = __hci_update_class_sync(hdev); > > + if (status) > > + goto done; > > + > > + status = __hci_update_eir_sync(hdev); > > + > > +done: > > bt_dev_dbg(hdev, "status 0x%02x", status); > > > > mgmt_class_complete(hdev, MGMT_OP_REMOVE_UUID, status); > > @@ -2186,7 +2189,6 @@ static int remove_uuid(struct sock *sk, struct hci_dev *hdev, void *data, > > struct mgmt_pending_cmd *cmd; > > struct bt_uuid *match, *tmp; > > u8 bt_uuid_any[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }; > > - struct hci_request req; > > int err, found; > > > > bt_dev_dbg(hdev, "sock %p", sk); > > @@ -2230,20 +2232,9 @@ static int remove_uuid(struct sock *sk, struct hci_dev *hdev, void *data, > > } > > > > update_class: > > - hci_req_init(&req, hdev); > > - > > - __hci_req_update_class(&req); > > - __hci_req_update_eir(&req); > > - > > - err = hci_req_run(&req, remove_uuid_complete); > > - if (err < 0) { > > - if (err != -ENODATA) > > - goto unlock; > > - > > - err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_REMOVE_UUID, 0, > > - hdev->dev_class, 3); > > + err = hci_cmd_sync_queue(hdev, __remove_uuid_sync, NULL); > > + if (err < 0) > > goto unlock; > > - } > > > > cmd = mgmt_pending_add(sk, MGMT_OP_REMOVE_UUID, hdev, data, len); > > if (!cmd) { > > @@ -2258,8 +2249,24 @@ static int remove_uuid(struct sock *sk, struct hci_dev *hdev, void *data, > > return err; > > } > > > > -static void set_class_complete(struct hci_dev *hdev, u8 status, u16 opcode) > > +static void __set_class_sync(struct hci_dev *hdev, void *data) > > { > > + uint8_t status = 0; > > + > > + hci_dev_lock(hdev); > > + > > + if (hci_dev_test_and_clear_flag(hdev, HCI_SERVICE_CACHE)) { > > + hci_dev_unlock(hdev); > > + cancel_delayed_work_sync(&hdev->service_cache); > > + hci_dev_lock(hdev); > > + status = __hci_update_eir_sync(hdev); > > + } > > + > > + hci_dev_unlock(hdev); > > + > > + if (!status) > > + status = __hci_update_class_sync(hdev); > > + > > bt_dev_dbg(hdev, "status 0x%02x", status); > > > > mgmt_class_complete(hdev, MGMT_OP_SET_DEV_CLASS, status); > > @@ -2270,7 +2277,6 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data, > > { > > struct mgmt_cp_set_dev_class *cp = data; > > struct mgmt_pending_cmd *cmd; > > - struct hci_request req; > > int err; > > > > bt_dev_dbg(hdev, "sock %p", sk); > > @@ -2302,26 +2308,9 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data, > > goto unlock; > > } > > > > - hci_req_init(&req, hdev); > > - > > - if (hci_dev_test_and_clear_flag(hdev, HCI_SERVICE_CACHE)) { > > - hci_dev_unlock(hdev); > > - cancel_delayed_work_sync(&hdev->service_cache); > > - hci_dev_lock(hdev); > > - __hci_req_update_eir(&req); > > - } > > - > > - __hci_req_update_class(&req); > > - > > - err = hci_req_run(&req, set_class_complete); > > - if (err < 0) { > > - if (err != -ENODATA) > > - goto unlock; > > - > > - err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_SET_DEV_CLASS, 0, > > - hdev->dev_class, 3); > > + err = hci_cmd_sync_queue(hdev, __set_class_sync, NULL); > > + if (err < 0) > > goto unlock; > > - } > > > > cmd = mgmt_pending_add(sk, MGMT_OP_SET_DEV_CLASS, hdev, data, len); > > if (!cmd) { > > @@ -5263,11 +5252,15 @@ static int unblock_device(struct sock *sk, struct hci_dev *hdev, void *data, > > return err; > > } > > > > +static void __set_device_id_sync(struct hci_dev *hdev, void *data) > > +{ > > + __hci_update_eir_sync(hdev); > > +} > > + > > static int set_device_id(struct sock *sk, struct hci_dev *hdev, void *data, > > u16 len) > > { > > struct mgmt_cp_set_device_id *cp = data; > > - struct hci_request req; > > int err; > > __u16 source; > > > > @@ -5289,9 +5282,7 @@ static int set_device_id(struct sock *sk, struct hci_dev *hdev, void *data, > > err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_SET_DEVICE_ID, 0, > > NULL, 0); > > > > - hci_req_init(&req, hdev); > > - __hci_req_update_eir(&req); > > - hci_req_run(&req, NULL); > > + hci_cmd_sync_queue(hdev, __set_device_id_sync, NULL); > > > > hci_dev_unlock(hdev); > > I dislike the prefix __sync. They are in a separate anyway. So keep is simple and also scrap the __ prefix. Sure, do you want to perhaps have it as hci_sync given those should indicate that they require hci_req_sync_lock to be held, or you think that given the we will be converting everything to use the cmd_sync work that is not really necessary? Btw, as part of the cleanup Im also planning to send a patch moving eir/adv data related code to eir.{c,h}, do you have anything against it? > > Regards > > Marcel > -- Luiz Augusto von Dentz
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 43b08bebae74..aecbdf99c216 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -302,6 +302,14 @@ struct amp_assoc { #define HCI_MAX_PAGES 3 +typedef void (*cmd_sync_work_func_t)(struct hci_dev *hdev, void *data); + +struct cmd_sync_work_entry { + struct list_head list; + cmd_sync_work_func_t func; + void *data; +}; + struct hci_dev { struct list_head list; struct mutex lock; @@ -463,6 +471,9 @@ struct hci_dev { struct work_struct power_on; struct delayed_work power_off; struct work_struct error_reset; + struct work_struct cmd_sync_work; + struct list_head cmd_sync_work_list; + struct mutex cmd_sync_work_lock; __u16 discov_timeout; struct delayed_work discov_off; @@ -1701,6 +1712,9 @@ void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode); struct sk_buff *hci_cmd_sync(struct hci_dev *hdev, u16 opcode, u32 plen, const void *param, u32 timeout); +int hci_cmd_sync_queue(struct hci_dev *hdev, cmd_sync_work_func_t func, + void *data); + u32 hci_conn_get_phy(struct hci_conn *conn); /* ----- HCI Sockets ----- */ diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index 7baf93eda936..5fb0079f64c1 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -2329,6 +2329,67 @@ static void hci_error_reset(struct work_struct *work) hci_dev_do_open(hdev); } +static void hci_cmd_sync_work(struct work_struct *work) +{ + struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_sync_work); + struct cmd_sync_work_entry *entry; + cmd_sync_work_func_t func; + void *data; + + bt_dev_dbg(hdev, ""); + + mutex_lock(&hdev->cmd_sync_work_lock); + entry = list_first_entry(&hdev->cmd_sync_work_list, + struct cmd_sync_work_entry, list); + if (entry) { + list_del(&entry->list); + func = entry->func; + data = entry->data; + kfree(entry); + } else { + func = NULL; + data = NULL; + } + mutex_unlock(&hdev->cmd_sync_work_lock); + + if (func) { + hci_req_sync_lock(hdev); + func(hdev, data); + hci_req_sync_unlock(hdev); + } +} + +int hci_cmd_sync_queue(struct hci_dev *hdev, cmd_sync_work_func_t func, + void *data) +{ + struct cmd_sync_work_entry *entry; + + entry = kmalloc(sizeof(*entry), GFP_KERNEL); + if (!entry) + return -ENOMEM; + + entry->func = func; + entry->data = data; + + mutex_lock(&hdev->cmd_sync_work_lock); + list_add_tail(&entry->list, &hdev->cmd_sync_work_list); + mutex_unlock(&hdev->cmd_sync_work_lock); + + queue_work(hdev->req_workqueue, &hdev->cmd_sync_work); + + return 0; +} + +static void hci_cmd_sync_clear(struct hci_dev *hdev) +{ + struct cmd_sync_work_entry *entry, *tmp; + + list_for_each_entry_safe(entry, tmp, &hdev->cmd_sync_work_list, list) { + list_del(&entry->list); + kfree(entry); + } +} + void hci_uuids_clear(struct hci_dev *hdev) { struct bt_uuid *uuid, *tmp; @@ -3845,6 +3906,10 @@ struct hci_dev *hci_alloc_dev(void) INIT_WORK(&hdev->error_reset, hci_error_reset); INIT_WORK(&hdev->suspend_prepare, hci_prepare_suspend); + INIT_WORK(&hdev->cmd_sync_work, hci_cmd_sync_work); + INIT_LIST_HEAD(&hdev->cmd_sync_work_list); + mutex_init(&hdev->cmd_sync_work_lock); + INIT_DELAYED_WORK(&hdev->power_off, hci_power_off); skb_queue_head_init(&hdev->rx_q); @@ -4005,6 +4070,9 @@ void hci_unregister_dev(struct hci_dev *hdev) cancel_work_sync(&hdev->power_on); + cancel_work_sync(&hdev->cmd_sync_work); + hci_cmd_sync_clear(hdev); + if (!test_bit(HCI_QUIRK_NO_SUSPEND_NOTIFIER, &hdev->quirks)) { hci_suspend_clear_tasks(hdev); unregister_pm_notifier(&hdev->suspend_notifier);