Re: [PATCH rtw-next v2 6/7] wifi: rtw88: 88xxa: adapt channel sensing to receiver gain

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

Hi Jie Feng and Bitterblue,

Thanks for fixing the scan latency. A few comments below.

> diff --git a/drivers/net/wireless/realtek/rtw88/phy.c b/drivers/net/wireless/realtek/rtw88/phy.c
> index 69998c0d7c1f851bb30d2702233d62e14b305bf0..7a04aec9a045f1e308a4c5d331c900e013996bd5 100644
> --- a/drivers/net/wireless/realtek/rtw88/phy.c
> +++ b/drivers/net/wireless/realtek/rtw88/phy.c
> @@ -377,6 +377,10 @@ void rtw_phy_dig_set_max_coverage(struct rtw_dev *rtwdev)
>               DIG_CVRG_MIN);
>
>       rtw_phy_dig_write(rtwdev, DIG_CVRG_MIN);
> +
> +     /* EDCCA thresholds can be relative to the IGI */
> +     if (!rtw_fw_feature_check(&rtwdev->fw, FW_FEATURE_ADAPTIVITY))
> +             rtw_phy_adaptivity(rtwdev);
>  }

This and the call in rtw_phy_dig_reset() also reach
rtw8822b_adaptivity() and rtw8822c_adaptivity() when the firmware
doesn't handle adaptivity. They use igi_history[0], so as far as I can
see they just write the same thresholds again, but since this is core
code it would be good to mention it in the commit message.

[ ... ]

> diff --git a/drivers/net/wireless/realtek/rtw88/rtw88xxa.c b/drivers/net/wireless/realtek/rtw88/rtw88xxa.c
> index 851d198f76f3b10d5ba77c7e07e4be9d7fd54e4b..ee72380757721dab7dc409aabc24b2b825969465 100644
> --- a/drivers/net/wireless/realtek/rtw88/rtw88xxa.c
> +++ b/drivers/net/wireless/realtek/rtw88/rtw88xxa.c

[ ... ]

> +#define EDCCA_LB_START_88XXA         (EDCCA_L2H_H2L_DIFF - 30)
> +#define EDCCA_LB_RETRY_88XXA         (EDCCA_LB_START_88XXA + 18)

Maybe a short comment on where the 18 comes from. As far as I can see it
is the vendor condition for searching a second time (IGI upper bound of
0x26 or lower), worked out for an initial L2H of -17.

[ ... ]

> +     rtw_write32(rtwdev, REG_DBGSEL, 0);
> +     for (i = 0; i < 3 &&
> +          rtw_read32(rtwdev, REG_DBGRPT) & BIT_DBG_CCA_BUSY; i++)
> +             usleep_range(3000, 3500);

The register read in the loop condition is a bit hard to follow. Maybe
read_poll_timeout() would read easier, like efuse.c uses it:

        read_poll_timeout(rtw_read32, val, !(val & BIT_DBG_CCA_BUSY),
                          3000, 9000, false, rtwdev, REG_DBGRPT);

[ ... ]

> +void rtw88xxa_adaptivity_init(struct rtw_dev *rtwdev)
> +{
> +     struct rtw_dm_info *dm_info = &rtwdev->dm_info;
> +     u32 dbgsel = rtw_read32(rtwdev, REG_DBGSEL);
> +     s8 l2h;
> +
> +     rtw_write32_clr(rtwdev, REG_TX_PTCL_CTRL, BIT_DIS_EDCCA);

Bitterblue reported on that "firmware failed to leave lps state" shows
up a lot more often with this patch and stops when EDCCA is disabled,
and you replied that it only prints a line in dmesg. On rtw-next it is
more than a message now. Since dc16902cad9c ("wifi: rtw88: ps: Trigger
firmware recovery when failing to leave LPS state"), rtw_fw_leave_lps_check() 
goes on to a firmware recovery after the warning:

        rtw_warn(rtwdev, "firmware failed to leave lps state\n");
        rtw_fw_dump_dbg_info(rtwdev);
        rtw_fw_recovery(rtwdev);

So a failed leave now ends in a firmware recovery, with a "firmware
crash, start reset and recover" WARN and ieee80211_restart_hw(). With
power saving on, rtw_watch_dog_work() calls rtw_leave_lps() every 2
seconds before the dynamic mechanisms, so I think this needs to be
solved before the patch can go in.

About a longer timeout, according to the comment in
__rtw_fw_leave_lps_check_reg() the firmware restores REG_TCR once the AP
has acked its null frame, so I think it would mostly hide the problem.
Leaving this patch out of v3 and sending it again once the cause is
known sounds fine to me, then the rest of the series doesn't have to
wait.

Bitterblue, did you turn EDCCA off through the edcca_enable debugfs file?
With this patch that only switches to RTW_EDCCA_NORMAL, and
rtw88xxa_adaptivity() then keeps EDCCA on with the higher normal
thresholds. If that is what made it stop, it would point at the
thresholds used in adaptivity mode (ETSI and JP regions) rather than at
EDCCA itself.

Jie Feng, maybe comparing the IGI in the hardware when the leave fails
with the IGI the thresholds were computed from would show more.

[ ... ]

> +     /* igi_history[0] keeps the pre-scan IGI while scanning */
> +     u8 igi = rtw_read32_mask(rtwdev, REG_RXIGI_A, 0x7f);

The comment sits on a line that doesn't use igi_history[0], so it reads
a bit confusing. Maybe something like "read the IGI in use,
igi_history[0] still holds the old value while scanning".

Best regards,
Luka Gejak