linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Pkshih <pkshih@realtek.com>
To: "s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
	"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc: "johannes@sipsolutions.net" <johannes@sipsolutions.net>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	"neojou@gmail.com" <neojou@gmail.com>,
	"kvalo@kernel.org" <kvalo@kernel.org>,
	"tony0620emma@gmail.com" <tony0620emma@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"martin.blumenstingl@googlemail.com" 
	<martin.blumenstingl@googlemail.com>,
	"linux@ulli-kroll.de" <linux@ulli-kroll.de>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"neo_jou@realtek.com" <neo_jou@realtek.com>
Subject: Re: [PATCH 06/10] rtw88: Add common USB chip support
Date: Fri, 20 May 2022 07:39:03 +0000	[thread overview]
Message-ID: <e9ca08c6facb8916fb9e5cbad05447321d3d0f43.camel@realtek.com> (raw)
In-Reply-To: <20220518082318.3898514-7-s.hauer@pengutronix.de>

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@realtek.com>
> Signed-off-by: Hans Ulli Kroll <linux@ulli-kroll.de>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  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



  parent reply	other threads:[~2022-05-20  7:39 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-18  8:23 [PATCH 00/10] RTW88: Add support for USB variants Sascha Hauer
2022-05-18  8:23 ` [PATCH 01/10] rtw88: Call rtw_fw_beacon_filter_config() with rtwdev->mutex held Sascha Hauer
2022-05-18  8:23 ` [PATCH 02/10] rtw88: Drop rf_lock Sascha Hauer
2022-05-20  3:49   ` Pkshih
2022-05-20  6:54     ` s.hauer
2022-05-18  8:23 ` [PATCH 03/10] rtw88: Drop h2c.lock Sascha Hauer
2022-05-18  8:23 ` [PATCH 04/10] rtw88: Drop coex mutex Sascha Hauer
2022-05-18  8:23 ` [PATCH 05/10] rtw88: Do not access registers while atomic Sascha Hauer
2022-05-20  6:06   ` Pkshih
2022-05-20  7:07     ` s.hauer
2022-05-20 17:32   ` Larry Finger
2022-05-18  8:23 ` [PATCH 06/10] rtw88: Add common USB chip support Sascha Hauer
2022-05-18  8:32   ` Sascha Hauer
2022-05-18  8:34     ` Johannes Berg
2022-05-19 13:58       ` Sascha Hauer
2022-05-20  7:39   ` Pkshih [this message]
2022-05-20  8:51     ` s.hauer
2022-05-20  9:23       ` Pkshih
2022-05-20 18:33   ` Larry Finger
2022-05-23  4:07   ` Hans Ulli Kroll
2022-05-18  8:23 ` [PATCH 07/10] rtw88: Add rtw8723du chipset support Sascha Hauer
2022-05-18 16:55   ` kernel test robot
2022-05-20  7:47   ` Pkshih
2022-05-20  8:16     ` s.hauer
2022-05-30  6:34   ` Icenowy Zheng
2022-05-18  8:23 ` [PATCH 08/10] rtw88: Add rtw8821cu " Sascha Hauer
2022-05-20  7:56   ` Pkshih
2022-05-18  8:23 ` [PATCH 09/10] rtw88: Add rtw8822bu " Sascha Hauer
2022-05-20  7:58   ` Pkshih
2022-05-23 11:56   ` Rin Cat (鈴猫)
2022-05-23 12:34     ` Sascha Hauer
2022-05-18  8:23 ` [PATCH 10/10] rtw88: Add rtw8822cu " Sascha Hauer
2022-05-20  8:03   ` Pkshih
2022-05-20  8:56     ` s.hauer
2022-05-23  4:07 ` [PATCH 00/10] RTW88: Add support for USB variants Hans Ulli Kroll
2022-05-23  6:53   ` Sascha Hauer
2022-05-23 11:39     ` Ping-Ke Shih
2022-05-24  6:54       ` s.hauer
2022-05-23 10:13   ` Sascha Hauer
2022-05-30  9:25 ` Kalle Valo
2022-05-30  9:52   ` Sascha Hauer
2022-05-30 10:07     ` Kalle Valo
2022-05-30 10:16       ` Sascha Hauer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e9ca08c6facb8916fb9e5cbad05447321d3d0f43.camel@realtek.com \
    --to=pkshih@realtek.com \
    --cc=johannes@sipsolutions.net \
    --cc=kernel@pengutronix.de \
    --cc=kvalo@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=linux@ulli-kroll.de \
    --cc=martin.blumenstingl@googlemail.com \
    --cc=neo_jou@realtek.com \
    --cc=neojou@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=s.hauer@pengutronix.de \
    --cc=tony0620emma@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).