On 10/8/2018 8:14 PM, Johannes Berg wrote:
> 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.
>
Sure, will work on to avoid the use of this static variable here.
>> +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?
>
Right, will remove these macro as header is already included.
>> +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).
I will refactor wilc_wfi_deinit_mon_interface() and submit more cleaner
version of this function.
Currently, I am not clear about which stack integration part is missing
. Can you please provide some more details about it.
Regards,
Ajay
