[v1] Bluetooth: btmtksdio: fix deadlock in close and reset paths

[PATCH] Bluetooth: btmtksdio: fix deadlock in close and reset paths

Posted by ZhaoJinming 1 month, 3 weeks ago

btmtksdio_close() and btmtksdio_reset() call cancel_work_sync() on
bdev->txrx_work while holding the sdio host lock, which is also acquired
by btmtksdio_txrx_work().  If txrx_work is queued when close/reset runs,
a worker thread may start it after the host lock is taken and block in
sdio_claim_host(), while cancel_work_sync() waits for the work to
finish.  The host lock is only released after cancel_work_sync()
returns, so both sides wait forever, deadlocking close/reset.

Fix this by releasing the sdio host lock before calling
cancel_work_sync(), then re-acquiring it afterwards.  The interrupt is
already disabled (sdio_release_irq() in close, C_INT_EN_CLR in reset)
before the work is cancelled, so no new work can be scheduled and
cancel_work_sync() fully quiesces txrx_work before the device is torn
down.  This mirrors the pattern already used by btmtksdio_flush(),
which cancels the work without holding the host lock.

Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
---
btmtksdio_close() and btmtksdio_reset() call cancel_work_sync() on
bdev->txrx_work while holding the sdio host lock that
btmtksdio_txrx_work() also acquires. If txrx_work is queued at that
point, a worker thread can start it and block in sdio_claim_host(),
while cancel_work_sync() waits for the work to finish and the host lock
is only released afterwards - a deadlock.

This series releases the host lock around cancel_work_sync() so the work
can always complete, mirroring the pattern already used by
btmtksdio_flush().
---

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

diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index c6f80c419e901e71b21e550449250a5a6755d100..879d580226d25ad398105a158de1545dace33718 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -746,8 +746,16 @@ static int btmtksdio_close(struct hci_dev *hdev)
 
 	sdio_release_irq(bdev->func);
 
+	/* No new work can be scheduled after sdio_release_irq(), so cancel the
+	 * work outside the sdio host lock. btmtksdio_txrx_work() also claims
+	 * the host, so canceling it while holding the lock would deadlock.
+	 */
+	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);
@@ -1293,8 +1301,18 @@ 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);
+
+	/* With the interrupt disabled, the SDIO IRQ handler can no longer
+	 * schedule txrx_work. Cancel the work outside the sdio host lock;
+	 * btmtksdio_txrx_work() also claims the host, so canceling it while
+	 * holding the lock would deadlock.
+	 */
+	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);
 	gpiod_set_value_cansleep(bdev->reset, 0);

---
base-commit: 0d839570765118029aa8bf4a95444c6a11aacf85
change-id: 20260806-btmtksdio-deadlock-fix-f9421f1a7879

Best regards,
-- 
ZhaoJinming <zhaojinming@uniontech.com>

Re: [PATCH] Bluetooth: btmtksdio: fix deadlock in close and reset paths

Posted by Luiz Augusto von Dentz 1 month, 3 weeks ago

Hi ZhaoJinming,

On Thu, Aug 6, 2026 at 2:36 AM ZhaoJinming <zhaojinming@uniontech.com> wrote:
>
> btmtksdio_close() and btmtksdio_reset() call cancel_work_sync() on
> bdev->txrx_work while holding the sdio host lock, which is also acquired
> by btmtksdio_txrx_work().  If txrx_work is queued when close/reset runs,
> a worker thread may start it after the host lock is taken and block in
> sdio_claim_host(), while cancel_work_sync() waits for the work to
> finish.  The host lock is only released after cancel_work_sync()
> returns, so both sides wait forever, deadlocking close/reset.
>
> Fix this by releasing the sdio host lock before calling
> cancel_work_sync(), then re-acquiring it afterwards.  The interrupt is
> already disabled (sdio_release_irq() in close, C_INT_EN_CLR in reset)
> before the work is cancelled, so no new work can be scheduled and
> cancel_work_sync() fully quiesces txrx_work before the device is torn
> down.  This mirrors the pattern already used by btmtksdio_flush(),
> which cancels the work without holding the host lock.
>
> Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
> ---
> btmtksdio_close() and btmtksdio_reset() call cancel_work_sync() on
> bdev->txrx_work while holding the sdio host lock that
> btmtksdio_txrx_work() also acquires. If txrx_work is queued at that
> point, a worker thread can start it and block in sdio_claim_host(),
> while cancel_work_sync() waits for the work to finish and the host lock
> is only released afterwards - a deadlock.
>
> This series releases the host lock around cancel_work_sync() so the work
> can always complete, mirroring the pattern already used by
> btmtksdio_flush().
> ---
>
> ---
>  drivers/bluetooth/btmtksdio.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
> index c6f80c419e901e71b21e550449250a5a6755d100..879d580226d25ad398105a158de1545dace33718 100644
> --- a/drivers/bluetooth/btmtksdio.c
> +++ b/drivers/bluetooth/btmtksdio.c
> @@ -746,8 +746,16 @@ static int btmtksdio_close(struct hci_dev *hdev)
>
>         sdio_release_irq(bdev->func);
>
> +       /* No new work can be scheduled after sdio_release_irq(), so cancel the
> +        * work outside the sdio host lock. btmtksdio_txrx_work() also claims
> +        * the host, so canceling it while holding the lock would deadlock.
> +        */
> +       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);
> @@ -1293,8 +1301,18 @@ 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);
> +
> +       /* With the interrupt disabled, the SDIO IRQ handler can no longer
> +        * schedule txrx_work. Cancel the work outside the sdio host lock;
> +        * btmtksdio_txrx_work() also claims the host, so canceling it while
> +        * holding the lock would deadlock.
> +        */
> +       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);
>         gpiod_set_value_cansleep(bdev->reset, 0);
>
> ---
> base-commit: 0d839570765118029aa8bf4a95444c6a11aacf85
> change-id: 20260806-btmtksdio-deadlock-fix-f9421f1a7879
>
> Best regards,
> --
> ZhaoJinming <zhaojinming@uniontech.com>

Sashiko found a problem:

https://sashiko.dev/#/patchset/FD5D03449312D17C%2B20260806-btmtksdio-deadlock-fix-v1-1-3a2d4392d117%40uniontech.com

-- 
Luiz Augusto von Dentz

[PATCH v2] Bluetooth: btmtksdio: fix deadlock in close and reset paths

Posted by ZhaoJinming 1 month, 3 weeks ago

btmtksdio_close() and btmtksdio_reset() call cancel_work_sync() on
bdev->txrx_work while holding the sdio host lock, which is also acquired
by btmtksdio_txrx_work().  If txrx_work is queued when close/reset runs,
a worker thread may start it after the host lock is taken and block in
sdio_claim_host(), while cancel_work_sync() waits for the work to
finish.  The host lock is only released after cancel_work_sync()
returns, so both sides wait forever, deadlocking close/reset.

Fix this by releasing the sdio host lock before calling
cancel_work_sync(), then re-acquiring it afterwards.

In btmtksdio_close() the interrupt is already disabled by
sdio_release_irq(), which also unregisters the IRQ handler, so no new
work can be scheduled and cancel_work_sync() fully quiesces txrx_work.

btmtksdio_reset() must additionally unregister the IRQ handler before
dropping the host lock: btmtksdio_txrx_work() unconditionally re-enables
the device interrupt (C_INT_EN_SET) when the handler is still registered,
so an in-flight worker would re-enable interrupts and be rescheduled
while the device is being reset, defeating the cancellation.  The IRQ is
re-claimed by btmtksdio_open() when the HCI device is re-opened after
the reset.

This mirrors the pattern already used by btmtksdio_flush(), which
cancels the work without holding the host lock.

Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
---
Changes in v2:
- Unregister the IRQ handler in btmtksdio_reset() before dropping the
  host lock, so a concurrent txrx_work cannot re-enable the device
  interrupt (C_INT_EN_SET) and be rescheduled during reset.
---
 drivers/bluetooth/btmtksdio.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index c6f80c419e901e71b21e550449250a5a6755d100..23650df4fb06114ab7be1f0b30eb61a15322b288 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -746,8 +746,16 @@ static int btmtksdio_close(struct hci_dev *hdev)
 
 	sdio_release_irq(bdev->func);
 
+	/* No new work can be scheduled after sdio_release_irq(), so cancel the
+	 * work outside the sdio host lock. btmtksdio_txrx_work() also claims
+	 * the host, so canceling it while holding the lock would deadlock.
+	 */
+	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);
@@ -1293,8 +1301,22 @@ 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);
+
+	/* Unregister the IRQ before releasing the host lock so that a
+	 * concurrently running btmtksdio_txrx_work() cannot re-enable the
+	 * device interrupt (C_INT_EN_SET) and be rescheduled while the device
+	 * is being reset. btmtksdio_txrx_work() also claims the host, so the
+	 * work must be cancelled outside the sdio host lock to avoid a
+	 * deadlock. The IRQ is re-claimed by btmtksdio_open() when the HCI
+	 * device is re-opened after the reset.
+	 */
+	sdio_release_irq(bdev->func);
+	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);
 	gpiod_set_value_cansleep(bdev->reset, 0);

---
base-commit: 0d839570765118029aa8bf4a95444c6a11aacf85
change-id: 20260806-btmtksdio-deadlock-fix-f9421f1a7879

Best regards,
-- 
ZhaoJinming <zhaojinming@uniontech.com>