DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B941A20841
Subject: Re: [PATCH 14/19] wilc: add linux_mon.c
To:     Johannes Berg <johannes@sipsolutions.net>,
        <linux-wireless@vger.kernel.org>
CC:     <kvalo@codeaurora.org>, <gregkh@linuxfoundation.org>,
        <ganesh.krishna@microchip.com>, <aditya.shankar@microchip.com>,
        <venkateswara.kaja@microchip.com>, <claudiu.beznea@microchip.com>,
        <adham.abozaeid@microchip.com>
References: <1537957525-11467-1-git-send-email-ajay.kathat@microchip.com>
 <1537957525-11467-15-git-send-email-ajay.kathat@microchip.com>
 <1539009873.3687.79.camel@sipsolutions.net>
From:   Ajay Singh <ajay.kathat@microchip.com>
Message-ID: <69cff0d4-735a-853d-5dd5-b3673c806a85@microchip.com>
Date:   Thu, 11 Oct 2018 12:42:28 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <1539009873.3687.79.camel@sipsolutions.net>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Received-SPF: None (protection.outlook.com: microchip.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Oct 2018 07:12:51.5087 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: d0008b15-7fdc-4e88-cdfe-08d62f48f82b
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3f4057f3-b418-4d4e-ba84-d55b4e897d88
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB2569
X-OriginatorOrg: microchip.com
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org


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