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
> + 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?
> + 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;
> +}
> +
[...]
