Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> wrote: > 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? Just C2H_ADAPTIVITY. With patch subject, people can't understand this change. > > >> > >> 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. The case ' skb_queue_len(&rtwusb->rx_free_queue) >= 8 - 4' is rare? If so, I think this is fine. If not, to repeatedly allocate and free would cause memory fragment, and higher probability to failed to allocate memory with GFP_ATOMIC. Also seemingly additional 4 persistent rx_skb is not a big cost.
