| Message ID | 20231018145540.34014-1-marcel@ziswiler.com |
|---|---|
| Headers | show |
| Series | Bluetooth: btnxpuart: Fixes | expand |
Hi Marcel, Thank you for your patch. > From: Marcel Ziswiler <marcel@ziswiler.com> > Sent: Wednesday, October 18, 2023 8:26 PM > To: linux-bluetooth@vger.kernel.org > Cc: Sherry Sun <sherry.sun@nxp.com>; Johan Hedberg > <johan.hedberg@gmail.com>; Luiz Augusto von Dentz > <luiz.dentz@gmail.com>; Neeraj sanjay kale <neeraj.sanjaykale@nxp.com>; > linux-kernel@vger.kernel.org; Marcel Holtmann <marcel@holtmann.org>; > Marcel Ziswiler <marcel.ziswiler@toradex.com>; Amitkumar Karwar > <amitkumar.karwar@nxp.com>; Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Subject: [PATCH v1 1/2] Bluetooth: btnxpuart: Fix btnxpuart_close > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > Unfortunately, btnxpuart_close() may trigger a BUG: scheduling while atomic. > Fix this by properly purging the transmit queue and freeing the receive skb. > > Fixes: 689ca16e5232 ("Bluetooth: NXP: Add protocol support for NXP > Bluetooth chipsets") > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > --- > > drivers/bluetooth/btnxpuart.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c > index b7e66b7ac570..9cb7529eef09 100644 > --- a/drivers/bluetooth/btnxpuart.c > +++ b/drivers/bluetooth/btnxpuart.c > @@ -1234,6 +1234,9 @@ static int btnxpuart_close(struct hci_dev *hdev) > > ps_wakeup(nxpdev); > serdev_device_close(nxpdev->serdev); > + skb_queue_purge(&nxpdev->txq); > + kfree_skb(nxpdev->rx_skb); > + nxpdev->rx_skb = NULL; > clear_bit(BTNXPUART_SERDEV_OPEN, &nxpdev->tx_state); > return 0; > } This is already done in btnxpuart_flush(), which is called by hci_dev_close_sync(), before it calls btnxpuart_close(). Is btnxpuart_flush() not called during your testing? Thanks, Neeraj
Hi Neeraj On Wed, 2023-10-18 at 15:36 +0000, Neeraj sanjay kale wrote: > Hi Marcel, > > Thank you for your patch. > > > From: Marcel Ziswiler <marcel@ziswiler.com> > > Sent: Wednesday, October 18, 2023 8:26 PM > > To: linux-bluetooth@vger.kernel.org > > Cc: Sherry Sun <sherry.sun@nxp.com>; Johan Hedberg > > <johan.hedberg@gmail.com>; Luiz Augusto von Dentz > > <luiz.dentz@gmail.com>; Neeraj sanjay kale <neeraj.sanjaykale@nxp.com>; > > linux-kernel@vger.kernel.org; Marcel Holtmann <marcel@holtmann.org>; > > Marcel Ziswiler <marcel.ziswiler@toradex.com>; Amitkumar Karwar > > <amitkumar.karwar@nxp.com>; Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Subject: [PATCH v1 1/2] Bluetooth: btnxpuart: Fix btnxpuart_close > > > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > > > Unfortunately, btnxpuart_close() may trigger a BUG: scheduling while atomic. > > Fix this by properly purging the transmit queue and freeing the receive skb. > > > > Fixes: 689ca16e5232 ("Bluetooth: NXP: Add protocol support for NXP > > Bluetooth chipsets") > > > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > --- > > > > drivers/bluetooth/btnxpuart.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c > > index b7e66b7ac570..9cb7529eef09 100644 > > --- a/drivers/bluetooth/btnxpuart.c > > +++ b/drivers/bluetooth/btnxpuart.c > > @@ -1234,6 +1234,9 @@ static int btnxpuart_close(struct hci_dev *hdev) > > > > ps_wakeup(nxpdev); > > serdev_device_close(nxpdev->serdev); > > + skb_queue_purge(&nxpdev->txq); > > + kfree_skb(nxpdev->rx_skb); > > + nxpdev->rx_skb = NULL; > > clear_bit(BTNXPUART_SERDEV_OPEN, &nxpdev->tx_state); > > return 0; > > } > This is already done in btnxpuart_flush(), which is called by hci_dev_close_sync(), before it calls > btnxpuart_close(). Yes, I was also wondering about that. > Is btnxpuart_flush() not called during your testing? Yes, I even added some more tracing to confirm this. However, without my fix (which BTW was inspired by looking at the former hci_mrvl.c driver) this bug is really occuring. Just keep loading/un-loading the kernel module a few times any you may hit it. > Thanks, > Neeraj Cheers Marcel
Hi Marcel > > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > > > > > Unfortunately, btnxpuart_close() may trigger a BUG: scheduling while > atomic. > > > Fix this by properly purging the transmit queue and freeing the receive > skb. > > > > > > Fixes: 689ca16e5232 ("Bluetooth: NXP: Add protocol support for NXP > > > Bluetooth chipsets") > > > > > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > > --- > > > > > > drivers/bluetooth/btnxpuart.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/bluetooth/btnxpuart.c > > > b/drivers/bluetooth/btnxpuart.c index b7e66b7ac570..9cb7529eef09 > > > 100644 > > > --- a/drivers/bluetooth/btnxpuart.c > > > +++ b/drivers/bluetooth/btnxpuart.c > > > @@ -1234,6 +1234,9 @@ static int btnxpuart_close(struct hci_dev > > > *hdev) > > > > > > ps_wakeup(nxpdev); > > > serdev_device_close(nxpdev->serdev); > > > + skb_queue_purge(&nxpdev->txq); > > > + kfree_skb(nxpdev->rx_skb); > > > + nxpdev->rx_skb = NULL; > > > clear_bit(BTNXPUART_SERDEV_OPEN, &nxpdev->tx_state); > > > return 0; > > > } > > This is already done in btnxpuart_flush(), which is called by > > hci_dev_close_sync(), before it calls btnxpuart_close(). > > Yes, I was also wondering about that. > > > Is btnxpuart_flush() not called during your testing? > > Yes, I even added some more tracing to confirm this. However, without my > fix (which BTW was inspired by looking at the former hci_mrvl.c driver) this > bug is really occuring. Just keep loading/un-loading the kernel module a few > times any you may hit it. > Our QA team has been running load/unload tests for quite some time now, and such an issue was never reported. I am not sure why you do not see the btnxpuart_flush () been called, but I tested this patch on my setup, where both, btnxpuart_flush() and btnxpuart_close() are called, and I don’t see any issue due to kfree_skb and txq purge been done twice. This looks ok to me. Reviewed-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
Hi Marcel > > On Wed, 2023-10-18 at 15:28 +0000, Neeraj sanjay kale wrote: > > Hi Marcel, > > > > Thank you for your patch. This change looks good to me. > > > > I think the scenario you are testing/resolving is: > > 1) Load btnxpuart.ko first (which "may" load BT-only FW if chip is > > powered on) > > 2) Power cycle or power on chip > > 3) Load WLAN driver with combo FW > > Right? > > Yes, kinda, but there are really a slew of issues with this driver or the > combination of it with mwifiex_sdio: > > 1. Shared power-down pin (PD#) between Bluetooth and Wi-Fi > Issue: There is currently no concept in the Linux kernel to achieve this. Last > failed attempt [1]. > Workaround: Instead of using mmc-pwrseq tied to the Wi-Fi driver > (mwifiex_sdio) this could be hogged at boot. > However, then it may no longer easily be controlled e.g. for a (manual) > power-cycle. > A novel approach might be using a regulator which could be shared, however, > this would require us implementing regulator support in btnxpuart. Not sure > whether you would actually approve us doing so. > > 2. Bluetooth driver (btnxpuart) vs. Wi-Fi driver (mwifiex) load order > Issue: By default, the Bluetooth driver (btnxpuart) loads before the Wi-Fi > driver (mwifiex) and using regular mmc-pwrseq for mwifiex_sdio will only > power-on the module late. > Workaround: The install directive in modprobe.conf could be (ab-)used to > enforce mwifiex/mwifiex_sdio to be loaded first. However, doing so adds an > unnecessary dependency with user space and is in general discouraged. > > 3. Distinguish powered-on vs. powered-off state > Issue: Without that knowledge the driver has a hard time figuring out > whether or not it needs to load the firmware as only if it is powered-on > (and/or without firmware) will the Bluetooth part of the module send its > boot signature. So the absence of such boot signature may mean either > firmware is already loaded or module powered-off. > Workaround: The Bluetooth UART RTS pin seems to activate an internal pull- > up upon the module being powered on. > However, programmatically it is rather hard to determine this as the regular > UART driver (now driving RTS) usually gets loaded. > > 4. Determine whether or not to load the firmware > Issue: If it is without firmware (and powered-on) the boot loader will send its > boot signature. If there is no boot signature it could as well also still be > powered-off. > Workaround: Also check CTS as it goes up if the firmware is loaded. > Unfortunately, integrating this in btnxpuart is not so trivial as serdev > introduces its own asynchronous concurrency which is already used in > existing checks. > > 5. Deploy separate firmware > Issue: Does not really solve anything resp. as the power-down pin is shared > this seems kinda pointless. > > Your input on any of those topics is much appreciated. I have tested this patch on my setup, and the addition of CTS check is useful, as it may not seem to be a good idea to rely on bootloader signatures to determine if FW is running. I have also verified that this patch does not break any existing functionality (for other customers). This seem good to me. Reviewed-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com> Thanks, Neeraj > > > > From: Marcel Ziswiler <marcel@ziswiler.com> > > > Sent: Wednesday, October 18, 2023 8:26 PM > > > To: linux-bluetooth@vger.kernel.org > > > Cc: Sherry Sun <sherry.sun@nxp.com>; Johan Hedberg > > > <johan.hedberg@gmail.com>; Luiz Augusto von Dentz > > > <luiz.dentz@gmail.com>; Neeraj sanjay kale > > > <neeraj.sanjaykale@nxp.com>; linux-kernel@vger.kernel.org; Marcel > > > Holtmann <marcel@holtmann.org>; Marcel Ziswiler > > > <marcel.ziswiler@toradex.com>; Amitkumar Karwar > > > <amitkumar.karwar@nxp.com>; Ilpo Järvinen > > > <ilpo.jarvinen@linux.intel.com> > > > Subject: [PATCH v1 2/2] Bluetooth: btnxpuart: Fix nxp_setup > > > > > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > > > > > 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@toradex.com> > > > > > > --- > > > 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. > > > > > > 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); > > > } > > > -- > > > 2.36.1
Dear Marcel, Thank you for your patch. Am 18.10.23 um 16:55 schrieb Marcel Ziswiler: > From: Marcel Ziswiler <marcel.ziswiler@toradex.com> “Fix nxp_setup” is really generic. A more specific message would be great. Maybe: Wait up to 10 s for firmware. > 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@toradex.com> > > --- > 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. > 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? Kind regards, Paul
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@toradex.com> > > “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@toradex.com> > > > > --- > > 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@toradex.com > Kind regards, > > Paul Cheers Marcel
> -----Original Message----- > From: Marcel Ziswiler <marcel@ziswiler.com> > Sent: 2023年10月18日 22:56 > To: linux-bluetooth@vger.kernel.org > Cc: Sherry Sun <sherry.sun@nxp.com>; Johan Hedberg > <johan.hedberg@gmail.com>; Luiz Augusto von Dentz > <luiz.dentz@gmail.com>; Neeraj sanjay kale <neeraj.sanjaykale@nxp.com>; > linux-kernel@vger.kernel.org; Marcel Holtmann <marcel@holtmann.org>; > Marcel Ziswiler <marcel.ziswiler@toradex.com>; Amitkumar Karwar > <amitkumar.karwar@nxp.com>; Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Subject: [PATCH v1 2/2] Bluetooth: btnxpuart: Fix nxp_setup > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > 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@toradex.com> > > --- > 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. > > 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). Hi Marcel, Please remove the description of "over SDIO (mwifiex_sdio)" here, because the Wi-Fi driver corresponding to the NXP BT chip not only supports the SDIO interface, but also supports the PCIe bus. That is to say, combo firmware can be loaded via the SDIO or PCIe bus. Best Regards Sherry > + * 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); > } > -- > 2.36.1
From: Marcel Ziswiler <marcel.ziswiler@toradex.com> This series fixes a few issues we have seen during bring-up of Bluetooth on the Verdin AM62 which uses a NXP IW416 based u-blox MAYA-W1. Marcel Ziswiler (2): Bluetooth: btnxpuart: Fix btnxpuart_close Bluetooth: btnxpuart: Fix nxp_setup drivers/bluetooth/btnxpuart.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)