Re: [PATCH rtw-next 3/3] wifi: rtw88: reset the stations outside of RCU on firmware recovery

[Luka Gejak <luka.gejak@linux.dev>]

Hi Mehmet,

Thanks for the fix. It looks good to me, though I have one small comment
below, but I don't think it should block the patch.

> diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
> index ce65a1a80840..8e7ae747b900 100644
> --- a/drivers/net/wireless/realtek/rtw88/main.c
> +++ b/drivers/net/wireless/realtek/rtw88/main.c
> @@ -664,7 +664,7 @@ static void __fw_recovery_work(struct rtw_dev *rtwdev)
>       WARN(1, "firmware crash, start reset and recover\n");
>
>       rtw_iterate_keys_atomic(rtwdev, NULL, rtw_reset_key_iter, rtwdev);
> -     rtw_iterate_stas_atomic(rtwdev, rtw_reset_sta_iter, rtwdev);
> +     rtw_iterate_stas(rtwdev, rtw_reset_sta_iter, rtwdev);
>       rtw_iterate_vifs_atomic(rtwdev, rtw_reset_vif_iter, rtwdev);

One difference to the atomic iterator: rtw_collect_sta_iter() allocates
an entry per station with GFP_ATOMIC and silently skips the station when
that fails:

        stas_entry = kmalloc_obj(*stas_entry, GFP_ATOMIC);
        if (!stas_entry)
                return;

For the other rtw_iterate_stas() users that only loses one update, but
here it skips rtw_sta_remove(). So if I read it right, the reconfig then
adds that station again with rtw_sta_add(), sta_cnt stays one too high
from then on (rtw8822c_cfo_track() for example resets its tracking unless
sta_cnt is 1), and the old macid of an AP client is never released.

It needs the allocation to fail during a recovery, so it is very
unlikely, and I don't see a simple way around it without the wiphy lock.
Just mentioning it, I am fine with the patch as is.

Reviewed-by: Luka Gejak <luka.gejak@linux.dev>

Best regards,
Luka Gejak