[PATCH v4 0/2] Bluetooth: btmtksdio: teardown fixes

[PATCH v4 0/2] Bluetooth: btmtksdio: teardown fixes

[Sergey Senozhatsky]

This fixes several teardown issues:

     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
     [..]

close/flush can deadlock when run concurrently with btmtksdio_txrx_work().
In addition btmtksdio_txrx_work() re-enables interrupts regardless of
close/flush being executed on another CPU.

v3 -> v4:
- fix commit message linter warnings/errors (tabs, subject line over 80
  chars).

Sergey Senozhatsky (2):
  Bluetooth: btmtksdio: test for BUS IO errors in btmtksdio_txrx_work()
  Bluetooth: btmtksdio: call cancel_work_sync() out of host lock scope

 drivers/bluetooth/btmtksdio.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

--
2.54.0.1189.g8c84645362-goog

[PATCH v4 1/2] Bluetooth: btmtksdio: test for BUS 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.1189.g8c84645362-goog

[PATCH v4 2/2] Bluetooth: btmtksdio: call cancel_work_sync() out 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.1189.g8c84645362-goog

Re: [PATCH v4 0/2] Bluetooth: btmtksdio: teardown fixes

[Sergey Senozhatsky]

On (26/06/18 12:13), Sergey Senozhatsky wrote:
> This fixes several teardown issues:
> 
>      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
>      [..]
> 
> close/flush can deadlock when run concurrently with btmtksdio_txrx_work().
> In addition btmtksdio_txrx_work() re-enables interrupts regardless of
> close/flush being executed on another CPU.
> 
> v3 -> v4:
> - fix commit message linter warnings/errors (tabs, subject line over 80
>   chars).
> 
> Sergey Senozhatsky (2):
>   Bluetooth: btmtksdio: test for BUS IO errors in btmtksdio_txrx_work()
>   Bluetooth: btmtksdio: call cancel_work_sync() out of host lock scope

Do the patches look good enough to pick up?

Re: [PATCH v4 0/2] Bluetooth: btmtksdio: teardown fixes

[Sergey Senozhatsky]

On (26/06/24 14:19), Sergey Senozhatsky wrote:
> On (26/06/18 12:13), Sergey Senozhatsky wrote:
> > This fixes several teardown issues:
> > 
> >      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
> >      [..]
> > 
> > close/flush can deadlock when run concurrently with btmtksdio_txrx_work().
> > In addition btmtksdio_txrx_work() re-enables interrupts regardless of
> > close/flush being executed on another CPU.
> > 
> > v3 -> v4:
> > - fix commit message linter warnings/errors (tabs, subject line over 80
> >   chars).
> > 
> > Sergey Senozhatsky (2):
> >   Bluetooth: btmtksdio: test for BUS IO errors in btmtksdio_txrx_work()
> >   Bluetooth: btmtksdio: call cancel_work_sync() out of host lock scope
> 
> Do the patches look good enough to pick up?

Gentle ping.