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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5f5c42585e168e252a5fa3f43325aaa360f6d27a.camel@sipsolutions.net>
Date: Fri, 21 Jun 2024 20:20:08 +0200
From: Johannes Berg <johannes@...solutions.net>
To: David Lin <yu-hao.lin@....com>, linux-wireless@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, briannorris@...omium.org,
 kvalo@...nel.org,  francesco@...cini.it, tsung-hsien.hsieh@....com
Subject: 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 :)

>  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