Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> wrote:
> On 08/11/2024 04:40, Ping-Ke Shih wrote:
> > Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> wrote:
> >>>
> >> USB RX aggregation improves the RX speed on certain ARM systems, like
> >> the NanoPi NEO Core2. With RTL8811AU, before: 30 Mbps, after: 224 Mbps.
> >>
> >> The out-of-tree driver uses aggregation size of 7 in USB 3 mode, but
> >> that doesn't work here. rtw88 advertises support for receiving AMSDU
> >> in AMPDU, so the AP sends larger frames, up to ~5100 bytes. With a size
> >> of 7 RTL8812AU frequently tries to aggregate more frames than will fit
> >> in 32768 bytes. Use a size of 6 instead.
> >>
> >> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx>
> >> ---
> >> drivers/net/wireless/realtek/rtw88/usb.c | 30 ++++++++++++++++++++++++
> >> 1 file changed, 30 insertions(+)
> >>
> >> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> >> index 752bca05b9af..9172af63500b 100644
> >> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> >> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> >> @@ -790,6 +790,32 @@ static void rtw_usb_dynamic_rx_agg_v1(struct rtw_dev *rtwdev, bool enable)
> >> rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16);
> >> }
> >>
> >> +static void rtw_usb_dynamic_rx_agg_v2(struct rtw_dev *rtwdev, bool enable)
> >> +{
> >> + struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
> >> + u8 size, timeout;
> >> + u16 val16;
> >> +
> >
> > How about a shortcut?
> >
> > if (!enable) {
> > size = 0x0;
> > timeout = 0x1;
> >
> > goto rx_agg;
> > }
> >
> >> + if (rtwusb->udev->speed == USB_SPEED_SUPER) {
> >> + size = 0x6;
> >> + timeout = 0x1a;
> >> + } else {
> >> + size = 0x5;
> >> + timeout = 0x20;
> >> + }
> >> +
> >> + if (!enable) {
> >> + size = 0x0;
> >> + timeout = 0x1;
> >> + }
> >> +
> >
> > rx_agg:
> >
> >> + val16 = u16_encode_bits(size, BIT_RXDMA_AGG_PG_TH) |
> >> + u16_encode_bits(timeout, BIT_DMA_AGG_TO_V1);
> >> +
> >> + rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, val16);
> >> + rtw_write8_set(rtwdev, REG_TXDMA_PQ_MAP, BIT_RXDMA_AGG_EN);
> >> +}
>
> Hmm, I don't like it.
Honestly, I don't like it so much.
> What about this?
>
> if (!enable) {
> size = 0x0;
> timeout = 0x1;
> } else if (rtwusb->udev->speed == USB_SPEED_SUPER) {
> size = 0x6;
> timeout = 0x1a;
> } else {
> size = 0x5;
> timeout = 0x20;
> }
I thought about this too, but a flaw is first condition is for !enable
and later two conditions are for enable.
I think the logic below is clear, but more indent though.
if (!enable) {
size = 0x0;
timeout = 0x1;
} else {
if (rtwusb->udev->speed == USB_SPEED_SUPER) {
size = 0x6;
timeout = 0x1a;
} else {
size = 0x5;
timeout = 0x20;
}
}
Mine, yours and my last proposal are fine to me. You can decide to take
one of them.
