RE: [PATCH rtw-next v3] wifi: rtw89: set RA init rate level when the station RSSI is known

[Ping-Ke Shih <pkshih@realtek.com>]

V. S. <spam@acio.nl> wrote:
> Ping-Ke Shih <pkshih@realtek.com> wrote:
> > Not only RSSI 0 but RSSI 1 can fall into this case, but I think it is harmless.
> 
> Agreed.
> 
> > How about put into ra-> instead of rtwsta_link-> ?
> >
> > Please put it on bottom with a comment like below, because the struct rtw89_ra_info
> > is a copy of H2C command (at initial version).
> 
> OK. One point before I send v4: rtw89_phy_ra_sta_update() starts with
> memset(ra, 0, sizeof(*ra)).

I forgot this point. 

How about memset(ra, 0, offsetof(typeof(*ra), rssi_unknown)) ?

And, additional comment ... (see below)

> The RA update calls it every 2 seconds, so
> the memset clears the flag before the RA update can read it. Thus, the
> RA update must keep the flag in a local variable before the call. This
> is my proposal for v4:
> 
> --- a/drivers/net/wireless/realtek/rtw89/core.h
> +++ b/drivers/net/wireless/realtek/rtw89/core.h
> @@ struct rtw89_ra_info {
>   u8 macid_msb:2;
>   u8 band:2; /* enum rtw89_band */
>   u8 is_new_dbgreg:1;
> + /* driver helpers */

/* driver helpers (not clear by rtw89_phy_ra_sta_update()) */

> + bool rssi_unknown:1;
>  };
> 
> --- a/drivers/net/wireless/realtek/rtw89/phy.c
> +++ b/drivers/net/wireless/realtek/rtw89/phy.c
> @@ void rtw89_phy_ra_update_sta_link(struct rtw89_dev *rtwdev,
>   struct rtw89_ra_info *ra = &rtwsta_link->ra;
> + bool rssi_unknown = ra->rssi_unknown;
>   struct ieee80211_link_sta *link_sta;
> @@
>   rcu_read_unlock();
> 
> + ra->init_rate_lv = rtw89_phy_ra_get_init_rate_lv(rtwsta_link);
> + if (rssi_unknown && ra->init_rate_lv)
> + ra->upd_all = 1;
> + else
> + ra->rssi_unknown = rssi_unknown;
>   if (changed & IEEE80211_RC_SUPP_RATES_CHANGED)
>   ra->upd_mask = 1;
> @@ void rtw89_phy_ra_assoc(struct rtw89_dev *rtwdev, struct
> rtw89_sta_link *rtwsta_link)
>   ra->init_rate_lv = rtw89_phy_ra_get_init_rate_lv(rtwsta_link);
> + ra->rssi_unknown = !ra->init_rate_lv;
>   ra->upd_all = 1;
> 
> Another option is to keep the flag in struct rtw89_sta_link, as in v3.
> Then the memset does not clear it, and the RA update does not need the
> local variable. Which one do you prefer? I will test the change on
> RTL8912AU before I send v4.

Please check my opinion. If that is not good, maybe we can reconsider v3.