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

[Ping-Ke Shih]

yhchen312@xxxxxxxxx <yhchen312@xxxxxxxxx> wrote:
> From: "Yuhang.chen" <yhchen312@xxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Yuhang.chen <yhchen312@xxxxxxxxx>
> ---
>  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