On 23.08.2018 17:36, Ajay Singh wrote:

On Thu, 23 Aug 2018 11:11:18 +0300
Claudiu Beznea [off-list ref] wrote:

quoted

On 14.08.2018 09:50, Ajay Singh wrote:

quoted

Remove the use of static variable 'terminated_handle' and instead
move in wilc_vif struct.
After moving this variable to wilc_vif struct its not required to
keep 'terminated_handle', so changed it to boolean type.  

You can remove it at all and use wilc->hif_deinit_lock mutex also in
wilc_scan_complete_received() and wilc_network_info_received() it is
used in wilc_gnrl_async_info_received().

In my understanding, 'terminated_handle' is used to know the
status when interface is getting deinit(). During deinitialization
of an interface if any async event received for the interface(from
firmware) should be ignored.

'terminated_handle' true only inside mutex. So, outside of it it will be
false, so *mostly* it will tell you when mutex is locked for deinit.
*Mostly*, because context switches may happen while a mutex is locked.

With the current approach you have this code:

int wilc_deinit(struct wilc_vif *vif)
{
	// ...
	mutex_lock(&vif->wilc->hif_deinit_lock);

	// (A)

	vif->is_termination_progress = true;
	// ...
	vif->is_termination_progress = false;

	mutex_unlokc(&vif->wilc->hif_deinit_lock);
}

And:

void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
{
	// ...
	if (!hif_drv || vif->is_termination_progress) {
		netdev_err(vif->ndev, "driver not init[%p]\n", hif_drv);
		return;
	}

	// ...
	
	// (B)
	result = wilc_enqueue_work(msg);	
	// ...
}

And:

static int wilc_enqueue_work(struct host_if_msg *msg)

{

        INIT_WORK(&msg->work, msg->fn);



        if (!msg->vif || !msg->vif->wilc || !msg->vif->wilc->hif_workqueue)

                return -EINVAL;


	// (C)
        if (!queue_work(msg->vif->wilc->hif_workqueue, &msg->work))

                return -EINVAL;



        return 0;

}


You may have the following scenario:
1. context switch in wilc_deinit() just after the mutex is taken (point A
above). vif->is_termination_progress = false at this point.

2. a new packet is received and wilc_network_info_received() gets executed
and execution reaches point B, goes to wilc_enqueue_work() and suspend at
point C then context switch.

3. wilc_deinit() resumes and finish its execution.

4. wilc_enqueue_work() resumes and queue_work() is executed but you already
freed the hif_workqueue. You will have a crash here.

Notice that hif_drv is not set to NULL on wilc_deinit() after it is kfree().

In my opinion its not right to only wait for the mutex in any of
callback e.g wilc_scan_complete_received() because it will delay the
handling of callback and try to process the event once lock is
available for the interface which is already de-initialized.

But this is already done for wilc_gnrl_async_info_received().

Based on my understand 'mutex' alone is not enough to
handle this and we some extra check to know if interface is down.

terminated_handle will *mostly* tell you when the mutex is locked, nothing
more.

I will

check more about this patch how to handle the extra scenario for this
case.
Please suggest if someone has better idea on how to handle this. 

quoted

quoted

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/host_interface.c     | 11 +++++------
 drivers/staging/wilc1000/wilc_wfi_netdevice.h |  1 +
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c

b/drivers/staging/wilc1000/host_interface.c index
d5d81843..f71f451f 100644 ---
a/drivers/staging/wilc1000/host_interface.c +++
b/drivers/staging/wilc1000/host_interface.c @@ -185,7 +185,6 @@
struct join_bss_param { u8 start_time[4];
 };
 
-static struct host_if_drv *terminated_handle;
 static u8 p2p_listen_state;
 static struct timer_list periodic_rssi;
 static struct wilc_vif *periodic_rssi_vif;

@@ -3505,7 +3504,7 @@ int wilc_deinit(struct wilc_vif *vif)
 
 	mutex_lock(&vif->wilc->hif_deinit_lock);
 
-	terminated_handle = hif_drv;
+	vif->is_termination_progress = true;
 
 	del_timer_sync(&hif_drv->scan_timer);
 	del_timer_sync(&hif_drv->connect_timer);

@@ -3543,7 +3542,7 @@ int wilc_deinit(struct wilc_vif *vif)
 	kfree(hif_drv);
 
 	vif->wilc->clients_count--;
-	terminated_handle = NULL;
+	vif->is_termination_progress = false;
 	mutex_unlock(&vif->wilc->hif_deinit_lock);
 	return result;
 }

@@ -3565,7 +3564,7 @@ void wilc_network_info_received(struct wilc

*wilc, u8 *buffer, u32 length) return;
 	hif_drv = vif->hif_drv;
 
-	if (!hif_drv || hif_drv == terminated_handle) {
+	if (!hif_drv || vif->is_termination_progress) {
 		netdev_err(vif->ndev, "driver not init[%p]\n",
hif_drv); return;
 	}

@@ -3611,7 +3610,7 @@ void wilc_gnrl_async_info_received(struct

wilc *wilc, u8 *buffer, u32 length) 
 	hif_drv = vif->hif_drv;
 
-	if (!hif_drv || hif_drv == terminated_handle) {
+	if (!hif_drv || vif->is_termination_progress) {
 		mutex_unlock(&wilc->hif_deinit_lock);
 		return;
 	}

@@ -3662,7 +3661,7 @@ void wilc_scan_complete_received(struct wilc

*wilc, u8 *buffer, u32 length) return;
 	hif_drv = vif->hif_drv;
 
-	if (!hif_drv || hif_drv == terminated_handle)
+	if (!hif_drv || vif->is_termination_progress)
 		return;
 
 	if (hif_drv->usr_scan_req.scan_result) {

diff --git a/drivers/staging/wilc1000/wilc_wfi_netdevice.h

b/drivers/staging/wilc1000/wilc_wfi_netdevice.h index
eb00e42..ba606d0 100644 ---
a/drivers/staging/wilc1000/wilc_wfi_netdevice.h +++
b/drivers/staging/wilc1000/wilc_wfi_netdevice.h @@ -121,6 +121,7 @@
struct wilc_vif { struct timer_list during_ip_timer;
 	bool obtaining_ip;
 	u8 mc_mac_addr_list[WILC_MULTICAST_TABLE_SIZE][ETH_ALEN];
+	bool is_termination_progress;
 };
 
 struct wilc {