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

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

[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.

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.
    
    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.

 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;
 	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;
 
 	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;
-- 
2.50.1

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

[Luiz Augusto von Dentz]

Hi Pauli,

On Mon, Jul 14, 2025 at 10:03 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> 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.
>
> 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.
>
>     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.

We may want to report this to Intel, let me check internally.

>     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.
>
>  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;

We may also need the status or are you planning on reusing the
existing pkt_status? In any case we may want to add something to
iso-tester to confirm this is working as intended and catch possible
regressions.

>         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);
> +               }

In case we want to reuse the pkt_status then perhaps the order shall
be pk_seqnum followed by pkt_status otherwise we need a struct that
holds them both.

>         }
>
>         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;
>
>         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;
> --
> 2.50.1
>


-- 
Luiz Augusto von Dentz

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

[Pauli Virtanen]

Hi Luiz,

ma, 2025-07-14 kello 10:15 -0400, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Mon, Jul 14, 2025 at 10:03 AM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > 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.
> > 
> > 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.
> > 
> >     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.
> 
> We may want to report this to Intel, let me check internally.
> 
> >     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.
> > 
> >  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;
> 
> We may also need the status or are you planning on reusing the
> existing pkt_status? In any case we may want to add something to
> iso-tester to confirm this is working as intended and catch possible
> regressions.

BT_PKT_STATUS + BT_SCM_PKT_STATUS are already implemented for ISO, and
there is test for it in iso-tester.c

ISO Receive Packet Status - Success

How it works in this version is that user application that wants to get
both does

	opt = 1;
	setsockopt(fd, SOL_BLUETOOTH, BT_PKT_STATUS, &opt, sizeof(opt));
	opt = 1;
	setsockopt(fd, SOL_BLUETOOTH, BT_PKT_SEQNUM, &opt, sizeof(opt));
	...
	uint16_t seqnum;
	uint8_t status;
	for (cmsg=CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
		if (cmsg->cmsg_level != SOL_BLUETOOTH)
			continue;
		if (cmsg->cmsg_type == BT_SCM_PKT_SEQNUM)
			memcpy(&seqnum, CMSG_DATA(cmsg), sizeof(uint16_t));
		else if (cmsg->cmsg_type == BT_SCM_PKT_STATUS)
			memcpy(&status, CMSG_DATA(cmsg), sizeof(uint8_t));
	}

In theory we might indeed also change BT_SCM_PKT_STATUS to a struct
like

	struct bt_iso_pkt_status {
		u8 status;
		u16 seqnum;
	} __packed;

It's then not really fully compatible with any existing applications,
since applications may be eg using something like

	char buf[CMSG_SPACE(uint8_t)];

to reserve space for the BT_PKT_STATUS CMSG, which then won't
necessarily fit anymore. Maybe it could be changed just for ISO, but
then different socket types would have different CMSG size for the same
SCM.

I think it's probably OK to use separate CMSG like here, then user
application can also know if kernel supports the socket option.

> >         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);
> > +               }
> 
> In case we want to reuse the pkt_status then perhaps the order shall
> be pk_seqnum followed by pkt_status otherwise we need a struct that
> holds them both.

The order of the CMSG shouldn't matter if they have separate BT_SCM
types & socket flags.

> 
> >         }
> > 
> >         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;
> > 
> >         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;
> > --
> > 2.50.1
> > 
> 

-- 
Pauli Virtanen

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

[Luiz Augusto von Dentz]

Hi Pauli,

On Mon, Jul 14, 2025 at 10:45 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> ma, 2025-07-14 kello 10:15 -0400, Luiz Augusto von Dentz kirjoitti:
> > Hi Pauli,
> >
> > On Mon, Jul 14, 2025 at 10:03 AM Pauli Virtanen <pav@iki.fi> wrote:
> > >
> > > 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.
> > >
> > > 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.
> > >
> > >     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.
> >
> > We may want to report this to Intel, let me check internally.
> >
> > >     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.
> > >
> > >  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;
> >
> > We may also need the status or are you planning on reusing the
> > existing pkt_status? In any case we may want to add something to
> > iso-tester to confirm this is working as intended and catch possible
> > regressions.
>
> BT_PKT_STATUS + BT_SCM_PKT_STATUS are already implemented for ISO, and
> there is test for it in iso-tester.c
>
> ISO Receive Packet Status - Success

Great, I forgot we had a test already, makes sense now.

> How it works in this version is that user application that wants to get
> both does
>
>         opt = 1;
>         setsockopt(fd, SOL_BLUETOOTH, BT_PKT_STATUS, &opt, sizeof(opt));
>         opt = 1;
>         setsockopt(fd, SOL_BLUETOOTH, BT_PKT_SEQNUM, &opt, sizeof(opt));
>         ...
>         uint16_t seqnum;
>         uint8_t status;
>         for (cmsg=CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
>                 if (cmsg->cmsg_level != SOL_BLUETOOTH)
>                         continue;
>                 if (cmsg->cmsg_type == BT_SCM_PKT_SEQNUM)
>                         memcpy(&seqnum, CMSG_DATA(cmsg), sizeof(uint16_t));
>                 else if (cmsg->cmsg_type == BT_SCM_PKT_STATUS)
>                         memcpy(&status, CMSG_DATA(cmsg), sizeof(uint8_t));
>         }
>
> In theory we might indeed also change BT_SCM_PKT_STATUS to a struct
> like
>
>         struct bt_iso_pkt_status {
>                 u8 status;
>                 u16 seqnum;
>         } __packed;
>
> It's then not really fully compatible with any existing applications,
> since applications may be eg using something like
>
>         char buf[CMSG_SPACE(uint8_t)];
>
> to reserve space for the BT_PKT_STATUS CMSG, which then won't
> necessarily fit anymore. Maybe it could be changed just for ISO, but
> then different socket types would have different CMSG size for the same
> SCM.
>
> I think it's probably OK to use separate CMSG like here, then user
> application can also know if kernel supports the socket option.

Yeah, we might need to document though, I will try to put together the
initial doc/iso.rst so we can document it in the future. btw are
missing examples to how to handle timestamping handling on the likes
of doc/l2cap.rst and doc/sco.rst, it would be really great if we could
sort that out as well.

> > >         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);
> > > +               }
> >
> > In case we want to reuse the pkt_status then perhaps the order shall
> > be pk_seqnum followed by pkt_status otherwise we need a struct that
> > holds them both.
>
> The order of the CMSG shouldn't matter if they have separate BT_SCM
> types & socket flags.

You mean because they are normally processed in a loop so all options
shall be related to each other? That makes sense if they can only
appear once, which I guess is the case here.

> >
> > >         }
> > >
> > >         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;
> > >
> > >         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;
> > > --
> > > 2.50.1
> > >
> >
>
> --
> Pauli Virtanen



-- 
Luiz Augusto von Dentz

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

[Paul Menzel]

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?

> 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?

>      
>      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.

>   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.

>   	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

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

[Luiz Augusto von Dentz]

Hi Paul,

On Mon, Jul 14, 2025 at 10:15 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> 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?
>
> > 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?

Looks like btmon don't actually print it, though that probably is
something we want to add along with handling of timestamp if that is
used.

> >
> >      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.
>
> >   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.
>
> >       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



-- 
Luiz Augusto von Dentz

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

[Pauli Virtanen]

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

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

[Paul Menzel]

Dear Pauli,


Thank you for your prompt reply.


Am 14.07.25 um 16:53 schrieb Pauli Virtanen:

> ma, 2025-07-14 kello 16:15 +0200, Paul Menzel kirjoitti:

>> 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/

Awesome. Can this be referenced in the commit message?

>>> 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".

Now I see it. Thank you!

>>>       
>>>       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 that AX210 drops some reports is mentioned
> in the commit message.

True.

>>>    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.

Understood.

>>>    	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

RE: Bluetooth: ISO: add socket option to report packet seqnum via CMSG

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3327 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=982088

---Test result---

Test Summary:
CheckPatch                    PENDING   0.44 seconds
GitLint                       PENDING   0.34 seconds
SubjectPrefix                 PASS      0.07 seconds
BuildKernel                   PASS      24.06 seconds
CheckAllWarning               PASS      26.92 seconds
CheckSparse                   WARNING   29.90 seconds
BuildKernel32                 PASS      24.59 seconds
TestRunnerSetup               PASS      482.03 seconds
TestRunner_l2cap-tester       PASS      28.08 seconds
TestRunner_iso-tester         FAIL      41.16 seconds
TestRunner_bnep-tester        PASS      5.95 seconds
TestRunner_mgmt-tester        FAIL      134.62 seconds
TestRunner_rfcomm-tester      PASS      9.35 seconds
TestRunner_sco-tester         PASS      14.66 seconds
TestRunner_ioctl-tester       PASS      9.96 seconds
TestRunner_mesh-tester        FAIL      11.50 seconds
TestRunner_smp-tester         PASS      9.74 seconds
TestRunner_userchan-tester    PASS      6.10 seconds
IncrementalBuild              PENDING   0.59 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/af_bluetooth.c:248:25: warning: context imbalance in 'bt_accept_enqueue' - different lock contexts for basic blocknet/bluetooth/iso.c:2322:28: warning: incorrect type in assignment (different base types)net/bluetooth/iso.c:2322:28:    expected unsigned short [usertype] snnet/bluetooth/iso.c:2322:28:    got restricted __le16 [usertype] snnet/bluetooth/iso.c:2333:28: warning: incorrect type in assignment (different base types)net/bluetooth/iso.c:2333:28:    expected unsigned short [usertype] snnet/bluetooth/iso.c:2333:28:    got restricted __le16 [usertype] sn
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
Total: 130, Passed: 127 (97.7%), Failed: 2, Not Run: 1

Failed Test Cases
ISO Send - TX Timestamping                           Failed       0.233 seconds
ISO Send - TX CMSG Timestamping                      Failed       0.236 seconds
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 485 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
LL Privacy - Add Device 3 (AL is full)               Failed       0.228 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.061 seconds
Mesh - Send cancel - 2                               Timed out    1.997 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

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

[Pauli Virtanen]

ma, 2025-07-14 kello 07:37 -0700, bluez.test.bot@gmail.com kirjoitti:
> This is automated email and please do not reply to this email!
> 
> Dear submitter,
> 
> Thank you for submitting the patches to the linux bluetooth mailing list.
> This is a CI test results with your patch series:
> PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=982088
> 
> ---Test result---
> 
> Test Summary:
> CheckPatch                    PENDING   0.44 seconds
> GitLint                       PENDING   0.34 seconds
> SubjectPrefix                 PASS      0.07 seconds
> BuildKernel                   PASS      24.06 seconds
> CheckAllWarning               PASS      26.92 seconds
> CheckSparse                   WARNING   29.90 seconds
> BuildKernel32                 PASS      24.59 seconds
> TestRunnerSetup               PASS      482.03 seconds
> TestRunner_l2cap-tester       PASS      28.08 seconds
> TestRunner_iso-tester         FAIL      41.16 seconds
> TestRunner_bnep-tester        PASS      5.95 seconds
> TestRunner_mgmt-tester        FAIL      134.62 seconds
> TestRunner_rfcomm-tester      PASS      9.35 seconds
> TestRunner_sco-tester         PASS      14.66 seconds
> TestRunner_ioctl-tester       PASS      9.96 seconds
> TestRunner_mesh-tester        FAIL      11.50 seconds
> TestRunner_smp-tester         PASS      9.74 seconds
> TestRunner_userchan-tester    PASS      6.10 seconds
> IncrementalBuild              PENDING   0.59 seconds
> 
> Details
> ##############################
> Test: CheckPatch - PENDING
> Desc: Run checkpatch.pl script
> Output:
> 
> ##############################
> Test: GitLint - PENDING
> Desc: Run gitlint
> Output:
> 
> ##############################
> Test: CheckSparse - WARNING
> Desc: Run sparse tool with linux kernel
> Output:
> net/bluetooth/af_bluetooth.c:248:25: warning: context imbalance in 'bt_accept_enqueue' - different lock contexts for basic blocknet/bluetooth/iso.c:2322:28: warning: incorrect type in assignment (different base types)net/bluetooth/iso.c:2322:28:    expected unsigned short [usertype] snnet/bluetooth/iso.c:2322:28:    got restricted __le16 [usertype] snnet/bluetooth/iso.c:2333:28: warning: incorrect type in assignment (different base types)net/bluetooth/iso.c:2333:28:    expected unsigned short [usertype] snnet/bluetooth/iso.c:2333:28:    got restricted __le16 [usertype] sn
> ##############################
> Test: TestRunner_iso-tester - FAIL
> Desc: Run iso-tester with test-runner
> Output:
> Total: 130, Passed: 127 (97.7%), Failed: 2, Not Run: 1
> 
> Failed Test Cases
> ISO Send - TX Timestamping                           Failed       0.233 seconds
> ISO Send - TX CMSG Timestamping                      Failed       0.236 seconds

These are false positives, they fail since the test is trying to use
BT_POLL_ERRQUEUE socket option which does not exist in kernel, but has
same value as the new BT_PTK_SEQNUM.

The following patch removes the references to the non-existing kernel
feature:

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

> ##############################
> Test: TestRunner_mgmt-tester - FAIL
> Desc: Run mgmt-tester with test-runner
> Output:
> Total: 490, Passed: 485 (99.0%), Failed: 1, Not Run: 4
> 
> Failed Test Cases
> LL Privacy - Add Device 3 (AL is full)               Failed       0.228 seconds
> ##############################
> Test: TestRunner_mesh-tester - FAIL
> Desc: Run mesh-tester with test-runner
> Output:
> Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0
> 
> Failed Test Cases
> Mesh - Send cancel - 1                               Timed out    2.061 seconds
> Mesh - Send cancel - 2                               Timed out    1.997 seconds
> ##############################
> Test: IncrementalBuild - PENDING
> Desc: Incremental build with the patches in the series
> Output:
> 
> 
> 
> ---
> Regards,
> Linux Bluetooth

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

[Luiz Augusto von Dentz]

Hi Pauli,

On Mon, Jul 14, 2025 at 11:25 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> ma, 2025-07-14 kello 07:37 -0700, bluez.test.bot@gmail.com kirjoitti:
> > This is automated email and please do not reply to this email!
> >
> > Dear submitter,
> >
> > Thank you for submitting the patches to the linux bluetooth mailing list.
> > This is a CI test results with your patch series:
> > PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=982088
> >
> > ---Test result---
> >
> > Test Summary:
> > CheckPatch                    PENDING   0.44 seconds
> > GitLint                       PENDING   0.34 seconds
> > SubjectPrefix                 PASS      0.07 seconds
> > BuildKernel                   PASS      24.06 seconds
> > CheckAllWarning               PASS      26.92 seconds
> > CheckSparse                   WARNING   29.90 seconds
> > BuildKernel32                 PASS      24.59 seconds
> > TestRunnerSetup               PASS      482.03 seconds
> > TestRunner_l2cap-tester       PASS      28.08 seconds
> > TestRunner_iso-tester         FAIL      41.16 seconds
> > TestRunner_bnep-tester        PASS      5.95 seconds
> > TestRunner_mgmt-tester        FAIL      134.62 seconds
> > TestRunner_rfcomm-tester      PASS      9.35 seconds
> > TestRunner_sco-tester         PASS      14.66 seconds
> > TestRunner_ioctl-tester       PASS      9.96 seconds
> > TestRunner_mesh-tester        FAIL      11.50 seconds
> > TestRunner_smp-tester         PASS      9.74 seconds
> > TestRunner_userchan-tester    PASS      6.10 seconds
> > IncrementalBuild              PENDING   0.59 seconds
> >
> > Details
> > ##############################
> > Test: CheckPatch - PENDING
> > Desc: Run checkpatch.pl script
> > Output:
> >
> > ##############################
> > Test: GitLint - PENDING
> > Desc: Run gitlint
> > Output:
> >
> > ##############################
> > Test: CheckSparse - WARNING
> > Desc: Run sparse tool with linux kernel
> > Output:
> > net/bluetooth/af_bluetooth.c:248:25: warning: context imbalance in 'bt_accept_enqueue' - different lock contexts for basic blocknet/bluetooth/iso.c:2322:28: warning: incorrect type in assignment (different base types)net/bluetooth/iso.c:2322:28:    expected unsigned short [usertype] snnet/bluetooth/iso.c:2322:28:    got restricted __le16 [usertype] snnet/bluetooth/iso.c:2333:28: warning: incorrect type in assignment (different base types)net/bluetooth/iso.c:2333:28:    expected unsigned short [usertype] snnet/bluetooth/iso.c:2333:28:    got restricted __le16 [usertype] sn
> > ##############################
> > Test: TestRunner_iso-tester - FAIL
> > Desc: Run iso-tester with test-runner
> > Output:
> > Total: 130, Passed: 127 (97.7%), Failed: 2, Not Run: 1
> >
> > Failed Test Cases
> > ISO Send - TX Timestamping                           Failed       0.233 seconds
> > ISO Send - TX CMSG Timestamping                      Failed       0.236 seconds
>
> These are false positives, they fail since the test is trying to use
> BT_POLL_ERRQUEUE socket option which does not exist in kernel, but has
> same value as the new BT_PTK_SEQNUM.
>
> The following patch removes the references to the non-existing kernel
> feature:
>
> https://lore.kernel.org/linux-bluetooth/2ffec6539fe38318c713b48985aaddda9671f258.1752501450.git.pav@iki.fi/

Did we end up reusing the same id? In that case I would just skip it
and use the next to avoid such confusion.

> > ##############################
> > Test: TestRunner_mgmt-tester - FAIL
> > Desc: Run mgmt-tester with test-runner
> > Output:
> > Total: 490, Passed: 485 (99.0%), Failed: 1, Not Run: 4
> >
> > Failed Test Cases
> > LL Privacy - Add Device 3 (AL is full)               Failed       0.228 seconds
> > ##############################
> > Test: TestRunner_mesh-tester - FAIL
> > Desc: Run mesh-tester with test-runner
> > Output:
> > Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0
> >
> > Failed Test Cases
> > Mesh - Send cancel - 1                               Timed out    2.061 seconds
> > Mesh - Send cancel - 2                               Timed out    1.997 seconds
> > ##############################
> > Test: IncrementalBuild - PENDING
> > Desc: Incremental build with the patches in the series
> > Output:
> >
> >
> >
> > ---
> > Regards,
> > Linux Bluetooth
>


-- 
Luiz Augusto von Dentz

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

[Simon Horman]

On Mon, Jul 14, 2025 at 05:02:57PM +0300, Pauli Virtanen wrote:
> 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.
> 
> Signed-off-by: Pauli Virtanen <pav@iki.fi>

Hi Pauli,

Some minor feedback from my side.

The byte order annotations around the sequence number seem inconsistent.
And my guess is that __le16 should be consistently used to hold the sequence
number.

Sparse says:

  net/bluetooth/iso.c:2322:28: warning: incorrect type in assignment (different base types)
  net/bluetooth/iso.c:2322:28:    expected unsigned short [usertype] sn
  net/bluetooth/iso.c:2322:28:    got restricted __le16 [usertype] sn
  net/bluetooth/iso.c:2333:28: warning: incorrect type in assignment (different base types)
  net/bluetooth/iso.c:2333:28:    expected unsigned short [usertype] sn
  net/bluetooth/iso.c:2333:28:    got restricted __le16 [usertype] sn