Hi, On Tue, Jul 15, 2025 at 9:37 AM Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote: > > Hi Pauli, > > On Tue, Jul 15, 2025 at 9:30 AM Pauli Virtanen <pav@xxxxxx> wrote: > > > > Hi Yang, > > > > ma, 2025-07-07 kello 10:38 +0800, Yang Li via B4 Relay kirjoitti: > > > From: Yang Li <yang.li@xxxxxxxxxxx> > > > > > > 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@xxxxxxxxxxx> > > > --- > > > Changes in v4: > > > - Optimizing the code > > > - Link to v3: https://lore.kernel.org/r/20250704-iso_ts-v3-1-2328bc602961@xxxxxxxxxxx > > > > > > Changes in v3: > > > - Change to use hwtimestamp > > > - Link to v2: https://lore.kernel.org/r/20250702-iso_ts-v2-1-723d199c8068@xxxxxxxxxxx > > > > > > 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@xxxxxxxxxxx > > > --- > > > 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: > > 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@xxxxxx/ > > 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
