RE: [PATCH v3 4/5] rtw88: add set_bitrate_mask support

[Tony Chuang <yhchuang@xxxxxxxxxxx>]

> [mailto:linux-wireless-owner@xxxxxxxxxxxxxxx] On Behalf
> > Of Chris Chiu
> > Sent: Tuesday, October 22, 2019 8:49 PM
> > To: Tony Chuang
> > Cc: Kalle Valo; linux-wireless; Brian Norris
> > Subject: Re: [PATCH v3 4/5] rtw88: add set_bitrate_mask support
> >
> > On Tue, Oct 22, 2019 at 6:04 PM <yhchuang@xxxxxxxxxxx> wrote:
> > >
> > > From: Tzu-En Huang <tehuang@xxxxxxxxxxx>
> > >
> > > Support setting bit rate from upper layer.
> > > After configuring the original rate control result in the driver, the
> > > result is then masked by the bit rate mask received from the ops
> > > set_bitrate_mask. Lastly, the masked result will be sent to firmware.
> > >
> > > Signed-off-by: Tzu-En Huang <tehuang@xxxxxxxxxxx>
> > > Signed-off-by: Yan-Hsuan Chuang <yhchuang@xxxxxxxxxxx>
> > > ---
> > >
> > > v1 -> v2
> > >   * No change
> > >
> > > v2 -> v3
> > >   * use u64_encode_bits
> > >
> > >  drivers/net/wireless/realtek/rtw88/mac80211.c | 53 +++++++++++++
> > >  drivers/net/wireless/realtek/rtw88/main.c     | 78
> +++++++++++++++----
> > >  drivers/net/wireless/realtek/rtw88/main.h     |  3 +
> > >  3 files changed, 118 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/realtek/rtw88/mac80211.c
> > b/drivers/net/wireless/realtek/rtw88/mac80211.c
> > > index bc04cc280a96..2247bd61e716 100644
> > > --- a/drivers/net/wireless/realtek/rtw88/mac80211.c
> > > +++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
> > > @@ -684,6 +686,56 @@ static void rtw_ops_flush(struct ieee80211_hw
> *hw,
> > >         mutex_unlock(&rtwdev->mutex);
> > >  }
> > >
> > > +struct rtw_iter_bitrate_mask_data {
> > > +       struct rtw_dev *rtwdev;
> > > +       struct ieee80211_vif *vif;
> > > +       const struct cfg80211_bitrate_mask *mask;
> > > +};
> > > +
> > > +static void rtw_ra_mask_info_update_iter(void *data, struct
> ieee80211_sta *sta)
> > > +{
> > > +       struct rtw_iter_bitrate_mask_data *br_data = data;
> > > +       struct rtw_sta_info *si = (struct rtw_sta_info *)sta->drv_priv;
> > > +
> > > +       if (si->vif != br_data->vif)
> > > +               return;
> > > +
> > > +       /* free previous mask setting */
> > > +       kfree(si->mask);
> >
> > You may want to do NULL check for si->mask before kfree.
> 
> kfree checks NULL by itself, and checkpatch also warns this kind of needless
> checking.
> 
> >
> > > +       si->mask = kmemdup(br_data->mask, sizeof(struct
> cfg80211_bitrate_mask),
> > > +                          GFP_ATOMIC);
> > > +       if (!si->mask) {
> > > +               si->use_cfg_mask = false;
> > > +               return;
> > > +       }
> > > +
> > > +       si->use_cfg_mask = true;
> > > +       rtw_update_sta_info(br_data->rtwdev, si);
> > > +}
> > > +
> > > diff --git a/drivers/net/wireless/realtek/rtw88/main.c
> b/drivers/net/wireless/realtek/rtw88/main.c
> > > index 47e74f0aec06..e53143132a9b 100644
> > > --- a/drivers/net/wireless/realtek/rtw88/main.c
> > > +++ b/drivers/net/wireless/realtek/rtw88/main.c
> > > @@ -612,12 +612,71 @@ static u8 get_rate_id(u8 wireless_set, enum
> rtw_bandwidth bw_mode, u8 tx_num)
> > >  #define RA_MASK_OFDM_IN_HT_2G  0x00010
> > >  #define RA_MASK_OFDM_IN_HT_5G  0x00030
> > >
> > > +static u64 rtw_update_rate_mask(struct rtw_dev *rtwdev,
> > > +                               struct rtw_sta_info *si,
> > > +                               u64 ra_mask, bool is_vht_enable,
> > > +                               u8 wireless_set)
> > > +{
> > > +       struct rtw_hal *hal = &rtwdev->hal;
> > > +       const struct cfg80211_bitrate_mask *mask = si->mask;
> > > +       u64 cfg_mask = GENMASK(63, 0);
> > > +       u8 rssi_level, band;
> > > +
> > > +       if (wireless_set != WIRELESS_CCK) {
> > > +               rssi_level = si->rssi_level;
> > > +               if (rssi_level == 0)
> > > +                       ra_mask &= 0xffffffffffffffffULL;
> > > +               else if (rssi_level == 1)
> > > +                       ra_mask &= 0xfffffffffffffff0ULL;
> > > +               else if (rssi_level == 2)
> > > +                       ra_mask &= 0xffffffffffffefe0ULL;
> > > +               else if (rssi_level == 3)
> > > +                       ra_mask &= 0xffffffffffffcfc0ULL;
> > > +               else if (rssi_level == 4)
> > > +                       ra_mask &= 0xffffffffffff8f80ULL;
> > > +               else if (rssi_level >= 5)
> > > +                       ra_mask &= 0xffffffffffff0f00ULL;
> > > +       }
> >
> > Would be better to enumerate rssi_level instead of 0 to 5. Does level
> > 0 means bad rssi or good?
> >

I think 0 is the lowest level. And I think we don't need to add an enum for
this kind of levels, as the number just presents the meaning of it.

Yan-Hsuan