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
