On Mon, Nov 28, 2022 at 02:00:54AM +0000, Ping-Ke Shih wrote: > > > > -----Original Message----- > > From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> > > Sent: Tuesday, November 22, 2022 10:52 PM > > To: linux-wireless@xxxxxxxxxxxxxxx > > Cc: Neo Jou <neojou@xxxxxxxxx>; Hans Ulli Kroll <linux@xxxxxxxxxxxxx>; Ping-Ke Shih <pkshih@xxxxxxxxxxx>; > > Yan-Hsuan Chuang <tony0620emma@xxxxxxxxx>; Kalle Valo <kvalo@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; > > linux-kernel@xxxxxxxxxxxxxxx; Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>; > > kernel@xxxxxxxxxxxxxx; Johannes Berg <johannes@xxxxxxxxxxxxxxxx>; Alexander Hochbaum <alex@xxxxxxxxxx>; > > Da Xue <da@libre.computer>; Bernie Huang <phhuang@xxxxxxxxxxx>; Viktor Petrenko <g0000ga@xxxxxxxxx>; > > Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>; neo_jou <neo_jou@xxxxxxxxxxx> > > 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@xxxxxxxxxxx> > > Signed-off-by: Hans Ulli Kroll <linux@xxxxxxxxxxxxx> > > Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> > > --- > > > > 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 |
