Re: [PATCH v1 2/2] Bluetooth: btnxpuart: Fix nxp_setup

[Marcel Ziswiler]

Hi Paul

On Thu, 2023-10-19 at 18:44 +0200, Paul Menzel wrote:
> Dear Marcel,
> 
> 
> Thank you for your patch.

Thanks for your feedback.

> Am 18.10.23 um 16:55 schrieb Marcel Ziswiler:
> > From: Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx>
> 
> “Fix nxp_setup” is really generic. A more specific message would be 
> great. Maybe: Wait up to 10 s for firmware.

Okay, I can do that.

I admit, I had a hard time finding a real good description as the whole thing is rather completely flawed.

> > Unfortunately, nxp_setup() may inadvertently assume that the
> > firmware is already running while the module is not even powered yet.
> > Fix this by waiting up to 10 seconds for the CTS to go up as the combo
> > firmware might be loaded by the Wi-Fi driver over SDIO (mwifiex_sdio).
> > 
> > Fixes: 689ca16e5232 ("Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets")
> > 
> > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx>
> > 
> > ---
> > This is what may happen without this fix:
> > [  284.588177] Bluetooth: hci0: Opcode 0x0c03 failed: -110
> > [  286.636167] Bluetooth: hci0: Setting wake-up method failed (-110)
> > Unfortunately, even re-loading the btnxpuart kernel module would not
> > recover from this condition.
> 
> I’d add that information to the commit message.

Okay, will do.

> >   drivers/bluetooth/btnxpuart.c | 10 ++++++++++
> >   1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> > index 9cb7529eef09..4b83a0aa3459 100644
> > --- a/drivers/bluetooth/btnxpuart.c
> > +++ b/drivers/bluetooth/btnxpuart.c
> > @@ -1021,6 +1021,16 @@ static int nxp_setup(struct hci_dev *hdev)
> >   		if (err < 0)
> >   			return err;
> >   	} else {
> > +		/* The combo firmware might be loaded by the Wi-Fi driver over SDIO (mwifiex_sdio).
> > +		 * We wait up to 10s for the CTS to go up. Afterwards, we know that the firmware is
> > +		 * really ready.
> > +		 */
> > +		err = serdev_device_wait_for_cts(nxpdev->serdev, true, 10000);
> > +		if (err) {
> > +			bt_dev_err(nxpdev->hdev, "Wait for CTS failed with %d", err);
> > +			return err;
> > +		}
> > +
> >   		bt_dev_dbg(hdev, "FW already running.");
> >   		clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
> >   	}
> 
> Isn’t there another way to find out, if the firmware is there? Adding an 
> arbitrary timeout of ten seconds sounds like a fundamental flaw in the 
> driver?

Yes, exactly. You might have read my previous message [1]. The whole driver/firmware, well the entire thing is
basically rather flawed. But then I am just trying to incrementally make it at least half-way usable for our
use case as well.

[1] https://lore.kernel.org/all/dca8bc7fec5f527cac2e280cd8ed4edae1f473ea.camel@xxxxxxxxxxx

> Kind regards,
> 
> Paul

Cheers

Marcel