Message ID | 20240621075208.513497-1-yu-hao.lin@nxp.com |
---|---|
Headers | show |
Series | wifi: nxpwifi: create nxpwifi to support iw61x | expand |
On 24-06-21 15:51:26, David Lin wrote: > Signed-off-by: David Lin <yu-hao.lin@nxp.com> Hi David, Please read the ./Documentation/process/submitting-patches.rst Just a hint. There is no commit message. > --- > drivers/net/wireless/nxp/nxpwifi/11ac.c | 366 ++++++++++++++++++++++++ > 1 file changed, 366 insertions(+) > create mode 100644 drivers/net/wireless/nxp/nxpwifi/11ac.c > > diff --git a/drivers/net/wireless/nxp/nxpwifi/11ac.c b/drivers/net/wireless/nxp/nxpwifi/11ac.c > new file mode 100644 > index 000000000000..3e14ee602cdc > --- /dev/null > +++ b/drivers/net/wireless/nxp/nxpwifi/11ac.c > @@ -0,0 +1,366 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * NXP Wireless LAN device driver: 802.11ac > + * > + * Copyright 2011-2024 NXP > + */ [...]
Hi Abel, > From: Abel Vesa <abel.vesa@linaro.org> > Sent: Friday, June 28, 2024 4:22 PM > To: David Lin <yu-hao.lin@nxp.com> > Cc: linux-wireless@vger.kernel.org; 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 01/43] wifi: nxpwifi: add 11ac.c > > 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 24-06-21 15:51:26, David Lin wrote: > > Signed-off-by: David Lin <yu-hao.lin@nxp.com> > > Hi David, > > Please read the ./Documentation/process/submitting-patches.rst > > Just a hint. There is no commit message. > > > --- > > drivers/net/wireless/nxp/nxpwifi/11ac.c | 366 > ++++++++++++++++++++++++ > > 1 file changed, 366 insertions(+) > > create mode 100644 drivers/net/wireless/nxp/nxpwifi/11ac.c > > > > diff --git a/drivers/net/wireless/nxp/nxpwifi/11ac.c > b/drivers/net/wireless/nxp/nxpwifi/11ac.c > > new file mode 100644 > > index 000000000000..3e14ee602cdc > > --- /dev/null > > +++ b/drivers/net/wireless/nxp/nxpwifi/11ac.c > > @@ -0,0 +1,366 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * NXP Wireless LAN device driver: 802.11ac > > + * > > + * Copyright 2011-2024 NXP > > + */ > > [...] Thanks for your information. For new driver, it should be submitted one file per patch first. Once if accepted, it will become one single patch. That is the reason, I won't give commit message for the patch of a single file. David
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