On 16/06/2025 06:10, Ping-Ke Shih wrote:
> Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> 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@xxxxxxxxx>
>
> 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;
>> +}
>> +
>
>
> [...]
>
