RE: [PATCH 00/43] wifi: nxpwifi: create nxpwifi to support iw61x

RE: [PATCH 00/43] wifi: nxpwifi: create nxpwifi to support iw61x

[Jeff Chen]

> -----Original Message-----
> 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
> 
> 
> On Fri, 2024-06-21 at 15:51 +0800, David Lin wrote:
> >
> >   wifi: nxpwifi: add ioctl.h
> 
> even the name here sounds questionable :)

Renamed ioctl.h and sta_ioctl.h to cfg.h and sta_cfg.h

> >  48 files changed, 34928 insertions(+)
> >
> 
> This is ... huge. I don't know who could possibly review it at all.

Since this is a new driver, any suggestions on how we can make it easier for review?

> 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>

Removed in V3

> having a bunch of (sometimes wrong!) element definitions in a driver:
> 
> > +struct ieee_types_aid {
> ...
> > +     u16 aid;

Fixed in V3

> 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;

Consolidated to two workqueues and one Rx tasklet in V3.

> but also simple things like not wanting to use ERR_PTR()?

Addressed in V3

> > +static int nxpwifi_register(void *card, struct device *dev,
> > +                         struct nxpwifi_if_ops *if_ops, void
> > +**padapter)
> 
> (padapter is an out parameter)

Fixed in V3

> Why random numbers for cookies instead of just assigning from a static
> variable:
> 
> > +             *cookie = get_random_u32() | 1;

Use static variable in V3

> Open-coding -EPERM?
> 
> > +     if (nxpwifi_deinit_priv_params(priv))
> > +             return -1;

Addressed in V3

> 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;

Addressed in V3

Hi Johannes, 

We have addressed the comments you sent on June 22, 2024 in patches V3. Please review and let us know if you have any additional feedback or suggestions.

Thanks.

Jeff

adding new drivers (was: Re: [PATCH 00/43] wifi: nxpwifi: create nxpwifi to support iw61x)

[Johannes Berg]

Hi Jeff, all,

> > This is ... huge. I don't know who could possibly review it at all.
> 
> Since this is a new driver, any suggestions on how we can make it easier for review?

No, not really. Or, well, there was a suggestion to integrate it a bit
more with mwifiex, as to reduce the duplicated code. I don't find that
suggestion as entirely ill-founded as you seem to, given that large
sections of the code such are simply copy/pasted.

Now that doesn't mean you should necessarily _integrate_ the new device
into the old driver, but I don't see how doing some refactoring to share
some code really would be all that problematic, though we might have to
make some changes to the layout such as moving nxpwifi under the nxp/
dir now.

If I ask git about it (after s/nxpwifi/mwifiex/ and some other trivial
renames) then it says a lot of files are effectively the same:
...
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/11ac.c                |  104 +-
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/11ac.h                |    7 +-
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/11h.c                 |  190 +++-
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/11n.c                 |  190 ++--
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/11n.h                 |   58 +-
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/11n_aggr.c            |   51 +-
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/11n_aggr.h            |    2 +-
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/11n_rxreorder.c       |  173 +---
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/11n_rxreorder.h       |   12 +-
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/cfp.c                 |  143 +--
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/ethtool.c             |    2 +-
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/ie.c                  |  100 +-
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/sta_rx.c              |   76 +-
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/sta_tx.c              |   62 +-
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/txrx.c                |   62 +-
 drivers/net/wireless/{marvell/mwifiex => nxp/nxpwifi}/uap_txrx.c            |   74 +-
...

and in many cases here the diff looks larger than it really is because
nxpwifi appears to be missing features (TDLS, IBSS?) removed USB
support, and did some (minor) code cleanups (and anti-cleanups.)

I'm glad you now use 'struct element' instead of having your own 'struct
ieee_types_header', but that's an obvious trivial transformation even
for mwifiex, you can do that with spatch?

For a total of:
 66 files changed, 10945 insertions(+), 21952 deletions(-)
so your driver actually shrinks to about 30%, i.e. 70% of the code are
simply copied from mwifiex (and that's a conservative estimate, since it
contains a lot of the trivial cleanups you made in those lines that are
still different.)

Side note: integrating it would probably also somewhat hide issues in
your code, such as open-coding "Bubble sort" when we have a perfectly
fine implementation of sorting in the sort() function? Or using
ktime_get_ns() in an odd way for deriving random numbers, what's up with
that?? But I wonder how much you even care if you just copied such code
into the new driver without thinking about it at all.
Another example is the obvious inconsistency wrt. endian annotations, I
cannot believe that the firmware adjusts to host endian for some
commands, so I guess you must simply not care I guess. But then why do
it partially? I suppose because copy/paste is easier?


I also get it though - you don't want to maintain code that's well over
decade old, however by having 70% of the old code copied into the new
driver it seems like you're now painting yourself into that same corner
again.


> We have addressed the comments you sent on June 22, 2024 in patches V3.
> Please review and let us know if you have any additional feedback or suggestions.

If you've been reading the list (have you?) you should know that Kalle
just left a little while ago, and I'm definitely not going to be able to
really review everything at this time.


Also! Another important thing to me right now is that it doesn't seem
like you're interacting much with this community other than dumping this
code and asking for it to get reviewed and merged. I literally just
asked for everyone to start reviewing each other's patches [1] because I
will not be able to review everything (and I don't even think I (as
maintainer) _should_ necessarily be reviewing everything, I see it more
as keeping an eye on the overall wifi-specific architecture, APIs etc. I
don't see why I should point out obviously odd things like using
ktime_get_ns() for random numbers.)

[1] https://lore.kernel.org/linux-wireless/21896d2788b8bc6c7fcb534cd43e75671a57f494.camel@sipsolutions.net/


Now I'll admit that nobody else has taking that up so far either as far
as reviewing goes, but I really still hope they will... You could even
set a good example here I guess, and have developers learn something in
the process; getting folks exposed to the mailing list and thereby
current ideas, trends and how other drivers work is almost certainly a
really good thing for your own driver/development too. See, for example,
how Kalle said it simplified the locking etc. much to use the wiphy
mutex with wiphy_lock() throughout the ath12k driver now.


Anyway ... I should stop. If this feels like a bit of a rant, I guess
that's because it is. I'm not sure everyone else is doing better (*looks
over at Broadcom*), and the cc33xx folks would probably do well to pay
attention here too. At least here I am trying to help people out, but
like I see here - I offer a bit of review and then I'm asked to do "the
review" that makes the driver get accepted.

Maybe I care too much? But drivers/staging/ is supposed to be the free-
for-all thing, not drivers/net/wireless/.

Anyway, I think there are issues to address here and I'm not going to
merge a driver that's 70% copy/pasted, we have that with the older
realtek drivers and it's awful. You can submit it to the net-next tree
or staging if you like, I'm not going to block it on the grounds of
something here being bad wrt. WiFi, even if Linux support feels like a
bit of a second thought for the firmware (e.g. the lack of probe_client,
this ought to be a trivial feature to actually support if you cared
about the integration with the Linux wifi stack? Or
nxpwifi_cfg80211_change_station() being a no-op? How does that even
_work_ at all?)

johannes