lkml.org 
[lkml]   [2022]   [Nov]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v3 07/11] rtw88: Add common USB chip support
On Mon, Nov 28, 2022 at 02:00:54AM +0000, Ping-Ke Shih wrote:
>
>
> > -----Original Message-----
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > Sent: Tuesday, November 22, 2022 10:52 PM
> > To: linux-wireless@vger.kernel.org
> > Cc: Neo Jou <neojou@gmail.com>; Hans Ulli Kroll <linux@ulli-kroll.de>; Ping-Ke Shih <pkshih@realtek.com>;
> > Yan-Hsuan Chuang <tony0620emma@gmail.com>; Kalle Valo <kvalo@kernel.org>; netdev@vger.kernel.org;
> > linux-kernel@vger.kernel.org; Martin Blumenstingl <martin.blumenstingl@googlemail.com>;
> > kernel@pengutronix.de; Johannes Berg <johannes@sipsolutions.net>; Alexander Hochbaum <alex@appudo.com>;
> > Da Xue <da@libre.computer>; Bernie Huang <phhuang@realtek.com>; Viktor Petrenko <g0000ga@gmail.com>;
> > Sascha Hauer <s.hauer@pengutronix.de>; neo_jou <neo_jou@realtek.com>
> > Subject: [PATCH v3 07/11] rtw88: Add common USB chip support
> >
> > 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>
> > ---
> >
> > Notes:
> > Changes since v2:
> > - Fix buffer length for aggregated tx packets
> > - Increase maximum transmit buffer size to 20KiB as found in downstream drivers
> > - Change register write functions to synchronous accesses instead of just firing
> > a URB without waiting for its completion
> > - requeue rx URBs directly in completion handler rather than having a workqueue
> > for it.
> >
> > Changes since v1:
> > - Make checkpatch.pl clean
> > - Drop WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL flag
> > - Use 'ret' as variable name for return values
> > - Sort variable declarations in reverse Xmas tree order
> > - Change potentially endless loop to a limited loop
> > - Change locking to be more obviously correct
> > - drop unnecessary check for !rtwdev
> > - make sure the refill workqueue is not restarted again after we have
> > cancelled it
> >
> > 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 | 4 +
> > 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 | 918 ++++++++++++++++++++
> > drivers/net/wireless/realtek/rtw88/usb.h | 107 +++
> > 9 files changed, 1073 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/Kconfig b/drivers/net/wireless/realtek/rtw88/Kconfig
> > index e3d7cb6c12902..1624c5db69bac 100644
> > --- a/drivers/net/wireless/realtek/rtw88/Kconfig
> > +++ b/drivers/net/wireless/realtek/rtw88/Kconfig
> > @@ -16,6 +16,9 @@ config RTW88_CORE
> > config RTW88_PCI
> > tristate
> >
> > +config RTW88_USB
> > + tristate
> > +
> > config RTW88_8822B
> > tristate
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/Makefile b/drivers/net/wireless/realtek/rtw88/Makefile
> > index 834c66ec0af9e..9e095f8181483 100644
> > --- a/drivers/net/wireless/realtek/rtw88/Makefile
> > +++ b/drivers/net/wireless/realtek/rtw88/Makefile
> > @@ -45,4 +45,6 @@ obj-$(CONFIG_RTW88_8821CE) += rtw88_8821ce.o
> > rtw88_8821ce-objs := rtw8821ce.o
> >
> > obj-$(CONFIG_RTW88_PCI) += rtw88_pci.o
> > +obj-$(CONFIG_RTW88_USB) += rtw88_usb.o
> > rtw88_pci-objs := pci.o
> > +rtw88_usb-objs := usb.o
>
> nit: I prefer not interleaving with PCI.

Ok.

> > +static u32 rtw_usb_read(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);
> > +
> > + rtwusb->usb_data_index++;
> > + rtwusb->usb_data_index &= (RTW_USB_MAX_RXTX_COUNT - 1);
> > +
> > + spin_unlock_irqrestore(&rtwusb->usb_lock, flags);
> > +
> > + data = &rtwusb->usb_data[rtwusb->usb_data_index];
>
> Don't you need to hold &rtwusb->usb_lock to access rtwusb->usb_data_index?
> rtw_usb_write() has similar code.

Right. Will rewrite to:

spin_lock_irqsave(&rtwusb->usb_lock, flags);
idx = rtwusb->usb_data_index;
rtwusb->usb_data_index = (idx + 1) & (RTW_USB_MAX_RXTX_COUNT - 1);

spin_unlock_irqrestore(&rtwusb->usb_lock, flags);

data = &rtwusb->usb_data[idx];

> > +static void rtw_usb_write_port_tx_complete(struct urb *urb)
> > +{
> > + struct rtw_usb_txcb *txcb = urb->context;
> > + struct rtw_dev *rtwdev = txcb->rtwdev;
> > + struct ieee80211_hw *hw = rtwdev->hw;
> > +
> > + while (true) {
>
> Is it possible to have a hard limit to prevent unexpected infinite loop?

Yes, that would be possible, but do you think it's necessary?

Each *txcb is used only once, It's allocated in rtw_usb_tx_agg_skb() and
&txcb->tx_ack_queue is filled with a limited number of skbs there. These
skbs is then iterated over in rtw_usb_write_port_tx_complete(), so I don't
see a way how we could end up in an infinite loop here.

It's not that &txcb->tx_ack_queue is filled in a concurrent thread while we
try to catch up in rtw_usb_write_port_tx_complete().

> > + skb_head = dev_alloc_skb(RTW_USB_MAX_XMITBUF_SZ);
> > + if (!skb_head) {
> > + skb_head = skb_iter;
> > + goto queue;
> > + }
> > +
> > + data_ptr = skb_head->data;
> > +
> > + while (skb_iter) {
> > + unsigned long flags;
> > +
> > + memcpy(data_ptr, skb_iter->data, skb_iter->len);
> > + skb_put(skb_head, skb_iter->len + align_next);
>
> skb_put(skb_head, align_next);
> skb_put_data(skb_head, skb_iter->data, skb_iter->len);
>
> Then, don't need to maintain 'data_ptr'.

Right. Looks much better this way.

> > + error = usb_submit_urb(rxcb->rx_urb, GFP_ATOMIC);
> > + if (error) {
> > + kfree_skb(rxcb->rx_skb);
> > + if (error != -ENODEV)
> > + rtw_err(rtwdev, "Err sending rx data urb %d\n",
> > + error);
>
> nit: straighten rtw_err()

Ok.

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 |
\
 
 \ /
  Last update: 2022-11-28 11:31    [W:0.076 / U:0.904 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site