Re: [PATCH 06/10] rtw88: Add common USB chip support

From: Pkshih
Date: Fri May 20 2022 - 03:39:47 EST

Next message: Pablo Neira Ayuso: "Re: linux-next: build failure after merge of the net-next tree"
Previous message: Hsin-Yi Wang: "Re: [PATCH v2 3/3] squashfs: implement readahead"
In reply to: Sascha Hauer: "Re: [PATCH 06/10] rtw88: Add common USB chip support"
Next in thread: s.hauer@xxxxxxxxxxxxxx: "Re: [PATCH 06/10] rtw88: Add common USB chip support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, 2022-05-18 at 10:23 +0200, Sascha Hauer wrote:
> Add the common bits and pieces to add USB support to the RTW88 driver.
> This is based on https://github.com/ulli-kroll/rtw88-usb.git which
> itself is first written by Neo Jou.
>
> Signed-off-by: neo_jou <neo_jou@xxxxxxxxxxx>
> Signed-off-by: Hans Ulli Kroll <linux@xxxxxxxxxxxxx>
> Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> ---
> drivers/net/wireless/realtek/rtw88/Kconfig | 3 +
> drivers/net/wireless/realtek/rtw88/Makefile | 2 +
> drivers/net/wireless/realtek/rtw88/mac.c | 3 +
> drivers/net/wireless/realtek/rtw88/main.c | 5 +
> drivers/net/wireless/realtek/rtw88/main.h | 4 +
> drivers/net/wireless/realtek/rtw88/reg.h | 1 +
> drivers/net/wireless/realtek/rtw88/tx.h | 31 +
> drivers/net/wireless/realtek/rtw88/usb.c | 1051 +++++++++++++++++++
> drivers/net/wireless/realtek/rtw88/usb.h | 109 ++
> 9 files changed, 1209 insertions(+)
> create mode 100644 drivers/net/wireless/realtek/rtw88/usb.c
> create mode 100644 drivers/net/wireless/realtek/rtw88/usb.h
>

[...]

> diff --git a/drivers/net/wireless/realtek/rtw88/reg.h b/drivers/net/wireless/realtek/rtw88/reg.h
> index 84ba9ec489c37..a928899030863 100644
> --- a/drivers/net/wireless/realtek/rtw88/reg.h
> +++ b/drivers/net/wireless/realtek/rtw88/reg.h
> @@ -184,6 +184,7 @@
> #define BIT_TXDMA_VIQ_MAP(x) \
^^^^^^^ replace 8 spaces by one tab

> (((x) & BIT_MASK_TXDMA_VIQ_MAP) << BIT_SHIFT_TXDMA_VIQ_MAP)
> #define REG_TXDMA_PQ_MAP 0x010C
> +#define BIT_RXDMA_ARBBW_EN BIT(0)
> #define BIT_SHIFT_TXDMA_BEQ_MAP 8
> #define BIT_MASK_TXDMA_BEQ_MAP 0x3
> #define BIT_TXDMA_BEQ_MAP(x) \
^^^^^ use tab

> diff --git a/drivers/net/wireless/realtek/rtw88/tx.h b/drivers/net/wireless/realtek/rtw88/tx.h
> index 56371eff9f7ff..c02d7a15895c6 100644
> --- a/drivers/net/wireless/realtek/rtw88/tx.h
> +++ b/drivers/net/wireless/realtek/rtw88/tx.h
> @@ -67,6 +67,14 @@
> le32p_replace_bits((__le32 *)(txdesc) + 0x03, value, BIT(15))
> #define SET_TX_DESC_BT_NULL(txdesc, value) \
> le32p_replace_bits((__le32 *)(txdesc) + 0x02, value, BIT(23))
> +#define SET_TX_DESC_TXDESC_CHECKSUM(txdesc, value) \
> + le32p_replace_bits((__le32 *)(txdesc) + 0x07, value, GENMASK(15, 0))
> +#define SET_TX_DESC_DMA_TXAGG_NUM(txdesc, value) \
> + le32p_replace_bits((__le32 *)(txdesc) + 0x07, value, GENMASK(31, 24))
> +#define GET_TX_DESC_PKT_OFFSET(txdesc) \
> + le32_get_bits(*((__le32 *)(txdesc) + 0x01), GENMASK(28, 24))
> +#define GET_TX_DESC_QSEL(txdesc) \
^^^^ use tab

I think you can run ./script/checkpatch.pl to find out these coding style issues.

> + le32_get_bits(*((__le32 *)(txdesc) + 0x01), GENMASK(12, 8))
>
> enum rtw_tx_desc_queue_select {
> TX_DESC_QSEL_TID0 = 0,
> @@ -119,4 +127,27 @@ rtw_tx_write_data_h2c_get(struct rtw_dev *rtwdev,
> struct rtw_tx_pkt_info *pkt_info,
> u8 *buf, u32 size);
>
>

[...]

> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> new file mode 100644
> index 0000000000000..7641ea6f6ad1a
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> @@ -0,0 +1,1051 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +/* Copyright(c) 2018-2019 Realtek Corporation
> + */
> +
> +#include <linux/module.h>
> +#include <linux/usb.h>
> +#include <linux/mutex.h>
> +#include "main.h"
> +#include "debug.h"
> +#include "reg.h"
> +#include "tx.h"
> +#include "rx.h"
> +#include "fw.h"
> +#include "ps.h"
> +#include "usb.h"
> +
> +#define RTW_USB_MAX_RXQ_LEN 128
> +
> +struct rtw_usb_txcb {
> + struct rtw_dev *rtwdev;
> + struct sk_buff_head tx_ack_queue;
> +};
> +
> +static void rtw_usb_fill_tx_checksum(struct rtw_usb *rtwusb,
> + struct sk_buff *skb, int agg_num)
> +{
> + struct rtw_dev *rtwdev = rtwusb->rtwdev;
> + struct rtw_tx_pkt_info pkt_info;
> +
> + SET_TX_DESC_DMA_TXAGG_NUM(skb->data, agg_num);
> + pkt_info.pkt_offset = GET_TX_DESC_PKT_OFFSET(skb->data);
> + rtw_tx_fill_txdesc_checksum(rtwdev, &pkt_info, skb->data);
> +}
> +
> +static void usbctrl_async_callback(struct urb *urb)
> +{
> + /* free dr */
> + kfree(urb->setup_packet);
> + /* free databuf */
> + kfree(urb->transfer_buffer);
> +}
> +
> +static int usbctrl_vendorreq_async_write(struct usb_device *udev, u8 request,
> + u16 value, u16 index, void *pdata,
> + u16 len)
> +{
> + int rc;

Normally, we use 'ret' as return code instead.

> + unsigned int pipe;
> + u8 reqtype;
> + struct usb_ctrlrequest *dr;
> + struct urb *urb;
> + const u16 databuf_maxlen = RTW_USB_VENQT_MAX_BUF_SIZE;
> + u8 *databuf;

declare in reverse X'mas tree:

const u16 databuf_maxlen = RTW_USB_VENQT_MAX_BUF_SIZE;
struct usb_ctrlrequest *dr;
unsigned int pipe;
struct urb *urb;
u8 *databuf;
int ret;

> +
> + if (WARN_ON_ONCE(len > databuf_maxlen))
> + len = databuf_maxlen;
> +
> + pipe = usb_sndctrlpipe(udev, 0); /* write_out */
> + reqtype = RTW_USB_CMD_WRITE;
> +
> + dr = kzalloc(sizeof(*dr), GFP_ATOMIC);
> + if (!dr)
> + return -ENOMEM;
> +
> + databuf = kmemdup(pdata, len, GFP_ATOMIC);
> + if (!databuf) {
> + kfree(dr);
> + return -ENOMEM;
> + }
> +
> + urb = usb_alloc_urb(0, GFP_ATOMIC);
> + if (!urb) {
> + kfree(databuf);
> + kfree(dr);
> + return -ENOMEM;
> + }
> +
> + dr->bRequestType = reqtype;
> + dr->bRequest = request;
> + dr->wValue = cpu_to_le16(value);
> + dr->wIndex = cpu_to_le16(index);
> + dr->wLength = cpu_to_le16(len);
> +
> + usb_fill_control_urb(urb, udev, pipe,
> + (unsigned char *)dr, databuf, len,
> + usbctrl_async_callback, NULL);
> + rc = usb_submit_urb(urb, GFP_ATOMIC);
> + if (rc < 0) {
> + kfree(databuf);
> + kfree(dr);
> + }
> +
> + usb_free_urb(urb);
> +
> + return rc;
> +}
> +
> +static u32 rtw_usb_read_sync(struct rtw_dev *rtwdev, u32 addr, u16 len)
> +{
> + struct rtw_usb *rtwusb = rtw_get_usb_priv(rtwdev);
> + struct usb_device *udev = rtwusb->udev;
> + __le32 *data;
> + unsigned long flags;
> + int ret;
> + static int count;
> +
> + spin_lock_irqsave(&rtwusb->usb_lock, flags);
> +
> + if (++rtwusb->usb_data_index >= RTW_USB_MAX_RX_COUNT)
> + rtwusb->usb_data_index = 0;
> + data = &rtwusb->usb_data[rtwusb->usb_data_index];
> +
> + spin_unlock_irqrestore(&rtwusb->usb_lock, flags);
> +
> + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> + RTW_USB_CMD_REQ, RTW_USB_CMD_READ, addr,
^^^^ align open parenthesis (codging style)

checkpatch.pl can help this.

> + RTW_USB_VENQT_CMD_IDX, data, len, 1000);
> + if (ret < 0 && ret != -ENODEV && count++ < 4)
> + rtw_err(rtwdev, "reg 0x%x, usbctrl_vendorreq failed with %d\n",
> + addr, ret);
> +
> + return le32_to_cpu(*data);
> +}
> +

[...]

> +
> +static void rtw_usb_rx_refill_work(struct work_struct *work)
> +{
> + struct rtw_usb *rtwusb = container_of(work, struct rtw_usb, rx_refill_work);
> + struct rtw_dev *rtwdev = rtwusb->rtwdev;
> + struct rx_usb_ctrl_block *rxcb;
> + unsigned long flags;
> + int error;
> +
> + do {
> + spin_lock_irqsave(&rtwusb->rx_data_list_lock, flags);
> +
> + rxcb = list_first_entry_or_null(&rtwusb->rx_data_free,
> + struct rx_usb_ctrl_block, list);
> +
> + spin_unlock_irqrestore(&rtwusb->rx_data_list_lock, flags);
> + if (!rxcb)
> + return;
> +
> + rxcb->rx_skb = alloc_skb(RTW_USB_MAX_RECVBUF_SZ, GFP_KERNEL);
> + if (!rxcb->rx_skb) {
> + rtw_err(rtwdev, "could not allocate rx skbuff\n");
> + return;
> + }
> +
> + usb_fill_bulk_urb(rxcb->rx_urb, rtwusb->udev,
> + usb_rcvbulkpipe(rtwusb->udev, rtwusb->pipe_in),
> + rxcb->rx_skb->data, RTW_USB_MAX_RECVBUF_SZ,
> + rtw_usb_read_port_complete, rxcb);
> +
> + spin_lock_irqsave(&rtwusb->rx_data_list_lock, flags);
> + list_move(&rxcb->list, &rtwusb->rx_data_used);
> + spin_unlock_irqrestore(&rtwusb->rx_data_list_lock, flags);
> +
> + error = usb_submit_urb(rxcb->rx_urb, GFP_KERNEL);
> + if (error) {
> + kfree_skb(rxcb->rx_skb);
> + if (error != -ENODEV)
> + rtw_err(rtwdev, "Err sending rx data urb %d\n",
> + error);
> + rtw_usb_rx_data_put(rtwusb, rxcb);
> +
> + return;
> + }
> + } while (true);

Can we have a limit of 'for(;<limit;)' insetad of 'while (true)'?

> +}
> +
> +static void rtw_usb_cancel_rx_bufs(struct rtw_usb *rtwusb)
> +{
> + struct rx_usb_ctrl_block *rxcb;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&rtwusb->rx_data_list_lock, flags);
> +
> + while (true) {
> + rxcb = list_first_entry_or_null(&rtwusb->rx_data_used,
> + struct rx_usb_ctrl_block, list);
> +
> + spin_unlock_irqrestore(&rtwusb->rx_data_list_lock, flags);
> +
> + if (!rxcb)
> + break;
> +
> + usb_kill_urb(rxcb->rx_urb);
> +
> + spin_lock_irqsave(&rtwusb->rx_data_list_lock, flags);
> + list_move(&rxcb->list, &rtwusb->rx_data_free);
> + }
> +}

The spin_lock pairs are not intuitive.
Can we change this chunk to

while (true) {
spin_lock();
rxcb = list_first_entry_or_null();
spin_unlock()

if (!rxcb)
return;

usb_free_urb();

spin_lock();
list_del();
spin_unlock();
}

The drawback is lock/unlock twice in single loop.

rtw_usb_free_rx_bufs() has similar coding.

> +
> +static void rtw_usb_free_rx_bufs(struct rtw_usb *rtwusb)
> +{
> + struct rx_usb_ctrl_block *rxcb;
> + unsigned long flags;
> +
> + rtw_usb_cancel_rx_bufs(rtwusb);
> +
> + spin_lock_irqsave(&rtwusb->rx_data_list_lock, flags);
> +
> + while (true) {
> + rxcb = list_first_entry_or_null(&rtwusb->rx_data_free, struct rx_usb_ctrl_block,
> list);
> +
> + spin_unlock_irqrestore(&rtwusb->rx_data_list_lock, flags);
> +
> + if (!rxcb)
> + break;
> +
> + usb_free_urb(rxcb->rx_urb);
> +
> + spin_lock_irqsave(&rtwusb->rx_data_list_lock, flags);
> + list_del(&rxcb->list);
> + }

> diff --git a/drivers/net/wireless/realtek/rtw88/usb.h b/drivers/net/wireless/realtek/rtw88/usb.h
> new file mode 100644
> index 0000000000000..4d714372f265c
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/usb.h
> @@ -0,0 +1,109 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> +/* Copyright(c) 2018-2019 Realtek Corporation
> + */
> +
> +#ifndef __RTW_USB_H_
> +#define __RTW_USB_H_
> +
> +#define FW_8192C_START_ADDRESS 0x1000
> +#define FW_8192C_END_ADDRESS 0x5FFF
^^^^^^^^^^ use tab

> +
> +#define RTW_USB_MAX_RX_COUNT 100
> +#define RTW_USB_VENQT_MAX_BUF_SIZE 254
> +#define MAX_USBCTRL_VENDORREQ_TIMES 10
> +
> +#define RTW_USB_CMD_READ 0xc0
> +#define RTW_USB_CMD_WRITE 0x40
> +#define RTW_USB_CMD_REQ 0x05
> +
> +#define RTW_USB_VENQT_CMD_IDX 0x00
^^^^^ use space
> +
> +#define RTW_USB_SUPER_SPEED_BULK_SIZE 1024
> +#define RTW_USB_HIGH_SPEED_BULK_SIZE 512
> +#define RTW_USB_FULL_SPEED_BULK_SIZE 64
> +

--
Ping-Ke

Next message: Pablo Neira Ayuso: "Re: linux-next: build failure after merge of the net-next tree"
Previous message: Hsin-Yi Wang: "Re: [PATCH v2 3/3] squashfs: implement readahead"
In reply to: Sascha Hauer: "Re: [PATCH 06/10] rtw88: Add common USB chip support"
Next in thread: s.hauer@xxxxxxxxxxxxxx: "Re: [PATCH 06/10] rtw88: Add common USB chip support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]