Re: [PATCH] Bluetooth: ISO: add socket option to report packet seqnum via CMSG

[Pauli Virtanen <pav@iki.fi>]

Hi,

ma, 2025-07-14 kello 16:15 +0200, Paul Menzel kirjoitti:
> Dear Pauli,
> 
> 
> Thank you for your patch.
> 
> 
> Am 14.07.25 um 16:02 schrieb Pauli Virtanen:
> > User applications need a way to track which ISO interval a given SDU
> > belongs to, to properly detect packet loss. All controllers do not set
> > timestamps, and it's not guaranteed user application receives all packet
> > reports (small socket buffer, or controller doesn't send all reports
> > like Intel AX210 is doing).
> > 
> > Add socket option BT_PKT_SEQNUM that enables reporting of received
> > packet ISO sequence number in BT_SCM_PKT_SEQNUM CMSG.
> 
> Are there user applications already supporting this, so it can be tested?

I sent the associated tests to linux-bluetooth list

https://lore.kernel.org/linux-bluetooth/c9a75585e3640d8a1efca0bf96158eec1ca25fdc.1752501450.git.pav@iki.fi/

> 
> > Signed-off-by: Pauli Virtanen <pav@iki.fi>
> > ---
> > 
> > Notes:
> >      Intel AX210 is not sending all reports:
> >      
> >      $ btmon -r dump.btsnoop -I -C90|grep -A1 'ISO Data RX: Handle 2304'
> >      ...
> >      > ISO Data RX: Handle 2304 flags 0x02 dlen 64                      #1713 [hci0] 22.567744
> >              dd 01 3c 00 6d 08 e9 14 1e 3b 85 7b 35 c2 25 0b  ..<.m....;.{5.%.
> >      --
> >      > ISO Data RX: Handle 2304 flags 0x02 dlen 64                      #1718 [hci0] 22.573745
> >              de 01 3c 00 41 65 22 4f 99 9b 0b b6 ff cb 06 00  ..<.Ae"O........
> >      --
> >      > ISO Data RX: Handle 2304 flags 0x02 dlen 64                      #1727 [hci0] 22.587933
> >              e0 01 3c 00 8b 6e 33 44 65 51 ee d7 e0 ee 49 d8  ..<..n3DeQ....I.
> >      --
> >      > ISO Data RX: Handle 2304 flags 0x02 dlen 64                      #1732 [hci0] 22.596742
> >              e1 01 3c 00 a7 48 54 a7 c1 9f dc 37 66 fe 04 ab  ..<..HT....7f...
> >      ...
> >      
> >      Here, report for packet with sequence number 0x01df is missing.
> 
> Sorry, but where are the sequence number in the trace?

It's the first two bytes, see Core specification Vol 4E Sec 5.4.5 "HCI
ISO Data packets".

> >      
> >      This may be spec violation by the controller, see Core v6.1 pp. 3702
> >      
> >          All SDUs shall be sent to the upper layer including the indication
> >          of validity of data. A report shall be sent to the upper layer if
> >          the SDU is completely missing.
> >      
> >      Regardless, it will be easier for user applications to see the HW
> >      sequence numbers directly, so they don't have to count packets and it's
> >      in any case more reliable if packets get dropped due to socket buffer
> >      size.
> 
> I wouldn’t mind to have the note in the commit message.

I'm not sure it's a spec violation --- the text in the specification is
not fully clear what "All SDUs" means in the context here --- so I
don't really want to say so in the commit message.

The limited socket buffer and tha AX210 drops some reports is mentioned
in the commit message.

> 
> >   include/net/bluetooth/bluetooth.h |  9 ++++++++-
> >   net/bluetooth/af_bluetooth.c      |  7 +++++++
> >   net/bluetooth/iso.c               | 21 ++++++++++++++++++---
> >   3 files changed, 33 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index 114299bd8b98..0e31779a3341 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -244,6 +244,10 @@ struct bt_codecs {
> >   
> >   #define BT_ISO_BASE		20
> >   
> > +#define BT_PKT_SEQNUM		21
> > +
> > +#define BT_SCM_PKT_SEQNUM	0x05
> > +
> >   __printf(1, 2)
> >   void bt_info(const char *fmt, ...);
> >   __printf(1, 2)
> > @@ -391,7 +395,8 @@ struct bt_sock {
> >   enum {
> >   	BT_SK_DEFER_SETUP,
> >   	BT_SK_SUSPEND,
> > -	BT_SK_PKT_STATUS
> > +	BT_SK_PKT_STATUS,
> > +	BT_SK_PKT_SEQNUM,
> >   };
> >   
> >   struct bt_sock_list {
> > @@ -475,6 +480,7 @@ struct bt_skb_cb {
> >   	u8 pkt_type;
> >   	u8 force_active;
> >   	u16 expect;
> > +	u16 pkt_seqnum;
> 
> Excuse my ignorance, just want to make sure, the type is big enough.

The hardware sequence number is also 16 bits.

> >   	u8 incoming:1;
> >   	u8 pkt_status:2;
> >   	union {
> > @@ -488,6 +494,7 @@ struct bt_skb_cb {
> >   
> >   #define hci_skb_pkt_type(skb) bt_cb((skb))->pkt_type
> >   #define hci_skb_pkt_status(skb) bt_cb((skb))->pkt_status
> > +#define hci_skb_pkt_seqnum(skb) bt_cb((skb))->pkt_seqnum
> >   #define hci_skb_expect(skb) bt_cb((skb))->expect
> >   #define hci_skb_opcode(skb) bt_cb((skb))->hci.opcode
> >   #define hci_skb_event(skb) bt_cb((skb))->hci.req_event
> > diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> > index 6ad2f72f53f4..44b7acb20a67 100644
> > --- a/net/bluetooth/af_bluetooth.c
> > +++ b/net/bluetooth/af_bluetooth.c
> > @@ -364,6 +364,13 @@ int bt_sock_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> >   			put_cmsg(msg, SOL_BLUETOOTH, BT_SCM_PKT_STATUS,
> >   				 sizeof(pkt_status), &pkt_status);
> >   		}
> > +
> > +		if (test_bit(BT_SK_PKT_SEQNUM, &bt_sk(sk)->flags)) {
> > +			u16 pkt_seqnum = hci_skb_pkt_seqnum(skb);
> > +
> > +			put_cmsg(msg, SOL_BLUETOOTH, BT_SCM_PKT_SEQNUM,
> > +				 sizeof(pkt_seqnum), &pkt_seqnum);
> > +		}
> >   	}
> >   
> >   	skb_free_datagram(sk, skb);
> > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> > index fc22782cbeeb..469450bb6b6c 100644
> > --- a/net/bluetooth/iso.c
> > +++ b/net/bluetooth/iso.c
> > @@ -1687,6 +1687,17 @@ static int iso_sock_setsockopt(struct socket *sock, int level, int optname,
> >   			clear_bit(BT_SK_PKT_STATUS, &bt_sk(sk)->flags);
> >   		break;
> >   
> > +	case BT_PKT_SEQNUM:
> > +		err = copy_safe_from_sockptr(&opt, sizeof(opt), optval, optlen);
> > +		if (err)
> > +			break;
> > +
> > +		if (opt)
> > +			set_bit(BT_SK_PKT_SEQNUM, &bt_sk(sk)->flags);
> > +		else
> > +			clear_bit(BT_SK_PKT_SEQNUM, &bt_sk(sk)->flags);
> > +		break;
> > +
> >   	case BT_ISO_QOS:
> >   		if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND &&
> >   		    sk->sk_state != BT_CONNECT2 &&
> > @@ -2278,7 +2289,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;
> > -	__u16 pb, ts, len;
> > +	__u16 pb, ts, len, sn;
> 
> Use `seqnum` for consistency with the parts above.
> 
> >   
> >   	if (!conn)
> >   		goto drop;
> > @@ -2308,6 +2319,7 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
> >   				goto drop;
> >   			}
> >   
> > +			sn = hdr->sn;
> >   			len = __le16_to_cpu(hdr->slen);
> >   		} else {
> >   			struct hci_iso_data_hdr *hdr;
> > @@ -2318,18 +2330,20 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
> >   				goto drop;
> >   			}
> >   
> > +			sn = hdr->sn;
> >   			len = __le16_to_cpu(hdr->slen);
> >   		}
> >   
> >   		flags  = hci_iso_data_flags(len);
> >   		len    = hci_iso_data_len(len);
> >   
> > -		BT_DBG("Start: total len %d, frag len %d flags 0x%4.4x", len,
> > -		       skb->len, flags);
> > +		BT_DBG("Start: total len %d, frag len %d flags 0x%4.4x sn %d",
> > +		       len, skb->len, flags, sn);
> >   
> >   		if (len == skb->len) {
> >   			/* Complete frame received */
> >   			hci_skb_pkt_status(skb) = flags & 0x03;
> > +			hci_skb_pkt_seqnum(skb) = sn;
> >   			iso_recv_frame(conn, skb);
> >   			return;
> >   		}
> > @@ -2352,6 +2366,7 @@ void iso_recv(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
> >   			goto drop;
> >   
> >   		hci_skb_pkt_status(conn->rx_skb) = flags & 0x03;
> > +		hci_skb_pkt_seqnum(conn->rx_skb) = sn;
> >   		skb_copy_from_linear_data(skb, skb_put(conn->rx_skb, skb->len),
> >   					  skb->len);
> >   		conn->rx_len = len - skb->len;
> 
> 
> Kind regards,
> 
> Paul

-- 
Pauli Virtanen