netdev - Re: [PATCH 06/10] rtw88: Add common USB chip support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a0c7fb57d0e9c1a36f328f4e523643ba6009630f.camel@realtek.com>
Date:   Fri, 20 May 2022 09:23:11 +0000
From:   Pkshih <pkshih@...ltek.com>
To:     "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>
CC:     "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-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "martin.blumenstingl@...glemail.com" 
        <martin.blumenstingl@...glemail.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux@...i-kroll.de" <linux@...i-kroll.de>,
        "neo_jou@...ltek.com" <neo_jou@...ltek.com>
Subject: Re: [PATCH 06/10] rtw88: Add common USB chip support

On Fri, 2022-05-20 at 10:51 +0200, s.hauer@...gutronix.de wrote:
> 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
> > > 
> > 
> > > +}
> > > +
> > > +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);
> 	}
> 

With the new one, I can easily check spin_lock/_unlock is paired, so
I vote it.

--
Ping-Ke