Re: [PATCH rtw-next v1 11/13] wifi: rtw89: Add usb.{c,h}

[Bitterblue Smith <rtl8821cerfe2@gmail.com>]

On 13/05/2025 09:12, 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>
>> ---
>>  drivers/net/wireless/realtek/rtw89/usb.c | 1030 ++++++++++++++++++++++
>>  drivers/net/wireless/realtek/rtw89/usb.h |   61 ++
>>  2 files changed, 1091 insertions(+)
>>  create mode 100644 drivers/net/wireless/realtek/rtw89/usb.c
>>  create mode 100644 drivers/net/wireless/realtek/rtw89/usb.h
>>
>> diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
>> new file mode 100644
>> index 000000000000..6e8a544b352c
>> --- /dev/null
>> +++ b/drivers/net/wireless/realtek/rtw89/usb.c
>> @@ -0,0 +1,1030 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
>> +/* Copyright(c) 2025  Realtek Corporation
>> + */
>> +
>> +#include <linux/usb.h>
>> +#include "debug.h"
>> +#include "mac.h"
>> +#include "reg.h"
>> +#include "txrx.h"
>> +#include "usb.h"
>> +
>> +static void rtw89_usb_vendorreq(struct rtw89_dev *rtwdev, u32 addr,
>> +                               void *data, u16 len, u8 reqtype)
>> +{
>> +       struct rtw89_usb *rtwusb = rtw89_get_usb_priv(rtwdev);
>> +       struct usb_device *udev = rtwusb->udev;
>> +       unsigned int pipe;
>> +       u16 value, index;
>> +       int attempt, ret;
>> +
>> +       value = addr & 0x0000ffff;
>> +       index = (addr & 0x00ff0000) >> 16;
> 
> u16_get_bits(addr, GENMASK(23, 16)) ?
> 
> 
>> +
>> +       mutex_lock(&rtwusb->vendor_req_mutex);
> 
> rtw89 takes wiphy_lock for control path. Is there any case more than 
> one threads run at the same time?
> 

Maybe not. I just copied this from the vendor driver. How can I be
sure only one thread runs?

I added this above the mutex_lock() today:

	if (mutex_is_locked(&rtwusb->vendor_req_mutex))
		pr_err("mutex already locked elsewhere\n");

So far it hasn't printed the message.

>> +
>> +       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);
> 
> By "rtwusb->vendor_req_buf = kmalloc(sizeof(u32), GFP_KERNEL);", it seems like 
> buffer size of vendor_req_buf is only 4 bytes. Is it enough space to do
> memcpy()? Also, why not just a local variable ?
> 

It is enough space, len can only be 1, 2, or 4 here. It's not a local
variable because usb_control_msg() needs memory suitable for DMA.

> 
>> +               }
>> +
>> +               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),
>> +                                  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;
>> +               }
>> +       }
>> +
>> +       mutex_unlock(&rtwusb->vendor_req_mutex);
>> +}
>> +
>> +static u32 rtw89_usb_read_cmac(struct rtw89_dev *rtwdev, u32 addr)
>> +{
>> +       u32 addr32, val32, shift;
>> +       __le32 data = 0;
>> +       int count;
>> +
>> +       addr32 = addr & ~0x3;
>> +       shift = (addr & 0x3) * 8;
>> +
>> +       for (count = 0; ; count++) {
>> +               rtw89_usb_vendorreq(rtwdev, addr32, &data, 4,
>> +                                   RTW89_USB_VENQT_READ);
>> +
>> +               val32 = le32_to_cpu(data);
>> +
> 
> no empty line between assignment and immediate checking. 
> 
>> +               if (val32 != RTW89_R32_DEAD)
>> +                       break;
>> +
>> +               if (count >= MAC_REG_POOL_COUNT) {
>> +                       rtw89_warn(rtwdev, "%s: addr %#x = %#x\n",
>> +                                  __func__, addr32, val32);
>> +                       val32 = RTW89_R32_DEAD;
>> +                       break;
>> +               }
>> +
>> +               rtw89_write32(rtwdev, R_AX_CK_EN, B_AX_CMAC_ALLCKEN);
>> +       }
>> +
>> +       return val32 >> shift;
>> +}
> 
> [...]
> 
>> +static void rtw89_usb_write_port_complete(struct urb *urb)
>> +{
>> +       struct rtw89_usb_tx_ctrl_block *txcb = urb->context;
>> +       struct rtw89_dev *rtwdev = txcb->rtwdev;
>> +       struct ieee80211_tx_info *info;
>> +       struct sk_buff *skb;
>> +
>> +       while (true) {
> 
> Is it possible that more than one skb are in txcb->tx_ack_queue?
> With USB aggregation afterword, it will become possible?
> 

Right now there is only one skb in txcb->tx_ack_queue. But yes,
with USB TX aggregation it will have several skb. The maximum
number depends on the chip. I think RTL8851BU can take up to 9 in
one USB transfer.

>> +               skb = skb_dequeue(&txcb->tx_ack_queue);
>> +               if (!skb)
>> +                       break;
>> +
>> +               info = IEEE80211_SKB_CB(skb);
>> +               ieee80211_tx_info_clear_status(info);
>> +
>> +               if (urb->status == 0) {
>> +                       if (info->flags & IEEE80211_TX_CTL_NO_ACK)
>> +                               info->flags |= IEEE80211_TX_STAT_NOACK_TRANSMITTED;
>> +                       else
>> +                               info->flags |= IEEE80211_TX_STAT_ACK;
>> +               }
>> +
>> +               ieee80211_tx_status_irqsafe(rtwdev->hw, skb);
>> +       }
>> +
>> +       switch (urb->status) {
>> +       case 0:
>> +       case -EPIPE:
>> +       case -EPROTO:
>> +       case -EINPROGRESS:
>> +       case -ENOENT:
>> +       case -ECONNRESET:
>> +               break;
>> +       default:
>> +               set_bit(RTW89_FLAG_UNPLUGGED, rtwdev->flags);
>> +               break;
>> +       }
>> +
>> +       kfree(txcb);
>> +       usb_free_urb(urb);
>> +}
>> +
> 
> [...]
> 
>> +
>> +static void rtw89_usb_read_port_complete(struct urb *urb);
> 
> Move to beginning of this file. 
> 
>> +
>> +static void rtw89_usb_rx_resubmit(struct rtw89_usb *rtwusb,
>> +                                 struct rtw89_usb_rx_ctrl_block *rxcb,
>> +                                 gfp_t gfp)
>> +{
>> +       struct rtw89_dev *rtwdev = rtwusb->rtwdev;
>> +       struct sk_buff *rx_skb;
>> +       int error;
> 
> Why not 'int ret' in commom? 
> 
> [...]
> 
>> +
>> +static void rtw89_usb_cancel_rx_bufs(struct rtw89_usb *rtwusb)
>> +{
>> +       struct rtw89_usb_rx_ctrl_block *rxcb;
>> +       int i;
>> +
>> +       for (i = 0; i < RTW89_USB_RXCB_NUM; i++) {
>> +               rxcb = &rtwusb->rx_cb[i];
>> +               usb_kill_urb(rxcb->rx_urb);
> 
> rtw89_usb_disconnect() calls rtw89_usb_cancel_rx_bufs() and then call
> rtw89_usb_free_rx_bufs(). Is it a problem that usb_kill_urb() is called twice?
> 

I think it's not a problem because nothing bad happened so far.
But usb_kill_urb() in rtw89_usb_free_rx_bufs() is unnecessary so
I'll remove it.

>> +       }
>> +}
>> +
>> +static void rtw89_usb_free_rx_bufs(struct rtw89_usb *rtwusb)
>> +{
>> +       struct rtw89_usb_rx_ctrl_block *rxcb;
>> +       int i;
>> +
>> +       for (i = 0; i < RTW89_USB_RXCB_NUM; i++) {
>> +               rxcb = &rtwusb->rx_cb[i];
>> +
>> +               usb_kill_urb(rxcb->rx_urb);
>> +               usb_free_urb(rxcb->rx_urb);
>> +       }
>> +}
>> +
> 
> [...]
> 
>> +
>> +static int rtw89_usb_ops_mac_lv1_rcvy(struct rtw89_dev *rtwdev,
>> +                                     enum rtw89_lv1_rcvy_step step)
>> +{
>> +       u32 reg, mask;
>> +
>> +       switch (rtwdev->chip->chip_id) {
>> +       case RTL8851B:
>> +       case RTL8852A:
>> +       case RTL8852B:
>> +               reg = R_AX_USB_WLAN0_1;
>> +               mask = B_AX_USBRX_RST | B_AX_USBTX_RST;
>> +               break;
>> +       case RTL8852C:
>> +               reg = R_AX_USB_WLAN0_1_V1;
>> +               mask = B_AX_USBRX_RST_V1 | B_AX_USBTX_RST_V1;
>> +               break;
>> +       default:
>> +               rtw89_err(rtwdev, "%s: fix me\n", __func__);
>> +               return -EOPNOTSUPP;
>> +       }
>> +
>> +       switch (step) {
>> +       case RTW89_LV1_RCVY_STEP_1:
>> +               rtw89_write32_set(rtwdev, reg, mask);
>> +
>> +               msleep(30);
>> +
> 
> maybe no need this empty line.
> 
>> +               break;
>> +       case RTW89_LV1_RCVY_STEP_2:
>> +               rtw89_write32_clr(rtwdev, reg, mask);
>> +
> 
> no need this empty line.
> 
>> +               break;
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
> 
> [...]
> 
>> +static int rtw89_usb_intf_init(struct rtw89_dev *rtwdev,
>> +                              struct usb_interface *intf)
>> +{
>> +       struct rtw89_usb *rtwusb = rtw89_get_usb_priv(rtwdev);
>> +       int ret;
>> +
>> +       ret = rtw89_usb_parse(rtwdev, intf);
>> +       if (ret)
>> +               return ret;
>> +
>> +       rtwusb->vendor_req_buf = kmalloc(sizeof(u32), GFP_KERNEL);
> 
> I mentioned this allocation before. 
> 
>> +       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);
>> +
>> +       mutex_init(&rtwusb->vendor_req_mutex);
>> +
>> +       return 0;
>> +}
>> +
> 
> [...]
> 
>> diff --git a/drivers/net/wireless/realtek/rtw89/usb.h b/drivers/net/wireless/realtek/rtw89/usb.h
>> new file mode 100644
>> index 000000000000..86caae1b9d0b
>> --- /dev/null
>> +++ b/drivers/net/wireless/realtek/rtw89/usb.h
>> @@ -0,0 +1,61 @@
>> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
>> +/* Copyright(c) 2025  Realtek Corporation
>> + */
>> +
>> +#ifndef __RTW89_USB_H__
>> +#define __RTW89_USB_H__
>> +
> 
> [...]
> 
>> +
>> +static inline struct rtw89_usb *rtw89_get_usb_priv(struct rtw89_dev *rtwdev)
> 
> Not sure if it is worth of this wrapper. Or just rtw89_usb_priv()? 
> 

I like it because it's shorter than (struct rtw89_usb *)rtwdev->priv.

>> +{
>> +       return (struct rtw89_usb *)rtwdev->priv;
>> +}
>> +
>> +int rtw89_usb_probe(struct usb_interface *intf,
>> +                   const struct usb_device_id *id);
>> +void rtw89_usb_disconnect(struct usb_interface *intf);
>> +
>> +#endif
>> --
>> 2.49.0
>