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
