On 13/05/2025 09:12, 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>
>> ---
>> 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
>
