MIME-Version: 1.0
References: <1fd2d4523c139deda93aab2c31f1508d79c32472.1676921889.git.pav@iki.fi>
In-Reply-To: <1fd2d4523c139deda93aab2c31f1508d79c32472.1676921889.git.pav@iki.fi>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 21 Feb 2023 14:08:31 -0800
Message-ID: <CABBYNZ+kGVtCXN1X7ZOG64mLfWfXBah_Qo5+cy2dGWQMbNidoA@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: ISO: fix timestamped HCI ISO data packet parsing
To:     Pauli Virtanen <pav@iki.fi>
Cc:     linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Pauli,

On Mon, Feb 20, 2023 at 11:42 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Use correct HCI ISO data packet header struct when the packet has
> timestamp. The timestamp, when present, goes before the other fields
> (Core v5.3 4E 5.4.5), so the structs are not compatible.
>
> Fixes: ccf74f2390d6 ("Bluetooth: Add BTPROTO_ISO socket type")
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>
> Notes:
>     My hardware doesn't seem to produce timestamped packets, so this is not
>     properly tested, except to the extent that it doesn't break the
>     non-timestamped code path.
>
>     Regardless, the current state of things looks wrong, so sending this to
>     the list in any case.

Perhaps we should first attempt to enable this in the emulator, Id use
BT_HCI_CMD_LE_READ_ISO_TX_SYNC as hint that the host is interested in
knowing the timestamp:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/emulator/btdev.c#n5752

>  net/bluetooth/iso.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index 2dabef488eaa..cb959e8eac18 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -1621,7 +1621,6 @@ static void iso_disconn_cfm(struct hci_conn *hcon, __u8 reason)
>  void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
>  {
>         struct iso_conn *conn = hcon->iso_data;
> -       struct hci_iso_data_hdr *hdr;
>         __u16 pb, ts, len;
>
>         if (!conn)
> @@ -1643,6 +1642,8 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
>                 }
>
>                 if (ts) {
> +                       struct hci_iso_ts_data_hdr *hdr;
> +
>                         /* TODO: add timestamp to the packet? */

Perhaps we should use skb_set_delivery_time to set the timestamp as
received by the controller? That said I don't know if the unit would
be compatible.

>                         hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
>                         if (!hdr) {
> @@ -1650,15 +1651,19 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
>                                 goto drop;
>                         }
>
> +                       len = __le16_to_cpu(hdr->slen);
>                 } else {
> +                       struct hci_iso_data_hdr *hdr;
> +
>                         hdr = skb_pull_data(skb, HCI_ISO_DATA_HDR_SIZE);
>                         if (!hdr) {
>                                 BT_ERR("Frame is too short (len %d)", skb->len);
>                                 goto drop;
>                         }
> +
> +                       len = __le16_to_cpu(hdr->slen);
>                 }
>
> -               len    = __le16_to_cpu(hdr->slen);
>                 flags  = hci_iso_data_flags(len);
>                 len    = hci_iso_data_len(len);
>
> --
> 2.39.2
>


-- 
Luiz Augusto von Dentz