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: <66add9bf780aca773d41cce3ea42589388de2543.camel@iki.fi>
Subject: Re: [PATCH v2] 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:   Wed, 21 Jun 2023 23:33:08 +0300
In-Reply-To: <20230621195407.977001-1-luiz.dentz@gmail.com>
References: <20230621195407.977001-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,

ke, 2023-06-21 kello 12:54 -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..6fca6d9f1b34 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 le16_to_cpu(ev->interval) * 125 / 100;
> +	/* Convert ISO Interval (1.25 ms slots) to latency (ms) */
> +	qos->ucast.out.latency =3D le16_to_cpu(ev->interval) * 125 / 100;
> =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;

Are the ucast.latency and ucast.interval the right way around here?

When I trying to use this in userspace, I expected ucast.interval
contains the ISO interval, because in Set CIG Parameters we use
ucast.interval to specify the SDU_Interval, and ucast.latency is used
for the Transport_Latency_C_To_P and Transport_Latency_P_To_C.

With real numbers the event (AX210<->AX210) looks like this

> HCI Event: LE Meta Event (0x3e) plen 29               #3493 [hci0] 486.97=
8955
      LE Connected Isochronous Stream Established (0x19)
        Status: Success (0x00)
        Connection Handle: 2560
        CIG Synchronization Delay: 4020 us (0x000fb4)
        CIS Synchronization Delay: 4020 us (0x000fb4)
        Central to Peripheral Latency: 94020 us (0x016f44)
        Peripheral to Central Latency: 94020 us (0x016f44)
        Central to Peripheral PHY: LE 2M (0x02)
        Peripheral to Central PHY: LE 2M (0x02)
        Number of Subevents: 2
        Central to Peripheral Burst Number: 1
        Peripheral to Central Burst Number: 1
        Central to Peripheral Flush Timeout: 10
        Peripheral to Central Flush Timeout: 10
        Central to Peripheral MTU: 240
        Peripheral to Central MTU: 120
        ISO Interval: 8


> +	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) {

--=20
Pauli Virtanen