Re: [PATCH wireless-next v2 6/7] wifi: rt2800usb: enable RX bulk aggregation

[Stanislaw Gruszka <stf_xl@wp.pl>]

On Fri, Nov 06, 2026 at 01:18:19PM +0800, Jie Feng wrote:
> On Thu, 5 Nov 2026 at 16:10, Stanislaw Gruszka <stf_xl@wp.pl> wrote:
> >
> > On Sun, Nov 01, 2026 at 12:51:13AM +0800, Ho Jie Feng wrote:
> > > Enable RX bulk aggregation on high speed USB, to receive more
> > > frames per URB.
> > >
> > > RX buffers are now 24KB, so reduce it to 16 buffers instead of 128,
> > > so that they take 512 KB in total instead of 4 MB.
> > >
> > > Test results:
> > >    before after
> > > TX 30.5   30.5
> > > RX 67.8   80.9
>  ...
> > > --- a/drivers/net/wireless/ralink/rt2x00/rt2800usb.c
> > > +++ b/drivers/net/wireless/ralink/rt2x00/rt2800usb.c
> > > @@ -301,15 +301,15 @@ static int rt2800usb_enable_radio(struct rt2x00_dev *rt2x00dev)
> > >               return -EIO;
> > >
> > >       rt2x00_set_field32(&reg, USB_DMA_CFG_PHY_CLEAR, 0);
> > > -     rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_EN, 0);
> > > +     rt2x00_set_field32(&reg, USB_DMA_CFG_RX_BULK_AGG_EN,
> > > +                        rt2x00dev->rx->usb_maxpacket == 512);
> >
> > Any idea why this condition ? We can not have aggregation on slower
> > or faster USB?
> usb_maxpacket is 512 for high-speed and 64 for full-speed, and I don't think
> rt2800usb has any superspeed devices? In that case I could make it into
> a maxpacket >= 512 check instead for robustness.
> 
> Both the ralink drivers and mt7601 disable it on full speed usb. My best guess
> would be that waiting for a large aggregated buffer over full speed is slow
> and increases latency.
> 
> https://github.com/boundarydevices/rt5572/blob/master/common/cmm_mac_usb.c#L1333

Ok, so this is fine.

> ...
> > > +static unsigned int rt2800usb_get_rx_seg_len(void *data, unsigned int len)
> > > +{
> > > +     unsigned int min_seg_len = RXINFO_DESC_SIZE + RXWI_DESC_SIZE_4WORDS +
> > > +                                RXD_DESC_SIZE;
> > > +     unsigned int rx_pkt_len;
> > > +     u32 word;
> > > +
> > > +     if (len < min_seg_len)
> > > +             return 0;
> > > +
> > > +     word = rt2x00_desc_read(data, 0);
> > > +     rx_pkt_len = rt2x00_get_field32(word, RXINFO_W0_USB_DMA_RX_PKT_LEN);
> > > +
> > > +     if (!rx_pkt_len ||
> > > +         RXINFO_DESC_SIZE + rx_pkt_len + RXD_DESC_SIZE > len ||
> > > +         rx_pkt_len & 0x3)
> >
> > This is indication of we do something wrong, similar when len < min_seg_len.
> > Maybe wrap those with WARN_ON_ONCE() ?
> >
> Should I put one WARN_ON_ONCE for the full condition, or wrap it individually
> Wrapping it individually might be easier for debugging if someone
> sends their dmesg.

For this condition, I would add one WARN_ON_ONCE for rx_pkt_len non-zero
and alignment validity, and another one for <= len validity.

> > >
> > > +
> > > +     return RXINFO_DESC_SIZE + rx_pkt_len + RXD_DESC_SIZE;
> > > +}
> > > +
> > >  static void rt2800usb_fill_rxdone(struct queue_entry *entry,
> > >                                 struct rxdone_entry_desc *rxdesc)
> > >  {
> > > @@ -706,6 +727,7 @@ static const struct rt2x00lib_ops rt2800usb_rt2x00_ops = {
> > >       .clear_beacon           = rt2800_clear_beacon,
> > >       .get_tx_data_len        = rt2800usb_get_tx_data_len,
> > >       .fill_rxdone            = rt2800usb_fill_rxdone,
> > > +     .get_rx_seg_len         = rt2800usb_get_rx_seg_len,
> > >       .config_shared_key      = rt2800_config_shared_key,
> > >       .config_pairwise_key    = rt2800_config_pairwise_key,
> > >       .config_filter          = rt2800_config_filter,
> > > @@ -725,8 +747,8 @@ static void rt2800usb_queue_init(struct data_queue *queue)
> > >
> > >       switch (queue->qid) {
> > >       case QID_RX:
> > > -             queue->limit = 128;
> > > -             queue->data_size = AGGREGATION_SIZE;
> > > +             queue->limit = 16;
> > > +             queue->data_size = MAX_RXBULK_SIZE;
> >
> > Since we enable aggregation conditionally, this probably should also be
> > conditional. But the issue is that we do not have
> > rt2x00dev->rx->usb_maxpacket value available yet.
> Due to full-speed USB being 40x slower than high speed, I think
> the reduction in queue limit might not be a problem.

Ok, it makes sense then do change this unconditionally.

> > We could just enable aggregation unconditionally,
> > but I'm not sure it will work with some older chips like RT2870?
> On my tests it works on RT3070 too, but I don't have any others to
> test. The ralink driver also does it for all chips too.

I tested on RT3071, RT5370, RT5572. I was worried about older
chips, but looked at old staging driver for RT2860 and it enables
USB aggregation too:
https://github.com/torvalds/linux/blob/v2.6.35/drivers/staging/rt2860/common/cmm_mac_usb.c#L738

So we are fine to do this change.

Regards
Stanislaw