| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20221128103000.GC29728@pengutronix.de>
Date: Mon, 28 Nov 2022 11:30:00 +0100
From: Sascha Hauer <s.hauer@...gutronix.de>
To: Ping-Ke Shih <pkshih@...ltek.com>
Cc: "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
Neo Jou <neojou@...il.com>,
Hans Ulli Kroll <linux@...i-kroll.de>,
Yan-Hsuan Chuang <tony0620emma@...il.com>,
Kalle Valo <kvalo@...nel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
"kernel@...gutronix.de" <kernel@...gutronix.de>,
Johannes Berg <johannes@...solutions.net>,
Alexander Hochbaum <alex@...udo.com>,
Da Xue <da@...re.computer>, Bernie Huang <phhuang@...ltek.com>,
Viktor Petrenko <g0000ga@...il.com>,
neo_jou <neo_jou@...ltek.com>
Subject: Re: [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@...gutronix.de>
> > Sent: Tuesday, November 22, 2022 10:52 PM
> > To: linux-wireless@...r.kernel.org
> > Cc: Neo Jou <neojou@...il.com>; Hans Ulli Kroll <linux@...i-kroll.de>; Ping-Ke Shih <pkshih@...ltek.com>;
> > Yan-Hsuan Chuang <tony0620emma@...il.com>; Kalle Valo <kvalo@...nel.org>; netdev@...r.kernel.org;
> > linux-kernel@...r.kernel.org; Martin Blumenstingl <martin.blumenstingl@...glemail.com>;
> > kernel@...gutronix.de; Johannes Berg <johannes@...solutions.net>; Alexander Hochbaum <alex@...udo.com>;
> > Da Xue <da@...re.computer>; Bernie Huang <phhuang@...ltek.com>; Viktor Petrenko <g0000ga@...il.com>;
> > Sascha Hauer <s.hauer@...gutronix.de>; neo_jou <neo_jou@...ltek.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@...ltek.com>
> > Signed-off-by: Hans Ulli Kroll <linux@...i-kroll.de>
> > Signed-off-by: Sascha Hauer <s.hauer@...gutronix.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 |
Powered by blists - more mailing lists