Re: [PATCH V6 3/4] Bluetooth: btnxpuart: Add M.2 Bluetooth device support using pwrseq

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Brittle logic causing probe failure on valid hardware topologies.

Pre-existing issues:
- [High] Race condition exposing device before initializing synchronization primitives.
- [High] Use-after-free and locking a destroyed mutex during device teardown.
--

--- Patch [3]: [PATCH V6 3/4] Bluetooth: btnxpuart: Add M.2 Bluetooth device support using pwrseq ---
commit 885432311fa71ac754d16037ae3e761e8192452c
Author: Sherry Sun <sherry.sun@nxp.com>

Bluetooth: btnxpuart: Add M.2 Bluetooth device support using pwrseq

Power supply to the M.2 Bluetooth device attached to the host using M.2
connector is controlled using the uart pwrseq device. This commit adds
support for getting the pwrseq device if the OF graph link is present.

> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> --- a/drivers/bluetooth/btnxpuart.c
> +++ b/drivers/bluetooth/btnxpuart.c

[ ... ]

> @@ -1872,11 +1875,26 @@ static int nxp_serdev_probe(struct serdev_device *serdev)
>  		return err;
>  	}
>  
> +	if (of_graph_is_present(dev_of_node(&serdev->ctrl->dev))) {
> +		struct pwrseq_desc *pwrseq;
> +
> +		pwrseq = pwrseq_get(&serdev->ctrl->dev, "uart");
> +		if (IS_ERR(pwrseq))
> +			return dev_err_probe(&serdev->dev, PTR_ERR(pwrseq),
> +					     "failed to get pwrseq\n");

[Severity: High]
If the parent UART node has an OF graph for other purposes, like audio
routing or multiplexing, will this unconditionally fail the driver probe?

When of_graph_is_present() returns true, pwrseq_get() might return an
error like -ENOENT if there is no power sequencer configured for the
uart target.

Could this logic be made less brittle to support valid hardware topologies
where the UART has an OF graph but no power sequencer?

> +
> +		nxpdev->pwrseq = pwrseq;
> +		err = pwrseq_power_on(pwrseq);
> +		if (err)
> +			goto err_pwrseq_put;
> +	}

[ ... ]

> @@ -1907,13 +1925,16 @@ static int nxp_serdev_probe(struct serdev_device *serdev)
>  	if (bacmp(&ba, BDADDR_ANY))
>  		hci_set_quirk(hdev, HCI_QUIRK_USE_BDADDR_PROPERTY);
>  
> -	if (hci_register_dev(hdev) < 0) {
> +	err = hci_register_dev(hdev);
> +	if (err < 0) {
>  		dev_err(&serdev->dev, "Can't register HCI device\n");
>  		goto probe_fail;
>  	}
>  
> -	if (ps_setup(hdev))
> +	if (ps_setup(hdev)) {
> +		err = -ENODEV;
>  		goto probe_fail_unregister;
> +	}

[Severity: High]
This is a pre-existing issue, but does calling hci_register_dev() before
ps_setup() create a race condition during initialization?

When hci_register_dev() is called, it schedules the power_on work
asynchronously, which opens the UART port via btnxpuart_open().

Once the UART is open, incoming data triggers the receive path:

btnxpuart_receive_buf()
  ps_wakeup()

ps_wakeup() attempts to lock psdata->ps_lock and schedule psdata->work.

If ps_setup() has not yet completed its initialization of these structures,
could this result in operating on an uninitialized mutex or workqueue?

>  
>  	hci_devcd_register(hdev, nxp_coredump, nxp_coredump_hdr,

[ ... ]

> @@ -1952,6 +1976,8 @@ static void nxp_serdev_remove(struct serdev_device *serdev)
>  		}
>  	}
>  
>  	ps_cleanup(nxpdev);
>  	hci_unregister_dev(hdev);

[Severity: High]
This is a pre-existing issue, but does calling ps_cleanup() before
hci_unregister_dev() cause a use-after-free or a lock on a destroyed mutex?

ps_cleanup() destroys psdata->ps_lock and cancels psdata->work.

However, hci_unregister_dev() shuts down the device. During this shutdown
process, the UART can still receive data or send commands.

If data is received or sent, the paths btnxpuart_receive_buf() to
ps_wakeup() or nxp_enqueue() will attempt to acquire the destroyed
psdata->ps_lock.

Should hci_unregister_dev() be called before cleaning up the
synchronization primitives?

>  	reset_control_assert(nxpdev->pdn);
> +	if (nxpdev->pwrseq)
> +		pwrseq_put(nxpdev->pwrseq);
>  	hci_free_dev(hdev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713023435.235765-1-sherry.sun@oss.nxp.com?part=3