| Message ID | 20240621075208.513497-1-yu-hao.lin@nxp.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | wifi: nxpwifi: create nxpwifi to support iw61x | expand |
On Fri, Jun 21, 2024 at 12:52 AM David Lin <yu-hao.lin@nxp.com> wrote: > This driver is a derivative of existing Mwifiex [1] and based on similar > full-MAC architecture [2]. For the record, mwifiex is a fairly awful driver. For one, its locking schemes are generally unhelpful or nonexistent, and sometimes placebo-like (as in, they look like they are protecting certain data, but they do a very poor job of it). So I'm not sure this is a promising start. It was just yesterday, in fact, that I was telling a colleague that if mwifiex was proposed for inclusion in mainline today, I would reject it. Also, I'm far from interested in reviewing a new driver here. My only interest in mwifiex is in making sure existing hardware (especially those used on Chromebooks) doesn't get significantly worse. That interest doesn't extend to "nxpwifi". I just want to be up-front about it, and that you might as well drop me from the CC list. (Of course, that's not a requirement. I can ignore email too.) Brian
On Fri, 2024-06-21 at 15:51 +0800, David Lin wrote: > > wifi: nxpwifi: add ioctl.h even the name here sounds questionable :) > 48 files changed, 34928 insertions(+) > This is ... huge. I don't know who could possibly review it at all. A quick look suggests that it's got a bunch of things we probably really don't want to do that way any more, like using semaphores in a wifi driver: > +#include <linux/semaphore.h> having a bunch of (sometimes wrong!) element definitions in a driver: > +struct ieee_types_aid { ... > + u16 aid; embedding a (default?) wireless_dev when clearly the driver supports more than one netdev/wdev: > + struct wireless_dev wdev; Having multiple own workqueues is probably also unreasonable: > + struct workqueue_struct *dfs_cac_workqueue; > + struct workqueue_struct *dfs_chan_sw_workqueue; > + struct workqueue_struct *workqueue; > + struct workqueue_struct *rx_workqueue; > + struct workqueue_struct *host_mlme_workqueue; as is a misnamed mutex, but really you could use wiphy work and likely not have a mutex at all: > + /* mutex for scan */ > + struct mutex async_mutex; (even mac80211 only has one mutex left, and that's for a specific case where otherwise we have some issues!) questionable locking schemes, as evidenced simply by "is something locked" variables existing: > + bool rx_locked; > + bool main_locked; locking code, rather than data? > + /* spin lock for main process */ > + spinlock_t main_proc_lock; but also simple things like not wanting to use ERR_PTR()? > +static int nxpwifi_register(void *card, struct device *dev, > + struct nxpwifi_if_ops *if_ops, void **padapter) (padapter is an out parameter) Why random numbers for cookies instead of just assigning from a static variable: > + *cookie = get_random_u32() | 1; Open-coding -EPERM? > + if (nxpwifi_deinit_priv_params(priv)) > + return -1; Using -EFAULT for FW errors seems like a really bad idea: > + if (nxpwifi_drv_get_data_rate(priv, &rate)) { > + nxpwifi_dbg(priv->adapter, ERROR, > + "getting data rate error\n"); > + return -EFAULT; But I really just scrolled through this briefly, this wasn't a real review. I don't know who could do a real review, but as is, it looks like someone _should_. johannes
Hi Johannes, > From: Johannes Berg <johannes@sipsolutions.net> > Sent: Saturday, June 22, 2024 2:20 AM > To: David Lin <yu-hao.lin@nxp.com>; linux-wireless@vger.kernel.org > Cc: linux-kernel@vger.kernel.org; briannorris@chromium.org; > kvalo@kernel.org; francesco@dolcini.it; Pete Hsieh > <tsung-hsien.hsieh@nxp.com> > Subject: [EXT] Re: [PATCH 00/43] wifi: nxpwifi: create nxpwifi to support iw61x > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report > this email' button > > > On Fri, 2024-06-21 at 15:51 +0800, David Lin wrote: > > > > wifi: nxpwifi: add ioctl.h > > even the name here sounds questionable :) > > > 48 files changed, 34928 insertions(+) > > > > This is ... huge. I don't know who could possibly review it at all. > > A quick look suggests that it's got a bunch of things we probably really don't > want to do that way any more, like > > using semaphores in a wifi driver: > > > +#include <linux/semaphore.h> > > having a bunch of (sometimes wrong!) element definitions in a driver: > > > +struct ieee_types_aid { > ... > > + u16 aid; > > embedding a (default?) wireless_dev when clearly the driver supports more > than one netdev/wdev: > > > + struct wireless_dev wdev; > > Having multiple own workqueues is probably also unreasonable: > > > + struct workqueue_struct *dfs_cac_workqueue; > > + struct workqueue_struct *dfs_chan_sw_workqueue; > > + struct workqueue_struct *workqueue; > > + struct workqueue_struct *rx_workqueue; > > + struct workqueue_struct *host_mlme_workqueue; > > as is a misnamed mutex, but really you could use wiphy work and likely not > have a mutex at all: > > > + /* mutex for scan */ > > + struct mutex async_mutex; > > (even mac80211 only has one mutex left, and that's for a specific case where > otherwise we have some issues!) > > questionable locking schemes, as evidenced simply by "is something locked" > variables existing: > > > + bool rx_locked; > > + bool main_locked; > > locking code, rather than data? > > > + /* spin lock for main process */ > > + spinlock_t main_proc_lock; > > but also simple things like not wanting to use ERR_PTR()? > > > +static int nxpwifi_register(void *card, struct device *dev, > > + struct nxpwifi_if_ops *if_ops, void > > +**padapter) > > (padapter is an out parameter) > > Why random numbers for cookies instead of just assigning from a static > variable: > > > + *cookie = get_random_u32() | 1; > > Open-coding -EPERM? > > > + if (nxpwifi_deinit_priv_params(priv)) > > + return -1; > > Using -EFAULT for FW errors seems like a really bad idea: > > > + if (nxpwifi_drv_get_data_rate(priv, &rate)) { > > + nxpwifi_dbg(priv->adapter, ERROR, > > + "getting data rate error\n"); > > + return -EFAULT; > > > But I really just scrolled through this briefly, this wasn't a real review. I don't > know who could do a real review, but as is, it looks like someone _should_. > > Johannes Enhancement of nxpwifi based on your comments is ongoing. Thanks, David
This series adds support for IW61x which is a new family of 2.4/5 GHz dual-band 1x1 Wi-Fi 6, Bluetooth/Bluetooth Low Energy 5.2 and 15.4 tri-radio single chip by NXP. These devices support 20/40/80MHz single spatial stream in both STA and AP mode. Communication to the IW61x is done via SDIO interface This driver is a derivative of existing Mwifiex [1] and based on similar full-MAC architecture [2]. It has been tested with i.MX8M Mini evaluation kits in both AP and STA mode. All code passes sparse and checkpatch Data sheet (require registration): https://www.nxp.com/products/wireless-connectivity/wi-fi-plus-bluetooth- plus-802-15-4/2-4-5-ghz-dual-band-1x1-wi-fi-6-802-11ax-plus-bluetooth-5- 4-plus-802-15-4-tri-radio-solution:IW612 Known gaps to be addressed in the following patches, - Enable 11ax capabilities. This initial patch support up to 11ac. - Support DFS channel. This initial patch doesn't support DFS channel in both AP/STA mode. This patch is presented as a request for comment with the intention of being made into a patch after initial feedbacks are addressed [1] We had considered adding IW61x to mwifiex driver, however due to FW architecture, host command interface and supported features are significantly different, we have to create the new nxpwifi driver. Subsequent NXP chipsets will be added and sustained in this new driver. [2] Some features, as of now, WPA2/WPA3 personal/enterprise are offloaded to host wpa_supplicant/hostapd. David Lin (43): wifi: nxpwifi: add 11ac.c wifi: nxpwifi: add 11ac.h wifi: nxpwifi: add 11h.c wifi: nxpwifi: add 11n.c wifi: nxpwifi: add 11n.h wifi: nxpwifi: add 11n_aggr.c wifi: nxpwifi: add 11n_aggr.h wifi: nxpwifi: add 11n_rxreorder.c wifi: nxpwifi: add 11n_rxreorder.h wifi: nxpwifi: add cfg80211.c wifi: nxpwifi: add cfg80211.h wifi: nxpwifi: add cfp.c wifi: nxpwifi: add cmdevt.c wifi: nxpwifi: add cmdevt.h wifi: nxpwifi: add debugfs.c wifi: nxpwifi: add decl.h wifi: nxpwifi: add ethtool.c wifi: nxpwifi: add fw.h wifi: nxpwifi: add ie.c wifi: nxpwifi: add init.c wifi: nxpwifi: add ioctl.h wifi: nxpwifi: add join.c wifi: nxpwifi: add main.c wifi: nxpwifi: add main.h wifi: nxpwifi: add scan.c wifi: nxpwifi: add sdio.c wifi: nxpwifi: add sdio.h wifi: nxpwifi: add sta_cmd.c wifi: nxpwifi: add sta_event.c wifi: nxpwifi: add sta_ioctl.c wifi: nxpwifi: add sta_rx.c wifi: nxpwifi: add sta_tx.c wifi: nxpwifi: add txrx.c wifi: nxpwifi: add uap_cmd.c wifi: nxpwifi: add uap_event.c wifi: nxpwifi: add uap_txrx.c wifi: nxpwifi: add util.c wifi: nxpwifi: add util.h wifi: nxpwifi: add wmm.c wifi: nxpwifi: add wmm.h wifi: nxpwifi: add nxp sdio vendor id and iw61x device id wifi: nxpwifi: add Makefile and Kconfig files for nxpwifi compilation wifi: nxpwifi: add nxpwifi related information to MAINTAINERS MAINTAINERS | 7 + drivers/net/wireless/Kconfig | 1 + drivers/net/wireless/Makefile | 1 + drivers/net/wireless/nxp/Kconfig | 17 + drivers/net/wireless/nxp/Makefile | 3 + drivers/net/wireless/nxp/nxpwifi/11ac.c | 366 ++ drivers/net/wireless/nxp/nxpwifi/11ac.h | 33 + drivers/net/wireless/nxp/nxpwifi/11h.c | 432 ++ drivers/net/wireless/nxp/nxpwifi/11n.c | 851 ++++ drivers/net/wireless/nxp/nxpwifi/11n.h | 163 + drivers/net/wireless/nxp/nxpwifi/11n_aggr.c | 278 ++ drivers/net/wireless/nxp/nxpwifi/11n_aggr.h | 21 + .../net/wireless/nxp/nxpwifi/11n_rxreorder.c | 928 ++++ .../net/wireless/nxp/nxpwifi/11n_rxreorder.h | 72 + drivers/net/wireless/nxp/nxpwifi/Kconfig | 22 + drivers/net/wireless/nxp/nxpwifi/Makefile | 38 + drivers/net/wireless/nxp/nxpwifi/cfg80211.c | 3773 +++++++++++++++++ drivers/net/wireless/nxp/nxpwifi/cfg80211.h | 19 + drivers/net/wireless/nxp/nxpwifi/cfp.c | 484 +++ drivers/net/wireless/nxp/nxpwifi/cmdevt.c | 1285 ++++++ drivers/net/wireless/nxp/nxpwifi/cmdevt.h | 92 + drivers/net/wireless/nxp/nxpwifi/debugfs.c | 1042 +++++ drivers/net/wireless/nxp/nxpwifi/decl.h | 299 ++ drivers/net/wireless/nxp/nxpwifi/ethtool.c | 58 + drivers/net/wireless/nxp/nxpwifi/fw.h | 2262 ++++++++++ drivers/net/wireless/nxp/nxpwifi/ie.c | 502 +++ drivers/net/wireless/nxp/nxpwifi/init.c | 696 +++ drivers/net/wireless/nxp/nxpwifi/ioctl.h | 445 ++ drivers/net/wireless/nxp/nxpwifi/join.c | 910 ++++ drivers/net/wireless/nxp/nxpwifi/main.c | 1726 ++++++++ drivers/net/wireless/nxp/nxpwifi/main.h | 1507 +++++++ drivers/net/wireless/nxp/nxpwifi/scan.c | 2894 +++++++++++++ drivers/net/wireless/nxp/nxpwifi/sdio.c | 2646 ++++++++++++ drivers/net/wireless/nxp/nxpwifi/sdio.h | 340 ++ drivers/net/wireless/nxp/nxpwifi/sta_cmd.c | 3229 ++++++++++++++ drivers/net/wireless/nxp/nxpwifi/sta_event.c | 858 ++++ drivers/net/wireless/nxp/nxpwifi/sta_ioctl.c | 1320 ++++++ drivers/net/wireless/nxp/nxpwifi/sta_rx.c | 244 ++ drivers/net/wireless/nxp/nxpwifi/sta_tx.c | 215 + drivers/net/wireless/nxp/nxpwifi/txrx.c | 362 ++ drivers/net/wireless/nxp/nxpwifi/uap_cmd.c | 1170 +++++ drivers/net/wireless/nxp/nxpwifi/uap_event.c | 483 +++ drivers/net/wireless/nxp/nxpwifi/uap_txrx.c | 498 +++ drivers/net/wireless/nxp/nxpwifi/util.c | 751 ++++ drivers/net/wireless/nxp/nxpwifi/util.h | 90 + drivers/net/wireless/nxp/nxpwifi/wmm.c | 1397 ++++++ drivers/net/wireless/nxp/nxpwifi/wmm.h | 95 + include/linux/mmc/sdio_ids.h | 3 + 48 files changed, 34928 insertions(+) create mode 100644 drivers/net/wireless/nxp/Kconfig create mode 100644 drivers/net/wireless/nxp/Makefile create mode 100644 drivers/net/wireless/nxp/nxpwifi/11ac.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/11ac.h create mode 100644 drivers/net/wireless/nxp/nxpwifi/11h.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/11n.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/11n.h create mode 100644 drivers/net/wireless/nxp/nxpwifi/11n_aggr.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/11n_aggr.h create mode 100644 drivers/net/wireless/nxp/nxpwifi/11n_rxreorder.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/11n_rxreorder.h create mode 100644 drivers/net/wireless/nxp/nxpwifi/Kconfig create mode 100644 drivers/net/wireless/nxp/nxpwifi/Makefile create mode 100644 drivers/net/wireless/nxp/nxpwifi/cfg80211.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/cfg80211.h create mode 100644 drivers/net/wireless/nxp/nxpwifi/cfp.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/cmdevt.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/cmdevt.h create mode 100644 drivers/net/wireless/nxp/nxpwifi/debugfs.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/decl.h create mode 100644 drivers/net/wireless/nxp/nxpwifi/ethtool.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/fw.h create mode 100644 drivers/net/wireless/nxp/nxpwifi/ie.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/init.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/ioctl.h create mode 100644 drivers/net/wireless/nxp/nxpwifi/join.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/main.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/main.h create mode 100644 drivers/net/wireless/nxp/nxpwifi/scan.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/sdio.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/sdio.h create mode 100644 drivers/net/wireless/nxp/nxpwifi/sta_cmd.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/sta_event.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/sta_ioctl.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/sta_rx.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/sta_tx.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/txrx.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/uap_cmd.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/uap_event.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/uap_txrx.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/util.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/util.h create mode 100644 drivers/net/wireless/nxp/nxpwifi/wmm.c create mode 100644 drivers/net/wireless/nxp/nxpwifi/wmm.h base-commit: 238d636723a30311e20fde0a361662e829fe488b