Re: [PATCH] wifi: rtw88: usb: Further limit the TX aggregation

Bitterblue Smith
2024-05-31T21:10:30Z

On 30/05/2024 09:36, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>> Currently the number of frames sent to the chip in a single USB Request
>> Block is limited only by the size of the TX buffer, which is 20 KiB.
>> Testing reveals that as many as 13 frames get aggregated. This is more
>> than what any of the chips would like to receive. RTL8822CU, RTL8822BU,
>> and RTL8821CU want at most 3 frames, and RTL8723DU wants only 1 frame
>> per URB.
>>
>> RTL8723DU in particular reliably malfunctions during a speed test. All
>> traffic seems to stop. Pinging the AP no longer works.
>>
>> Fix this problem by limiting the number of frames sent to the chip in a
>> single URB according to what each chip likes.
>>
>> Also configure RTL8822CU, RTL8822BU, and RTL8821CU to expect 3 frames
>> per URB.
>>
>> RTL8703B may or may not be found in USB devices. Declare that it wants
>> only 1 frame per URB, just in case.
>>
>> Tested with RTL8723DU and RTL8811CU.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> ---
>> drivers/net/wireless/realtek/rtw88/mac.c | 12 ++++++++++++
>> drivers/net/wireless/realtek/rtw88/main.h | 2 ++
>> drivers/net/wireless/realtek/rtw88/reg.h | 1 +
>> drivers/net/wireless/realtek/rtw88/rtw8703b.c | 1 +
>> drivers/net/wireless/realtek/rtw88/rtw8723d.c | 1 +
>> drivers/net/wireless/realtek/rtw88/rtw8821c.c | 1 +
>> drivers/net/wireless/realtek/rtw88/rtw8822b.c | 1 +
>> drivers/net/wireless/realtek/rtw88/rtw8822c.c | 1 +
>> drivers/net/wireless/realtek/rtw88/usb.c | 4 +++-
>> 9 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtw88/mac.c b/drivers/net/wireless/realtek/rtw88/mac.c
>> index 0dba8aae7716..fca90759c268 100644
>> --- a/drivers/net/wireless/realtek/rtw88/mac.c
>> +++ b/drivers/net/wireless/realtek/rtw88/mac.c
>> @@ -1186,6 +1186,7 @@ static int __priority_queue_cfg(struct rtw_dev *rtwdev,
>> {
>> const struct rtw_chip_info *chip = rtwdev->chip;
>> struct rtw_fifo_conf *fifo = &rtwdev->fifo;
>> + u8 val8;
>>
>> rtw_write16(rtwdev, REG_FIFOPAGE_INFO_1, pg_tbl->hq_num);
>> rtw_write16(rtwdev, REG_FIFOPAGE_INFO_2, pg_tbl->lq_num);
>> @@ -1201,6 +1202,17 @@ static int __priority_queue_cfg(struct rtw_dev *rtwdev,
>> rtw_write16(rtwdev, REG_FIFOPAGE_CTRL_2 + 2, fifo->rsvd_boundary);
>> rtw_write16(rtwdev, REG_BCNQ1_BDNY_V1, fifo->rsvd_boundary);
>> rtw_write32(rtwdev, REG_RXFF_BNDY, chip->rxff_size - C2H_PKT_BUF - 1);
>> +
>> + if (rtwdev->hci.type == RTW_HCI_TYPE_USB) {
>> + val8 = rtw_read8(rtwdev, REG_AUTO_LLT_V1);
>> + u8p_replace_bits(&val8, chip->usb_tx_agg_desc_num,
>> + BIT_MASK_BLK_DESC_NUM);
>> + rtw_write8(rtwdev, REG_AUTO_LLT_V1, val8);
>
> rtw_write8_mask(rtwdev, REG_AUTO_LLT_V1, BIT_MASK_BLK_DESC_NUM, chip->usb_tx_agg_desc_num);
>
>> +
>> + rtw_write8(rtwdev, REG_AUTO_LLT_V1 + 3, chip->usb_tx_agg_desc_num);
>> + rtw_write8_set(rtwdev, REG_TXDMA_OFFSET_CHK + 1, BIT(1));
>> + }
>> +
>> rtw_write8_set(rtwdev, REG_AUTO_LLT_V1, BIT_AUTO_INIT_LLT_V1);
>>
>> if (!check_hw_ready(rtwdev, REG_AUTO_LLT_V1, BIT_AUTO_INIT_LLT_V1, 0))
>> diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h
>> index 49894331f7b4..49a3fd4fb7dc 100644
>> --- a/drivers/net/wireless/realtek/rtw88/main.h
>> +++ b/drivers/net/wireless/realtek/rtw88/main.h
>> @@ -1197,6 +1197,8 @@ struct rtw_chip_info {
>> u16 fw_fifo_addr[RTW_FW_FIFO_MAX];
>> const struct rtw_fwcd_segs *fwcd_segs;
>>
>> + u8 usb_tx_agg_desc_num;
>> +
>
> Please keep order of field and instance declaration, like rtw8703b_hw_spec.
>
>> u8 default_1ss_tx_path;
>>
>> bool path_div_supported;
>> diff --git a/drivers/net/wireless/realtek/rtw88/reg.h b/drivers/net/wireless/realtek/rtw88/reg.h
>> index b122f226924b..572651b3aa4b 100644
>> --- a/drivers/net/wireless/realtek/rtw88/reg.h
>> +++ b/drivers/net/wireless/realtek/rtw88/reg.h
>> @@ -270,6 +270,7 @@
>> #define BIT_MASK_BCN_HEAD_1_V1 0xfff
>> #define REG_AUTO_LLT_V1 0x0208
>> #define BIT_AUTO_INIT_LLT_V1 BIT(0)
>> +#define BIT_MASK_BLK_DESC_NUM 0xf0
>
> 0xf0 --> GENMASK(7, 4)
>
>> #define REG_DWBCN0_CTRL 0x0208
>> #define BIT_BCN_VALID BIT(16)
>> #define REG_TXDMA_OFFSET_CHK 0x020C
>> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8703b.c
>> b/drivers/net/wireless/realtek/rtw88/rtw8703b.c
>> index 8919f9e11f03..222608de33cd 100644
>> --- a/drivers/net/wireless/realtek/rtw88/rtw8703b.c
>> +++ b/drivers/net/wireless/realtek/rtw88/rtw8703b.c
>> @@ -2013,6 +2013,7 @@ const struct rtw_chip_info rtw8703b_hw_spec = {
>> .tx_stbc = false,
>> .max_power_index = 0x3f,
>> .ampdu_density = IEEE80211_HT_MPDU_DENSITY_16,
>> + .usb_tx_agg_desc_num = 1, /* Not sure if this chip has USB interface */
>
> The position to declare this field is very different from others.
>
> Not sure when we messed up the order, but please don't feel free to add one,
> keep the order.
>
> If you have time, could you help to align all of them across chips?
> If not, I will take my time to do that after your patch.
>

Sure, I will do that.