[PATCH v1 0/2] Bluetooth: btnxpuart: Fixes

[PATCH v1 0/2] Bluetooth: btnxpuart: Fixes

[Marcel Ziswiler]

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(+)

-- 
2.36.1

[PATCH v1 1/2] Bluetooth: btnxpuart: Fix btnxpuart_close

[Marcel Ziswiler]

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>
---
This is the kernel trace this commit fixes:
[   29.270685] BUG: scheduling while atomic: kworker/u3:0/55/0x00000002
[   29.277062] Modules linked in: snd_soc_simple_card snd_soc_simple_card_utils crct10dif_ce btnxpuart usb_f_ncm u_ether rtc_ti_k3 k3_j72xx_bandgap mwifiex_sdio mwifiex snd_soc_davinci_mcasp snd_soc_ti_udma sa2ul snd_soc_ti_edma snd_soc_ti_sdma authenc ina2xx snd_soc_nau8822 tps65219_pwrbutton at24 ti_ads1015 industrialio_triggered_buffer kfifo_buf lm75 rtc_ds1307 spi_omap2_mcspi 8021q garp mrp stp llc cfg80211 bluetooth ecdh_generic ecc rfkill libcomposite fuse drm backlight ipv6
[   29.319532] CPU: 0 PID: 55 Comm: kworker/u3:0 Not tainted 6.6.0-rc5-next-20231010-dirty #35
[   29.327883] Hardware name: Toradex Verdin AM62 WB on Verdin Development Board (DT)
[   29.335450] Workqueue: hci0 hci_power_off [bluetooth]
[   29.340775] Call trace:
[   29.343220]  dump_backtrace+0x98/0x118
[   29.346984]  show_stack+0x18/0x24
[   29.350303]  dump_stack_lvl+0x48/0x60
[   29.353971]  dump_stack+0x18/0x24
[   29.357287]  __schedule_bug+0x50/0x68
[   29.360953]  __schedule+0x7f0/0xa94
[   29.364446]  schedule+0x34/0xc8
[   29.367588]  rpm_resume+0x170/0x6c8
[   29.371083]  __pm_runtime_resume+0x4c/0x90
[   29.375182]  omap8250_set_mctrl+0x2c/0xbc
[   29.379198]  serial8250_set_mctrl+0x20/0x40
[   29.383387]  uart_update_mctrl+0x58/0x78
[   29.387311]  uart_dtr_rts+0x104/0x114
[   29.390975]  tty_port_shutdown+0xd4/0xdc
[   29.394903]  tty_port_close+0x40/0xbc
[   29.398567]  uart_close+0x34/0x9c
[   29.401882]  ttyport_close+0x50/0x94
[   29.405460]  serdev_device_close+0x40/0x50
[   29.409557]  btnxpuart_close+0x48/0xbc [btnxpuart]
[   29.414364]  hci_dev_close_sync+0x2ec/0x754 [bluetooth]
[   29.419747]  hci_dev_do_close+0x2c/0x70 [bluetooth]
[   29.424772]  hci_power_off+0x20/0x64 [bluetooth]
[   29.429536]  process_one_work+0x138/0x244
[   29.433551]  worker_thread+0x320/0x438
[   29.437302]  kthread+0x114/0x118
[   29.440533]  ret_from_fork+0x10/0x20
[   29.445838] BUG: scheduling while atomic: kworker/u3:0/55/0x00000000
[   29.453556] Modules linked in: snd_soc_simple_card snd_soc_simple_card_utils crct10dif_ce btnxpuart usb_f_ncm u_ether rtc_ti_k3 k3_j72xx_bandgap mwifiex_sdio mwifiex snd_soc_davinci_mcasp snd_soc_ti_udma sa2ul snd_soc_ti_edma snd_soc_ti_sdma authenc ina2xx snd_soc_nau8822 tps65219_pwrbutton at24 ti_ads1015 industrialio_triggered_buffer kfifo_buf lm75 rtc_ds1307 spi_omap2_mcspi 8021q garp mrp stp llc cfg80211 bluetooth ecdh_generic ecc rfkill libcomposite fuse drm backlight ipv6
[   29.496085] CPU: 0 PID: 55 Comm: kworker/u3:0 Tainted: G        W          6.6.0-rc5-next-20231010-dirty #35
[   29.505912] Hardware name: Toradex Verdin AM62 WB on Verdin Development Board (DT)
[   29.513478] Workqueue: hci0 hci_power_off [bluetooth]
[   29.518787] Call trace:
[   29.521232]  dump_backtrace+0x98/0x118
[   29.524993]  show_stack+0x18/0x24
[   29.528312]  dump_stack_lvl+0x48/0x60
[   29.531980]  dump_stack+0x18/0x24
[   29.535296]  __schedule_bug+0x50/0x68
[   29.538961]  __schedule+0x7f0/0xa94
[   29.542452]  schedule+0x34/0xc8
[   29.545593]  rpm_resume+0x170/0x6c8
[   29.549087]  __pm_runtime_resume+0x4c/0x90
[   29.553186]  omap_8250_pm+0x28/0x110
[   29.556766]  serial8250_pm+0x18/0x3c
[   29.560347]  uart_tty_port_shutdown+0xa4/0x114
[   29.564792]  tty_port_shutdown+0x84/0xdc
[   29.568718]  tty_port_close+0x40/0xbc
[   29.572382]  uart_close+0x34/0x9c
[   29.575697]  ttyport_close+0x50/0x94
[   29.579275]  serdev_device_close+0x40/0x50
[   29.583373]  btnxpuart_close+0x48/0xbc [btnxpuart]
[   29.588179]  hci_dev_close_sync+0x2ec/0x754 [bluetooth]
[   29.593562]  hci_dev_do_close+0x2c/0x70 [bluetooth]
[   29.598587]  hci_power_off+0x20/0x64 [bluetooth]
[   29.603353]  process_one_work+0x138/0x244
[   29.607369]  worker_thread+0x320/0x438
[   29.611119]  kthread+0x114/0x118
[   29.614352]  ret_from_fork+0x10/0x20

 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;
 }
-- 
2.36.1

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

[Marcel Ziswiler]

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

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

[Neeraj sanjay kale]

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?

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

RE: [PATCH v1 1/2] Bluetooth: btnxpuart: Fix btnxpuart_close

[Neeraj sanjay kale]

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

RE: Bluetooth: btnxpuart: Fixes

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3531 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=794372

---Test result---

Test Summary:
CheckPatch                    PASS      1.22 seconds
GitLint                       FAIL      0.87 seconds
SubjectPrefix                 PASS      0.12 seconds
BuildKernel                   PASS      40.70 seconds
CheckAllWarning               PASS      44.32 seconds
CheckSparse                   PASS      50.43 seconds
CheckSmatch                   PASS      136.47 seconds
BuildKernel32                 PASS      39.48 seconds
TestRunnerSetup               PASS      605.01 seconds
TestRunner_l2cap-tester       PASS      36.73 seconds
TestRunner_iso-tester         PASS      82.05 seconds
TestRunner_bnep-tester        PASS      13.00 seconds
TestRunner_mgmt-tester        PASS      253.02 seconds
TestRunner_rfcomm-tester      PASS      19.14 seconds
TestRunner_sco-tester         PASS      21.88 seconds
TestRunner_ioctl-tester       PASS      21.50 seconds
TestRunner_mesh-tester        PASS      17.43 seconds
TestRunner_smp-tester         PASS      16.74 seconds
TestRunner_userchan-tester    PASS      13.37 seconds
IncrementalBuild              PASS      43.08 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v1,1/2] Bluetooth: btnxpuart: Fix btnxpuart_close

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
14: B1 Line exceeds max length (485>80): "[   29.277062] Modules linked in: snd_soc_simple_card snd_soc_simple_card_utils crct10dif_ce btnxpuart usb_f_ncm u_ether rtc_ti_k3 k3_j72xx_bandgap mwifiex_sdio mwifiex snd_soc_davinci_mcasp snd_soc_ti_udma sa2ul snd_soc_ti_edma snd_soc_ti_sdma authenc ina2xx snd_soc_nau8822 tps65219_pwrbutton at24 ti_ads1015 industrialio_triggered_buffer kfifo_buf lm75 rtc_ds1307 spi_omap2_mcspi 8021q garp mrp stp llc cfg80211 bluetooth ecdh_generic ecc rfkill libcomposite fuse drm backlight ipv6"
15: B1 Line exceeds max length (94>80): "[   29.319532] CPU: 0 PID: 55 Comm: kworker/u3:0 Not tainted 6.6.0-rc5-next-20231010-dirty #35"
16: B1 Line exceeds max length (85>80): "[   29.327883] Hardware name: Toradex Verdin AM62 WB on Verdin Development Board (DT)"
46: B1 Line exceeds max length (485>80): "[   29.453556] Modules linked in: snd_soc_simple_card snd_soc_simple_card_utils crct10dif_ce btnxpuart usb_f_ncm u_ether rtc_ti_k3 k3_j72xx_bandgap mwifiex_sdio mwifiex snd_soc_davinci_mcasp snd_soc_ti_udma sa2ul snd_soc_ti_edma snd_soc_ti_sdma authenc ina2xx snd_soc_nau8822 tps65219_pwrbutton at24 ti_ads1015 industrialio_triggered_buffer kfifo_buf lm75 rtc_ds1307 spi_omap2_mcspi 8021q garp mrp stp llc cfg80211 bluetooth ecdh_generic ecc rfkill libcomposite fuse drm backlight ipv6"
47: B1 Line exceeds max length (111>80): "[   29.496085] CPU: 0 PID: 55 Comm: kworker/u3:0 Tainted: G        W          6.6.0-rc5-next-20231010-dirty #35"
48: B1 Line exceeds max length (85>80): "[   29.505912] Hardware name: Toradex Verdin AM62 WB on Verdin Development Board (DT)"


---
Regards,
Linux Bluetooth

Re: [PATCH v1 1/2] Bluetooth: btnxpuart: Fix btnxpuart_close

[Marcel Ziswiler]

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

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

[Marcel Ziswiler]

Hi Neeraj

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.

Thanks!

> Thanks,
> Neeraj

Cheers

Marcel

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

Re: [PATCH v1 1/2] Bluetooth: btnxpuart: Fix btnxpuart_close

[Neeraj sanjay kale]

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>

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

[Neeraj sanjay kale]

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

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

[Paul Menzel]

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

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@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

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

[Sherry Sun]

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

Re: [PATCH v1 1/2] Bluetooth: btnxpuart: Fix btnxpuart_close

[Francesco Dolcini]

Hello all,

On Wed, Oct 18, 2023 at 04:55:39PM +0200, Marcel Ziswiler wrote:
> 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>
> ---
> This is the kernel trace this commit fixes:
> [   29.270685] BUG: scheduling while atomic: kworker/u3:0/55/0x00000002

I just hit this bug with 6.7-rc2, I think it would be worth to
apply this fix.


[   70.443965] BUG: scheduling while atomic: kworker/u5:0/65/0x00000002
[   70.450649] Modules linked in: 8021q garp mrp stp llc usb_f_ncm u_ether spidev mwifiex_sdio mwifiex snd_soc_simple_card snd_soc_simple_card_utils crct10dif_ce cfg80211 k3_j72xx_bandgap rti_wdt rtc_ti_k3 btnxpuart bluetooth ecdh_generic ecc snd_soc_davinci_mcasp sa2ul sha256_generic rfkill snd_soc_ti_udma libsha256 snd_soc_ti_edma authenc tidss snd_soc_ti_sdma omap_mailbox drm_dma_helper ti_ads1015 industrialio_triggered_buffer lontium_lt8912b snd_soc_wm8904 kfifo_buf lm75 at24 tps65219_pwrbutton ina2xx m_can_platform rtc_ds1307 tc358768 m_can display_connector spi_omap2_mcspi pwm_tiehrpwm can_dev drm_kms_helper libcomposite fuse drm backlight ipv6
[   70.509384] CPU: 0 PID: 65 Comm: kworker/u5:0 Not tainted 6.7.0-rc2-00147-gf1a09972a45a #1
[   70.517747] Hardware name: Toradex Verdin AM62 WB on Dahlia Board (DT)
[   70.524334] Workqueue: hci0 hci_power_on [bluetooth]
[   70.529870] Call trace:
[   70.532349]  dump_backtrace+0x94/0xec
[   70.536103]  show_stack+0x18/0x24
[   70.539491]  dump_stack_lvl+0x48/0x60
[   70.543210]  dump_stack+0x18/0x24
[   70.546575]  __schedule_bug+0x50/0x68
[   70.550312]  __schedule+0x7c4/0xa64
[   70.553874]  schedule+0x34/0xa0
[   70.557083]  schedule_timeout+0x180/0x25c
[   70.561151]  wait_for_completion_timeout+0x80/0x15c
[   70.566101]  ti_sci_set_device_state+0x164/0x22c
[   70.570790]  ti_sci_cmd_get_device_exclusive+0x18/0x24
[   70.575991]  ti_sci_pd_power_on+0x28/0x48
[   70.580073]  _genpd_power_on+0x94/0x154
[   70.583964]  genpd_power_on.part.0+0xa4/0x174
[   70.588383]  genpd_runtime_resume+0x118/0x294
[   70.592800]  __rpm_callback+0x48/0x140
[   70.596616]  rpm_callback+0x6c/0x78
[   70.600165]  rpm_resume+0x3bc/0x59c
[   70.603714]  __pm_runtime_resume+0x4c/0x90
[   70.607870]  omap8250_set_mctrl+0x2c/0xc0
[   70.611954]  serial8250_set_mctrl.part.0+0x18/0x34
[   70.616801]  serial8250_set_mctrl+0x18/0x28
[   70.621038]  uart_update_mctrl+0x58/0x78
[   70.625024]  uart_dtr_rts+0x108/0x118
[   70.628750]  tty_port_shutdown+0x88/0xd8
[   70.632744]  tty_port_close+0x50/0xac
[   70.636472]  uart_close+0x28/0x80
[   70.639850]  ttyport_close+0x50/0x94
[   70.643500]  serdev_device_close+0x40/0x50
[   70.647664]  btnxpuart_close+0x24/0x84 [btnxpuart]
[   70.652553]  hci_dev_open_sync+0x3f8/0x9dc [bluetooth]
[   70.658144]  hci_dev_do_open+0x28/0x48 [bluetooth]
[   70.663377]  hci_power_on+0x4c/0x2ac [bluetooth]
[   70.668441]  process_scheduled_works+0x16c/0x28c
[   70.673135]  worker_thread+0x16c/0x2e0
[   70.676950]  kthread+0x11c/0x128
[   70.680247]  ret_from_fork+0x10/0x20


 

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

[Neeraj Sanjay Kale]

Hi Marcel,

> Hi Neeraj
> 
> 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 sent a new patch with subject: "[RFC PATCH] Bluetooth: btnxpuart: Fix nxp_setup in case chip is powered on late"

I would very much appreciate if you could have a look at the patch and let me know if it solves the above issues you mentioned in a neat way.

As per the new change, if no bootloader signatures are read, and CTS is high, the setup function returns an error, preventing HCI intializations.
After every 5 seconds, we attempt a power-on, which results in calling the setup function again to check for bootloader signatures and CTS line.
This "polling" sequence will continue as long as the module stays inserted, or until the btnxpuart driver downloads BT-only FW or the mwifiex downloads the combo FW (or both modules download BT-only/Wi-Fi only respective FWs).

This handling is supposed to come into picture only when mwifiex pwrseq controls the shared PDn pin of the chip, while the normal setup sequence should work as it did earlier for others.

Thanks,
Neeraj

Re: [PATCH v1 1/2] Bluetooth: btnxpuart: Fix btnxpuart_close

[Francesco Dolcini]

Hi Luiz,

On Fri, Nov 24, 2023 at 09:26:11PM +0100, Francesco Dolcini wrote:
> On Wed, Oct 18, 2023 at 04:55:39PM +0200, Marcel Ziswiler wrote:
> > 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>
> > ---
> > This is the kernel trace this commit fixes:
> > [   29.270685] BUG: scheduling while atomic: kworker/u3:0/55/0x00000002
> 
> I just hit this bug with 6.7-rc2, I think it would be worth to
> apply this fix.

Do you need any change for having this patch (1/2) applied? Do you want this
to be re-sent without the second patch (2/2) from this series that is
maybe more controversial?

Let me know how I can help,

Thanks,
Francesco

Re: [PATCH v1 1/2] Bluetooth: btnxpuart: Fix btnxpuart_close

[Luiz Augusto von Dentz]

Hi Francesco,

On Mon, Mar 4, 2024 at 11:52 AM Francesco Dolcini <francesco@dolcini.it> wrote:
>
> Hi Luiz,
>
> On Fri, Nov 24, 2023 at 09:26:11PM +0100, Francesco Dolcini wrote:
> > On Wed, Oct 18, 2023 at 04:55:39PM +0200, Marcel Ziswiler wrote:
> > > 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>
> > > ---
> > > This is the kernel trace this commit fixes:
> > > [   29.270685] BUG: scheduling while atomic: kworker/u3:0/55/0x00000002
> >
> > I just hit this bug with 6.7-rc2, I think it would be worth to
> > apply this fix.
>
> Do you need any change for having this patch (1/2) applied? Do you want this
> to be re-sent without the second patch (2/2) from this series that is
> maybe more controversial?

Yes please just resend it.

> Let me know how I can help,
>
> Thanks,
> Francesco
>


-- 
Luiz Augusto von Dentz