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

[Pkshih <pkshih@xxxxxxxxxxx>]

> -----Original Message-----
> From: linux-wireless-owner@xxxxxxxxxxxxxxx [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?
> 
> > --
> > 2.17.1
> >
> 
> ------Please consider the environment before printing this e-mail.