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@xxxxxxxxxxxxxxxx/
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
