Hi Luiz,
> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> Sent: Wednesday, February 21, 2024 4:44 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: [RFC] Bluetooth: ISO: Reassemble fragmented BASE
>
> Hi Iulia,
>
> On Tue, Feb 20, 2024 at 11:35 AM Iulia Tanasescu
> <iulia.tanasescu@xxxxxxx> wrote:
> >
> > For a Broadcast Sink, this adds support for reassembling fragmented PA
> > data. This is needed if the BASE comes fragmented in multiple PA reports.
> > After all PA fragments have been reassembled, the BASE can be retrieved
> > by the user via getsockopt.
> >
> > PA fragments could be reassembled inside hci_event.c and a complete PA
> > report could be indicated to iso.c, or like it is currently, if PA
> > fragments are indicated separately, reassembly would need to be done in
> > iso.c.
> >
> > For a Broadcast Source, the user sets the BASE via setsockopt. The full
> > PA data is built at bis bind, inside conn->le_per_adv_data. This array
> > would be fit to hold the reassembled PA data on the Sink side as well,
> > but a listening socket does not have an associated hcon...Currently for
> > the Broadcast Sink, a hcon is created only after the first BIGInfo report,
> > which arrives after the PA sync established event and all initial PA
> > reports. The hcon is notified to iso.c, a child socket is created and it
> > is notified to userspace, to mark the PA sync step as completed.
> >
> > I see 2 possibilities:
> >
> > 1. Keep a pa_data array inside the iso sock struct, use it to reassemble
> > PA fragments, and extract BASE from it when needed.
> >
> > 2. Instead of creating the hcon after the first BIGInfo report, the hcon
> > could be created when the PA sync established event is received. The
> > le_per_adv_data can be used to reassemble the PA fragments, and the
> hcon
> > will be notified to iso.c after the first BIGInfo report, when all
> > information necessary to create and populate the child socket is
> > available.
> >
> > In this patch I am showing the first idea, but I think the second one
> > may provide a design more in line with the broadcast source scenario,
> > when handling PA data and BASE.
> >
> > Are there any opinions about this?
>
> I was actually going to ask about why we don't have an hcon at the
> early stages of PA Sync, Im doing some optimizations so we scan based
> on the QoS PHY which should be faster than scanning in all support
> PHYs like we do today, but at that point there is no hcon and
> therefore the is no visible QoS since the socket info is not public.
I agree, so similar to iso_connect_bis, I will create a hcon for the
parent socket at iso_listen_bis, and I will use hcon->le_per_adv_data
to reassemble PA reports.
>
> > Signed-off-by: Iulia Tanasescu <iulia.tanasescu@xxxxxxx>
> > ---
> > include/net/bluetooth/hci.h | 7 +++-
> > net/bluetooth/iso.c | 79 +++++++++++++++++++++++++++----------
> > 2 files changed, 64 insertions(+), 22 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 08cb5cb249a4..a856e88719d2 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -1,7 +1,7 @@
> > /*
> > BlueZ - Bluetooth protocol stack for Linux
> > Copyright (C) 2000-2001 Qualcomm Incorporated
> > - Copyright 2023 NXP
> > + Copyright 2023-2024 NXP
> >
> > Written 2000,2001 by Maxim Krasnyansky <maxk@xxxxxxxxxxxx>
> >
> > @@ -2037,6 +2037,7 @@ struct hci_cp_le_set_per_adv_params {
> > } __packed;
> >
> > #define HCI_MAX_PER_AD_LENGTH 252
> > +#define HCI_MAX_PER_AD_TOT_LENGTH 1650
> >
> > #define HCI_OP_LE_SET_PER_ADV_DATA 0x203f
> > struct hci_cp_le_set_per_adv_data {
> > @@ -2797,6 +2798,10 @@ struct hci_ev_le_per_adv_report {
> > __u8 data[];
> > } __packed;
> >
> > +#define LE_PA_DATA_COMPLETE 0x00
> > +#define LE_PA_DATA_MORE_TO_COME 0x01
> > +#define LE_PA_DATA_TRUNCATED 0x02
> > +
> > #define HCI_EV_LE_EXT_ADV_SET_TERM 0x12
> > struct hci_evt_le_ext_adv_set_term {
> > __u8 status;
> > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > index 04f6572d35f1..e9a3b4f74e2f 100644
> > --- a/net/bluetooth/iso.c
> > +++ b/net/bluetooth/iso.c
> > @@ -3,7 +3,7 @@
> > * BlueZ - Bluetooth protocol stack for Linux
> > *
> > * Copyright (C) 2022 Intel Corporation
> > - * Copyright 2023 NXP
> > + * Copyright 2023-2024 NXP
> > */
> >
> > #include <linux/module.h>
> > @@ -70,8 +70,15 @@ struct iso_pinfo {
> > unsigned long flags;
> > struct bt_iso_qos qos;
> > bool qos_user_set;
> > - __u8 base_len;
> > - __u8 base[BASE_MAX_LENGTH];
> > + union {
> > + __u8 base_len;
> > + __u16 pa_data_len;
> > + };
> > + union {
> > + __u8 base[BASE_MAX_LENGTH];
> > + __u8 pa_data[HCI_MAX_PER_AD_TOT_LENGTH];
> > + };
> > + __u16 pa_data_offset;
> > struct iso_conn *conn;
> > };
> >
> > @@ -1573,7 +1580,7 @@ static int iso_sock_getsockopt(struct socket
> *sock, int level, int optname,
> > struct sock *sk = sock->sk;
> > int len, err = 0;
> > struct bt_iso_qos *qos;
> > - u8 base_len;
> > + size_t base_len;
> > u8 *base;
> >
> > BT_DBG("sk %p", sk);
> > @@ -1612,13 +1619,20 @@ static int iso_sock_getsockopt(struct socket
> *sock, int level, int optname,
> > break;
> >
> > case BT_ISO_BASE:
> > - if (sk->sk_state == BT_CONNECTED &&
> > - !bacmp(&iso_pi(sk)->dst, BDADDR_ANY)) {
> > - base_len = iso_pi(sk)->conn->hcon->le_per_adv_data_len;
> > - base = iso_pi(sk)->conn->hcon->le_per_adv_data;
> > - } else {
> > + if (!bacmp(&iso_pi(sk)->dst, BDADDR_ANY)) {
> > + /* For a Broadcast Source, the BASE was stored
> > + * in iso_pi(sk)->base.
> > + */
> > base_len = iso_pi(sk)->base_len;
> > base = iso_pi(sk)->base;
> > + } else {
> > + /* For a Broadcast Sink, the complete data received in
> > + * PA reports is stored. Extract BASE from there.
> > + */
> > + base = eir_get_service_data(iso_pi(sk)->pa_data,
> > + iso_pi(sk)->pa_data_len,
> > + EIR_BAA_SERVICE_UUID,
> > + &base_len);
> > }
> >
> > len = min_t(unsigned int, len, base_len);
> > @@ -1834,8 +1848,9 @@ static void iso_conn_ready(struct iso_conn
> *conn)
> > bacpy(&iso_pi(sk)->dst, &hcon->dst);
> > iso_pi(sk)->dst_type = hcon->dst_type;
> > iso_pi(sk)->sync_handle = iso_pi(parent)->sync_handle;
> > - memcpy(iso_pi(sk)->base, iso_pi(parent)->base, iso_pi(parent)-
> >base_len);
> > - iso_pi(sk)->base_len = iso_pi(parent)->base_len;
> > + memcpy(iso_pi(sk)->pa_data, iso_pi(parent)->pa_data,
> > + iso_pi(parent)->pa_data_len);
> > + iso_pi(sk)->pa_data_len = iso_pi(parent)->pa_data_len;
> >
> > hci_conn_hold(hcon);
> > iso_chan_add(conn, sk, parent);
> > @@ -1904,8 +1919,8 @@ int iso_connect_ind(struct hci_dev *hdev,
> bdaddr_t *bdaddr, __u8 *flags)
> > * a BIG Info it attempts to check if there any listening socket with
> > * the same sync_handle and if it does then attempt to create a sync.
> > * 3. HCI_EV_LE_PER_ADV_REPORT: When a PA report is received, it is
> stored
> > - * in iso_pi(sk)->base so it can be passed up to user, in the case of a
> > - * broadcast sink.
> > + * in iso_pi(sk)->pa_data so the BASE can later be passed up to user, in
> > + * the case of a broadcast sink.
> > */
> > ev1 = hci_recv_event_data(hdev, HCI_EV_LE_PA_SYNC_ESTABLISHED);
> > if (ev1) {
> > @@ -1961,16 +1976,38 @@ 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);
> > if (ev3) {
> > - size_t base_len = ev3->length;
> > - u8 *base;
> > -
> > sk = iso_get_sock_listen(&hdev->bdaddr, bdaddr,
> > iso_match_sync_handle_pa_report, ev3);
> > - base = eir_get_service_data(ev3->data, ev3->length,
> > - EIR_BAA_SERVICE_UUID, &base_len);
> > - if (base && sk && base_len <= sizeof(iso_pi(sk)->base)) {
> > - memcpy(iso_pi(sk)->base, base, base_len);
> > - iso_pi(sk)->base_len = base_len;
> > +
> > + if (!sk)
> > + goto done;
> > +
> > + if (ev3->data_status == LE_PA_DATA_TRUNCATED) {
> > + /* The controller was unable to retrieve PA data. */
> > + memset(iso_pi(sk)->pa_data, 0,
> > + HCI_MAX_PER_AD_TOT_LENGTH);
> > + iso_pi(sk)->pa_data_len = 0;
> > + iso_pi(sk)->pa_data_offset = 0;
> > + return lm;
> > + }
> > +
> > + if (iso_pi(sk)->pa_data_offset + ev3->length >
> > + HCI_MAX_PER_AD_TOT_LENGTH)
> > + goto done;
> > +
> > + memcpy(iso_pi(sk)->pa_data + iso_pi(sk)->pa_data_offset,
> > + ev3->data, ev3->length);
> > + iso_pi(sk)->pa_data_offset += ev3->length;
> > +
> > + if (ev3->data_status == LE_PA_DATA_COMPLETE) {
> > + /* All PA data has been received. */
> > + iso_pi(sk)->pa_data_len = iso_pi(sk)->pa_data_offset;
> > + iso_pi(sk)->pa_data_offset = 0;
> > + } else {
> > + /* This is a PA data fragment. Keep pa_data_len set to 0
> > + * until all data has been reassembled.
> > + */
> > + iso_pi(sk)->pa_data_len = 0;
> > }
> > } else {
> > sk = iso_get_sock_listen(&hdev->bdaddr, BDADDR_ANY, NULL,
> NULL);
> > --
> > 2.39.2
> >
>
>
> --
> Luiz Augusto von Dentz
Regards,
Iulia
