On 01/10/2024 04:25, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> wrote:
>>
>> alloc_skb fails (silently) therefore the RX URB is not submitted
>> ever again. There are only 4 RX URBs.
>
> Though only 4 RX URB, it might be possible more than 4 RX skb.
> In rtw_usb_read_port_complete(), queue RX skb into rtwusb->rx_queue, and kick
> off rx_work. It means some RX skb are inflight, but not sure how many.
>
>>
>> static void rtw_usb_rx_resubmit(struct rtw_usb *rtwusb, struct rx_usb_ctrl_block *rxcb)
>> {
>> struct rtw_dev *rtwdev = rtwusb->rtwdev;
>> int error;
>>
>> rxcb->rx_skb = alloc_skb(RTW_USB_MAX_RECVBUF_SZ, GFP_ATOMIC);
>> if (!rxcb->rx_skb)
>> return;
>>
>> 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);
>>
>> I added an error message there:
>>
>> rxcb->rx_skb = alloc_skb(RTW_USB_MAX_RECVBUF_SZ, GFP_ATOMIC);
>> if (!rxcb->rx_skb) {
>> rtw_err(rtwdev, "failed to allocate rx_skb\n");
>> return;
>> }
>
> My first thought is to change GFP_ATOMIC to GFP_KERNEL, but kernel documentation
> notes that
> "NEVER SLEEP IN A COMPLETION HANDLER. These are often called in atomic context."
> However, I feel it is possible to do rtw_usb_rx_resubmit() in a work.
>
Yes, maybe even in the existing rx_work.
> Another thought is to allocate a new skb with size urb->actual_length, and
> copy received data to the new skb, and queue to rtwusb->rx_queue. Then reuse
> the original rx_skb. This thought is based on what urb->actual_length would
> be smaller than RTW_USB_MAX_RECVBUF_SZ, but not very sure if this is fact.
>
>
I think actual_length is often close to RTW_USB_MAX_RECVBUF_SZ (32768).
Only with RTL8723DU it's small, probably around 1600 bytes, because it
doesn't use aggregation.
