* [PATCH 0/1] Bluetooth: Add support for creating multiple BISes @ 2023-05-17 7:27 Iulia Tanasescu 2023-05-17 7:27 ` [PATCH 1/1] " Iulia Tanasescu 0 siblings, 1 reply; 8+ messages in thread From: Iulia Tanasescu @ 2023-05-17 7:27 UTC (permalink / raw) To: linux-bluetooth; +Cc: Iulia Tanasescu This patch implements support for creating a BIG with multiple BISes. Iulia Tanasescu (1): Bluetooth: Add support for creating multiple BISes include/net/bluetooth/bluetooth.h | 2 + include/net/bluetooth/hci.h | 7 ++ include/net/bluetooth/hci_core.h | 32 ++++++- include/net/bluetooth/iso.h | 14 +++ net/bluetooth/hci_conn.c | 150 ++++++++++++++++++++++++------ net/bluetooth/hci_core.c | 18 ++++ net/bluetooth/hci_event.c | 98 +++++++++++++++---- net/bluetooth/iso.c | 4 + 8 files changed, 277 insertions(+), 48 deletions(-) base-commit: e6e576ec4e728b201a801374b0cec649a4473908 -- 2.34.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] Bluetooth: Add support for creating multiple BISes 2023-05-17 7:27 [PATCH 0/1] Bluetooth: Add support for creating multiple BISes Iulia Tanasescu @ 2023-05-17 7:27 ` Iulia Tanasescu 2023-05-17 7:33 ` Paul Menzel ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Iulia Tanasescu @ 2023-05-17 7:27 UTC (permalink / raw) To: linux-bluetooth; +Cc: Iulia Tanasescu It is required for some configurations to have multiple BISes as part of the same BIG, which is now covered by iso-tester in the following test case: ISO Broadcaster AC 13 - Success Signed-off-by: Iulia Tanasescu <iulia.tanasescu@nxp.com> --- include/net/bluetooth/bluetooth.h | 2 + include/net/bluetooth/hci.h | 7 ++ include/net/bluetooth/hci_core.h | 32 ++++++- include/net/bluetooth/iso.h | 14 +++ net/bluetooth/hci_conn.c | 150 ++++++++++++++++++++++++------ net/bluetooth/hci_core.c | 18 ++++ net/bluetooth/hci_event.c | 98 +++++++++++++++---- net/bluetooth/iso.c | 4 + 8 files changed, 277 insertions(+), 48 deletions(-) diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h index 1b4230cd42a3..28a3b105fdf3 100644 --- a/include/net/bluetooth/bluetooth.h +++ b/include/net/bluetooth/bluetooth.h @@ -198,6 +198,8 @@ struct bt_iso_bcast_qos { __u8 sync_cte_type; __u8 mse; __u16 timeout; + __u8 dummy[2]; /* Dummy octets for padding compatibility with old BlueZ */ + __u8 num_bis; }; struct bt_iso_qos { diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h index 07df96c47ef4..7567cbecf937 100644 --- a/include/net/bluetooth/hci.h +++ b/include/net/bluetooth/hci.h @@ -1,6 +1,7 @@ /* BlueZ - Bluetooth protocol stack for Linux Copyright (C) 2000-2001 Qualcomm Incorporated + Copyright 2023 NXP Written 2000,2001 by Maxim Krasnyansky <maxk@qualcomm.com> @@ -2812,6 +2813,12 @@ struct hci_evt_le_create_big_complete { __le16 bis_handle[]; } __packed; +#define HCI_EVT_LE_TERM_BIG_COMPLETE 0x1c +struct hci_evt_le_term_big_complete { + __u8 handle; + __u8 reason; +} __packed; + #define HCI_EVT_LE_BIG_SYNC_ESTABILISHED 0x1d struct hci_evt_le_big_sync_estabilished { __u8 status; diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 8baf34639939..2b2f25bea6bd 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -583,6 +583,7 @@ struct hci_dev { struct list_head pend_le_reports; struct list_head blocked_keys; struct list_head local_codecs; + struct list_head bigs; struct hci_dev_stats stat; @@ -973,7 +974,6 @@ enum { HCI_CONN_NEW_LINK_KEY, HCI_CONN_SCANNING, HCI_CONN_AUTH_FAILURE, - HCI_CONN_PER_ADV, }; static inline bool hci_conn_ssp_enabled(struct hci_conn *conn) @@ -1258,6 +1258,31 @@ static inline struct hci_conn *hci_conn_hash_lookup_big(struct hci_dev *hdev, return NULL; } +static inline struct hci_conn * +hci_conn_hash_lookup_big_state(struct hci_dev *hdev, + __u8 handle, __u16 state) +{ + struct hci_conn_hash *h = &hdev->conn_hash; + struct hci_conn *c; + + rcu_read_lock(); + + list_for_each_entry_rcu(c, &h->list, list) { + if (bacmp(&c->dst, BDADDR_ANY) || c->type != ISO_LINK || + c->state != state) + continue; + + if (handle == c->iso_qos.bcast.big) { + rcu_read_unlock(); + return c; + } + } + + rcu_read_unlock(); + + return NULL; +} + static inline struct hci_conn *hci_conn_hash_lookup_state(struct hci_dev *hdev, __u8 type, __u16 state) { @@ -1369,6 +1394,8 @@ void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active); void hci_conn_failed(struct hci_conn *conn, u8 status); +int hci_le_create_big(struct hci_conn *conn, struct bt_iso_qos *qos); + /* * hci_conn_get() and hci_conn_put() are used to control the life-time of an * "hci_conn" object. They do not guarantee that the hci_conn object is running, @@ -1576,6 +1603,9 @@ struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list, bdaddr_t *addr, u8 addr_type); +struct iso_big *hci_bigs_list_lookup(struct list_head *list, + __u8 handle); + void hci_uuids_clear(struct hci_dev *hdev); void hci_link_keys_clear(struct hci_dev *hdev); diff --git a/include/net/bluetooth/iso.h b/include/net/bluetooth/iso.h index 3f4fe8b78e1b..2deddb80499d 100644 --- a/include/net/bluetooth/iso.h +++ b/include/net/bluetooth/iso.h @@ -3,6 +3,7 @@ * BlueZ - Bluetooth protocol stack for Linux * * Copyright (C) 2022 Intel Corporation + * Copyright 2023 NXP */ #ifndef __ISO_H @@ -29,4 +30,17 @@ struct sockaddr_iso { struct sockaddr_iso_bc iso_bc[]; }; +struct iso_bis { + __u16 handle; + bool assigned; +}; + +/* hdev BIG list entry */ +struct iso_big { + struct list_head list; + __u8 handle; + __u8 num_bis; + struct iso_bis bis[ISO_MAX_NUM_BIS]; +}; + #endif /* __ISO_H */ diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c index f75ef12f18f7..57e52de6f21d 100644 --- a/net/bluetooth/hci_conn.c +++ b/net/bluetooth/hci_conn.c @@ -35,6 +35,7 @@ #include <net/bluetooth/mgmt.h> #include "hci_request.h" +#include "hci_debugfs.h" #include "smp.h" #include "a2mp.h" #include "eir.h" @@ -826,13 +827,6 @@ static int terminate_big_sync(struct hci_dev *hdev, void *data) hci_remove_ext_adv_instance_sync(hdev, d->bis, NULL); - /* Check if ISO connection is a BIS and terminate BIG if there are - * no other connections using it. - */ - hci_conn_hash_list_state(hdev, find_bis, ISO_LINK, BT_CONNECTED, d); - if (d->count) - return 0; - return hci_le_terminate_big_sync(hdev, d->big, HCI_ERROR_LOCAL_HOST_TERM); } @@ -914,11 +908,25 @@ static int hci_le_big_terminate(struct hci_dev *hdev, u8 big, u16 sync_handle) static void bis_cleanup(struct hci_conn *conn) { struct hci_dev *hdev = conn->hdev; + struct iso_list_data data; + struct iso_big *big; bt_dev_dbg(hdev, "conn %p", conn); if (conn->role == HCI_ROLE_MASTER) { - if (!test_and_clear_bit(HCI_CONN_PER_ADV, &conn->flags)) + big = hci_bigs_list_lookup(&hdev->bigs, conn->iso_qos.bcast.big); + + for (int i = 0; i < big->num_bis; i++) + if (!big->bis[i].assigned) + return; + + data.count = 0; + data.big = conn->iso_qos.bcast.big; + data.bis = conn->iso_qos.bcast.bis; + + hci_conn_hash_list_state(hdev, bis_list, ISO_LINK, BT_CONNECTED, + &data); + if (data.count) return; hci_le_terminate_big(hdev, conn->iso_qos.bcast.big, @@ -1486,13 +1494,40 @@ static int qos_set_bis(struct hci_dev *hdev, struct bt_iso_qos *qos) return 0; } +static int hci_match_bis_params(struct hci_dev *hdev, struct bt_iso_qos *qos, + __u8 base_len, __u8 *base, __u16 bis_state) +{ + struct hci_conn *conn; + __u8 eir[HCI_MAX_PER_AD_LENGTH]; + + if (base_len && base) + base_len = eir_append_service_data(eir, 0, 0x1851, base, base_len); + + conn = hci_conn_hash_lookup_big_state(hdev, qos->bcast.big, bis_state); + + if (memcmp(qos, &conn->iso_qos, sizeof(*qos)) || + base_len != conn->le_per_adv_data_len || + memcmp(conn->le_per_adv_data, eir, base_len)) + return -EADDRINUSE; + + return 0; +} + /* This function requires the caller holds hdev->lock */ static struct hci_conn *hci_add_bis(struct hci_dev *hdev, bdaddr_t *dst, - struct bt_iso_qos *qos) + struct bt_iso_qos *qos, __u8 base_len, + __u8 *base, bool *big_create, + bool *connected) { struct hci_conn *conn; struct iso_list_data data; int err; + int i; + struct iso_big *big; + __u16 handle; + + *big_create = false; + *connected = false; /* Let's make sure that le is enabled.*/ if (!hci_dev_test_flag(hdev, HCI_LE_ENABLED)) { @@ -1509,26 +1544,71 @@ static struct hci_conn *hci_add_bis(struct hci_dev *hdev, bdaddr_t *dst, if (err) return ERR_PTR(err); - data.big = qos->bcast.big; - data.bis = qos->bcast.bis; - data.count = 0; + /* Check if BIG is already created */ + big = hci_bigs_list_lookup(&hdev->bigs, qos->bcast.big); + if (!big) { + /* Check if there are other BISes bound to the same BIG */ + data.big = qos->bcast.big; + data.bis = qos->bcast.bis; + data.count = 0; - /* Check if there is already a matching BIG/BIS */ - hci_conn_hash_list_state(hdev, bis_list, ISO_LINK, BT_BOUND, &data); - if (data.count) - return ERR_PTR(-EADDRINUSE); + hci_conn_hash_list_state(hdev, bis_list, ISO_LINK, BT_BOUND, &data); + if (data.count) { + /* Check QoS and base parameters against the + * other BOUND connections + */ + err = hci_match_bis_params(hdev, qos, base_len, base, BT_BOUND); + goto done; + } - conn = hci_conn_hash_lookup_bis(hdev, dst, qos->bcast.big, qos->bcast.bis); - if (conn) - return ERR_PTR(-EADDRINUSE); + *big_create = true; + goto done; + } + + conn = hci_conn_hash_lookup_big_state(hdev, qos->bcast.big, BT_CONNECTED); + if (!conn) { + /* BIG is in the process of terminating. + * Check BIS parameters against other BOUND connections if any, + * and mark BIS as bound for the BIG. BIG will be recreated + * after receiving the HCI_EVT_LE_TERM_BIG_COMPLETE event + */ + err = hci_match_bis_params(hdev, qos, base_len, base, BT_BOUND); + goto done; + } + + /* BIG is already created. Check that QoS and + * base parameters match the BIG + */ + err = hci_match_bis_params(hdev, qos, base_len, base, BT_CONNECTED); + if (!err) { + /* Try to assign a bis handle */ + for (i = 0; i < big->num_bis; i++) { + if (big->bis[i].assigned) + continue; + + handle = big->bis[i].handle; + big->bis[i].assigned = true; + *connected = true; + break; + } + + if (i == big->num_bis) + err = -EADDRINUSE; + } + +done: + if (err) + return ERR_PTR(err); conn = hci_conn_add(hdev, ISO_LINK, dst, HCI_ROLE_MASTER); if (!conn) return ERR_PTR(-ENOMEM); - set_bit(HCI_CONN_PER_ADV, &conn->flags); conn->state = BT_CONNECT; + if (*connected) + conn->handle = handle; + hci_conn_hold(conn); return conn; } @@ -1736,7 +1816,7 @@ static void cis_list(struct hci_conn *conn, void *data) cis_add(d, &conn->iso_qos); } -static int hci_le_create_big(struct hci_conn *conn, struct bt_iso_qos *qos) +int hci_le_create_big(struct hci_conn *conn, struct bt_iso_qos *qos) { struct hci_dev *hdev = conn->hdev; struct hci_cp_le_create_big cp; @@ -1745,7 +1825,7 @@ static int hci_le_create_big(struct hci_conn *conn, struct bt_iso_qos *qos) cp.handle = qos->bcast.big; cp.adv_handle = qos->bcast.bis; - cp.num_bis = 0x01; + cp.num_bis = qos->bcast.num_bis; hci_cpu_to_le24(qos->bcast.out.interval, cp.bis.sdu_interval); cp.bis.sdu = cpu_to_le16(qos->bcast.out.sdu); cp.bis.latency = cpu_to_le16(qos->bcast.out.latency); @@ -2156,9 +2236,12 @@ struct hci_conn *hci_connect_bis(struct hci_dev *hdev, bdaddr_t *dst, { struct hci_conn *conn; int err; + bool big_create = false; + bool connected = false; /* We need hci_conn object using the BDADDR_ANY as dst */ - conn = hci_add_bis(hdev, dst, qos); + conn = hci_add_bis(hdev, dst, qos, base_len, base, + &big_create, &connected); if (IS_ERR(conn)) return conn; @@ -2171,18 +2254,27 @@ struct hci_conn *hci_connect_bis(struct hci_dev *hdev, bdaddr_t *dst, conn->le_per_adv_data_len = base_len; } - /* Queue start periodic advertising and create BIG */ - err = hci_cmd_sync_queue(hdev, create_big_sync, conn, - create_big_complete); - if (err < 0) { - hci_conn_drop(conn); - return ERR_PTR(err); + if (big_create) { + /* Queue start periodic advertising and create BIG */ + err = hci_cmd_sync_queue(hdev, create_big_sync, conn, + create_big_complete); + if (err < 0) { + hci_conn_drop(conn); + return ERR_PTR(err); + } } hci_iso_qos_setup(hdev, conn, &qos->bcast.out, conn->le_tx_phy ? conn->le_tx_phy : hdev->le_tx_def_phys); + if (connected) { + conn->state = BT_CONNECTED; + hci_debugfs_create_conn(conn); + hci_conn_add_sysfs(conn); + hci_iso_setup_path(conn); + } + return conn; } diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index a856b1051d35..0dd9161f7157 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -2,6 +2,7 @@ BlueZ - Bluetooth protocol stack for Linux Copyright (C) 2000-2001 Qualcomm Incorporated Copyright (C) 2011 ProFUSION Embedded Systems + Copyright 2023 NXP Written 2000,2001 by Maxim Krasnyansky <maxk@qualcomm.com> @@ -38,6 +39,7 @@ #include <net/bluetooth/bluetooth.h> #include <net/bluetooth/hci_core.h> #include <net/bluetooth/l2cap.h> +#include <net/bluetooth/iso.h> #include <net/bluetooth/mgmt.h> #include "hci_request.h" @@ -2264,6 +2266,20 @@ struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list, return NULL; } +/* This function requires the caller holds hdev->lock */ +struct iso_big *hci_bigs_list_lookup(struct list_head *list, + __u8 handle) +{ + struct iso_big *big; + + list_for_each_entry(big, list, list) { + if (big->handle == handle) + return big; + } + + return NULL; +} + /* This function requires the caller holds hdev->lock */ struct hci_conn_params *hci_conn_params_add(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type) @@ -2525,6 +2541,8 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv) INIT_LIST_HEAD(&hdev->monitored_devices); INIT_LIST_HEAD(&hdev->local_codecs); + INIT_LIST_HEAD(&hdev->bigs); + INIT_WORK(&hdev->rx_work, hci_rx_work); INIT_WORK(&hdev->cmd_work, hci_cmd_work); INIT_WORK(&hdev->tx_work, hci_tx_work); diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index d00ef6e3fc45..ddf55fa4703a 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -30,6 +30,7 @@ #include <net/bluetooth/bluetooth.h> #include <net/bluetooth/hci_core.h> #include <net/bluetooth/mgmt.h> +#include <net/bluetooth/iso.h> #include "hci_request.h" #include "hci_debugfs.h" @@ -3903,6 +3904,11 @@ static void hci_cs_le_create_big(struct hci_dev *hdev, u8 status) bt_dev_dbg(hdev, "status 0x%2.2x", status); } +static void hci_cs_le_term_big(struct hci_dev *hdev, u8 status) +{ + bt_dev_dbg(hdev, "status 0x%2.2x", status); +} + static u8 hci_cc_set_per_adv_param(struct hci_dev *hdev, void *data, struct sk_buff *skb) { @@ -4275,6 +4281,7 @@ static const struct hci_cs { HCI_CS(HCI_OP_LE_EXT_CREATE_CONN, hci_cs_le_ext_create_conn), HCI_CS(HCI_OP_LE_CREATE_CIS, hci_cs_le_create_cis), HCI_CS(HCI_OP_LE_CREATE_BIG, hci_cs_le_create_big), + HCI_CS(HCI_OP_LE_TERM_BIG, hci_cs_le_term_big), }; static void hci_cmd_status_evt(struct hci_dev *hdev, void *data, @@ -6910,6 +6917,9 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data, { struct hci_evt_le_create_big_complete *ev = data; struct hci_conn *conn; + struct iso_big *big; + struct hci_conn_hash *h = &hdev->conn_hash; + __u8 bis_idx = 0; BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); @@ -6919,30 +6929,78 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data, hci_dev_lock(hdev); - conn = hci_conn_hash_lookup_big(hdev, ev->handle); - if (!conn) - goto unlock; + if (!ev->status) { + /* Add the created BIG to the list */ + big = kzalloc(sizeof(*big), GFP_KERNEL); + if (!big) + return; - if (conn->type != ISO_LINK) { - bt_dev_err(hdev, - "Invalid connection link type handle 0x%2.2x", - ev->handle); - goto unlock; + big->handle = ev->handle; + big->num_bis = ev->num_bis; + + for (int i = 0; i < ev->num_bis; i++) { + big->bis[i].handle = __le16_to_cpu(ev->bis_handle[i]); + big->bis[i].assigned = false; + } + + list_add(&big->list, &hdev->bigs); } - if (ev->num_bis) - conn->handle = __le16_to_cpu(ev->bis_handle[0]); + rcu_read_lock(); - if (!ev->status) { - conn->state = BT_CONNECTED; - hci_debugfs_create_conn(conn); - hci_conn_add_sysfs(conn); - hci_iso_setup_path(conn); - goto unlock; + /* Connect all BISes that are bound to the BIG */ + list_for_each_entry_rcu(conn, &h->list, list) { + if (bacmp(&conn->dst, BDADDR_ANY) || conn->type != ISO_LINK || + conn->state != BT_BOUND || + conn->iso_qos.bcast.big != ev->handle) + continue; + + if (ev->status) { + hci_connect_cfm(conn, ev->status); + hci_conn_del(conn); + } + + if (big->num_bis > bis_idx) { + conn->handle = __le16_to_cpu(big->bis[bis_idx].handle); + big->bis[bis_idx].assigned = true; + bis_idx++; + + conn->state = BT_CONNECTED; + hci_debugfs_create_conn(conn); + hci_conn_add_sysfs(conn); + hci_iso_setup_path(conn); + continue; + } } - hci_connect_cfm(conn, ev->status); - hci_conn_del(conn); + rcu_read_unlock(); + hci_dev_unlock(hdev); +} + +static void hci_le_term_big_complete_evt(struct hci_dev *hdev, void *data, + struct sk_buff *skb) +{ + struct hci_evt_le_term_big_complete *ev = data; + struct iso_big *big; + struct hci_conn *conn; + + BT_DBG("%s reason 0x%2.2x", hdev->name, ev->reason); + + hci_dev_lock(hdev); + + big = hci_bigs_list_lookup(&hdev->bigs, ev->handle); + + if (big) { + list_del(&big->list); + kfree(big); + } + + /* If there are any bound connections to the BIG, recreate it */ + conn = hci_conn_hash_lookup_big_state(hdev, ev->handle, BT_BOUND); + if (!conn) + goto unlock; + + hci_le_create_big(conn, &conn->iso_qos); unlock: hci_dev_unlock(hdev); @@ -7089,6 +7147,10 @@ static const struct hci_le_ev { hci_le_create_big_complete_evt, sizeof(struct hci_evt_le_create_big_complete), HCI_MAX_EVENT_SIZE), + /* [0x1c = HCI_EVT_LE_TERM_BIG_COMPLETE] */ + HCI_LE_EV(HCI_EVT_LE_TERM_BIG_COMPLETE, + hci_le_term_big_complete_evt, + sizeof(struct hci_evt_le_term_big_complete)), /* [0x1d = HCI_EV_LE_BIG_SYNC_ESTABILISHED] */ HCI_LE_EV_VL(HCI_EVT_LE_BIG_SYNC_ESTABILISHED, hci_le_big_sync_established_evt, diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c index 34d55a85d8f6..416ed416fffa 100644 --- a/net/bluetooth/iso.c +++ b/net/bluetooth/iso.c @@ -717,6 +717,7 @@ static struct bt_iso_qos default_qos = { .sync_cte_type = 0x00, .mse = 0x00, .timeout = 0x4000, + .num_bis = 0x01, }, }; @@ -1249,6 +1250,9 @@ static bool check_bcast_qos(struct bt_iso_qos *qos) if (qos->bcast.timeout < 0x000a || qos->bcast.timeout > 0x4000) return false; + if (qos->bcast.num_bis < 0x01 || qos->bcast.num_bis > ISO_MAX_NUM_BIS) + return false; + return true; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] Bluetooth: Add support for creating multiple BISes 2023-05-17 7:27 ` [PATCH 1/1] " Iulia Tanasescu @ 2023-05-17 7:33 ` Paul Menzel 2023-05-18 7:19 ` Iulia Tanasescu 2023-05-17 7:57 ` bluez.test.bot 2023-05-17 17:55 ` [PATCH 1/1] " Luiz Augusto von Dentz 2 siblings, 1 reply; 8+ messages in thread From: Paul Menzel @ 2023-05-17 7:33 UTC (permalink / raw) To: Iulia Tanasescu; +Cc: linux-bluetooth Dear Iulia, Thank you for your patch. Am 17.05.23 um 09:27 schrieb Iulia Tanasescu: > It is required for some configurations to have multiple BISes as part > of the same BIG, which is now covered by iso-tester in the following test > case: > > ISO Broadcaster AC 13 - Success Thank you for adding a test. Did you also test it on hardware? If so, please document your test setup. A diffstat over hundred lines should have a more elaborate commit message. Could you please add a short note about the implementation? > Signed-off-by: Iulia Tanasescu <iulia.tanasescu@nxp.com> > --- > include/net/bluetooth/bluetooth.h | 2 + > include/net/bluetooth/hci.h | 7 ++ > include/net/bluetooth/hci_core.h | 32 ++++++- > include/net/bluetooth/iso.h | 14 +++ > net/bluetooth/hci_conn.c | 150 ++++++++++++++++++++++++------ > net/bluetooth/hci_core.c | 18 ++++ > net/bluetooth/hci_event.c | 98 +++++++++++++++---- > net/bluetooth/iso.c | 4 + > 8 files changed, 277 insertions(+), 48 deletions(-) > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > index 1b4230cd42a3..28a3b105fdf3 100644 > --- a/include/net/bluetooth/bluetooth.h > +++ b/include/net/bluetooth/bluetooth.h > @@ -198,6 +198,8 @@ struct bt_iso_bcast_qos { > __u8 sync_cte_type; > __u8 mse; > __u16 timeout; > + __u8 dummy[2]; /* Dummy octets for padding compatibility with old BlueZ */ > + __u8 num_bis; > }; > > struct bt_iso_qos { > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > index 07df96c47ef4..7567cbecf937 100644 > --- a/include/net/bluetooth/hci.h > +++ b/include/net/bluetooth/hci.h > @@ -1,6 +1,7 @@ > /* > BlueZ - Bluetooth protocol stack for Linux > Copyright (C) 2000-2001 Qualcomm Incorporated > + Copyright 2023 NXP Above, Copyright is followed by (C). Should it be consistent? […] Kind regards, Paul ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] Bluetooth: Add support for creating multiple BISes 2023-05-17 7:33 ` Paul Menzel @ 2023-05-18 7:19 ` Iulia Tanasescu 0 siblings, 0 replies; 8+ messages in thread From: Iulia Tanasescu @ 2023-05-18 7:19 UTC (permalink / raw) To: pmenzel; +Cc: iulia.tanasescu, linux-bluetooth Hello Paul, Thank you for your feedback, I will update the patch and I will resubmit. Regards, Iulia ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: Bluetooth: Add support for creating multiple BISes 2023-05-17 7:27 ` [PATCH 1/1] " Iulia Tanasescu 2023-05-17 7:33 ` Paul Menzel @ 2023-05-17 7:57 ` bluez.test.bot 2023-05-17 17:55 ` [PATCH 1/1] " Luiz Augusto von Dentz 2 siblings, 0 replies; 8+ messages in thread From: bluez.test.bot @ 2023-05-17 7:57 UTC (permalink / raw) To: linux-bluetooth, iulia.tanasescu [-- Attachment #1: Type: text/plain, Size: 2712 bytes --] This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=748305 ---Test result--- Test Summary: CheckPatch FAIL 5.46 seconds GitLint PASS 0.30 seconds SubjectPrefix PASS 0.11 seconds BuildKernel PASS 32.19 seconds CheckAllWarning PASS 35.31 seconds CheckSparse WARNING 40.44 seconds CheckSmatch WARNING 109.45 seconds BuildKernel32 PASS 31.65 seconds TestRunnerSetup PASS 453.92 seconds TestRunner_l2cap-tester PASS 17.19 seconds TestRunner_iso-tester PASS 21.60 seconds TestRunner_bnep-tester PASS 5.70 seconds TestRunner_mgmt-tester PASS 115.16 seconds TestRunner_rfcomm-tester PASS 9.01 seconds TestRunner_sco-tester PASS 8.41 seconds TestRunner_ioctl-tester PASS 9.73 seconds TestRunner_mesh-tester PASS 7.14 seconds TestRunner_smp-tester PASS 8.22 seconds TestRunner_userchan-tester PASS 5.94 seconds IncrementalBuild PASS 29.80 seconds Details ############################## Test: CheckPatch - FAIL Desc: Run checkpatch.pl script Output: [1/1] Bluetooth: Add support for creating multiple BISes WARNING: please, no spaces at the start of a line #505: FILE: net/bluetooth/hci_core.c:5: + Copyright 2023 NXP$ CHECK: Avoid CamelCase: <Copyright> #505: FILE: net/bluetooth/hci_core.c:5: + Copyright 2023 NXP total: 0 errors, 1 warnings, 1 checks, 542 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13244298.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. ############################## Test: CheckSparse - WARNING Desc: Run sparse tool with linux kernel Output: net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):net/bluetooth/hci_event.c:6964:40: warning: cast to restricted __le16 ############################## Test: CheckSmatch - WARNING Desc: Run smatch tool with source Output: net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h): --- Regards, Linux Bluetooth ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] Bluetooth: Add support for creating multiple BISes 2023-05-17 7:27 ` [PATCH 1/1] " Iulia Tanasescu 2023-05-17 7:33 ` Paul Menzel 2023-05-17 7:57 ` bluez.test.bot @ 2023-05-17 17:55 ` Luiz Augusto von Dentz 2023-05-18 7:15 ` Iulia Tanasescu 2 siblings, 1 reply; 8+ messages in thread From: Luiz Augusto von Dentz @ 2023-05-17 17:55 UTC (permalink / raw) To: Iulia Tanasescu; +Cc: linux-bluetooth Hi Iulia, On Wed, May 17, 2023 at 12:32 AM Iulia Tanasescu <iulia.tanasescu@nxp.com> wrote: > > It is required for some configurations to have multiple BISes as part > of the same BIG, which is now covered by iso-tester in the following test > case: > > ISO Broadcaster AC 13 - Success > > Signed-off-by: Iulia Tanasescu <iulia.tanasescu@nxp.com> > --- > include/net/bluetooth/bluetooth.h | 2 + > include/net/bluetooth/hci.h | 7 ++ > include/net/bluetooth/hci_core.h | 32 ++++++- > include/net/bluetooth/iso.h | 14 +++ > net/bluetooth/hci_conn.c | 150 ++++++++++++++++++++++++------ > net/bluetooth/hci_core.c | 18 ++++ > net/bluetooth/hci_event.c | 98 +++++++++++++++---- > net/bluetooth/iso.c | 4 + > 8 files changed, 277 insertions(+), 48 deletions(-) > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h > index 1b4230cd42a3..28a3b105fdf3 100644 > --- a/include/net/bluetooth/bluetooth.h > +++ b/include/net/bluetooth/bluetooth.h > @@ -198,6 +198,8 @@ struct bt_iso_bcast_qos { > __u8 sync_cte_type; > __u8 mse; > __u16 timeout; > + __u8 dummy[2]; /* Dummy octets for padding compatibility with old BlueZ */ > + __u8 num_bis; Don't think that is going to work, each BIS should have its own dedicated socket otherwise we have multiplex/demultiplex at userspace level which is not what we intended with the current design. I think we can just use a similar design as to how we group CIG with use of DEFER_SETUP so userspace can bind all BIS/socket to a BIG before we created it: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/iso.c#n366 And we something like hci_le_create_cis_sync does when creating the BIG which is to wait until hci_conn->state is set to BT_CONNECT to issue the Create BIG with all the BIS: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/hci_sync.c#n6192 > }; > > struct bt_iso_qos { > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > index 07df96c47ef4..7567cbecf937 100644 > --- a/include/net/bluetooth/hci.h > +++ b/include/net/bluetooth/hci.h > @@ -1,6 +1,7 @@ > /* > BlueZ - Bluetooth protocol stack for Linux > Copyright (C) 2000-2001 Qualcomm Incorporated > + Copyright 2023 NXP > > Written 2000,2001 by Maxim Krasnyansky <maxk@qualcomm.com> > > @@ -2812,6 +2813,12 @@ struct hci_evt_le_create_big_complete { > __le16 bis_handle[]; > } __packed; > > +#define HCI_EVT_LE_TERM_BIG_COMPLETE 0x1c > +struct hci_evt_le_term_big_complete { > + __u8 handle; > + __u8 reason; > +} __packed; > + > #define HCI_EVT_LE_BIG_SYNC_ESTABILISHED 0x1d > struct hci_evt_le_big_sync_estabilished { > __u8 status; > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index 8baf34639939..2b2f25bea6bd 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -583,6 +583,7 @@ struct hci_dev { > struct list_head pend_le_reports; > struct list_head blocked_keys; > struct list_head local_codecs; > + struct list_head bigs; > > struct hci_dev_stats stat; > > @@ -973,7 +974,6 @@ enum { > HCI_CONN_NEW_LINK_KEY, > HCI_CONN_SCANNING, > HCI_CONN_AUTH_FAILURE, > - HCI_CONN_PER_ADV, Not really sure why you are removing this flag? > }; > > static inline bool hci_conn_ssp_enabled(struct hci_conn *conn) > @@ -1258,6 +1258,31 @@ static inline struct hci_conn *hci_conn_hash_lookup_big(struct hci_dev *hdev, > return NULL; > } > > +static inline struct hci_conn * > +hci_conn_hash_lookup_big_state(struct hci_dev *hdev, > + __u8 handle, __u16 state) > +{ > + struct hci_conn_hash *h = &hdev->conn_hash; > + struct hci_conn *c; > + > + rcu_read_lock(); > + > + list_for_each_entry_rcu(c, &h->list, list) { > + if (bacmp(&c->dst, BDADDR_ANY) || c->type != ISO_LINK || > + c->state != state) > + continue; > + > + if (handle == c->iso_qos.bcast.big) { > + rcu_read_unlock(); > + return c; > + } > + } > + > + rcu_read_unlock(); > + > + return NULL; > +} > + > static inline struct hci_conn *hci_conn_hash_lookup_state(struct hci_dev *hdev, > __u8 type, __u16 state) > { > @@ -1369,6 +1394,8 @@ void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active); > > void hci_conn_failed(struct hci_conn *conn, u8 status); > > +int hci_le_create_big(struct hci_conn *conn, struct bt_iso_qos *qos); > + > /* > * hci_conn_get() and hci_conn_put() are used to control the life-time of an > * "hci_conn" object. They do not guarantee that the hci_conn object is running, > @@ -1576,6 +1603,9 @@ struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list, > bdaddr_t *addr, > u8 addr_type); > > +struct iso_big *hci_bigs_list_lookup(struct list_head *list, > + __u8 handle); > + > void hci_uuids_clear(struct hci_dev *hdev); > > void hci_link_keys_clear(struct hci_dev *hdev); > diff --git a/include/net/bluetooth/iso.h b/include/net/bluetooth/iso.h > index 3f4fe8b78e1b..2deddb80499d 100644 > --- a/include/net/bluetooth/iso.h > +++ b/include/net/bluetooth/iso.h > @@ -3,6 +3,7 @@ > * BlueZ - Bluetooth protocol stack for Linux > * > * Copyright (C) 2022 Intel Corporation > + * Copyright 2023 NXP > */ > > #ifndef __ISO_H > @@ -29,4 +30,17 @@ struct sockaddr_iso { > struct sockaddr_iso_bc iso_bc[]; > }; > > +struct iso_bis { > + __u16 handle; > + bool assigned; > +}; > + > +/* hdev BIG list entry */ > +struct iso_big { > + struct list_head list; > + __u8 handle; > + __u8 num_bis; > + struct iso_bis bis[ISO_MAX_NUM_BIS]; > +}; > + > #endif /* __ISO_H */ > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > index f75ef12f18f7..57e52de6f21d 100644 > --- a/net/bluetooth/hci_conn.c > +++ b/net/bluetooth/hci_conn.c > @@ -35,6 +35,7 @@ > #include <net/bluetooth/mgmt.h> > > #include "hci_request.h" > +#include "hci_debugfs.h" > #include "smp.h" > #include "a2mp.h" > #include "eir.h" > @@ -826,13 +827,6 @@ static int terminate_big_sync(struct hci_dev *hdev, void *data) > > hci_remove_ext_adv_instance_sync(hdev, d->bis, NULL); > > - /* Check if ISO connection is a BIS and terminate BIG if there are > - * no other connections using it. > - */ > - hci_conn_hash_list_state(hdev, find_bis, ISO_LINK, BT_CONNECTED, d); > - if (d->count) > - return 0; > - > return hci_le_terminate_big_sync(hdev, d->big, > HCI_ERROR_LOCAL_HOST_TERM); > } > @@ -914,11 +908,25 @@ static int hci_le_big_terminate(struct hci_dev *hdev, u8 big, u16 sync_handle) > static void bis_cleanup(struct hci_conn *conn) > { > struct hci_dev *hdev = conn->hdev; > + struct iso_list_data data; > + struct iso_big *big; > > bt_dev_dbg(hdev, "conn %p", conn); > > if (conn->role == HCI_ROLE_MASTER) { > - if (!test_and_clear_bit(HCI_CONN_PER_ADV, &conn->flags)) > + big = hci_bigs_list_lookup(&hdev->bigs, conn->iso_qos.bcast.big); > + > + for (int i = 0; i < big->num_bis; i++) > + if (!big->bis[i].assigned) > + return; > + > + data.count = 0; > + data.big = conn->iso_qos.bcast.big; > + data.bis = conn->iso_qos.bcast.bis; > + > + hci_conn_hash_list_state(hdev, bis_list, ISO_LINK, BT_CONNECTED, > + &data); > + if (data.count) > return; > > hci_le_terminate_big(hdev, conn->iso_qos.bcast.big, > @@ -1486,13 +1494,40 @@ static int qos_set_bis(struct hci_dev *hdev, struct bt_iso_qos *qos) > return 0; > } > > +static int hci_match_bis_params(struct hci_dev *hdev, struct bt_iso_qos *qos, > + __u8 base_len, __u8 *base, __u16 bis_state) > +{ > + struct hci_conn *conn; > + __u8 eir[HCI_MAX_PER_AD_LENGTH]; > + > + if (base_len && base) > + base_len = eir_append_service_data(eir, 0, 0x1851, base, base_len); > + > + conn = hci_conn_hash_lookup_big_state(hdev, qos->bcast.big, bis_state); > + > + if (memcmp(qos, &conn->iso_qos, sizeof(*qos)) || > + base_len != conn->le_per_adv_data_len || > + memcmp(conn->le_per_adv_data, eir, base_len)) > + return -EADDRINUSE; > + > + return 0; > +} > + > /* This function requires the caller holds hdev->lock */ > static struct hci_conn *hci_add_bis(struct hci_dev *hdev, bdaddr_t *dst, > - struct bt_iso_qos *qos) > + struct bt_iso_qos *qos, __u8 base_len, > + __u8 *base, bool *big_create, > + bool *connected) > { > struct hci_conn *conn; > struct iso_list_data data; > int err; > + int i; > + struct iso_big *big; > + __u16 handle; > + > + *big_create = false; > + *connected = false; > > /* Let's make sure that le is enabled.*/ > if (!hci_dev_test_flag(hdev, HCI_LE_ENABLED)) { > @@ -1509,26 +1544,71 @@ static struct hci_conn *hci_add_bis(struct hci_dev *hdev, bdaddr_t *dst, > if (err) > return ERR_PTR(err); > > - data.big = qos->bcast.big; > - data.bis = qos->bcast.bis; > - data.count = 0; > + /* Check if BIG is already created */ > + big = hci_bigs_list_lookup(&hdev->bigs, qos->bcast.big); > + if (!big) { > + /* Check if there are other BISes bound to the same BIG */ > + data.big = qos->bcast.big; > + data.bis = qos->bcast.bis; > + data.count = 0; > > - /* Check if there is already a matching BIG/BIS */ > - hci_conn_hash_list_state(hdev, bis_list, ISO_LINK, BT_BOUND, &data); > - if (data.count) > - return ERR_PTR(-EADDRINUSE); > + hci_conn_hash_list_state(hdev, bis_list, ISO_LINK, BT_BOUND, &data); > + if (data.count) { > + /* Check QoS and base parameters against the > + * other BOUND connections > + */ > + err = hci_match_bis_params(hdev, qos, base_len, base, BT_BOUND); > + goto done; > + } > > - conn = hci_conn_hash_lookup_bis(hdev, dst, qos->bcast.big, qos->bcast.bis); > - if (conn) > - return ERR_PTR(-EADDRINUSE); > + *big_create = true; > + goto done; > + } > + > + conn = hci_conn_hash_lookup_big_state(hdev, qos->bcast.big, BT_CONNECTED); > + if (!conn) { > + /* BIG is in the process of terminating. > + * Check BIS parameters against other BOUND connections if any, > + * and mark BIS as bound for the BIG. BIG will be recreated > + * after receiving the HCI_EVT_LE_TERM_BIG_COMPLETE event > + */ > + err = hci_match_bis_params(hdev, qos, base_len, base, BT_BOUND); > + goto done; > + } > + > + /* BIG is already created. Check that QoS and > + * base parameters match the BIG > + */ > + err = hci_match_bis_params(hdev, qos, base_len, base, BT_CONNECTED); > + if (!err) { > + /* Try to assign a bis handle */ > + for (i = 0; i < big->num_bis; i++) { > + if (big->bis[i].assigned) > + continue; > + > + handle = big->bis[i].handle; > + big->bis[i].assigned = true; > + *connected = true; > + break; > + } > + > + if (i == big->num_bis) > + err = -EADDRINUSE; > + } > + > +done: > + if (err) > + return ERR_PTR(err); > > conn = hci_conn_add(hdev, ISO_LINK, dst, HCI_ROLE_MASTER); > if (!conn) > return ERR_PTR(-ENOMEM); > > - set_bit(HCI_CONN_PER_ADV, &conn->flags); > conn->state = BT_CONNECT; > > + if (*connected) > + conn->handle = handle; > + > hci_conn_hold(conn); > return conn; > } > @@ -1736,7 +1816,7 @@ static void cis_list(struct hci_conn *conn, void *data) > cis_add(d, &conn->iso_qos); > } > > -static int hci_le_create_big(struct hci_conn *conn, struct bt_iso_qos *qos) > +int hci_le_create_big(struct hci_conn *conn, struct bt_iso_qos *qos) > { > struct hci_dev *hdev = conn->hdev; > struct hci_cp_le_create_big cp; > @@ -1745,7 +1825,7 @@ static int hci_le_create_big(struct hci_conn *conn, struct bt_iso_qos *qos) > > cp.handle = qos->bcast.big; > cp.adv_handle = qos->bcast.bis; > - cp.num_bis = 0x01; > + cp.num_bis = qos->bcast.num_bis; > hci_cpu_to_le24(qos->bcast.out.interval, cp.bis.sdu_interval); > cp.bis.sdu = cpu_to_le16(qos->bcast.out.sdu); > cp.bis.latency = cpu_to_le16(qos->bcast.out.latency); > @@ -2156,9 +2236,12 @@ struct hci_conn *hci_connect_bis(struct hci_dev *hdev, bdaddr_t *dst, > { > struct hci_conn *conn; > int err; > + bool big_create = false; > + bool connected = false; > > /* We need hci_conn object using the BDADDR_ANY as dst */ > - conn = hci_add_bis(hdev, dst, qos); > + conn = hci_add_bis(hdev, dst, qos, base_len, base, > + &big_create, &connected); > if (IS_ERR(conn)) > return conn; > > @@ -2171,18 +2254,27 @@ struct hci_conn *hci_connect_bis(struct hci_dev *hdev, bdaddr_t *dst, > conn->le_per_adv_data_len = base_len; > } > > - /* Queue start periodic advertising and create BIG */ > - err = hci_cmd_sync_queue(hdev, create_big_sync, conn, > - create_big_complete); > - if (err < 0) { > - hci_conn_drop(conn); > - return ERR_PTR(err); > + if (big_create) { > + /* Queue start periodic advertising and create BIG */ > + err = hci_cmd_sync_queue(hdev, create_big_sync, conn, > + create_big_complete); > + if (err < 0) { > + hci_conn_drop(conn); > + return ERR_PTR(err); > + } > } > > hci_iso_qos_setup(hdev, conn, &qos->bcast.out, > conn->le_tx_phy ? conn->le_tx_phy : > hdev->le_tx_def_phys); > > + if (connected) { > + conn->state = BT_CONNECTED; > + hci_debugfs_create_conn(conn); > + hci_conn_add_sysfs(conn); > + hci_iso_setup_path(conn); > + } > + > return conn; > } > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index a856b1051d35..0dd9161f7157 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -2,6 +2,7 @@ > BlueZ - Bluetooth protocol stack for Linux > Copyright (C) 2000-2001 Qualcomm Incorporated > Copyright (C) 2011 ProFUSION Embedded Systems > + Copyright 2023 NXP > > Written 2000,2001 by Maxim Krasnyansky <maxk@qualcomm.com> > > @@ -38,6 +39,7 @@ > #include <net/bluetooth/bluetooth.h> > #include <net/bluetooth/hci_core.h> > #include <net/bluetooth/l2cap.h> > +#include <net/bluetooth/iso.h> > #include <net/bluetooth/mgmt.h> > > #include "hci_request.h" > @@ -2264,6 +2266,20 @@ struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list, > return NULL; > } > > +/* This function requires the caller holds hdev->lock */ > +struct iso_big *hci_bigs_list_lookup(struct list_head *list, > + __u8 handle) > +{ > + struct iso_big *big; > + > + list_for_each_entry(big, list, list) { > + if (big->handle == handle) > + return big; > + } > + > + return NULL; > +} > + > /* This function requires the caller holds hdev->lock */ > struct hci_conn_params *hci_conn_params_add(struct hci_dev *hdev, > bdaddr_t *addr, u8 addr_type) > @@ -2525,6 +2541,8 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv) > INIT_LIST_HEAD(&hdev->monitored_devices); > > INIT_LIST_HEAD(&hdev->local_codecs); > + INIT_LIST_HEAD(&hdev->bigs); > + > INIT_WORK(&hdev->rx_work, hci_rx_work); > INIT_WORK(&hdev->cmd_work, hci_cmd_work); > INIT_WORK(&hdev->tx_work, hci_tx_work); > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index d00ef6e3fc45..ddf55fa4703a 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -30,6 +30,7 @@ > #include <net/bluetooth/bluetooth.h> > #include <net/bluetooth/hci_core.h> > #include <net/bluetooth/mgmt.h> > +#include <net/bluetooth/iso.h> > > #include "hci_request.h" > #include "hci_debugfs.h" > @@ -3903,6 +3904,11 @@ static void hci_cs_le_create_big(struct hci_dev *hdev, u8 status) > bt_dev_dbg(hdev, "status 0x%2.2x", status); > } > > +static void hci_cs_le_term_big(struct hci_dev *hdev, u8 status) > +{ > + bt_dev_dbg(hdev, "status 0x%2.2x", status); > +} > + > static u8 hci_cc_set_per_adv_param(struct hci_dev *hdev, void *data, > struct sk_buff *skb) > { > @@ -4275,6 +4281,7 @@ static const struct hci_cs { > HCI_CS(HCI_OP_LE_EXT_CREATE_CONN, hci_cs_le_ext_create_conn), > HCI_CS(HCI_OP_LE_CREATE_CIS, hci_cs_le_create_cis), > HCI_CS(HCI_OP_LE_CREATE_BIG, hci_cs_le_create_big), > + HCI_CS(HCI_OP_LE_TERM_BIG, hci_cs_le_term_big), > }; > > static void hci_cmd_status_evt(struct hci_dev *hdev, void *data, > @@ -6910,6 +6917,9 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data, > { > struct hci_evt_le_create_big_complete *ev = data; > struct hci_conn *conn; > + struct iso_big *big; > + struct hci_conn_hash *h = &hdev->conn_hash; > + __u8 bis_idx = 0; > > BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); > > @@ -6919,30 +6929,78 @@ static void hci_le_create_big_complete_evt(struct hci_dev *hdev, void *data, > > hci_dev_lock(hdev); > > - conn = hci_conn_hash_lookup_big(hdev, ev->handle); > - if (!conn) > - goto unlock; > + if (!ev->status) { > + /* Add the created BIG to the list */ > + big = kzalloc(sizeof(*big), GFP_KERNEL); > + if (!big) > + return; > > - if (conn->type != ISO_LINK) { > - bt_dev_err(hdev, > - "Invalid connection link type handle 0x%2.2x", > - ev->handle); > - goto unlock; > + big->handle = ev->handle; > + big->num_bis = ev->num_bis; > + > + for (int i = 0; i < ev->num_bis; i++) { > + big->bis[i].handle = __le16_to_cpu(ev->bis_handle[i]); > + big->bis[i].assigned = false; > + } > + > + list_add(&big->list, &hdev->bigs); > } > > - if (ev->num_bis) > - conn->handle = __le16_to_cpu(ev->bis_handle[0]); > + rcu_read_lock(); > > - if (!ev->status) { > - conn->state = BT_CONNECTED; > - hci_debugfs_create_conn(conn); > - hci_conn_add_sysfs(conn); > - hci_iso_setup_path(conn); > - goto unlock; > + /* Connect all BISes that are bound to the BIG */ > + list_for_each_entry_rcu(conn, &h->list, list) { > + if (bacmp(&conn->dst, BDADDR_ANY) || conn->type != ISO_LINK || > + conn->state != BT_BOUND || > + conn->iso_qos.bcast.big != ev->handle) > + continue; > + > + if (ev->status) { > + hci_connect_cfm(conn, ev->status); > + hci_conn_del(conn); > + } > + > + if (big->num_bis > bis_idx) { > + conn->handle = __le16_to_cpu(big->bis[bis_idx].handle); > + big->bis[bis_idx].assigned = true; > + bis_idx++; > + > + conn->state = BT_CONNECTED; > + hci_debugfs_create_conn(conn); > + hci_conn_add_sysfs(conn); > + hci_iso_setup_path(conn); > + continue; > + } > } > > - hci_connect_cfm(conn, ev->status); > - hci_conn_del(conn); > + rcu_read_unlock(); > + hci_dev_unlock(hdev); > +} > + > +static void hci_le_term_big_complete_evt(struct hci_dev *hdev, void *data, > + struct sk_buff *skb) > +{ > + struct hci_evt_le_term_big_complete *ev = data; > + struct iso_big *big; > + struct hci_conn *conn; > + > + BT_DBG("%s reason 0x%2.2x", hdev->name, ev->reason); > + > + hci_dev_lock(hdev); > + > + big = hci_bigs_list_lookup(&hdev->bigs, ev->handle); > + > + if (big) { > + list_del(&big->list); > + kfree(big); > + } > + > + /* If there are any bound connections to the BIG, recreate it */ > + conn = hci_conn_hash_lookup_big_state(hdev, ev->handle, BT_BOUND); > + if (!conn) > + goto unlock; > + > + hci_le_create_big(conn, &conn->iso_qos); > > unlock: > hci_dev_unlock(hdev); > @@ -7089,6 +7147,10 @@ static const struct hci_le_ev { > hci_le_create_big_complete_evt, > sizeof(struct hci_evt_le_create_big_complete), > HCI_MAX_EVENT_SIZE), > + /* [0x1c = HCI_EVT_LE_TERM_BIG_COMPLETE] */ > + HCI_LE_EV(HCI_EVT_LE_TERM_BIG_COMPLETE, > + hci_le_term_big_complete_evt, > + sizeof(struct hci_evt_le_term_big_complete)), > /* [0x1d = HCI_EV_LE_BIG_SYNC_ESTABILISHED] */ > HCI_LE_EV_VL(HCI_EVT_LE_BIG_SYNC_ESTABILISHED, > hci_le_big_sync_established_evt, > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c > index 34d55a85d8f6..416ed416fffa 100644 > --- a/net/bluetooth/iso.c > +++ b/net/bluetooth/iso.c > @@ -717,6 +717,7 @@ static struct bt_iso_qos default_qos = { > .sync_cte_type = 0x00, > .mse = 0x00, > .timeout = 0x4000, > + .num_bis = 0x01, > }, > }; > > @@ -1249,6 +1250,9 @@ static bool check_bcast_qos(struct bt_iso_qos *qos) > if (qos->bcast.timeout < 0x000a || qos->bcast.timeout > 0x4000) > return false; > > + if (qos->bcast.num_bis < 0x01 || qos->bcast.num_bis > ISO_MAX_NUM_BIS) > + return false; > + > return true; > } > > -- > 2.34.1 > -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] Bluetooth: Add support for creating multiple BISes 2023-05-17 17:55 ` [PATCH 1/1] " Luiz Augusto von Dentz @ 2023-05-18 7:15 ` Iulia Tanasescu 2023-05-18 17:13 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 8+ messages in thread From: Iulia Tanasescu @ 2023-05-18 7:15 UTC (permalink / raw) To: luiz.dentz; +Cc: iulia.tanasescu, linux-bluetooth Hi Luiz, Thank you for the review, let me better explain the flow that I proposed, because I think I should have added a more detailed description to this patch. I added the num_bis field to the QoS structure so that the user can specify from the start the total number of connections that will be opened for the BIG, but one socket will always be connected to an unique BIS. So the user will first open a socket, set the QoS options with the num_bis parameter set to the number of BISes, and then the user will call connect on that socket. The BIG will be created with the specified number of BISes, but the socket will only be connected to one of them. The rest of the connection handles will be stored in the "bigs" queue that I added. Later on, the user might decide to open new sockets, for the rest of the BISes that are created and stored in the queue. In this case, the connect API on the socket will not issue the LE Create BIG command again, but it will extract a connection handle from the queue and the socket will be connected instantly. As for the HCI_CONN_PER_ADV flag, I noticed that it was only checked in the "bis_cleanup" function, to decide whether the advertising set and the BIG should be terminated. I removed it because now I am only terminating the advertising set and the BIG if all of the BIS handles have been assigned and no other BISes are in the BT_CONNECTED state for that BIG, so I thought I might not need the flag anymore. I think it's a better idea to use DEFER_SETUP for binding multiple BISes to a BIG, instead of using the num_bis QoS parameter, so that I can keep each socket completely separated from information about other connections, so I will update the implementation. Regards, Iulia ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] Bluetooth: Add support for creating multiple BISes 2023-05-18 7:15 ` Iulia Tanasescu @ 2023-05-18 17:13 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 8+ messages in thread From: Luiz Augusto von Dentz @ 2023-05-18 17:13 UTC (permalink / raw) To: Iulia Tanasescu; +Cc: linux-bluetooth Hi Iulia, On Thu, May 18, 2023 at 12:15 AM Iulia Tanasescu <iulia.tanasescu@nxp.com> wrote: > > Hi Luiz, > > Thank you for the review, let me better explain the flow that I proposed, > because I think I should have added a more detailed description to this > patch. > > I added the num_bis field to the QoS structure so that the user can > specify from the start the total number of connections that will be > opened for the BIG, but one socket will always be connected to an > unique BIS. > > So the user will first open a socket, set the QoS options with the > num_bis parameter set to the number of BISes, and then the user will > call connect on that socket. The BIG will be created with the specified > number of BISes, but the socket will only be connected to one of them. > The rest of the connection handles will be stored in the "bigs" queue > that I added. > > Later on, the user might decide to open new sockets, for the rest of > the BISes that are created and stored in the queue. In this case, > the connect API on the socket will not issue the LE Create BIG command > again, but it will extract a connection handle from the queue and the > socket will be connected instantly. Ok, that makes more sense now. > As for the HCI_CONN_PER_ADV flag, I noticed that it was only checked > in the "bis_cleanup" function, to decide whether the advertising set > and the BIG should be terminated. I removed it because now I am only > terminating the advertising set and the BIG if all of the BIS handles > have been assigned and no other BISes are in the BT_CONNECTED state > for that BIG, so I thought I might not need the flag anymore. Hmm, I think Ive added it in case the BIG was not created yet but the socket is terminated then we need to clean up the periodic advertising set since we don't have a hci_conn yet. > I think it's a better idea to use DEFER_SETUP for binding multiple > BISes to a BIG, instead of using the num_bis QoS parameter, so that > I can keep each socket completely separated from information about > other connections, so I will update the implementation. Yeah, I think adding BIS on-demand based on the number of sockets open is more consistent to how we handle that in case of unicast. > Regards, > Iulia -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-05-18 17:13 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-17 7:27 [PATCH 0/1] Bluetooth: Add support for creating multiple BISes Iulia Tanasescu 2023-05-17 7:27 ` [PATCH 1/1] " Iulia Tanasescu 2023-05-17 7:33 ` Paul Menzel 2023-05-18 7:19 ` Iulia Tanasescu 2023-05-17 7:57 ` bluez.test.bot 2023-05-17 17:55 ` [PATCH 1/1] " Luiz Augusto von Dentz 2023-05-18 7:15 ` Iulia Tanasescu 2023-05-18 17:13 ` Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).