Re: [PATCH rtw-next v2 12/14] wifi: rtw89: Add usb.{c,h}

[Bitterblue Smith <rtl8821cerfe2@gmail.com>]

On 16/06/2025 06:10, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>> Add very basic USB support. No TX/RX aggregation, no TX queues, no
>> switching to USB 3 mode.
>>
>> RTL8851BU and RTL8832BU work.
>>
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> 
> Some minor suggestions. 
> 
> [...]
> 
>> +static void rtw89_usb_vendorreq(struct rtw89_dev *rtwdev, u32 addr,
>> +                               void *data, u16 len, u8 reqtype)
>> +{
>> +       struct rtw89_usb *rtwusb = rtw89_usb_priv(rtwdev);
>> +       struct usb_device *udev = rtwusb->udev;
>> +       unsigned int pipe;
>> +       u16 value, index;
>> +       int attempt, ret;
>> +
>> +       if (test_bit(RTW89_FLAG_UNPLUGGED, rtwdev->flags))
>> +               return;
>> +
>> +       value = u32_get_bits(addr, GENMASK(15, 0));
>> +       index = u32_get_bits(addr, GENMASK(23, 16));
>> +
>> +       for (attempt = 0; attempt < 10; attempt++) {
>> +               *rtwusb->vendor_req_buf = 0;
>> +
>> +               if (reqtype == RTW89_USB_VENQT_READ) {
>> +                       pipe = usb_rcvctrlpipe(udev, 0);
>> +               } else { /* RTW89_USB_VENQT_WRITE */
>> +                       pipe = usb_sndctrlpipe(udev, 0);
>> +
>> +                       memcpy(rtwusb->vendor_req_buf, data, len);
>> +               }
>> +
>> +               ret = usb_control_msg(udev, pipe, RTW89_USB_VENQT, reqtype,
>> +                                     value, index, rtwusb->vendor_req_buf,
>> +                                     len, 500);
>> +
>> +               if (ret == len) { /* Success */
>> +                       atomic_set(&rtwusb->continual_io_error, 0);
>> +
>> +                       if (reqtype == RTW89_USB_VENQT_READ)
>> +                               memcpy(data, rtwusb->vendor_req_buf, len);
>> +
>> +                       break;
>> +               }
>> +
>> +               if (ret == -ESHUTDOWN || ret == -ENODEV)
>> +                       set_bit(RTW89_FLAG_UNPLUGGED, rtwdev->flags);
>> +               else if (ret < 0)
>> +                       rtw89_warn(rtwdev,
>> +                                  "usb %s%u 0x%x fail ret=%d value=0x%x attempt=%d\n",
>> +                                  reqtype == RTW89_USB_VENQT_READ ? "read" : "write",
>> +                                  len * 8, addr, ret,
>> +                                  le32_to_cpup(rtwusb->vendor_req_buf),
> 
> vendor_req_buf isn't always set fully (4 bytes), it would print out partially
> incorrect value. Would you like `%*ph` format with variable length? Like
>   "value=%*ph", len, rtwusb->vendor_req_buf
> 

I think it's fine the way it is because vendor_req_buf is zeroed
at the beginning of the for loop.

>> +                                  attempt);
>> +               else if (ret > 0 && reqtype == RTW89_USB_VENQT_READ)
>> +                       memcpy(data, rtwusb->vendor_req_buf, len);
>> +
>> +               if (atomic_inc_return(&rtwusb->continual_io_error) > 4) {
>> +                       set_bit(RTW89_FLAG_UNPLUGGED, rtwdev->flags);
>> +                       break;
>> +               }
>> +       }
>> +}
>> +
> 
> [...]
> 
>> +
>> +static int rtw89_usb_fwcmd_submit(struct rtw89_dev *rtwdev,
>> +                                 struct rtw89_core_tx_request *tx_req)
>> +{
>> +       struct rtw89_tx_desc_info *desc_info = &tx_req->desc_info;
>> +       struct rtw89_usb_tx_ctrl_block *txcb;
>> +       struct sk_buff *skb = tx_req->skb;
>> +       int txdesc_size = rtwdev->chip->h2c_desc_size;
>> +       void *txdesc;
>> +       int ret;
>> +
>> +       if (((desc_info->pkt_size + txdesc_size) % 512) == 0) {
>> +               rtw89_info(rtwdev, "avoiding multiple of 512\n");
>> +               desc_info->pkt_size += 4;
>> +               skb_put(skb, 4);
>> +       }
>> +
>> +       txcb = kmalloc(sizeof(*txcb), GFP_ATOMIC);
>> +       if (!txcb)
>> +               return -ENOMEM;
>> +
>> +       txdesc = skb_push(skb, txdesc_size);
>> +       memset(txdesc, 0, txdesc_size);
>> +       rtw89_chip_fill_txdesc_fwcmd(rtwdev, desc_info, txdesc);
>> +
>> +       txcb->rtwdev = rtwdev;
>> +       skb_queue_head_init(&txcb->tx_ack_queue);
>> +
>> +       skb_queue_tail(&txcb->tx_ack_queue, skb);
>> +
>> +       ret = rtw89_usb_write_port(rtwdev, RTW89_DMA_H2C, skb->data, skb->len,
>> +                                  rtw89_usb_write_port_complete_fwcmd, txcb);
>> +
> 
> nit: no need empty line 
> 
>> +       if (ret) {
>> +               rtw89_err(rtwdev, "%s failed: %d\n", __func__, ret);
>> +
>> +               skb_dequeue(&txcb->tx_ack_queue);
>> +               kfree(txcb);
>> +       }
>> +
>> +       return ret;
>> +}
>> +
> 
> [...]
> 
>> +
>> +static void rtw89_usb_rx_handler(struct work_struct *work)
>> +{
>> +       struct rtw89_usb *rtwusb = container_of(work, struct rtw89_usb, rx_work);
>> +       struct rtw89_dev *rtwdev = rtwusb->rtwdev;
>> +       struct rtw89_rx_desc_info desc_info;
>> +       struct sk_buff *rx_skb;
>> +       struct sk_buff *skb;
>> +       u32 pkt_offset;
>> +       int limit;
>> +
>> +       for (limit = 0; limit < 200; limit++) {
>> +               rx_skb = skb_dequeue(&rtwusb->rx_queue);
>> +               if (!rx_skb)
>> +                       break;
>> +
>> +               if (skb_queue_len(&rtwusb->rx_queue) >= RTW89_USB_MAX_RXQ_LEN) {
>> +                       rtw89_warn(rtwdev, "rx_queue overflow\n");
>> +                       dev_kfree_skb_any(rx_skb);
>> +                       continue;
>> +               }
>> +
>> +               memset(&desc_info, 0, sizeof(desc_info));
>> +               rtw89_chip_query_rxdesc(rtwdev, &desc_info, rx_skb->data, 0);
>> +
>> +               skb = rtw89_alloc_skb_for_rx(rtwdev, desc_info.pkt_size);
>> +               if (!skb) {
>> +                       rtw89_debug(rtwdev, RTW89_DBG_HCI,
>> +                                   "failed to allocate RX skb of size %u\n",
>> +                                   desc_info.pkt_size);
>> +                       continue;
>> +               }
>> +
>> +               pkt_offset = desc_info.offset + desc_info.rxd_len;
>> +
>> +               skb_put_data(skb, rx_skb->data + pkt_offset,
>> +                            desc_info.pkt_size);
>> +
>> +               rtw89_core_rx(rtwdev, &desc_info, skb);
>> +
>> +               if (skb_queue_len(&rtwusb->rx_free_queue) >= RTW89_USB_RX_SKB_NUM)
>> +                       dev_kfree_skb_any(rx_skb);
>> +               else
>> +                       skb_queue_tail(&rtwusb->rx_free_queue, rx_skb);
>> +       }
>> +
>> +       if (limit == 200)
> 
> Not sure if it is worth to reschedule itself again under this condition?
> 

I guess it can't hurt.

>> +               rtw89_debug(rtwdev, RTW89_DBG_HCI,
>> +                           "left %d rx skbs in the queue for later\n",
>> +                           skb_queue_len(&rtwusb->rx_queue));
>> +}
>> +
> 
> [...]
> 
>> +static int rtw89_usb_intf_init(struct rtw89_dev *rtwdev,
>> +                              struct usb_interface *intf)
>> +{
>> +       struct rtw89_usb *rtwusb = rtw89_usb_priv(rtwdev);
>> +       int ret;
>> +
>> +       ret = rtw89_usb_parse(rtwdev, intf);
>> +       if (ret)
>> +               return ret;
>> +
>> +       rtwusb->vendor_req_buf = kmalloc(sizeof(u32), GFP_KERNEL);
> 
> sizeof(*rtwusb->vendor_req_buf) or sizeof(__le32)
> 
>> +       if (!rtwusb->vendor_req_buf)
>> +               return -ENOMEM;
>> +
>> +       rtwusb->udev = usb_get_dev(interface_to_usbdev(intf));
>> +
>> +       usb_set_intfdata(intf, rtwdev->hw);
>> +
>> +       SET_IEEE80211_DEV(rtwdev->hw, &intf->dev);
>> +
>> +       return 0;
>> +}
>> +
> 
> 
> [...]
>