* [PATCH] Bluetooth: ISO: add socket option to report packet seqnum via CMSG
@ 2025-07-14 14:02 Pauli Virtanen
2025-07-14 14:15 ` Luiz Augusto von Dentz
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Pauli Virtanen @ 2025-07-14 14:02 UTC (permalink / raw)
To: linux-bluetooth
Cc: Pauli Virtanen, marcel, johan.hedberg, luiz.dentz, davem,
edumazet, kuba, pabeni, horms, netdev, linux-kernel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Bluetooth: ISO: add socket option to report packet seqnum via CMSG
2025-07-14 14:02 [PATCH] Bluetooth: ISO: add socket option to report packet seqnum via CMSG Pauli Virtanen
@ 2025-07-14 14:15 ` Luiz Augusto von Dentz
2025-07-14 14:45 ` Pauli Virtanen
2025-07-14 14:15 ` Paul Menzel
2025-07-14 15:46 ` Simon Horman
2 siblings, 1 reply; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2025-07-14 14:15 UTC (permalink / raw)
To: Pauli Virtanen
Cc: linux-bluetooth, marcel, johan.hedberg, davem, edumazet, kuba,
pabeni, horms, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Bluetooth: ISO: add socket option to report packet seqnum via CMSG
2025-07-14 14:02 [PATCH] Bluetooth: ISO: add socket option to report packet seqnum via CMSG Pauli Virtanen
2025-07-14 14:15 ` Luiz Augusto von Dentz
@ 2025-07-14 14:15 ` Paul Menzel
2025-07-14 14:20 ` Luiz Augusto von Dentz
2025-07-14 14:53 ` Pauli Virtanen
2025-07-14 15:46 ` Simon Horman
2 siblings, 2 replies; 9+ messages in thread
From: Paul Menzel @ 2025-07-14 14:15 UTC (permalink / raw)
To: Pauli Virtanen
Cc: linux-bluetooth, marcel, johan.hedberg, luiz.dentz, davem,
edumazet, kuba, pabeni, horms, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Bluetooth: ISO: add socket option to report packet seqnum via CMSG
2025-07-14 14:15 ` Paul Menzel
@ 2025-07-14 14:20 ` Luiz Augusto von Dentz
2025-07-14 14:53 ` Pauli Virtanen
1 sibling, 0 replies; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2025-07-14 14:20 UTC (permalink / raw)
To: Paul Menzel
Cc: Pauli Virtanen, linux-bluetooth, marcel, johan.hedberg, davem,
edumazet, kuba, pabeni, horms, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Bluetooth: ISO: add socket option to report packet seqnum via CMSG
2025-07-14 14:15 ` Luiz Augusto von Dentz
@ 2025-07-14 14:45 ` Pauli Virtanen
2025-07-14 14:55 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 9+ messages in thread
From: Pauli Virtanen @ 2025-07-14 14:45 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: linux-bluetooth, marcel, johan.hedberg, davem, edumazet, kuba,
pabeni, horms, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Bluetooth: ISO: add socket option to report packet seqnum via CMSG
2025-07-14 14:15 ` Paul Menzel
2025-07-14 14:20 ` Luiz Augusto von Dentz
@ 2025-07-14 14:53 ` Pauli Virtanen
2025-07-14 14:58 ` Paul Menzel
1 sibling, 1 reply; 9+ messages in thread
From: Pauli Virtanen @ 2025-07-14 14:53 UTC (permalink / raw)
To: Paul Menzel
Cc: linux-bluetooth, marcel, johan.hedberg, luiz.dentz, davem,
edumazet, kuba, pabeni, horms, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Bluetooth: ISO: add socket option to report packet seqnum via CMSG
2025-07-14 14:45 ` Pauli Virtanen
@ 2025-07-14 14:55 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 9+ messages in thread
From: Luiz Augusto von Dentz @ 2025-07-14 14:55 UTC (permalink / raw)
To: Pauli Virtanen
Cc: linux-bluetooth, marcel, johan.hedberg, davem, edumazet, kuba,
pabeni, horms, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Bluetooth: ISO: add socket option to report packet seqnum via CMSG
2025-07-14 14:53 ` Pauli Virtanen
@ 2025-07-14 14:58 ` Paul Menzel
0 siblings, 0 replies; 9+ messages in thread
From: Paul Menzel @ 2025-07-14 14:58 UTC (permalink / raw)
To: Pauli Virtanen
Cc: linux-bluetooth, marcel, johan.hedberg, luiz.dentz, davem,
edumazet, kuba, pabeni, horms, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Bluetooth: ISO: add socket option to report packet seqnum via CMSG
2025-07-14 14:02 [PATCH] Bluetooth: ISO: add socket option to report packet seqnum via CMSG Pauli Virtanen
2025-07-14 14:15 ` Luiz Augusto von Dentz
2025-07-14 14:15 ` Paul Menzel
@ 2025-07-14 15:46 ` Simon Horman
2 siblings, 0 replies; 9+ messages in thread
From: Simon Horman @ 2025-07-14 15:46 UTC (permalink / raw)
To: Pauli Virtanen
Cc: linux-bluetooth, marcel, johan.hedberg, luiz.dentz, davem,
edumazet, kuba, pabeni, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-07-14 15:46 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-14 14:02 [PATCH] Bluetooth: ISO: add socket option to report packet seqnum via CMSG Pauli Virtanen
2025-07-14 14:15 ` Luiz Augusto von Dentz
2025-07-14 14:45 ` Pauli Virtanen
2025-07-14 14:55 ` Luiz Augusto von Dentz
2025-07-14 14:15 ` Paul Menzel
2025-07-14 14:20 ` Luiz Augusto von Dentz
2025-07-14 14:53 ` Pauli Virtanen
2025-07-14 14:58 ` Paul Menzel
2025-07-14 15:46 ` Simon Horman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).