On 19/11/2024 02:50, Ping-Ke Shih wrote: > Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> wrote: >> The USB driver uses four USB Request Blocks for RX. Before submitting >> one, it allocates a 32768 byte skb for the RX data. This allocation can >> fail, maybe due to temporary memory fragmentation. When the allocation >> fails, the corresponding URB is never submitted again. After four such >> allocation failures, all RX stops because the driver is not requesting >> data from the device anymore. >> >> Don't allocate a 32768 byte skb when submitting a USB Request Block >> (which happens very often). Instead preallocate 8 such skbs, and reuse >> them over and over. If all 8 are busy, allocate a new one. This is >> pretty rare. If the allocation fails, use a work to try again later. >> When there are enough free skbs again, free the excess skbs. >> >> Also, use WQ_BH for the RX workqueue. With a normal or high priority >> workqueue the skbs are processed too slowly when the system is even a >> little busy, like when opening a new page in a browser, and the driver >> runs out of free skbs and allocates a lot of new ones. >> >> Move C2H_ADAPTIVITY to the c2h workqueue instead of handling it directly >> from rtw_fw_c2h_cmd_rx_irqsafe(), which runs in the RX workqueue. It >> reads hardware registers, which is not "irqsafe" with USB. > > This part should be in a separated patch. > Do you mean just C2H_ADAPTIVITY or WQ_BH as well? >> >> This is more or less what the out-of-tree Realtek drivers do, except >> they use a tasklet instead of a BH workqueue. >> >> Tested with RTL8723DU, RTL8821AU, RTL8812AU, RTL8812BU, RTL8822CU, >> RTL8811CU. >> >> Closes: https://lore.kernel.org/linux-wireless/6e7ecb47-7ea0-433a-a19f-05f88a2edf6b@xxxxxxxxx/ >> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> >> --- >> drivers/net/wireless/realtek/rtw88/fw.c | 7 +-- >> drivers/net/wireless/realtek/rtw88/usb.c | 73 ++++++++++++++++++++---- >> drivers/net/wireless/realtek/rtw88/usb.h | 3 + >> 3 files changed, 67 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c >> index e6e9946fbf44..02389b7c6876 100644 >> --- a/drivers/net/wireless/realtek/rtw88/fw.c >> +++ b/drivers/net/wireless/realtek/rtw88/fw.c >> @@ -332,6 +332,9 @@ void rtw_fw_c2h_cmd_handle(struct rtw_dev *rtwdev, struct sk_buff *skb) >> case C2H_RA_RPT: >> rtw_fw_ra_report_handle(rtwdev, c2h->payload, len); >> break; >> + case C2H_ADAPTIVITY: >> + rtw_fw_adaptivity_result(rtwdev, c2h->payload, len); >> + break; >> default: >> rtw_dbg(rtwdev, RTW_DBG_FW, "C2H 0x%x isn't handled\n", c2h->id); >> break; >> @@ -367,10 +370,6 @@ void rtw_fw_c2h_cmd_rx_irqsafe(struct rtw_dev *rtwdev, u32 pkt_offset, >> rtw_fw_scan_result(rtwdev, c2h->payload, len); >> dev_kfree_skb_any(skb); >> break; >> - case C2H_ADAPTIVITY: >> - rtw_fw_adaptivity_result(rtwdev, c2h->payload, len); >> - dev_kfree_skb_any(skb); >> - break; >> default: >> /* pass offset for further operation */ >> *((u32 *)skb->cb) = pkt_offset; >> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c >> index 727569d4ef4b..5c2dfa2ced92 100644 >> --- a/drivers/net/wireless/realtek/rtw88/usb.c >> +++ b/drivers/net/wireless/realtek/rtw88/usb.c >> @@ -585,7 +585,7 @@ static void rtw_usb_rx_handler(struct work_struct *work) >> goto skip_packet; >> } >> >> - skb = alloc_skb(skb_len, GFP_KERNEL); >> + skb = alloc_skb(skb_len, GFP_ATOMIC); >> if (!skb) { >> rtw_dbg(rtwdev, RTW_DBG_USB, >> "failed to allocate RX skb of size %u\n", >> @@ -612,7 +612,11 @@ static void rtw_usb_rx_handler(struct work_struct *work) >> rx_desc += next_pkt; >> } while (rx_desc + pkt_desc_sz < rx_skb->data + rx_skb->len); >> >> - dev_kfree_skb_any(rx_skb); >> + if (skb_queue_len(&rtwusb->rx_free_queue) >= >> + RTW_USB_RX_SKB_NUM - RTW_USB_RXCB_NUM) >> + dev_kfree_skb_any(rx_skb); >> + else >> + skb_queue_tail(&rtwusb->rx_free_queue, rx_skb); > > Why not just queue and reuse rx_skb for all? That would be simpler. > I didn't want to let it allocate too many skbs. I didn't find any kind of limit in the official drivers, so maybe it would be fine. >> } >> } >> >> @@ -621,23 +625,57 @@ static void rtw_usb_read_port_complete(struct urb *urb); >> static void rtw_usb_rx_resubmit(struct rtw_usb *rtwusb, struct rx_usb_ctrl_block *rxcb) >> { >> struct rtw_dev *rtwdev = rtwusb->rtwdev; >> + struct sk_buff *rx_skb; >> + gfp_t priority = in_interrupt() ? GFP_ATOMIC : GFP_KERNEL; > > I remember in_interrupt() is not recommended. Can't we pass necessary gfp_t > via function argument by callers? > Yes, I can do that. >> int error; >> >> - rxcb->rx_skb = alloc_skb(RTW_USB_MAX_RECVBUF_SZ, GFP_ATOMIC); >> - if (!rxcb->rx_skb) >> - return; >> + rx_skb = skb_dequeue(&rtwusb->rx_free_queue); >> + if (!rx_skb) >> + rx_skb = alloc_skb(RTW_USB_MAX_RECVBUF_SZ, priority); >> + >> + if (!rx_skb) >> + goto try_later; >> + >> + rxcb->rx_skb = rx_skb; >> + >> + skb_reset_tail_pointer(rxcb->rx_skb); >> + rxcb->rx_skb->len = 0; > > How about moving these initialization upward before ' rxcb->rx_skb = rx_skb;'? > Statements can be shorter, and it is reasonable to initialize data before > assignment. > >> >> usb_fill_bulk_urb(rxcb->rx_urb, rtwusb->udev, >> usb_rcvbulkpipe(rtwusb->udev, rtwusb->pipe_in), >> rxcb->rx_skb->data, RTW_USB_MAX_RECVBUF_SZ, >> rtw_usb_read_port_complete, rxcb); >> >> - error = usb_submit_urb(rxcb->rx_urb, GFP_ATOMIC); >> + error = usb_submit_urb(rxcb->rx_urb, priority); > > Not sure if 'priority' fits the meaning. Maybe many kernel code just > uses 'gfp'. > >> if (error) { >> - kfree_skb(rxcb->rx_skb); >> + skb_queue_tail(&rtwusb->rx_free_queue, rxcb->rx_skb); >> + >> if (error != -ENODEV) >> rtw_err(rtwdev, "Err sending rx data urb %d\n", >> error); > > Since here rxcb->rx_skb != NULL, will it be a problem? The rxcb will not be > submitted again? Should all error cases go to try_later label? > Right, the rxcb will be submitted again only if the error was ENOMEM I don't know what other errors can be considered temporary. I never ran into the case where the error is not ENODEV. > >> + >> + if (error == -ENOMEM) >> + goto try_later; >> + } >> + >> + return; >> + >> +try_later: >> + rxcb->rx_skb = NULL; >> + queue_work(rtwusb->rxwq, &rtwusb->rx_urb_work); >> +} >> + >> +static void rtw_usb_rx_resubmit_work(struct work_struct *work) >> +{ >> + struct rtw_usb *rtwusb = container_of(work, struct rtw_usb, rx_urb_work); >> + struct rx_usb_ctrl_block *rxcb; >> + int i; >> + >> + for (i = 0; i < RTW_USB_RXCB_NUM; i++) { >> + rxcb = &rtwusb->rx_cb[i]; >> + >> + if (!rxcb->rx_skb) >> + rtw_usb_rx_resubmit(rtwusb, rxcb); >> } >> } >>
