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

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

[Bitterblue Smith]

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.

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
+ * 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;
 
-	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;
 }
 
 static int rtw_usb_write_data_h2c(struct rtw_dev *rtwdev, u8 *buf, u32 size)
-- 
2.55.0

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

[Ping-Ke Shih]

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)

> 
> 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.

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

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

[Bitterblue Smith]

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
>