Re: [BUG] Erratic behavior in btnxpuart on v6.18-rc2 - and a possible solution

[Francesco Valla <francesco@valla.it>]

On Tuesday, 21 October 2025 at 23:29:59 Calvin Owens <calvin@wbinvd.org> wrote:
> On Tuesday 10/21 at 14:20 -0700, Calvin Owens wrote:
> > On Tuesday 10/21 at 22:53 +0200, Francesco Valla wrote:
> > > Hello,
> > > 
> > > while testing Bluetooth on my NXP i.MX93 FRDM, which is equipped with an IW612
> > > Bluetooth chipset from NXP, I encountered an erratic bug during initialization.
> > > 
> > > While the firmware download always completed without errors, subsequent HCI
> > > communication would fail most of the time with:
> > > 
> > >     Frame reassembly failed (-84)
> > > 
> > > After some debug, I found the culprit to be this patch that was integrated as
> > > part of the current (v6.18) cycle:
> > > 
> > >     93f06f8f0daf Bluetooth: remove duplicate h4_recv_buf() in header [1]
> > > 
> > > The reason is simple: the h4_recv_buf() function from hci_h4.c, which is now
> > > used instead the "duplicated" one in the (now removed) h4_recv_buf.h, assumes
> > > that the private drvdata for the input struct hci_dev is a pointer to a
> > > struct hci_uart, but that's not the case for the btnxpuart driver. In this
> > > case, the information about padding and alignment are pretty random and
> > > depend on the content of the data that was incorrectly casted as a
> > > struct hci_uart.
> > > 
> > > The bug should impact also the other platforms that were touched by the
> > > same patch. 
> > 
> > Hi Francesco,
> > 
> > Thanks for investigating, this makes sense to me.
> > 
> > Funny enough, I specifically tested this on btnxpuart and saw no
> > problems. I suppose some kconfig difference or some other innocuous
> > patch moved structure fields around such that it triggered for you?
> > Not that it really matters...
> > 
> > > For the time being, I'd then propose to revert the commit.
> > 
> > Adding back all the duplicate code is not the right way forward, IMHO.
> > There must be some way to "mask" the problematic behavior for the
> > drivers which stash the different structure in drvdata, right?
> 
> Actually, the right approach is probably to tweak these drivers to do
> what the Intel driver does:
> 
> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/bluetooth/hci_intel.c#n869
> 
>     static int intel_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
>     {
>             struct hci_uart *hu = hci_get_drvdata(hdev);
>             struct intel_data *intel = hu->priv;
> 
> I'll spin that up unless I hear better from anyone else :)
>

Hi, thanks for the quick response!

That was my first thought, but the Intel driver actually _uses_ the hci_uart
structure, while btnxpuart and such would only piggy-back on it to be able to
use h4_recv_buf() (and struct hci_uart is huge!).

One possible solution would be to define an "inner" __h4_recv_buf() function
that accepts alignment and padding as arguments, and use that directly on
drivers that don't use struct hci_uart (PoC attached - I don't like the
__h4_recv_buf name but I don't really know how it should be called).

Regards,
Francesco

---

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index d5153fed0518..02511ef1a841 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -1756,8 +1756,9 @@ static size_t btnxpuart_receive_buf(struct serdev_device *serdev,
 
        ps_start_timer(nxpdev);
 
-       nxpdev->rx_skb = h4_recv_buf(nxpdev->hdev, nxpdev->rx_skb, data, count,
-                                    nxp_recv_pkts, ARRAY_SIZE(nxp_recv_pkts));
+       nxpdev->rx_skb = __h4_recv_buf(nxpdev->hdev, nxpdev->rx_skb, data, count,
+                                      nxp_recv_pkts, ARRAY_SIZE(nxp_recv_pkts),
+                                      0, NULL);
        if (IS_ERR(nxpdev->rx_skb)) {
                int err = PTR_ERR(nxpdev->rx_skb);
                /* Safe to ignore out-of-sync bootloader signatures */
diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
index 9070e31a68bf..c83c266ba506 100644
--- a/drivers/bluetooth/hci_h4.c
+++ b/drivers/bluetooth/hci_h4.c
@@ -151,27 +151,32 @@ int __exit h4_deinit(void)
        return hci_uart_unregister_proto(&h4p);
 }
 
-struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
-                           const unsigned char *buffer, int count,
-                           const struct h4_recv_pkt *pkts, int pkts_count)
+struct sk_buff *__h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
+                             const unsigned char *buffer, int count,
+                             const struct h4_recv_pkt *pkts, int pkts_count,
+                             u8 alignment, u8 *padding)
 {
-       struct hci_uart *hu = hci_get_drvdata(hdev);
-       u8 alignment = hu->alignment ? hu->alignment : 1;
-
        /* Check for error from previous call */
        if (IS_ERR(skb))
                skb = NULL;
 
+       if (alignment == 0)
+               alignment = 1;
+
+       WARN_ON_ONCE(alignment > 1 && !padding);
+
        while (count) {
                int i, len;
 
                /* remove padding bytes from buffer */
-               for (; hu->padding && count > 0; hu->padding--) {
-                       count--;
-                       buffer++;
+               if (padding) {
+                       for (; *padding && count > 0; *padding = *padding - 1) {
+                               count--;
+                               buffer++;
+                       }
+                       if (!count)
+                               break;
                }
-               if (!count)
-                       break;
 
                if (!skb) {
                        for (i = 0; i < pkts_count; i++) {
@@ -252,16 +257,20 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
                        }
 
                        if (!dlen) {
-                               hu->padding = (skb->len + 1) % alignment;
-                               hu->padding = (alignment - hu->padding) % alignment;
+                               if (padding) {
+                                       *padding = (skb->len + 1) % alignment;
+                                       *padding = (alignment - *padding) % alignment;
+                               }
 
                                /* No more data, complete frame */
                                (&pkts[i])->recv(hdev, skb);
                                skb = NULL;
                        }
                } else {
-                       hu->padding = (skb->len + 1) % alignment;
-                       hu->padding = (alignment - hu->padding) % alignment;
+                       if (padding) {
+                               *padding = (skb->len + 1) % alignment;
+                               *padding = (alignment - *padding) % alignment;
+                       }
 
                        /* Complete frame */
                        (&pkts[i])->recv(hdev, skb);
@@ -271,4 +280,16 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
 
        return skb;
 }
+EXPORT_SYMBOL_GPL(__h4_recv_buf);
+
+struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
+                           const unsigned char *buffer, int count,
+                           const struct h4_recv_pkt *pkts, int pkts_count)
+{
+       struct hci_uart *hu = hci_get_drvdata(hdev);
+       u8 alignment = hu->alignment ? hu->alignment : 1;
+
+       return __h4_recv_buf(hdev, skb, buffer, count, pkts, pkts_count,
+                            alignment, &hu->padding);
+}
 EXPORT_SYMBOL_GPL(h4_recv_buf);
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index cbbe79b241ce..0b61ee953fa4 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -162,6 +162,11 @@ struct h4_recv_pkt {
 int h4_init(void);
 int h4_deinit(void);
 
+struct sk_buff *__h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
+                             const unsigned char *buffer, int count,
+                             const struct h4_recv_pkt *pkts, int pkts_count,
+                             u8 alignment, u8 *padding);
+
 struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
                            const unsigned char *buffer, int count,
                            const struct h4_recv_pkt *pkts, int pkts_count);