[PATCH v2 0/3] Bluetooth: btmtksdio: teardown fixes

[PATCH v2 0/3] Bluetooth: btmtksdio: teardown fixes

[Sergey Senozhatsky]

This is v2 of teardown fixes.

We noticed a number of cases when btmtk close/teardown would hung:

     INFO: task kworker/u17:0:189 blocked for more than 122 seconds.
     __cancel_work_timer+0x3f4/0x460
     cancel_work_sync+0x1c/0x2c
     btmtksdio_flush+0x2c/0x40
     hci_dev_open_sync+0x10c4/0x2190
     [..]

There are several issues with the teardown (close/reset) code
in the driver.  First, the btmtksdio_txrx_work() potentially
can spin forever (infinite loop).  Second, close/flush can
deadlock when run concurrently with btmtksdio_txrx_work().

v1 -> v2:
- added two more patches (deadlock fix, interrupts re-enabling
  enhancement)

Sergey Senozhatsky (3):
  Bluetooth: btmtksdio: correct btmtksdio_txrx_work() loop timeout check
  Bluetooth: btmtksdio: test for bug IO errors in btmtksdio_txrx_work()
  Bluetooth: btmtksdio: call cancel_work_sync() outside of host lock
    scope

 drivers/bluetooth/btmtksdio.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

-- 
2.54.0.1136.gdb2ca164c4-goog

[PATCH v2 1/3] Bluetooth: btmtksdio: correct btmtksdio_txrx_work() loop timeout check

[Sergey Senozhatsky]

The btmtksdio_txrx_work() loop is expected to be terminated if running
for longer than 5*HZ.  However the timeout check is reversed:
time_is_before_jiffies(old_jiffies + 5*HZ) evaluates to true when
old_jiffies + 5*HZ is in the past i.e. when a timeout has occurred.
Using OR with time_is_before_jiffies(txrx_timeout) means that:
- before the 5-second timeout: the condition is `int_status || false`,
  so it loops as long as there are pending interrupts.
- after the 5-second timeout: the condition becomes `int_status || true`,
  which is always true.

Fix loop termination condition to actually enforce a 5*HZ timeout.

Fixes: 26270bc189ea4 ("Bluetooth: btmtksdio: move interrupt service to work")
Cc: stable@vger.kernel.org
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 drivers/bluetooth/btmtksdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index 5b0fab7b89b5..c6f80c419e90 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -620,7 +620,7 @@ static void btmtksdio_txrx_work(struct work_struct *work)
 			if (btmtksdio_rx_packet(bdev, rx_size) < 0)
 				bdev->hdev->stat.err_rx++;
 		}
-	} while (int_status || time_is_before_jiffies(txrx_timeout));
+	} while (int_status && time_is_after_jiffies(txrx_timeout));
 
 	/* Enable interrupt */
 	if (bdev->func->irq_handler)
-- 
2.54.0.1136.gdb2ca164c4-goog

Re: [PATCH v2 1/3] Bluetooth: btmtksdio: correct btmtksdio_txrx_work() loop timeout check

[Sean Wang]

Hi,

On Tue, Jun 16, 2026 at 6:15 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> The btmtksdio_txrx_work() loop is expected to be terminated if running
> for longer than 5*HZ.  However the timeout check is reversed:
> time_is_before_jiffies(old_jiffies + 5*HZ) evaluates to true when
> old_jiffies + 5*HZ is in the past i.e. when a timeout has occurred.
> Using OR with time_is_before_jiffies(txrx_timeout) means that:
> - before the 5-second timeout: the condition is `int_status || false`,
>   so it loops as long as there are pending interrupts.
> - after the 5-second timeout: the condition becomes `int_status || true`,
>   which is always true.
>
> Fix loop termination condition to actually enforce a 5*HZ timeout.
>
> Fixes: 26270bc189ea4 ("Bluetooth: btmtksdio: move interrupt service to work")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  drivers/bluetooth/btmtksdio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
> index 5b0fab7b89b5..c6f80c419e90 100644
> --- a/drivers/bluetooth/btmtksdio.c
> +++ b/drivers/bluetooth/btmtksdio.c
> @@ -620,7 +620,7 @@ static void btmtksdio_txrx_work(struct work_struct *work)
>                         if (btmtksdio_rx_packet(bdev, rx_size) < 0)
>                                 bdev->hdev->stat.err_rx++;
>                 }
> -       } while (int_status || time_is_before_jiffies(txrx_timeout));
> +       } while (int_status && time_is_after_jiffies(txrx_timeout));
>

This patch has already been merged, so I think the series should be
respun based on the latest code.

>         /* Enable interrupt */
>         if (bdev->func->irq_handler)
> --
> 2.54.0.1136.gdb2ca164c4-goog
>
>

Re: [PATCH v2 1/3] Bluetooth: btmtksdio: correct btmtksdio_txrx_work() loop timeout check

[Sergey Senozhatsky]

On (26/06/16 19:40), Sean Wang wrote:
> > The btmtksdio_txrx_work() loop is expected to be terminated if running
> > for longer than 5*HZ.  However the timeout check is reversed:
> > time_is_before_jiffies(old_jiffies + 5*HZ) evaluates to true when
> > old_jiffies + 5*HZ is in the past i.e. when a timeout has occurred.
> > Using OR with time_is_before_jiffies(txrx_timeout) means that:
> > - before the 5-second timeout: the condition is `int_status || false`,
> >   so it loops as long as there are pending interrupts.
> > - after the 5-second timeout: the condition becomes `int_status || true`,
> >   which is always true.
> >
> > Fix loop termination condition to actually enforce a 5*HZ timeout.
> >
> > Fixes: 26270bc189ea4 ("Bluetooth: btmtksdio: move interrupt service to work")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > ---
> >  drivers/bluetooth/btmtksdio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
> > index 5b0fab7b89b5..c6f80c419e90 100644
> > --- a/drivers/bluetooth/btmtksdio.c
> > +++ b/drivers/bluetooth/btmtksdio.c
> > @@ -620,7 +620,7 @@ static void btmtksdio_txrx_work(struct work_struct *work)
> >                         if (btmtksdio_rx_packet(bdev, rx_size) < 0)
> >                                 bdev->hdev->stat.err_rx++;
> >                 }
> > -       } while (int_status || time_is_before_jiffies(txrx_timeout));
> > +       } while (int_status && time_is_after_jiffies(txrx_timeout));
> >
> 
> This patch has already been merged, so I think the series should be
> respun based on the latest code.

Oh, I see.  Any chance it can be dropped from the tree or updated?
The patch is identical it's the commit message that has changed.
Otherwise, I can drop it from a v3 re-spin.

[PATCH v2 2/3] Bluetooth: btmtksdio: test for bug IO errors in btmtksdio_txrx_work()

[Sergey Senozhatsky]

btmtksdio_txrx_work() loop termination condition checks for
int_status being non-zero, however, this evaluates to true
even when sdio_readl() encounters BUS I/O error (in which
case int_status is 0xffffffff).  Break out of the loop if
sdio_readl() errors out.

Fixes: 26270bc189ea4 ("Bluetooth: btmtksdio: move interrupt service to work")
Cc: stable@vger.kernel.org
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 drivers/bluetooth/btmtksdio.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index c6f80c419e90..d8c8d2857527 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -574,7 +574,9 @@ static void btmtksdio_txrx_work(struct work_struct *work)
 	txrx_timeout = jiffies + 5 * HZ;
 
 	do {
-		int_status = sdio_readl(bdev->func, MTK_REG_CHISR, NULL);
+		int_status = sdio_readl(bdev->func, MTK_REG_CHISR, &err);
+		if (err < 0 || int_status == 0xffffffff)
+			break;
 
 		/* Ack an interrupt as soon as possible before any operation on
 		 * hardware.
-- 
2.54.0.1136.gdb2ca164c4-goog

[PATCH v2 3/3] Bluetooth: btmtksdio: call cancel_work_sync() outside of host lock scope

[Sergey Senozhatsky]

cancel_work_sync() should be called outside of host lock scope
in order to avoid circular locking scenario:

CPU0					CPU1
					close()/reset()
					sdio_claim_host()
txrx_work
  sdio_claim_host() // sleeps
					cancel_work_sync() // sleeps

In addition, when txrx_work() runs concurrently with close()/reset()
it better not to re-enable interrupts by testing for BTMTKSDIO_FUNC_ENABLED
and not BTMTKSDIO_HW_RESET_ACTIVE before C_INT_EN_SET write.  However,
btmtksdio_close() clears the BTMTKSDIO_FUNC_ENABLED too late (after
cancel_work_sync() call).  Move BTMTKSDIO_FUNC_ENABLED bit-clear earlier
so that txrx_work can see concurrent close().

Fixes: 26270bc189ea4 ("Bluetooth: btmtksdio: move interrupt service to work")
Cc: stable@vger.kernel.org
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 drivers/bluetooth/btmtksdio.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index d8c8d2857527..207d04cc2282 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -625,7 +625,9 @@ static void btmtksdio_txrx_work(struct work_struct *work)
 	} while (int_status && time_is_after_jiffies(txrx_timeout));
 
 	/* Enable interrupt */
-	if (bdev->func->irq_handler)
+	if (bdev->func->irq_handler &&
+	    test_bit(BTMTKSDIO_FUNC_ENABLED, &bdev->tx_state) &&
+	    !test_bit(BTMTKSDIO_HW_RESET_ACTIVE, &bdev->tx_state))
 		sdio_writel(bdev->func, C_INT_EN_SET, MTK_REG_CHLPCR, NULL);
 
 	sdio_release_host(bdev->func);
@@ -741,6 +743,8 @@ static int btmtksdio_close(struct hci_dev *hdev)
 	if (!test_bit(BTMTKSDIO_FUNC_ENABLED, &bdev->tx_state))
 		return 0;
 
+	clear_bit(BTMTKSDIO_FUNC_ENABLED, &bdev->tx_state);
+
 	sdio_claim_host(bdev->func);
 
 	/* Disable interrupt */
@@ -748,11 +752,12 @@ static int btmtksdio_close(struct hci_dev *hdev)
 
 	sdio_release_irq(bdev->func);
 
+	sdio_release_host(bdev->func);
 	cancel_work_sync(&bdev->txrx_work);
+	sdio_claim_host(bdev->func);
 
 	btmtksdio_fw_pmctrl(bdev);
 
-	clear_bit(BTMTKSDIO_FUNC_ENABLED, &bdev->tx_state);
 	sdio_disable_func(bdev->func);
 
 	sdio_release_host(bdev->func);
@@ -1295,7 +1300,10 @@ static void btmtksdio_reset(struct hci_dev *hdev)
 
 	sdio_writel(bdev->func, C_INT_EN_CLR, MTK_REG_CHLPCR, NULL);
 	skb_queue_purge(&bdev->txq);
+
+	sdio_release_host(bdev->func);
 	cancel_work_sync(&bdev->txrx_work);
+	sdio_claim_host(bdev->func);
 
 	gpiod_set_value_cansleep(bdev->reset, 1);
 	msleep(100);
-- 
2.54.0.1136.gdb2ca164c4-goog

Re: [PATCH v2 3/3] Bluetooth: btmtksdio: call cancel_work_sync() outside of host lock scope

[Sean Wang]

Hi,

On Tue, Jun 16, 2026 at 6:15 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> cancel_work_sync() should be called outside of host lock scope
> in order to avoid circular locking scenario:
>
> CPU0                                    CPU1
>                                         close()/reset()
>                                         sdio_claim_host()
> txrx_work
>   sdio_claim_host() // sleeps
>                                         cancel_work_sync() // sleeps
>
> In addition, when txrx_work() runs concurrently with close()/reset()
> it better not to re-enable interrupts by testing for BTMTKSDIO_FUNC_ENABLED
> and not BTMTKSDIO_HW_RESET_ACTIVE before C_INT_EN_SET write.  However,
> btmtksdio_close() clears the BTMTKSDIO_FUNC_ENABLED too late (after
> cancel_work_sync() call).  Move BTMTKSDIO_FUNC_ENABLED bit-clear earlier
> so that txrx_work can see concurrent close().
>
> Fixes: 26270bc189ea4 ("Bluetooth: btmtksdio: move interrupt service to work")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  drivers/bluetooth/btmtksdio.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
> index d8c8d2857527..207d04cc2282 100644
> --- a/drivers/bluetooth/btmtksdio.c
> +++ b/drivers/bluetooth/btmtksdio.c
> @@ -625,7 +625,9 @@ static void btmtksdio_txrx_work(struct work_struct *work)
>         } while (int_status && time_is_after_jiffies(txrx_timeout));
>
>         /* Enable interrupt */
> -       if (bdev->func->irq_handler)
> +       if (bdev->func->irq_handler &&
> +           test_bit(BTMTKSDIO_FUNC_ENABLED, &bdev->tx_state) &&
> +           !test_bit(BTMTKSDIO_HW_RESET_ACTIVE, &bdev->tx_state))
>                 sdio_writel(bdev->func, C_INT_EN_SET, MTK_REG_CHLPCR, NULL);
>
>         sdio_release_host(bdev->func);
> @@ -741,6 +743,8 @@ static int btmtksdio_close(struct hci_dev *hdev)
>         if (!test_bit(BTMTKSDIO_FUNC_ENABLED, &bdev->tx_state))
>                 return 0;
>
> +       clear_bit(BTMTKSDIO_FUNC_ENABLED, &bdev->tx_state);
> +
>         sdio_claim_host(bdev->func);
>
>         /* Disable interrupt */
> @@ -748,11 +752,12 @@ static int btmtksdio_close(struct hci_dev *hdev)
>
>         sdio_release_irq(bdev->func);
>
> +       sdio_release_host(bdev->func);
>         cancel_work_sync(&bdev->txrx_work);
> +       sdio_claim_host(bdev->func);
>
>         btmtksdio_fw_pmctrl(bdev);
>
> -       clear_bit(BTMTKSDIO_FUNC_ENABLED, &bdev->tx_state);
>         sdio_disable_func(bdev->func);
>
>         sdio_release_host(bdev->func);
> @@ -1295,7 +1300,10 @@ static void btmtksdio_reset(struct hci_dev *hdev)
>
>         sdio_writel(bdev->func, C_INT_EN_CLR, MTK_REG_CHLPCR, NULL);
>         skb_queue_purge(&bdev->txq);
> +
> +       sdio_release_host(bdev->func);
>         cancel_work_sync(&bdev->txrx_work);
> +       sdio_claim_host(bdev->func);
>
>         gpiod_set_value_cansleep(bdev->reset, 1);
>         msleep(100);

The patch looks good to me. Inspired by your patch,
do you think should we add another patch to keep txrx_work out of the
reset window by rejecting TX during reset,
ignoring reset-time interrupts, and making queued workers exit early?

Some code like:

--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -567,6 +567,8 @@ static void btmtksdio_txrx_work(struct work_struct *work)
        pm_runtime_get_sync(bdev->dev);

        sdio_claim_host(bdev->func);
+       if (test_bit(BTMTKSDIO_HW_RESET_ACTIVE, &bdev->tx_state))
+               goto out;

        /* Disable interrupt */
        sdio_writel(bdev->func, C_INT_EN_CLR, MTK_REG_CHLPCR, NULL);
@@ -628,6 +630,7 @@ static void btmtksdio_txrx_work(struct work_struct *work)
            !test_bit(BTMTKSDIO_HW_RESET_ACTIVE, &bdev->tx_state))
                sdio_writel(bdev->func, C_INT_EN_SET, MTK_REG_CHLPCR, NULL);

+out:
        sdio_release_host(bdev->func);

        pm_runtime_put_autosuspend(bdev->dev);
@@ -646,6 +649,9 @@ static void btmtksdio_interrupt(struct sdio_func *func)
        /* Disable interrupt */
        sdio_writel(bdev->func, C_INT_EN_CLR, MTK_REG_CHLPCR, NULL);

+       if (test_bit(BTMTKSDIO_HW_RESET_ACTIVE, &bdev->tx_state))
+               return;
+
        schedule_work(&bdev->txrx_work);
 }

@@ -1250,6 +1256,9 @@ static int btmtksdio_send_frame(struct hci_dev
*hdev, struct sk_buff *skb)
 {
        struct btmtksdio_dev *bdev = hci_get_drvdata(hdev);

+       if (test_bit(BTMTKSDIO_HW_RESET_ACTIVE, &bdev->tx_state))
+               return -EBUSY;
+
        switch (hci_skb_pkt_type(skb)) {
        case HCI_COMMAND_PKT:
                hdev->stat.cmd_tx++;

> --
> 2.54.0.1136.gdb2ca164c4-goog
>
>

Re: [PATCH v2 3/3] Bluetooth: btmtksdio: call cancel_work_sync() outside of host lock scope

[Sergey Senozhatsky]

On (26/06/16 19:56), Sean Wang wrote:
> The patch looks good to me. Inspired by your patch,
> do you think should we add another patch to keep txrx_work out of the
> reset window by rejecting TX during reset,
> ignoring reset-time interrupts, and making queued workers exit early?

I honestly don't know, it's hard for to me judge as I'm not all that
familiar with the code.  To make things more complex, I don't think we
see any crashes on reset path.  My personal preference maybe would be
to keep things the way they are?

> Some code like:
> 
> --- a/drivers/bluetooth/btmtksdio.c
> +++ b/drivers/bluetooth/btmtksdio.c
> @@ -567,6 +567,8 @@ static void btmtksdio_txrx_work(struct work_struct *work)
>         pm_runtime_get_sync(bdev->dev);
> 
>         sdio_claim_host(bdev->func);
> +       if (test_bit(BTMTKSDIO_HW_RESET_ACTIVE, &bdev->tx_state))
> +               goto out;

A nit: I think you can test_bit() outside of host lock scope.
Other than that I'm afraid I cannot be of much help here.

>         /* Disable interrupt */
>         sdio_writel(bdev->func, C_INT_EN_CLR, MTK_REG_CHLPCR, NULL);
> @@ -628,6 +630,7 @@ static void btmtksdio_txrx_work(struct work_struct *work)
>             !test_bit(BTMTKSDIO_HW_RESET_ACTIVE, &bdev->tx_state))
>                 sdio_writel(bdev->func, C_INT_EN_SET, MTK_REG_CHLPCR, NULL);
> 
> +out:
>         sdio_release_host(bdev->func);
> 
>         pm_runtime_put_autosuspend(bdev->dev);
> @@ -646,6 +649,9 @@ static void btmtksdio_interrupt(struct sdio_func *func)
>         /* Disable interrupt */
>         sdio_writel(bdev->func, C_INT_EN_CLR, MTK_REG_CHLPCR, NULL);
> 
> +       if (test_bit(BTMTKSDIO_HW_RESET_ACTIVE, &bdev->tx_state))
> +               return;
> +
>         schedule_work(&bdev->txrx_work);
>  }
> 
> @@ -1250,6 +1256,9 @@ static int btmtksdio_send_frame(struct hci_dev
> *hdev, struct sk_buff *skb)
>  {
>         struct btmtksdio_dev *bdev = hci_get_drvdata(hdev);
> 
> +       if (test_bit(BTMTKSDIO_HW_RESET_ACTIVE, &bdev->tx_state))
> +               return -EBUSY;
> +
>         switch (hci_skb_pkt_type(skb)) {
>         case HCI_COMMAND_PKT:
>                 hdev->stat.cmd_tx++;