| 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: <20220520085156.GE25578@pengutronix.de>
Date: Fri, 20 May 2022 10:51:56 +0200
From: "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>
To: Pkshih <pkshih@...ltek.com>
Cc: "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
"johannes@...solutions.net" <johannes@...solutions.net>,
"kernel@...gutronix.de" <kernel@...gutronix.de>,
"neojou@...il.com" <neojou@...il.com>,
"kvalo@...nel.org" <kvalo@...nel.org>,
"tony0620emma@...il.com" <tony0620emma@...il.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"martin.blumenstingl@...glemail.com"
<martin.blumenstingl@...glemail.com>,
"linux@...i-kroll.de" <linux@...i-kroll.de>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"neo_jou@...ltek.com" <neo_jou@...ltek.com>
Subject: Re: [PATCH 06/10] rtw88: Add common USB chip support
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@...ltek.com>
> > Signed-off-by: Hans Ulli Kroll <linux@...i-kroll.de>
> > Signed-off-by: Sascha Hauer <s.hauer@...gutronix.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 |
Powered by blists - more mailing lists