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;
Message-ID: <512cf6a2dd110bf634e391f92aa613ca44a7a756.camel@iki.fi>
Subject: Re: [PATCH v4] Bluetooth: hci_event: Fix parsing of CIS Established
 Event
From:   Pauli Virtanen <pav@iki.fi>
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
        linux-bluetooth@vger.kernel.org
Date:   Fri, 23 Jun 2023 20:37:22 +0300
In-Reply-To: <20230622192722.1248374-1-luiz.dentz@gmail.com>
References: <20230622192722.1248374-1-luiz.dentz@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.3 (3.48.3-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

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>
>=20
> The ISO Interval on CIS Established Event uses 1.25 ms slots:
>=20
>     BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E
>     page 2304:
>=20
>       Time =3D N * 1.25 ms
>=20
> In addition to that this always update the QoS settings based on CIS
> Established Event.
>=20
> 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(-)
>=20
> 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 hci_=
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);
> =20
> @@ -6846,21 +6847,30 @@ static void hci_le_cis_estabilished_evt(struct hc=
i_dev *hdev, void *data,
> =20
>  	pending =3D test_and_clear_bit(HCI_CONN_CREATE_CIS, &conn->flags);
> =20
> -	if (conn->role =3D=3D HCI_ROLE_SLAVE) {
> -		__le32 interval;
> +	qos =3D &conn->iso_qos;
> =20
> -		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->interval) *
> +						  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)

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.=C2=A0In the AX210 example it seems
this would give num =3D=3D 0. I guess this and other places that use these
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?
=20
> -		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->interval);
> -		conn->iso_qos.ucast.out.latency =3D le16_to_cpu(ev->interval);
> -		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_latency);
> +		qos->ucast.out.interval =3D get_unaligned_le24(ev->p_latency);
> +		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_latency);
> +		qos->ucast.in.interval =3D get_unaligned_le24(ev->p_latency);
> +		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;
>  	}
> =20
>  	if (!ev->status) {