Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191022100420.25116-1-yhchuang@realtek.com> <20191022100420.25116-5-yhchuang@realtek.com>
In-Reply-To: <20191022100420.25116-5-yhchuang@realtek.com>
From:   Chris Chiu <chiu@endlessm.com>
Date:   Tue, 22 Oct 2019 20:49:26 +0800
Message-ID: <CAB4CAwcBUHMR3btH9LGd=GzqU1mRmfmL0o4EOY4E8H1OxfpvMg@mail.gmail.com>
Subject: Re: [PATCH v3 4/5] rtw88: add set_bitrate_mask support
To:     Tony Chuang <yhchuang@realtek.com>
Cc:     Kalle Valo <kvalo@codeaurora.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        Brian Norris <briannorris@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Tue, Oct 22, 2019 at 6:04 PM <yhchuang@realtek.com> wrote:
>
> From: Tzu-En Huang <tehuang@realtek.com>
>
> 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@realtek.com>
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
> ---
>
> 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.

> +       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
>