Hello Dominique,
>
> Hi,
>
> Neeraj Sanjay Kale wrote on Wed, May 15, 2024 at 12:36:55PM +0530:
> > @@ -1269,8 +1271,10 @@ static int btnxpuart_flush(struct hci_dev
> > *hdev)
> >
> > cancel_work_sync(&nxpdev->tx_work);
> >
> > - kfree_skb(nxpdev->rx_skb);
> > - nxpdev->rx_skb = NULL;
> > + if (!IS_ERR_OR_NULL(nxpdev->rx_skb)) {
> > + kfree_skb(nxpdev->rx_skb);
> > + nxpdev->rx_skb = NULL;
> > + }
>
> This is an old patch but I hit that on our slightly old tree and was wondering
> about this patch: why does flush() have to free rx at all?
>
> I think this either needs a lock or (preferably) just remove this free:
> - This is inherently racy with btnxpuart_receive_buf() which is run in another
> workqueue with no lock involved as far as I understand, so this is not just
> about errors but you could also free something in a weird place here.
> As far as I understand, even if we don't do anything, the rx path will free the
> reply if no matching request was found.
> - looking at other drivers, the hdev->flush() call never does anything about rx
> and seems to only be about rx (ah, checking again as of master
> drivers/bluetooth/btmtkuart.c seems to have this same problem as of before
> this patch e.g. they're not checking for errors either... This probably needs
> something akin to this patch or removal as well. All other drivers have flush
> seem to be mostly about tx, but I do see some cancel work for rx as well so
> I'm a bit unclear as to what is expected of flush())
Well this comes from an example driver which Marcel had proposed for reference to create a standalone BT driver based on serdev.
https://lore.kernel.org/lkml/2991d8c04ce54f49aa292e9ac1968b7cb3b6a383.1530004712.git.sean.wang@xxxxxxxxxxxx/
When NXP (and probably Mediatek too) patches for hci_uart were rejected, standalone BTUART drivers were implemented based on this reference driver.
Later Francesco Dolcini added the rx_skb free logic in btnxpuart_close. To avoid breaking any functionality, we let rx_skb free logic to be kept in both places, close() and flush()
https://lore.kernel.org/all/20240304181421.14777-1-francesco@xxxxxxxxxx/T/
Based on the description you shared, I think it is safe to remove kfree_skb(rx_skb) from the flush() functions and keep it only in close() functions.
Thanks,
Neeraj
