[bug report] staging: wilc1000: move the allocation of cmd out of wilc_enqueue_cmd()

[Ajay Singh]

Hi Dan,


On Tue, 3 Jul 2018 14:56:55 +0300
Dan Carpenter <dan.carpenter at oracle.com> wrote:

> Hello Ajay Singh,
> 
> The patch ff52a57a7a42: "staging: wilc1000: move the allocation of
> cmd out of wilc_enqueue_cmd()" from Jun 26, 2018, leads to the
> following static checker warning:
> 
> 	drivers/staging/wilc1000/host_interface.c:3390 wilc_deinit()
> 	warn: inconsistent returns 'hif_deinit_lock'.

Thanks for reporting the static check warning.

> 
> drivers/staging/wilc1000/host_interface.c
>   3348          mutex_lock(&hif_deinit_lock);
>   3349  
>   3350          terminated_handle = hif_drv;
>   3351  
>   3352          del_timer_sync(&hif_drv->scan_timer);
>   3353          del_timer_sync(&hif_drv->connect_timer);
>   3354          del_timer_sync(&periodic_rssi);
>   3355          del_timer_sync(&hif_drv->remain_on_ch_timer);
>   3356  
>   3357          wilc_set_wfi_drv_handler(vif, 0, 0, 0);
>   3358          wait_for_completion(&hif_driver_comp);
>   3359  
>   3360          if (hif_drv->usr_scan_req.scan_result) {
>   3361
> hif_drv->usr_scan_req.scan_result(SCAN_EVENT_ABORTED, NULL,
> 3362
> hif_drv->usr_scan_req.arg,
> 3363                                                    NULL);
> 3364                  hif_drv->usr_scan_req.scan_result = NULL;
> 3365          } 3366 3367          hif_drv->hif_state = HOST_IF_IDLE;
>   3368  
>   3369          if (clients_count == 1) {
>   3370                  struct host_if_msg *msg;
>   3371  
>   3372                  msg = wilc_alloc_work(vif,
> handle_hif_exit_work, true); 3373                  if (IS_ERR(msg))
>   3374                          return PTR_ERR(msg);
>                                        ^^^^^^^^^^^^
> We should unlock.  Probably set terminated_handle to NULL as well?
> 

Sure, I will add the code to unlock and set the terminated_handle to
NULL and cleanup other variables.

As its error path during the cleanup flow, so i think we should
only avoid the posting of work_queue and rest of the cleanup code should
be executed to cleanup.

Something like below i.e instead of using direct return only block
code which depends on 'msg' under if condition and execute the rest of
cleanup code for all scenario.

	if (!IS_ERR(msg)) {
		result = wilc_enqueue_work(msg);
		if (result)
			netdev_err(vif->ndev, "deinit : Error(%d)\n",result);
		else
			wait_for_completion(&msg->work_comp);
		kfree(msg);
	}
 	destroy_workqueue(hif_workqueue);


I will include these changes and submit the patch. 

>   3375  
>   3376                  result = wilc_enqueue_work(msg);
>   3377                  if (result)
>   3378                          netdev_err(vif->ndev, "deinit :
> Error(%d)\n", result); 3379                  else
>   3380                          wait_for_completion(&msg->work_comp);
>   3381                  kfree(msg);
>   3382                  destroy_workqueue(hif_workqueue);
>   3383          }
>   3384  
>   3385          kfree(hif_drv);
>   3386  
>   3387          clients_count--;
>   3388          terminated_handle = NULL;
>   3389          mutex_unlock(&hif_deinit_lock);
>   3390          return result;
>   3391  }
> 
> regards,
> dan carpenter
> _______________________________________________
> devel mailing list
> devel at linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel