Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3628-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240416102501.42996-1-vlad.pruteanu@nxp.com> <20240416102501.42996-2-vlad.pruteanu@nxp.com>
In-Reply-To: <20240416102501.42996-2-vlad.pruteanu@nxp.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Tue, 16 Apr 2024 10:31:53 -0400
Message-ID: <CABBYNZJ_4ekgf03fUDTKyQzz1Ucg=7JJbh=PtaGSJjqsrtz0NA@mail.gmail.com>
Subject: Re: [PATCH 1/1] Bluetooth: hci_event: Fix setting of broadcast qos
 interval and latency
To: Vlad Pruteanu <vlad.pruteanu@nxp.com>
Cc: linux-bluetooth@vger.kernel.org, claudia.rosu@nxp.com, 
	mihai-octavian.urzica@nxp.com, silviu.barbulescu@nxp.com, 
	iulia.tanasescu@nxp.com, andrei.istodorescu@nxp.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Vlad,

On Tue, Apr 16, 2024 at 6:25=E2=80=AFAM Vlad Pruteanu <vlad.pruteanu@nxp.co=
m> wrote:
>
> The latency parameter of the broadcast qos is calculated wrongly.
>
> BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 6, Part G
>
> For framed PDUs:
> Transport_Latency_BIG =3D BIG_Sync_Delay + PTO x (NSE / BN -
> IRC) * ISO_Interval + ISO_Interval + SDU_Interval
>
> For unframed PDUs:
> Transport_Latency =3D BIG_Sync_Delay + (PTO x (NSE / BN - IRC) +
> 1) x ISO_Interval - SDU_Interval
>
> For broadcast sink the latency should be taken directly from the
> BIG Sync Established event and for the source it should be taken
> from the Create BIG Complete event.
>
> The interval parameter of the broadcast qos reffers to the SDU
> Interval and shouldn't be set based on the latency. The Host will
> read this value from the BASE.
>
> Signed-off-by: Vlad Pruteanu <vlad.pruteanu@nxp.com>
> ---
>  net/bluetooth/hci_event.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 868ffccff773..dff4bedc3e9b 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -6983,6 +6983,8 @@ static void hci_le_create_big_complete_evt(struct h=
ci_dev *hdev, void *data,
>
>                 if (!ev->status) {
>                         conn->state =3D BT_CONNECTED;
> +                       conn->iso_qos.bcast.out.latency =3D
> +                               DIV_ROUND_CLOSEST(get_unaligned_le24(ev->=
transport_delay), 1000);

You should probably add comments quoting the spec, also you quoted in
the description that framed vs unframed but then the code does not
attempt to differentiate, so perhaps that is not really important and
then we can remove it, the important part is what shall be used as
transport latency.

>                         set_bit(HCI_CONN_BIG_CREATED, &conn->flags);
>                         rcu_read_unlock();
>                         hci_debugfs_create_conn(conn);
> @@ -7029,7 +7031,6 @@ static void hci_le_big_sync_established_evt(struct =
hci_dev *hdev, void *data,
>
>         for (i =3D 0; i < ev->num_bis; i++) {
>                 u16 handle =3D le16_to_cpu(ev->bis[i]);
> -               __le32 interval;
>
>                 bis =3D hci_conn_hash_lookup_handle(hdev, handle);
>                 if (!bis) {
> @@ -7044,11 +7045,10 @@ static void hci_le_big_sync_established_evt(struc=
t hci_dev *hdev, void *data,
>                         set_bit(HCI_CONN_PA_SYNC, &bis->flags);
>
>                 bis->iso_qos.bcast.big =3D ev->handle;
> -               memset(&interval, 0, sizeof(interval));
> -               memcpy(&interval, ev->latency, sizeof(ev->latency));
> -               bis->iso_qos.bcast.in.interval =3D le32_to_cpu(interval);
> -               /* Convert ISO Interval (1.25 ms slots) to latency (ms) *=
/
> -               bis->iso_qos.bcast.in.latency =3D le16_to_cpu(ev->interva=
l) * 125 / 100;
> +               /* Convert Transport Latency (us) to Latency (msec) */
> +               bis->iso_qos.bcast.in.latency =3D
> +                       DIV_ROUND_CLOSEST(get_unaligned_le24(ev->latency)=
,
> +                                         1000);

Ditto as above.

>                 bis->iso_qos.bcast.in.sdu =3D le16_to_cpu(ev->max_pdu);
>
>                 if (!ev->status) {
> --
> 2.40.1
>


--=20
Luiz Augusto von Dentz