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

[Calvin Owens <calvin@wbinvd.org>]

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

> Any thoughts from anybody else? I should have time to spin something up
> tomorrow, if nobody beats me to it.
> 
> Thanks,
> Calvin
> 
> > Thank you
> > 
> > Regards,
> > Francesco Valla
> > 
> > [1] https://lore.kernel.org/linux-bluetooth/be8edf7f8ba8dea6c61272b02fb20a4ac7e1c5a5.1756179634.git.calvin@wbinvd.org/
> > 
> > 
> > 
> >  
> > 
> > 
> >