[PATCH] wifi: rtw89: 8852be: add .shutdown callback to quiesce device on reboot

[PATCH] wifi: rtw89: 8852be: add .shutdown callback to quiesce device on reboot

[yhchen312]

From: "Yuhang.chen" <yhchen312@gmail.com>

Since hardware rfkill polling was introduced, warm reboot on arm64
platforms can panic with an asynchronous SError when the rfkill poll
workqueue reads a register while the PCIe link is being torn down:

  SError Interrupt on CPU8, code 0x00000000be000011 -- SError
  Workqueue: events_power_efficient rfkill_poll [rfkill]
  Call trace:
    rtw89_pci_ops_read8+0x94/0x160 [rtw89_pci]
    rtw89_core_rfkill_poll+0x50/0x1e0 [rtw89_core]
    rtw89_ops_rfkill_poll+0x40/0x68 [rtw89_core]
    ieee80211_rfkill_poll+0x3c/0x70 [mac80211]
    cfg80211_rfkill_poll+0x40/0x2a0 [cfg80211]
    rfkill_poll+0x30/0x88 [rfkill]
  Kernel panic - not syncing: Asynchronous SError Interrupt

During device_shutdown() the 8852BE PCI driver has no .shutdown
callback, so the device is never quiesced and the rfkill polling keeps
running.  Once the platform brings the PCIe link down, the next MMIO
read targets a non-responding device and is reported as an asynchronous
SError, which is fatal on arm64.

Add a .shutdown callback that runs the existing remove path to stop the
rfkill polling, free interrupts and power down the chip before the link
goes away.  A NULL drvdata check guards the case where probe did not
fully complete.

Fixes: 0b38e6277aed ("wifi: rtw89: add support for hardware rfkill")
Cc: stable@vger.kernel.org
Signed-off-by: Yuhang.chen <yhchen312@gmail.com>
---
 drivers/net/wireless/realtek/rtw89/rtw8852be.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw89/rtw8852be.c b/drivers/net/wireless/realtek/rtw89/rtw8852be.c
index 5bc0a6a99d1d..9aebecbac067 100644
--- a/drivers/net/wireless/realtek/rtw89/rtw8852be.c
+++ b/drivers/net/wireless/realtek/rtw89/rtw8852be.c
@@ -92,11 +92,20 @@ static const struct pci_device_id rtw89_8852be_id_table[] = {
 };
 MODULE_DEVICE_TABLE(pci, rtw89_8852be_id_table);
 
+static void rtw89_8852be_shutdown(struct pci_dev *pdev)
+{
+	if (!pci_get_drvdata(pdev))
+		return;
+
+	rtw89_pci_remove(pdev);
+}
+
 static struct pci_driver rtw89_8852be_driver = {
 	.name		= "rtw89_8852be",
 	.id_table	= rtw89_8852be_id_table,
 	.probe		= rtw89_pci_probe,
 	.remove		= rtw89_pci_remove,
+	.shutdown       = rtw89_8852be_shutdown,
 	.driver.pm	= &rtw89_pm_ops,
 	.err_handler    = &rtw89_pci_err_handler,
 };
-- 
2.34.1

RE: [PATCH] wifi: rtw89: 8852be: add .shutdown callback to quiesce device on reboot

[Ping-Ke Shih]

yhchen312@gmail.com <yhchen312@gmail.com> wrote:
> From: "Yuhang.chen" <yhchen312@gmail.com>
> 
> Since hardware rfkill polling was introduced, warm reboot on arm64
> platforms can panic with an asynchronous SError when the rfkill poll
> workqueue reads a register while the PCIe link is being torn down:

Does it mean the work is running after .shutdown but before .remove?

> 
>   SError Interrupt on CPU8, code 0x00000000be000011 -- SError
>   Workqueue: events_power_efficient rfkill_poll [rfkill]
>   Call trace:
>     rtw89_pci_ops_read8+0x94/0x160 [rtw89_pci]
>     rtw89_core_rfkill_poll+0x50/0x1e0 [rtw89_core]
>     rtw89_ops_rfkill_poll+0x40/0x68 [rtw89_core]
>     ieee80211_rfkill_poll+0x3c/0x70 [mac80211]
>     cfg80211_rfkill_poll+0x40/0x2a0 [cfg80211]
>     rfkill_poll+0x30/0x88 [rfkill]
>   Kernel panic - not syncing: Asynchronous SError Interrupt
> 
> During device_shutdown() the 8852BE PCI driver has no .shutdown
> callback, so the device is never quiesced and the rfkill polling keeps
> running.  Once the platform brings the PCIe link down, the next MMIO
> read targets a non-responding device and is reported as an asynchronous
> SError, which is fatal on arm64.
> 
> Add a .shutdown callback that runs the existing remove path to stop the
> rfkill polling, free interrupts and power down the chip before the link
> goes away. A NULL drvdata check guards the case where probe did not
> fully complete.
> 
> Fixes: 0b38e6277aed ("wifi: rtw89: add support for hardware rfkill")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yuhang.chen <yhchen312@gmail.com>
> ---
>  drivers/net/wireless/realtek/rtw89/rtw8852be.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/rtw8852be.c
> b/drivers/net/wireless/realtek/rtw89/rtw8852be.c
> index 5bc0a6a99d1d..9aebecbac067 100644
> --- a/drivers/net/wireless/realtek/rtw89/rtw8852be.c
> +++ b/drivers/net/wireless/realtek/rtw89/rtw8852be.c
> @@ -92,11 +92,20 @@ static const struct pci_device_id rtw89_8852be_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(pci, rtw89_8852be_id_table);
> 
> +static void rtw89_8852be_shutdown(struct pci_dev *pdev)
> +{
> +       if (!pci_get_drvdata(pdev))
> +               return;
> +
> +       rtw89_pci_remove(pdev);

Not prefer calling rtw89_pci_remove() twice.

I think we can add a callback .rtw89_pci_shutdown() to set a flag
RTW89_FLAG_SHUTDOWN. In rtw89_ops_rfkill_poll(), just ignore polling, like:

@@ -2010,7 +2010,8 @@ static void rtw89_ops_rfkill_poll(struct ieee80211_hw *hw)
        lockdep_assert_wiphy(hw->wiphy);

        /* wl_disable GPIO get floating when entering LPS */
-       if (test_bit(RTW89_FLAG_RUNNING, rtwdev->flags))
+       if (test_bit(RTW89_FLAG_RUNNING, rtwdev->flags) ||
+           test_bit(RTW89_FLAG_SHUTDOWN, rtwdev->flags))
                return;

        rtw89_core_rfkill_poll(rtwdev, false);

This flag is very similar to USB RTW89_FLAG_UNPLUGGED flag, but I'd add a
new flag.

> +}
> +
>  static struct pci_driver rtw89_8852be_driver = {
>         .name           = "rtw89_8852be",
>         .id_table       = rtw89_8852be_id_table,
>         .probe          = rtw89_pci_probe,
>         .remove         = rtw89_pci_remove,
> +       .shutdown       = rtw89_8852be_shutdown,

I think this fix can apply to all PCI devices for this driver, right?

>         .driver.pm      = &rtw89_pm_ops,
>         .err_handler    = &rtw89_pci_err_handler,
>  };
> --
> 2.34.1