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

[Johannes Berg <johannes@sipsolutions.net>]

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