Received-SPF: pass (google.com: domain of linux-wireless+bounces-8366-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <6c341295-0f2b-4b8d-af6d-4e2746e56ddc@gmail.com>
Date: Sat, 1 Jun 2024 00:10:26 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: rtw88: usb: Further limit the TX aggregation
To: Ping-Ke Shih <pkshih@realtek.com>,
 "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc: Sascha Hauer <sha@pengutronix.de>
References: <0996d2d0-e7b8-4e43-ba12-63074ba9df1b@gmail.com>
 <c1f41ca8bd7d49e4801d620089ae0a4d@realtek.com>
Content-Language: en-US
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
In-Reply-To: <c1f41ca8bd7d49e4801d620089ae0a4d@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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.