Re: [PATCH v4] Bluetooth: ISO: Support SCM_TIMESTAMPING for ISO TS

[Yang Li <yang.li@amlogic.com>]

Hi Luiz,
> [ EXTERNAL EMAIL ]
>
> Hi,
>
> On Tue, Jul 15, 2025 at 9:37 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Pauli,
>>
>> On Tue, Jul 15, 2025 at 9:30 AM Pauli Virtanen <pav@iki.fi> wrote:
>>> Hi Yang,
>>>
>>> ma, 2025-07-07 kello 10:38 +0800, Yang Li via B4 Relay kirjoitti:
>>>> From: Yang Li <yang.li@amlogic.com>
>>>>
>>>> User-space applications (e.g. PipeWire) depend on
>>>> ISO-formatted timestamps for precise audio sync.
>>>>
>>>> The ISO ts is based on the controller’s clock domain,
>>>> so hardware timestamping (hwtimestamp) must be used.
>>>>
>>>> Ref: Documentation/networking/timestamping.rst,
>>>> section 3.1 Hardware Timestamping.
>>>>
>>>> Signed-off-by: Yang Li <yang.li@amlogic.com>
>>>> ---
>>>> Changes in v4:
>>>> - Optimizing the code
>>>> - Link to v3: https://lore.kernel.org/r/20250704-iso_ts-v3-1-2328bc602961@amlogic.com
>>>>
>>>> Changes in v3:
>>>> - Change to use hwtimestamp
>>>> - Link to v2: https://lore.kernel.org/r/20250702-iso_ts-v2-1-723d199c8068@amlogic.com
>>>>
>>>> Changes in v2:
>>>> - Support SOCK_RCVTSTAMPNS via CMSG for ISO sockets
>>>> - Link to v1: https://lore.kernel.org/r/20250429-iso_ts-v1-1-e586f30de6cb@amlogic.com
>>>> ---
>>>>   net/bluetooth/iso.c | 6 +++++-
>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
>>>> index fc22782cbeeb..677144bb6b94 100644
>>>> --- a/net/bluetooth/iso.c
>>>> +++ b/net/bluetooth/iso.c
>>>> @@ -2278,6 +2278,7 @@ 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 skb_shared_hwtstamps *hwts;
>>>>        __u16 pb, ts, len;
>>>>
>>>>        if (!conn)
>>>> @@ -2301,13 +2302,16 @@ 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? */
>>>>                        hdr = skb_pull_data(skb, HCI_ISO_TS_DATA_HDR_SIZE);
>>>>                        if (!hdr) {
>>>>                                BT_ERR("Frame is too short (len %d)", skb->len);
>>>>                                goto drop;
>>>>                        }
>>>>
>>>> +                     /*  Record the timestamp to skb*/
>>>> +                     hwts = skb_hwtstamps(skb);
>>>> +                     hwts->hwtstamp = us_to_ktime(le32_to_cpu(hdr->ts));
>>> Several lines below there is
>>>
>>>          conn->rx_skb = bt_skb_alloc(len, GFP_KERNEL);
>>>          skb_copy_from_linear_data(skb, skb_put(conn->rx_skb, skb-
>>>> len),
>>>                                                    skb->len);
>>>
>>> so timestamp should be copied explicitly also into conn->rx_skb,
>>> otherwise it gets lost when you have ACL-fragmented ISO packets.
>> Yep, it is not that the code is completely wrong but it is operating
>> on the original skb not in the rx_skb as you said, that said is only
>> the first fragment that contains the ts header so we only have to do
>> it once in case that was not clear.
> I might just do a fixup myself, something like the following:
>
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index 0a951c6514af..f48fb62e640d 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -2374,6 +2374,13 @@ void iso_recv(struct hci_conn *hcon, struct
> sk_buff *skb, u16 flags)
>                  skb_copy_from_linear_data(skb, skb_put(conn->rx_skb, skb->len),
>                                            skb->len);
>                  conn->rx_len = len - skb->len;
> +
> +               /* Copy timestamp from skb to rx_skb if present */
> +               if (ts) {
> +                       hwts = skb_hwtstamps(conn->rx_skb);
> +                       hwts->hwtstamp = skb_hwtstamps(skb)->hwtstamp;
> +               }
> +
>                  break;
>
>          case ISO_CONT:
>

Well, that's great!
Thanks so much for your help.
>>> It could also be useful to write a simple test case that extracts the
>>> timestamp from CMSG, see for example how it was done for BT_PKT_SEQNUM:
>>> https://lore.kernel.org/linux-bluetooth/b98b7691e4ba06550bb8f275cad0635bc9e4e8d2.1752511478.git.pav@iki.fi/
>>> bthost_send_iso() can take ts=true and some timestamp value.
>>>
>>>> +
>>>>                        len = __le16_to_cpu(hdr->slen);
>>>>                } else {
>>>>                        struct hci_iso_data_hdr *hdr;
>>>>
>>>> ---
>>>> base-commit: b8db3a9d4daeb7ff6a56c605ad6eca24e4da78ed
>>>> change-id: 20250421-iso_ts-c82a300ae784
>>>>
>>>> Best regards,
>>> --
>>> Pauli Virtanen
>>
>>
>> --
>> Luiz Augusto von Dentz
>
>
> --
> Luiz Augusto von Dentz