Re: [PATCH 2/4] Bluetooth: hci_serdev: Track whether the serial port is open

[Markus Probst <markus.probst@posteo.de>]

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

On Wed, 2026-10-28 at 22:34 +0000, Michael McAllister wrote:
> QCA setup temporarily closes the serial port while enabling the supplies.
> If enabling the supplies or reopening the port fails, PROTO_READY stays
> set. Error cleanup can then flush or close an already-closed port.
> 
> Track the open state separately from protocol readiness. Use the same
> open and close helpers in the transport and the QCA and Marvell drivers,
> so setup retries do not close the port twice. Skip the serial flush when
> the port is closed, while still flushing the protocol queues.
> 
> Close the port during unregister according to its actual open state.
> 
> Signed-off-by: Michael McAllister <me@michaelmcallister.org>
> ---
>  drivers/bluetooth/hci_mrvl.c   |  4 ++--
>  drivers/bluetooth/hci_qca.c    | 12 +++++------
>  drivers/bluetooth/hci_serdev.c | 37 +++++++++++++++++++++++++++-------
>  drivers/bluetooth/hci_uart.h   |  3 +++
>  4 files changed, 41 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_mrvl.c b/drivers/bluetooth/hci_mrvl.c
> index 5798a8db0..38d9add89 100644
> --- a/drivers/bluetooth/hci_mrvl.c
> +++ b/drivers/bluetooth/hci_mrvl.c
> @@ -76,7 +76,7 @@ static int mrvl_open(struct hci_uart *hu)
>  	hu->priv = mrvl;
>  
>  	if (hu->serdev) {
> -		ret = serdev_device_open(hu->serdev);
> +		ret = hci_uart_serdev_open(hu);
Hi,

Why is `serdev_device_open`/`hci_uart_serdev_open` even called here?

According to Sashiko [1], `hci_uart_register_device` already calls
`serdev_device_open` and directly after that `p->open` (effectively
`mvrl_open` and thus another `serdev_device_open`). If Sashiko is right
(which appears to be the case at first look), the device is always
opened twice on probe.

From what I can tell, opening it twice is *undefined behaviour*. This
patch adds a check if the device is open, before calling
`serdev_device_open`. This solves the issue (possibly by accident), but
does not explain why it exists in the first place.

My patch [2] would instead return the error -EALREADY, which could
prevent it to probe entirely. It would also allow calling
`serdev_device_close` on a already closed serdev device, which makes
the use in the commit message obsolete.

Thanks
- Markus Probst

[1]
https://lore.kernel.org/linux-serial/sashiko-outbox-211656@kernel.org/
[2]
https://lore.kernel.org/linux-serial/20261112-rust_serdev_probe_refactor-v6-1-3568ed56264c@posteo.de/


>  		if (ret)
>  			goto err;
>  	}
> @@ -95,7 +95,7 @@ static int mrvl_close(struct hci_uart *hu)
>  	BT_DBG("hu %p", hu);
>  
>  	if (hu->serdev)
> -		serdev_device_close(hu->serdev);
> +		hci_uart_serdev_close(hu);
>  
>  	skb_queue_purge(&mrvl->txq);
>  	skb_queue_purge(&mrvl->rawq);
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 8705c4543..3d9e6cd15 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1755,8 +1755,8 @@ static int qca_port_reopen(struct hci_uart *hu)
>  	 * Without this, we will have RTS and CTS synchronization
>  	 * issues.
>  	 */
> -	serdev_device_close(hu->serdev);
> -	ret = serdev_device_open(hu->serdev);
> +	hci_uart_serdev_close(hu);
> +	ret = hci_uart_serdev_open(hu);
>  	if (ret) {
>  		bt_dev_err(hu->hdev, "failed to open port");
>  		return ret;
> @@ -1780,12 +1780,12 @@ static int qca_regulator_init(struct hci_uart *hu)
>  	qcadev = serdev_device_get_drvdata(hu->serdev);
>  
>  	if (!qcadev->bt_power->vregs_on) {
> -		serdev_device_close(hu->serdev);
> +		hci_uart_serdev_close(hu);
>  		ret = qca_regulator_enable(qcadev);
>  		if (ret)
>  			return ret;
>  
> -		ret = serdev_device_open(hu->serdev);
> +		ret = hci_uart_serdev_open(hu);
>  		if (ret) {
>  			bt_dev_err(hu->hdev, "failed to open port");
>  			return ret;
> @@ -2061,8 +2061,8 @@ static int qca_setup(struct hci_uart *hu)
>  		if (retries < MAX_INIT_RETRIES) {
>  			bt_dev_warn(hdev, "Retry BT power ON:%d", retries);
>  			if (hu->serdev) {
> -				serdev_device_close(hu->serdev);
> -				ret = serdev_device_open(hu->serdev);
> +				hci_uart_serdev_close(hu);
> +				ret = hci_uart_serdev_open(hu);
>  				if (ret) {
>  					bt_dev_err(hdev, "failed to open port");
>  					return ret;
> diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c
> index 48ba017e2..05aa490a9 100644
> --- a/drivers/bluetooth/hci_serdev.c
> +++ b/drivers/bluetooth/hci_serdev.c
> @@ -21,6 +21,29 @@
>  
>  #include "hci_uart.h"
>  
> +int hci_uart_serdev_open(struct hci_uart *hu)
> +{
> +	int err;
> +
> +	if (hu->serdev_open)
> +		return 0;
> +
> +	err = serdev_device_open(hu->serdev);
> +	if (!err)
> +		hu->serdev_open = true;
> +
> +	return err;
> +}
> +
> +void hci_uart_serdev_close(struct hci_uart *hu)
> +{
> +	if (!hu->serdev_open)
> +		return;
> +
> +	hu->serdev_open = false;
> +	serdev_device_close(hu->serdev);
> +}
> +
>  static inline void hci_uart_tx_complete(struct hci_uart *hu, int pkt_type)
>  {
>  	struct hci_dev *hdev = hu->hdev;
> @@ -102,7 +125,8 @@ static int hci_uart_flush(struct hci_dev *hdev)
>  	}
>  
>  	/* Flush any pending characters in the driver and discipline. */
> -	serdev_device_write_flush(hu->serdev);
> +	if (hu->serdev_open)
> +		serdev_device_write_flush(hu->serdev);
>  
>  	if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
>  		hu->proto->flush(hu);
> @@ -123,7 +147,7 @@ static int hci_uart_open(struct hci_dev *hdev)
>  	 * BT OFF. Upon next BT ON UART port should be opened.
>  	 */
>  	if (!test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
> -		err = serdev_device_open(hu->serdev);
> +		err = hci_uart_serdev_open(hu);
>  		if (err)
>  			return err;
>  		set_bit(HCI_UART_PROTO_READY, &hu->flags);
> @@ -154,7 +178,7 @@ static int hci_uart_close(struct hci_dev *hdev)
>  	 */
>  	if (hci_test_quirk(hdev, HCI_QUIRK_NON_PERSISTENT_SETUP)) {
>  		clear_bit(HCI_UART_PROTO_READY, &hu->flags);
> -		serdev_device_close(hu->serdev);
> +		hci_uart_serdev_close(hu);
>  	}
>  
>  	return 0;
> @@ -314,7 +338,7 @@ int hci_uart_register_device_priv(struct hci_uart *hu,
>  	if (percpu_init_rwsem(&hu->proto_lock))
>  		return -ENOMEM;
>  
> -	err = serdev_device_open(hu->serdev);
> +	err = hci_uart_serdev_open(hu);
>  	if (err)
>  		goto err_rwsem;
>  
> @@ -385,7 +409,7 @@ int hci_uart_register_device_priv(struct hci_uart *hu,
>  	clear_bit(HCI_UART_PROTO_READY, &hu->flags);
>  	p->close(hu);
>  err_open:
> -	serdev_device_close(hu->serdev);
> +	hci_uart_serdev_close(hu);
>  err_rwsem:
>  	percpu_free_rwsem(&hu->proto_lock);
>  	return err;
> @@ -438,8 +462,7 @@ void hci_uart_unregister_device(struct hci_uart *hu)
>  	 */
>  	hu->proto->close(hu);
>  
> -	if (proto_ready)
> -		serdev_device_close(hu->serdev);
> +	hci_uart_serdev_close(hu);
>  
>  	hci_free_dev(hdev);
>  
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index 7fbe8dffa..2f8e346f3 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -66,6 +66,7 @@ struct hci_uart_proto {
>  struct hci_uart {
>  	struct tty_struct	*tty;
>  	struct serdev_device	*serdev;
> +	bool			serdev_open;
>  	struct hci_dev		*hdev;
>  	unsigned long		flags;
>  	unsigned long		hdev_flags;
> @@ -112,6 +113,8 @@ static inline int hci_uart_register_device(struct hci_uart *hu,
>  }
>  
>  void hci_uart_unregister_device(struct hci_uart *hu);
> +int hci_uart_serdev_open(struct hci_uart *hu);
> +void hci_uart_serdev_close(struct hci_uart *hu);
>  
>  int hci_uart_tx_wakeup(struct hci_uart *hu);
>  int hci_uart_wait_until_sent(struct hci_uart *hu);

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]