Hi Luiz,
> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> Sent: Wednesday, October 11, 2023 11:08 PM
> To: Iulia Tanasescu <iulia.tanasescu@xxxxxxx>
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Claudia Cristina Draghicescu
> <claudia.rosu@xxxxxxx>; Mihai-Octavian Urzica <mihai-
> octavian.urzica@xxxxxxx>; Silviu Florian Barbulescu
> <silviu.barbulescu@xxxxxxx>; Vlad Pruteanu <vlad.pruteanu@xxxxxxx>;
> Andrei Istodorescu <andrei.istodorescu@xxxxxxx>
> Subject: Re: [PATCH 2/2] Bluetoth: ISO: Fix broadcast event handling
>
> Hi Iulia,
>
> On Wed, Oct 11, 2023 at 7:24 AM Iulia Tanasescu <iulia.tanasescu@xxxxxxx>
> wrote:
> >
> > This fixes the way broadcast events are handled by the ISO layer:
> > A new slave BIS hcon is notified after the Command Complete for
> > LE Setup ISO Data Path is received, not after BIG Sync Established.
> >
> > The iso_match_pa_sync_flag function has been replaced with more
> > specific matching functions, depending on the event being handled.
> >
> > Signed-off-by: Iulia Tanasescu <iulia.tanasescu@xxxxxxx>
> > ---
> > include/net/bluetooth/hci_core.h | 3 +-
> > net/bluetooth/hci_core.c | 50 ++++++++++-----
> > net/bluetooth/iso.c | 101 +++++++++++++++++++++++++------
> > 3 files changed, 119 insertions(+), 35 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h
> b/include/net/bluetooth/hci_core.h
> > index 99865c23e461..a85c47abd88d 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -2127,7 +2127,8 @@ void hci_send_sco(struct hci_conn *conn, struct
> sk_buff *skb);
> > void hci_send_iso(struct hci_conn *conn, struct sk_buff *skb);
> >
> > void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode);
> > -void *hci_recv_event_data(struct hci_dev *hdev, __u8 event);
> > +void *hci_le_meta_evt_data(struct hci_dev *hdev, __u8 subevent);
> > +void *hci_cmd_complete_data(struct hci_dev *hdev, __u16 opcode);
> >
> > u32 hci_conn_get_phy(struct hci_conn *conn);
> >
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 195aea2198a9..5ccc6ef1b66b 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -3120,10 +3120,11 @@ void *hci_sent_cmd_data(struct hci_dev
> *hdev, __u16 opcode)
> > return hdev->sent_cmd->data + HCI_COMMAND_HDR_SIZE;
> > }
> >
> > -/* Get data from last received event */
> > -void *hci_recv_event_data(struct hci_dev *hdev, __u8 event)
> > +/* Get data from last received LE Meta event */
> > +void *hci_le_meta_evt_data(struct hci_dev *hdev, __u8 subevent)
> > {
> > struct hci_event_hdr *hdr;
> > + struct hci_ev_le_meta *ev;
> > int offset;
> >
> > if (!hdev->recv_event)
> > @@ -3132,21 +3133,42 @@ void *hci_recv_event_data(struct hci_dev
> *hdev, __u8 event)
> > hdr = (void *)hdev->recv_event->data;
> > offset = sizeof(*hdr);
> >
> > - if (hdr->evt != event) {
> > - /* In case of LE metaevent check the subevent match */
> > - if (hdr->evt == HCI_EV_LE_META) {
> > - struct hci_ev_le_meta *ev;
> > + if (hdr->evt != HCI_EV_LE_META)
> > + return NULL;
> >
> > - ev = (void *)hdev->recv_event->data + offset;
> > - offset += sizeof(*ev);
> > - if (ev->subevent == event)
> > - goto found;
> > - }
> > + ev = (void *)hdev->recv_event->data + offset;
> > + offset += sizeof(*ev);
> > + if (ev->subevent != subevent)
> > + return NULL;
> > +
> > + bt_dev_dbg(hdev, "subevent 0x%2.2x", subevent);
> > +
> > + return hdev->recv_event->data + offset;
> > +}
> > +
> > +/* Get data from last received Command Complete event */
> > +void *hci_cmd_complete_data(struct hci_dev *hdev, __u16 opcode)
> > +{
> > + struct hci_event_hdr *hdr;
> > + struct hci_ev_cmd_complete *ev;
> > + int offset;
> > +
> > + if (!hdev->recv_event)
> > + return NULL;
> > +
> > + hdr = (void *)hdev->recv_event->data;
> > + offset = sizeof(*hdr);
> > +
> > + if (hdr->evt != HCI_EV_CMD_COMPLETE)
> > + return NULL;
> > +
> > + ev = (void *)hdev->recv_event->data + offset;
> > + offset += sizeof(*ev);
> > +
> > + if (__le16_to_cpu(ev->opcode) != opcode)
> > return NULL;
> > - }
> >
> > -found:
> > - bt_dev_dbg(hdev, "event 0x%2.2x", event);
> > + bt_dev_dbg(hdev, "command complete event for 0x%4.4x", opcode);
> >
> > return hdev->recv_event->data + offset;
> > }
> > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > index 8ab7ea5ebedf..76c186f951d1 100644
> > --- a/net/bluetooth/iso.c
> > +++ b/net/bluetooth/iso.c
> > @@ -1560,6 +1560,16 @@ struct iso_list_data {
> > int count;
> > };
> >
> > +static bool iso_match_big_flag(struct sock *sk, void *data)
> > +{
> > + struct hci_evt_le_big_sync_estabilished *ev = data;
> > +
> > + if (!test_bit(BT_SK_PA_SYNC, &iso_pi(sk)->flags))
> > + return false;
> > +
> > + return ev->handle == iso_pi(sk)->qos.bcast.big;
> > +}
> > +
> > static bool iso_match_big(struct sock *sk, void *data)
> > {
> > struct hci_evt_le_big_sync_estabilished *ev = data;
> > @@ -1567,15 +1577,28 @@ static bool iso_match_big(struct sock *sk, void
> *data)
> > return ev->handle == iso_pi(sk)->qos.bcast.big;
> > }
> >
> > -static bool iso_match_pa_sync_flag(struct sock *sk, void *data)
> > +static bool iso_match_conn_big_flag(struct sock *sk, void *data)
> > +{
> > + struct hci_conn *hcon = data;
> > +
> > + if (!test_bit(BT_SK_PA_SYNC, &iso_pi(sk)->flags))
> > + return false;
> > +
> > + return hcon->iso_qos.bcast.big == iso_pi(sk)->qos.bcast.big;
> > +}
> > +
> > +static bool iso_match_conn_big(struct sock *sk, void *data)
> > {
> > - return test_bit(BT_SK_PA_SYNC, &iso_pi(sk)->flags);
> > + struct hci_conn *hcon = data;
> > +
> > + return hcon->iso_qos.bcast.big == iso_pi(sk)->qos.bcast.big;
> > }
> >
> > static void iso_conn_ready(struct iso_conn *conn)
> > {
> > struct sock *parent = NULL;
> > struct sock *sk = conn->sk;
> > + struct hci_rp_le_setup_iso_path *rp = NULL;
> > struct hci_ev_le_big_sync_estabilished *ev = NULL;
> > struct hci_ev_le_pa_sync_established *ev2 = NULL;
> > struct hci_evt_le_big_info_adv_report *ev3 = NULL;
> > @@ -1590,29 +1613,56 @@ static void iso_conn_ready(struct iso_conn
> *conn)
> > if (!hcon)
> > return;
> >
> > - if (test_bit(HCI_CONN_BIG_SYNC, &hcon->flags) ||
> > - test_bit(HCI_CONN_BIG_SYNC_FAILED, &hcon->flags)) {
> > - ev = hci_recv_event_data(hcon->hdev,
> > - HCI_EVT_LE_BIG_SYNC_ESTABILISHED);
> > + if (test_bit(HCI_CONN_BIG_SYNC, &hcon->flags)) {
> > + rp = hci_cmd_complete_data(hcon->hdev,
> > + HCI_OP_LE_SETUP_ISO_PATH);
> >
> > - /* Get reference to PA sync parent socket, if it exists */
> > - parent = iso_get_sock_listen(&hcon->src,
> > - &hcon->dst,
> > - iso_match_pa_sync_flag, NULL);
> > - if (!parent && ev)
> > + if (rp) {
> > + /* If defer setup was used to listen for this
> > + * event, get reference to the socket marked
> > + * with the BT_SK_PA_SYNC flag.
> > + */
> > parent = iso_get_sock_listen(&hcon->src,
> > &hcon->dst,
> > - iso_match_big, ev);
> > + iso_match_conn_big_flag,
> > + hcon);
> > +
> > + if (!parent)
> > + parent = iso_get_sock_listen(&hcon->src,
> > + &hcon->dst,
> > + iso_match_conn_big,
> > + hcon);
> > + }
> > + } else if (test_bit(HCI_CONN_BIG_SYNC_FAILED, &hcon->flags)) {
> > + ev = hci_le_meta_evt_data(hcon->hdev,
> > + HCI_EVT_LE_BIG_SYNC_ESTABILISHED);
> > +
> > + if (ev) {
> > + /* If defer setup was used to listen for this
> > + * event, get reference to the socket marked
> > + * with the BT_SK_PA_SYNC flag.
> > + */
> > + parent = iso_get_sock_listen(&hcon->src,
> > + &hcon->dst,
> > + iso_match_big_flag,
> > + ev);
> > +
> > + if (!parent)
> > + parent = iso_get_sock_listen(&hcon->src,
> > + &hcon->dst,
> > + iso_match_big,
> > + ev);
> > + }
> > } else if (test_bit(HCI_CONN_PA_SYNC_FAILED, &hcon->flags)) {
> > - ev2 = hci_recv_event_data(hcon->hdev,
> > - HCI_EV_LE_PA_SYNC_ESTABLISHED);
> > + ev2 = hci_le_meta_evt_data(hcon->hdev,
> > + HCI_EV_LE_PA_SYNC_ESTABLISHED);
> > if (ev2)
> > parent = iso_get_sock_listen(&hcon->src,
> > &hcon->dst,
> > iso_match_sid, ev2);
> > } else if (test_bit(HCI_CONN_PA_SYNC, &hcon->flags)) {
> > - ev3 = hci_recv_event_data(hcon->hdev,
> > - HCI_EVT_LE_BIG_INFO_ADV_REPORT);
> > + ev3 = hci_le_meta_evt_data(hcon->hdev,
> > + HCI_EVT_LE_BIG_INFO_ADV_REPORT);
> > if (ev3)
> > parent = iso_get_sock_listen(&hcon->src,
> > &hcon->dst,
>
> I really regret putting the handling of HCI in iso.c, it doesn't seem
> to be the right place to be processing HCI traffic and should be left
> to just handle socket related operations. The main problem with
> broadcast is that it requires multiples commands to setup but it
> should be possible to enclosure that into a cmd_sync function that
> properly serializes everything, we may need to do some cleanups first
> since he have been running cmd_sync callbacks in different files when
> that shall always be done hci_sync.c to make it simpler to reuse, then
> we can probably inline the sending of HCI_OP_LE_SETUP_ISO_PATH, etc,
> so we don't have to keep process them asynchronously via hci_event.c.
In iso_conn_ready HCI events are processed just to create child socket
and match with parent. Different events generate different socket types:
HCI_OP_LE_SETUP_ISO_PATH creates a BIS socket,
HCI_EVT_LE_BIG_INFO_ADV_REPORT creates a PA sync socket etc.
So the ISO layer needs to create new sockets and enqueue for accept
depending on event. I don't think this part can be moved somewhere else,
because it's related to socket handling.
The HCI state machine for setup is already enclosed in cmd_sync
functions, like hci_pa_create_sync:
https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/hci_conn.c#n2149
>
> > @@ -1700,6 +1750,16 @@ static bool iso_match_sid(struct sock *sk, void
> *data)
> > return ev->sid == iso_pi(sk)->bc_sid;
> > }
> >
> > +static bool iso_match_sync_handle_flag(struct sock *sk, void *data)
> > +{
> > + struct hci_evt_le_big_info_adv_report *ev = data;
> > +
> > + if (!test_bit(BT_SK_PA_SYNC, &iso_pi(sk)->flags))
> > + return false;
> > +
> > + return le16_to_cpu(ev->sync_handle) == iso_pi(sk)->sync_handle;
> > +}
> > +
> > static bool iso_match_sync_handle(struct sock *sk, void *data)
> > {
> > struct hci_evt_le_big_info_adv_report *ev = data;
> > @@ -1740,7 +1800,7 @@ int iso_connect_ind(struct hci_dev *hdev,
> bdaddr_t *bdaddr, __u8 *flags)
> > * in iso_pi(sk)->base so it can be passed up to user, in the case of a
> > * broadcast sink.
> > */
> > - ev1 = hci_recv_event_data(hdev, HCI_EV_LE_PA_SYNC_ESTABLISHED);
> > + ev1 = hci_le_meta_evt_data(hdev,
> HCI_EV_LE_PA_SYNC_ESTABLISHED);
> > if (ev1) {
> > sk = iso_get_sock_listen(&hdev->bdaddr, bdaddr, iso_match_sid,
> > ev1);
> > @@ -1750,11 +1810,12 @@ int iso_connect_ind(struct hci_dev *hdev,
> bdaddr_t *bdaddr, __u8 *flags)
> > goto done;
> > }
> >
> > - ev2 = hci_recv_event_data(hdev,
> HCI_EVT_LE_BIG_INFO_ADV_REPORT);
> > + ev2 = hci_le_meta_evt_data(hdev,
> HCI_EVT_LE_BIG_INFO_ADV_REPORT);
> > if (ev2) {
> > /* Try to get PA sync listening socket, if it exists */
> > sk = iso_get_sock_listen(&hdev->bdaddr, bdaddr,
> > - iso_match_pa_sync_flag, NULL);
> > + iso_match_sync_handle_flag,
> > + ev2);
> > if (!sk)
> > sk = iso_get_sock_listen(&hdev->bdaddr, bdaddr,
> > iso_match_sync_handle, ev2);
> > @@ -1780,7 +1841,7 @@ int iso_connect_ind(struct hci_dev *hdev,
> bdaddr_t *bdaddr, __u8 *flags)
> > }
> > }
> >
> > - ev3 = hci_recv_event_data(hdev, HCI_EV_LE_PER_ADV_REPORT);
> > + ev3 = hci_le_meta_evt_data(hdev, HCI_EV_LE_PER_ADV_REPORT);
> > if (ev3) {
> > sk = iso_get_sock_listen(&hdev->bdaddr, bdaddr,
> > iso_match_sync_handle_pa_report, ev3);
> > --
> > 2.39.2
> >
>
>
> --
> Luiz Augusto von Dentz
Regards,
Iulia
