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. > > 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. > } > } > > @@ -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? > 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? > + > + 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); > } > } > > @@ -653,8 +691,7 @@ static void rtw_usb_read_port_complete(struct urb *urb) > urb->actual_length < 24) { > rtw_err(rtwdev, "failed to get urb length:%d\n", > urb->actual_length); > - if (skb) > - dev_kfree_skb_any(skb); > + skb_queue_tail(&rtwusb->rx_free_queue, skb); > } else { > skb_put(skb, urb->actual_length); > skb_queue_tail(&rtwusb->rx_queue, skb); > @@ -662,6 +699,8 @@ static void rtw_usb_read_port_complete(struct urb *urb) > } > rtw_usb_rx_resubmit(rtwusb, rxcb); > } else { > + skb_queue_tail(&rtwusb->rx_free_queue, skb); > + > switch (urb->status) { > case -EINVAL: > case -EPIPE: > @@ -679,8 +718,6 @@ static void rtw_usb_read_port_complete(struct urb *urb) > rtw_err(rtwdev, "status %d\n", urb->status); > break; > } > - if (skb) > - dev_kfree_skb_any(skb); > } > } > > @@ -868,16 +905,26 @@ static struct rtw_hci_ops rtw_usb_ops = { > static int rtw_usb_init_rx(struct rtw_dev *rtwdev) > { > struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev); > + struct sk_buff *rx_skb; > + int i; > > - rtwusb->rxwq = create_singlethread_workqueue("rtw88_usb: rx wq"); > + rtwusb->rxwq = alloc_workqueue("rtw88_usb: rx wq", WQ_BH, 0); > if (!rtwusb->rxwq) { > rtw_err(rtwdev, "failed to create RX work queue\n"); > return -ENOMEM; > } > > skb_queue_head_init(&rtwusb->rx_queue); > + skb_queue_head_init(&rtwusb->rx_free_queue); > > INIT_WORK(&rtwusb->rx_work, rtw_usb_rx_handler); > + INIT_WORK(&rtwusb->rx_urb_work, rtw_usb_rx_resubmit_work); > + > + for (i = 0; i < RTW_USB_RX_SKB_NUM; i++) { > + rx_skb = alloc_skb(RTW_USB_MAX_RECVBUF_SZ, GFP_KERNEL); > + if (rx_skb) > + skb_queue_tail(&rtwusb->rx_free_queue, rx_skb); > + } > > return 0; > } > @@ -902,6 +949,8 @@ static void rtw_usb_deinit_rx(struct rtw_dev *rtwdev) > > flush_workqueue(rtwusb->rxwq); > destroy_workqueue(rtwusb->rxwq); > + > + skb_queue_purge(&rtwusb->rx_free_queue); > } > > static int rtw_usb_init_tx(struct rtw_dev *rtwdev) > diff --git a/drivers/net/wireless/realtek/rtw88/usb.h b/drivers/net/wireless/realtek/rtw88/usb.h > index 86697a5c0103..9b695b688b24 100644 > --- a/drivers/net/wireless/realtek/rtw88/usb.h > +++ b/drivers/net/wireless/realtek/rtw88/usb.h > @@ -38,6 +38,7 @@ > #define RTW_USB_RXAGG_TIMEOUT 10 > > #define RTW_USB_RXCB_NUM 4 > +#define RTW_USB_RX_SKB_NUM 8 > > #define RTW_USB_EP_MAX 4 > > @@ -81,7 +82,9 @@ struct rtw_usb { > > struct rx_usb_ctrl_block rx_cb[RTW_USB_RXCB_NUM]; > struct sk_buff_head rx_queue; > + struct sk_buff_head rx_free_queue; > struct work_struct rx_work; > + struct work_struct rx_urb_work; > }; > > static inline struct rtw_usb_tx_data *rtw_usb_get_tx_data(struct sk_buff *skb) > -- > 2.47.0
