On Fri, 2022-05-20 at 10:51 +0200, s.hauer@xxxxxxxxxxxxxx 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@xxxxxxxxxxx> > > > Signed-off-by: Hans Ulli Kroll <linux@xxxxxxxxxxxxx> > > > Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> > > > --- > > > 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
