Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230622192722.1248374-1-luiz.dentz@gmail.com> <512cf6a2dd110bf634e391f92aa613ca44a7a756.camel@iki.fi>
In-Reply-To: <512cf6a2dd110bf634e391f92aa613ca44a7a756.camel@iki.fi>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 23 Jun 2023 12:59:03 -0700
Message-ID: <CABBYNZ+9bP15aOu9Us9g3cC98Y_GidLnTJS-Y0fUhV9WNdUjdQ@mail.gmail.com>
Subject: Re: [PATCH v4] Bluetooth: hci_event: Fix parsing of CIS Established Event
To:     Pauli Virtanen <pav@iki.fi>
Cc:     linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Pauli,

On Fri, Jun 23, 2023 at 10:37=E2=80=AFAM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> to, 2023-06-22 kello 12:27 -0700, Luiz Augusto von Dentz kirjoitti:
> > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
> > The ISO Interval on CIS Established Event uses 1.25 ms slots:
> >
> >     BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E
> >     page 2304:
> >
> >       Time =3D N * 1.25 ms
> >
> > In addition to that this always update the QoS settings based on CIS
> > Established Event.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> >  net/bluetooth/hci_event.c | 36 +++++++++++++++++++++++-------------
> >  1 file changed, 23 insertions(+), 13 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index b1aefe4bb751..049aa7f6a7c5 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -6822,6 +6822,7 @@ static void hci_le_cis_estabilished_evt(struct hc=
i_dev *hdev, void *data,
> >  {
> >       struct hci_evt_le_cis_established *ev =3D data;
> >       struct hci_conn *conn;
> > +     struct bt_iso_qos *qos;
> >       bool pending =3D false;
> >       u16 handle =3D __le16_to_cpu(ev->handle);
> >
> > @@ -6846,21 +6847,30 @@ static void hci_le_cis_estabilished_evt(struct =
hci_dev *hdev, void *data,
> >
> >       pending =3D test_and_clear_bit(HCI_CONN_CREATE_CIS, &conn->flags)=
;
> >
> > -     if (conn->role =3D=3D HCI_ROLE_SLAVE) {
> > -             __le32 interval;
> > +     qos =3D &conn->iso_qos;
> >
> > -             memset(&interval, 0, sizeof(interval));
> > +     /* Convert ISO Interval (1.25 ms slots) to latency (ms) */
> > +     qos->ucast.in.latency =3D DIV_ROUND_CLOSEST(le16_to_cpu(ev->inter=
val) *
> > +                                               125, 100);
> > +     qos->ucast.out.latency =3D qos->ucast.in.latency;
>
> In Set CIG Parameters:
> u16 ucast.latency =3D Max_Transport_Latency (ms)
> u32 ucast.interval =3D SDU_Interval (us)
>
> Here:
> u16 ucast.latency =3D ISO_Interval (ms)
> u32 ucast.interval =3D Transport_Latency (us)

Yeah, it sounds like we should keep the same logic as in Set CIG
Parameters, this event is sort of weird because it does change the
units for whatever reason, anyway I will swap these around so they are
the same, anyway with the latest code it does pass all the iso-tester
tests.

> Currently BlueZ seems to not account for this swapping of the meanings
> of the QoS fields, eg. in client/player.c it has
>
>     num =3D (qos.ucast.out.latency * 1000 / qos.ucast.out.interval);
>     ...
>     ts.it_value.tv_nsec =3D qos->ucast.out.latency * 1000000
>
> and writes num packets in each interval. In the AX210 example it seems
> this would give num =3D=3D 0. I guess this and other places that use thes=
e
> need to be updated.
>
> Since field meanings anyway change, would 1.25ms unit for the
> ISO_Interval be better than 1ms so user side doesn't need to know how
> kernel rounds the number and undo that to get the actual value?
>
> > -             memcpy(&interval, ev->c_latency, sizeof(ev->c_latency));
> > -             conn->iso_qos.ucast.in.interval =3D le32_to_cpu(interval)=
;
> > -             memcpy(&interval, ev->p_latency, sizeof(ev->p_latency));
> > -             conn->iso_qos.ucast.out.interval =3D le32_to_cpu(interval=
);
> > -             conn->iso_qos.ucast.in.latency =3D le16_to_cpu(ev->interv=
al);
> > -             conn->iso_qos.ucast.out.latency =3D le16_to_cpu(ev->inter=
val);
> > -             conn->iso_qos.ucast.in.sdu =3D le16_to_cpu(ev->c_mtu);
> > -             conn->iso_qos.ucast.out.sdu =3D le16_to_cpu(ev->p_mtu);
> > -             conn->iso_qos.ucast.in.phy =3D ev->c_phy;
> > -             conn->iso_qos.ucast.out.phy =3D ev->p_phy;
> > +     switch (conn->role) {
> > +     case HCI_ROLE_SLAVE:
> > +             qos->ucast.in.interval =3D get_unaligned_le24(ev->c_laten=
cy);
> > +             qos->ucast.out.interval =3D get_unaligned_le24(ev->p_late=
ncy);
> > +             qos->ucast.in.sdu =3D le16_to_cpu(ev->c_mtu);
> > +             qos->ucast.out.sdu =3D le16_to_cpu(ev->p_mtu);
> > +             qos->ucast.in.phy =3D ev->c_phy;
> > +             qos->ucast.out.phy =3D ev->p_phy;
> > +             break;
> > +     case HCI_ROLE_MASTER:
> > +             qos->ucast.out.interval =3D get_unaligned_le24(ev->c_late=
ncy);
> > +             qos->ucast.in.interval =3D get_unaligned_le24(ev->p_laten=
cy);
> > +             qos->ucast.out.sdu =3D le16_to_cpu(ev->c_mtu);
> > +             qos->ucast.in.sdu =3D le16_to_cpu(ev->p_mtu);
> > +             qos->ucast.out.phy =3D ev->c_phy;
> > +             qos->ucast.in.phy =3D ev->p_phy;
> > +             break;
> >       }
> >
> >       if (!ev->status) {
>


--=20
Luiz Augusto von Dentz