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