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

From: Johannes Berg
Date: Fri Jun 21 2024 - 14:20:24 EST


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