Re: [PATCH 14/19] wilc: add linux_mon.c

[Johannes Berg <johannes@sipsolutions.net>]

On Wed, 2018-09-26 at 15:55 +0530, Ajay Singh wrote:
> 
> +static struct net_device *wilc_wfi_mon; /* global monitor netdev */

There might not exist platforms with multiple devices (yet), but it's
really bad practice to do this anyway.

> +static u8 srcadd[6];
> +static u8 bssid[6];
> +
> +#define IEEE80211_RADIOTAP_F_TX_RTS	0x0004  /* used rts/cts handshake */
> +#define IEEE80211_RADIOTAP_F_TX_FAIL	0x0001  /* failed due to excessive*/

Uh.. we have a radiotap include file and you already use it, why?

> +void wilc_wfi_deinit_mon_interface(void)
> +{
> +	bool rollback_lock = false;
> +
> +	if (wilc_wfi_mon) {
> +		if (rtnl_is_locked()) {
> +			rtnl_unlock();
> +			rollback_lock = true;
> +		}
> +		unregister_netdev(wilc_wfi_mon);
> +
> +		if (rollback_lock) {
> +			rtnl_lock();
> +			rollback_lock = false;
> +		}
> +		wilc_wfi_mon = NULL;
> +	}
> +}

Uh, no, you really cannot do conditional locking like this.

But seeing things like this pretty much destroys all of the confidence I
might have had of the code, so I'd say we cannot merge this until you
can demonstrate somebody more familiar with Linux has reviewed it, I'm
just doing a drive-by review for the stack integration aspects (and
haven't even found where that happens yet).

johannes