Re: [PATCH rtw-next v2] wifi: rtw88: usb: Download the reserved page synchronously

[Bitterblue Smith <rtl8821cerfe2@gmail.com>]

On 07/09/2026 06:39, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>> From: Mohammed Afifi <i.mafifi17@gmail.com>
>>
>> In AP mode the reserved page (which contains the beacon) is downloaded
>> to the chip by rtw_fw_write_data_rsvd_page(). That function writes the
>> data via rtw_hci_write_data_rsvd_page() and then immediately polls the
>> hardware BCN_VALID bit to confirm the firmware accepted the beacon.
> 
> It looks like downloading reserved page uses usb_submit_urb() and
> read/write IO uses usb_control_msg(). Do they go via different USB pipes?
> 
> (I just want to know more USB)
> 

I think control messages have their own pipe. The reserved page and other
bulk transfers go through the bulk out pipes. (usb_submit_urb() can send
control messages too.)

>>
>> On USB, rtw_usb_write_data_rsvd_page() ends up in rtw_usb_write_port(),
>> which submits the URB with usb_submit_urb() and returns as soon as the
>> transfer is queued. The completion callback only frees the skb. As a
>> result the BCN_VALID poll can run while the beacon is still in flight
>> on the bus, and the poll times out even though the download itself is
>> fine.
>>
>> This shows up as:
>>
>>   rtw_8822bu: error beacon valid
>>   rtw_8822bu: failed to download drv rsvd page
>>
>> The failure is intermittent because the poll retries sometimes cover
>> the transfer time. It occurs in bursts when something triggers repeated
>> beacon updates, such as stations associating and disassociating.
>>
>> Fix this by adding a synchronous bulk-out helper that waits for the URB
>> to complete, and using it for the reserved page download. Only the
>> reserved page / beacon path is made synchronous; the normal data TX
>> path is left untouched, so throughput is unaffected.
>>
>> The reserved page download runs in process context under rtwdev->mutex,
>> so sleeping there is safe.
>>
>> Closes: https://github.com/lwfinger/rtw88/issues/451
>> Signed-off-by: Mohammed Afifi <i.mafifi17@gmail.com>
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> ---
>> v2:
>>  - Fix compilation error in rtw_usb_write_data_rsvd_page().
>> ---
>>  drivers/net/wireless/realtek/rtw88/usb.c | 91 +++++++++++++++++++++++-
>>  1 file changed, 90 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
>> index c90802919473..d31297954f22 100644
>> --- a/drivers/net/wireless/realtek/rtw88/usb.c
>> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
>> @@ -394,6 +394,71 @@ static int rtw_usb_write_port(struct rtw_dev *rtwdev, u8 qsel, struct sk_buff *s
>>         return ret;
>>  }
>>
>> +struct rtw_usb_write_data_sync_cb {
>> +       struct completion done;
>> +       int status;
>> +};
>> +
>> +static void rtw_usb_write_port_sync_complete(struct urb *urb)
>> +{
>> +       struct rtw_usb_write_data_sync_cb *cb = urb->context;
>> +
>> +       cb->status = urb->status;
>> +       complete(&cb->done);
>> +}
>> +
>> +/* Synchronous bulk-out that returns only after the data has actually been
> 
> First line of comment block should be empty, but I'd remove this commet.
> (see below)
> 
>> + * transferred to the device. Required for the reserved-page / beacon
>> + * download: the caller polls the hardware BCN_VALID bit immediately after
>> + * this returns, so the bytes must be on the chip by then. The normal async
>> + * TX path races that poll, which shows up as intermittent
>> + * "error beacon valid" / "failed to download drv rsvd page" in AP mode,
>> + * more often on 5 GHz where the larger beacon takes longer to transfer.
>> + */
>> +static int rtw_usb_write_port_sync(struct rtw_dev *rtwdev, u8 qsel,
>> +                                  struct sk_buff *skb)
>> +{
>> +       struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
>> +       struct usb_device *usbd = rtwusb->udev;
>> +       struct rtw_usb_write_data_sync_cb cb;
>> +       int ep = qsel_to_ep(rtwusb, qsel);
>> +       unsigned int pipe;
>> +       struct urb *urb;
>> +       int ret;
>> +
>> +       if (ep < 0)
>> +               return ep;
>> +
>> +       urb = usb_alloc_urb(0, GFP_KERNEL);
>> +       if (!urb)
>> +               return -ENOMEM;
>> +
>> +       init_completion(&cb.done);
>> +       cb.status = -EINPROGRESS;
>> +
>> +       pipe = usb_sndbulkpipe(usbd, rtwusb->out_ep[ep]);
>> +       usb_fill_bulk_urb(urb, usbd, pipe, skb->data, skb->len,
>> +                         rtw_usb_write_port_sync_complete, &cb);
>> +       urb->transfer_flags |= URB_ZERO_PACKET;
>> +
>> +       ret = usb_submit_urb(urb, GFP_KERNEL);
>> +       if (ret)
>> +               goto out;
>> +
>> +       /* 5 s matches the vendor driver's bulk-out timeout */
>> +       if (!wait_for_completion_timeout(&cb.done, msecs_to_jiffies(5000))) {
>> +               usb_kill_urb(urb);
>> +               ret = -ETIMEDOUT;
>> +       } else {
>> +               ret = cb.status;
>> +       }
>> +
>> +out:
>> +       usb_free_urb(urb);
>> +
>> +       return ret;
>> +}
>> +
>>  static bool rtw_usb_tx_agg_skb(struct rtw_usb *rtwusb, struct sk_buff_head *list)
>>  {
>>         struct rtw_dev *rtwdev = rtwusb->rtwdev;
>> @@ -543,13 +608,37 @@ static int rtw_usb_write_data_rsvd_page(struct rtw_dev *rtwdev, u8 *buf,
>>  {
>>         const struct rtw_chip_info *chip = rtwdev->chip;
>>         struct rtw_tx_pkt_info pkt_info = {0};
>> +       struct sk_buff *skb;
>> +       int ret;
>>
>>         pkt_info.tx_pkt_size = size;
>>         pkt_info.qsel = TX_DESC_QSEL_BEACON;
>>         pkt_info.offset = chip->tx_pkt_desc_sz;
>>         pkt_info.ls = true;
>>
> 
> I feel you can remove comment above rtw_usb_write_port_sync(), and just have
> a simple comment here to describe using _sync version of
> rtw_usb_write_port_sync(), because callers check BCN_VALID immediately. 
> 
>> -       return rtw_usb_write_data(rtwdev, &pkt_info, buf);
>> +       skb = dev_alloc_skb(chip->tx_pkt_desc_sz + size);
>> +       if (!skb)
>> +               return -ENOMEM;
>> +
>> +       skb_reserve(skb, chip->tx_pkt_desc_sz);
>> +       skb_put_data(skb, buf, size);
>> +       skb_push(skb, chip->tx_pkt_desc_sz);
>> +       memset(skb->data, 0, chip->tx_pkt_desc_sz);
>> +       rtw_tx_fill_tx_desc(rtwdev, &pkt_info, skb);
>> +       rtw_tx_fill_txdesc_checksum(rtwdev, &pkt_info, skb->data);
>> +
>> +       /* Download the beacon/reserved page synchronously so that the caller's
>> +        * subsequent BCN_VALID poll observes the completed transfer instead of
>> +        * racing the async TX path.
>> +        */
>> +       ret = rtw_usb_write_port_sync(rtwdev, pkt_info.qsel, skb);
>> +       if (ret)
>> +               rtw_err(rtwdev, "failed to download rsvd page over USB, ret=%d\n",
>> +                       ret);
>> +
>> +       dev_kfree_skb_any(skb);
>> +
>> +       return ret;
> 
> Can we move this chunk into a function like rtw_usb_write_data_sync()?
> Or is it possible to extend arguments of rtw_usb_write_data()? Because they
> do many similar things.
> 
> But I don't ask to extend rtw_usb_write_port(), because
> rtw_usb_write_port_sync() is quite different; at least no many driver-specific
> settings.
> 

Looking at this more closely, I think rtw_usb_write_port() can be used
directly, with just a few changes in rtw_usb_write_data() and
rtw_usb_write_port_complete(). No need to add rtw_usb_write_port_sync()
and rtw_usb_write_port_sync_complete(), or to duplicate all that code
in rtw_usb_write_data_rsvd_page().

>>  }
>>
>>  static int rtw_usb_write_data_h2c(struct rtw_dev *rtwdev, u8 *buf, u32 size)
>> --
>> 2.55.0
>