linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "s.hauer@pengutronix.de" <s.hauer@pengutronix.de>
To: Pkshih <pkshih@realtek.com>
Cc: "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
	"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 10:51:56 +0200	[thread overview]
Message-ID: <20220520085156.GE25578@pengutronix.de> (raw)
In-Reply-To: <e9ca08c6facb8916fb9e5cbad05447321d3d0f43.camel@realtek.com>

On Fri, May 20, 2022 at 07:39:03AM +0000, Pkshih wrote:
> 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

This line is not added by me. There are spaces used before the
linebreaks throughout this file.

> > +	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)'?

Not sure if it's worth it, but yes, it shouldn't hurt either.

> 
> > +}
> > +
> > +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.

Yes, that's why I did it the way I did ;)

How about:

	while (true) {
		unsigned long flags;

		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);
		if (rxcb)
			list_del(&rxcb->list);

		spin_unlock_irqrestore(&rtwusb->rx_data_list_lock, flags);

		if (!rxcb)
			break;

		usb_free_urb(rxcb->rx_urb);
	}

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

  reply	other threads:[~2022-05-20  8:52 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
2022-05-20  8:51     ` s.hauer [this message]
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=20220520085156.GE25578@pengutronix.de \
    --to=s.hauer@pengutronix.de \
    --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=pkshih@realtek.com \
    --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).