Re: [RFC PATCH v3 2/4] wifi: rtl8xxxu: handle rate control for 8188e a per mac_id

["Georg Müller" <georgmueller@gmx.net>]

Am 16.03.26 um 07:35 schrieb Ping-Ke Shih:
> Georg Müller <georgmueller@gmx.net> wrote:
>> convert member ra_info to an array with one entry per mac id. This
>> allows having different rate control settings per connected station
>> in ap mode.
>>
>> The max_macid_num is conservative. The old driver used 32 [1], some
>> other sources said 64 [2]. I have not enough adapters to test any of the
>> higher values. Given the usage of this chipset in nano dongles, I think
>> the 16 might be high enough.
>>
>> [1]
>> https://github.com/lwfinger/rtl8188eu/blob/f5d1c8df2e2d8b217ea0113bf2cf3e37d
>> f8cb716/include/sta_info.h#L28
>> [2]
>> https://lore.kernel.org/linux-wireless/27e83382-4c84-1841-c428-d1e5143ea20c@
>> gmail.com/
>>
>> Signed-off-by: Georg Müller <georgmueller@gmx.net>
>> ---
>>   drivers/net/wireless/realtek/rtl8xxxu/8188e.c | 12 ++++++------
>>   drivers/net/wireless/realtek/rtl8xxxu/core.c  | 19 ++++++++++++++-----
>>   .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  3 ++-
>>   3 files changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/8188e.c
>> b/drivers/net/wireless/realtek/rtl8xxxu/8188e.c
>> index de2837a91bbe..607ca62194fc 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/8188e.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/8188e.c
>> @@ -1468,9 +1468,8 @@ static void rtl8188e_reset_ra_counter(struct
>> rtl8xxxu_ra_info *ra)
>>          ra->nsc_down = (n_threshold_high[rate_id] + n_threshold_low[rate_id])
>>>> 1;
>>   }
>>
>> -static void rtl8188e_rate_decision(struct rtl8xxxu_ra_info *ra)
>> +static void rtl8188e_rate_decision(struct rtl8xxxu_priv *priv, struct
>> rtl8xxxu_ra_info *ra)
>>   {
>> -       struct rtl8xxxu_priv *priv = container_of(ra, struct rtl8xxxu_priv,
>> ra_info);
>>          const u8 *retry_penalty_idx_0;
>>          const u8 *retry_penalty_idx_1;
>>          const u8 *retry_penalty_up_idx;
>> @@ -1669,7 +1668,7 @@ void rtl8188e_handle_ra_tx_report2(struct rtl8xxxu_priv
>> *priv, struct sk_buff *s
>>          u32 *_rx_desc = (u32 *)(skb->data - sizeof(struct rtl8xxxu_rxdesc16));
>>          struct rtl8xxxu_rxdesc16 *rx_desc = (struct rtl8xxxu_rxdesc16
>> *)_rx_desc;
>>          struct device *dev = &priv->udev->dev;
>> -       struct rtl8xxxu_ra_info *ra = &priv->ra_info;
>> +       struct rtl8xxxu_ra_info *ra;
>>          u32 tx_rpt_len = rx_desc->pktlen & 0x3ff;
>>          u32 items = tx_rpt_len / TX_RPT2_ITEM_SIZE;
> 
> The 'items' should be smaller or equal to RTL8188E_MAX_MAC_ID_NUM.

I will add a check.

>>          u64 macid_valid = ((u64)_rx_desc[5] << 32) | _rx_desc[4];
>> @@ -1688,6 +1687,7 @@ void rtl8188e_handle_ra_tx_report2(struct rtl8xxxu_priv
>> *priv, struct sk_buff *s
>>
>>          for (macid = 0; macid < items; macid++) {
>>                  valid = false;
>> +               ra = &priv->ra_info[macid];
>>
>>                  if (macid < 64)
>>                          valid = macid_valid & BIT(macid);
>> @@ -1704,7 +1704,7 @@ void rtl8188e_handle_ra_tx_report2(struct rtl8xxxu_priv
>> *priv, struct sk_buff *s
>>
>>                          if (ra->total > 0) {
>>                                  if (ra->ra_stage < 5)
>> -                                       rtl8188e_rate_decision(ra);
>> +                                       rtl8188e_rate_decision(priv, ra);
>>                                  else if (ra->ra_stage == 5)
>>
>> rtl8188e_power_training_try_state(ra);
>>                                  else /* ra->ra_stage == 6 */
>> @@ -1781,7 +1781,7 @@ rtl8188e_update_rate_mask(struct rtl8xxxu_priv *priv,
>>                            u32 ramask, u8 rateid, int sgi, int txbw_40mhz,
>>                            u8 macid)
>>   {
>> -       struct rtl8xxxu_ra_info *ra = &priv->ra_info;
>> +       struct rtl8xxxu_ra_info *ra = &priv->ra_info[macid];
>>
>>          ra->rate_id = rateid;
>>          ra->rate_mask = ramask;
>> @@ -1792,7 +1792,7 @@ rtl8188e_update_rate_mask(struct rtl8xxxu_priv *priv,
>>
>>   static void rtl8188e_ra_set_rssi(struct rtl8xxxu_priv *priv, u8 macid, u8 rssi)
>>   {
>> -       priv->ra_info.rssi_sta_ra = rssi;
>> +       priv->ra_info[macid].rssi_sta_ra = rssi;
>>   }
>>
>>   void rtl8188e_ra_info_init_all(struct rtl8xxxu_ra_info *ra)
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/core.c
>> b/drivers/net/wireless/realtek/rtl8xxxu/core.c
>> index 794187d28caa..5ad23c5c9305 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/core.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/core.c
>> @@ -3921,6 +3921,7 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw)
>>          struct device *dev = &priv->udev->dev;
>>          struct rtl8xxxu_fileops *fops = priv->fops;
>>          bool macpower;
>> +       u16 mac_id;
>>          int ret;
>>          u8 val8;
>>          u16 val16;
>> @@ -4393,9 +4394,16 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw)
>>                  priv->cfo_tracking.crystal_cap = priv->default_crystal_cap;
>>          }
>>
>> -       if (priv->rtl_chip == RTL8188E)
>> -               rtl8188e_ra_info_init_all(&priv->ra_info);
>> -
>> +       if (priv->rtl_chip == RTL8188E) {
>> +               priv->ra_info = kmalloc_array(RTL8188E_MAX_MAC_ID_NUM,
>> sizeof(*priv->ra_info), GFP_KERNEL);
> 
> How about devm_ series? Then, no need to free on error path or disconnection.

I didn't know about these and will change it in the next version.

>> +               if (!priv->ra_info) {
>> +                       ret = -ENOMEM;
>> +                       goto exit;
>> +               }
>> +               for (mac_id = 0; mac_id < RTL8188E_MAX_MAC_ID_NUM; mac_id++) {
>> +                       rtl8188e_ra_info_init_all(&priv->ra_info[mac_id]);
>> +               }
> 
> no need brace for single line statement.

I will change it in the next version.

>> +       }
>>          set_bit(RTL8XXXU_BC_MC_MACID, priv->mac_id_map);
>>          set_bit(RTL8XXXU_BC_MC_MACID1, priv->mac_id_map);
>>
>> @@ -5338,7 +5346,7 @@ rtl8xxxu_fill_txdesc_v3(struct ieee80211_hw *hw, struct
>> ieee80211_hdr *hdr,
>>   {
>>          struct rtl8xxxu_priv *priv = hw->priv;
>>          struct device *dev = &priv->udev->dev;
>> -       struct rtl8xxxu_ra_info *ra = &priv->ra_info;
>> +       struct rtl8xxxu_ra_info *ra = &priv->ra_info[macid];
>>          u8 *qc = ieee80211_get_qos_ctl(hdr);
>>          u8 tid = qc[0] & IEEE80211_QOS_CTL_TID_MASK;
>>          u32 rate = 0;
>> @@ -7895,6 +7903,7 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
>>   err_set_intfdata:
>>          usb_set_intfdata(interface, NULL);
>>
>> +       kfree(priv->ra_info);
>>          kfree(priv->fw_data);
>>          mutex_destroy(&priv->usb_buf_mutex);
>>          mutex_destroy(&priv->syson_indirect_access_mutex);
>> @@ -7924,7 +7933,7 @@ static void rtl8xxxu_disconnect(struct usb_interface
>> *interface)
>>          usb_set_intfdata(interface, NULL);
>>
>>          dev_info(&priv->udev->dev, "disconnecting\n");
>> -
>> +       kfree(priv->ra_info);
>>          kfree(priv->fw_data);
>>          mutex_destroy(&priv->usb_buf_mutex);
>>          mutex_destroy(&priv->syson_indirect_access_mutex);
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
>> index 4a744b5c1aec..9ce820ff4793 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
>> @@ -76,6 +76,7 @@
>>   #define RTL_FW_PAGE_SIZE               4096
>>   #define RTL8XXXU_FIRMWARE_POLL_MAX     1000
>>
>> +#define RTL8188E_MAX_MAC_ID_NUM                16
>>   #define RTL8723A_CHANNEL_GROUPS                3
>>   #define RTL8723A_MAX_RF_PATHS          2
>>   #define RTL8723B_CHANNEL_GROUPS                6
>> @@ -1915,7 +1916,7 @@ struct rtl8xxxu_priv {
>>          struct rtl8xxxu_btcoex bt_coex;
>>          struct rtl8xxxu_ra_report ra_report;
>>          struct rtl8xxxu_cfo_tracking cfo_tracking;
>> -       struct rtl8xxxu_ra_info ra_info;
>> +       struct rtl8xxxu_ra_info *ra_info;
> 
> It looks like this is also 8188e specific. Could you add comment by the way?
> (but please /**/ style)

ok

>>          u8 dynamic_tx_rpt_timing_counter; // 8188e specific
>>
>>          bool led_registered;
>> --
>> 2.53.0
>>
>