[PATCH 0/3] staging: wilc1000: semaphores to mutexes in host_interface

[PATCH 0/3] staging: wilc1000: semaphores to mutexes in host_interface

[Alison Schofield]

Patchset replaces semaphores with mutexes in the host_interface of 
wilc1000.  This is one piece of the task as listed in wilc1000/TODO.

Alison Schofield (3):
  staging: wilc1000: host_interface: remove unused semaphores
  staging: wilc1000: host_interface: replace semaphores with mutexes
  staging: wilc1000: host_interface: destroy mutexes at init/deinit

 drivers/staging/wilc1000/host_interface.c | 147 ++++++++++++++++--------------
 drivers/staging/wilc1000/host_interface.h |  12 +--
 2 files changed, 84 insertions(+), 75 deletions(-)

-- 
2.1.4

[PATCH 1/3] staging: wilc1000: host_interface: remove unused semaphores

[Alison Schofield]

Remove unused semaphore declarations, initializations, and unlocks.

The functions that locked these semaphores were previously removed,
so this cleans up the remains.

Signed-off-by: Alison Schofield <amsfield22@gmail.com>
---
 drivers/staging/wilc1000/host_interface.c | 7 -------
 drivers/staging/wilc1000/host_interface.h | 2 --
 2 files changed, 9 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index d1eedfb..66a0ac9 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -1973,7 +1973,6 @@ static s32 Handle_GetChnl(struct wilc_vif *vif)
 {
 	s32 result = 0;
 	struct wid wid;
-	struct host_if_drv *hif_drv = vif->hif_drv;
 
 	wid.id = (u16)WID_CURRENT_CHANNEL;
 	wid.type = WID_CHAR;
@@ -1988,8 +1987,6 @@ static s32 Handle_GetChnl(struct wilc_vif *vif)
 		result = -EFAULT;
 	}
 
-	up(&hif_drv->sem_get_chnl);
-
 	return result;
 }
 
@@ -2017,7 +2014,6 @@ static void Handle_GetLinkspeed(struct wilc_vif *vif)
 {
 	s32 result = 0;
 	struct wid wid;
-	struct host_if_drv *hif_drv = vif->hif_drv;
 
 	link_speed = 0;
 
@@ -2033,7 +2029,6 @@ static void Handle_GetLinkspeed(struct wilc_vif *vif)
 		result = -EFAULT;
 	}
 
-	up(&hif_drv->sem_get_link_speed);
 }
 
 static s32 Handle_GetStatistics(struct wilc_vif *vif,
@@ -3630,8 +3625,6 @@ int wilc_init(struct net_device *dev, struct host_if_drv **hif_drv_handler)
 	sema_init(&hif_drv->sem_test_key_block, 0);
 	sema_init(&hif_drv->sem_test_disconn_block, 0);
 	sema_init(&hif_drv->sem_get_rssi, 0);
-	sema_init(&hif_drv->sem_get_link_speed, 0);
-	sema_init(&hif_drv->sem_get_chnl, 0);
 	sema_init(&hif_drv->sem_inactive_time, 0);
 
 	if (clients_count == 0)	{
diff --git a/drivers/staging/wilc1000/host_interface.h b/drivers/staging/wilc1000/host_interface.h
index 5e65f2c..6135024 100644
--- a/drivers/staging/wilc1000/host_interface.h
+++ b/drivers/staging/wilc1000/host_interface.h
@@ -278,8 +278,6 @@ struct host_if_drv {
 	struct semaphore sem_test_key_block;
 	struct semaphore sem_test_disconn_block;
 	struct semaphore sem_get_rssi;
-	struct semaphore sem_get_link_speed;
-	struct semaphore sem_get_chnl;
 	struct semaphore sem_inactive_time;
 
 	struct timer_list scan_timer;
-- 
2.1.4

Re: [Outreachy kernel] [PATCH 1/3] staging: wilc1000: host_interface: remove unused semaphores

[Arnd Bergmann]

On Tuesday 16 February 2016 00:30:35 Alison Schofield wrote:
> Remove unused semaphore declarations, initializations, and unlocks.
> 
> The functions that locked these semaphores were previously removed,
> so this cleans up the remains.
> 
> Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> 

Nice!

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Re: [Outreachy kernel] [PATCH 1/3] staging: wilc1000: host_interface: remove unused semaphores

[Alison Schofield]

On Tue, Feb 16, 2016 at 09:33:38AM +0100, Arnd Bergmann wrote:
> On Tuesday 16 February 2016 00:30:35 Alison Schofield wrote:
> > Remove unused semaphore declarations, initializations, and unlocks.
> > 
> > The functions that locked these semaphores were previously removed,
> > so this cleans up the remains.
> > 
> > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> > 
> 
> Nice!
> 
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>

I'm wondering if I should have cleaned this area up more. I was 
cleaning up the semaphores because the functions that locked those
HOST_IF_MSG's has been removed.  However, the "handlers" for these 
returned HOST_IF_MSG's still exist, as does the case in the 
hostIFthread() switch.

They seem like artifacts, but then again, they could be needed for
some pass-thru mode.

Leave it, clean it, or seek more info?
Thanks,
alisons

alisons

Re: [Outreachy kernel] [PATCH 1/3] staging: wilc1000: host_interface: remove unused semaphores

[Arnd Bergmann]

On Thursday 18 February 2016 22:45:53 Alison Schofield wrote:
> On Tue, Feb 16, 2016 at 09:33:38AM +0100, Arnd Bergmann wrote:
> > On Tuesday 16 February 2016 00:30:35 Alison Schofield wrote:
> > > Remove unused semaphore declarations, initializations, and unlocks.
> > > 
> > > The functions that locked these semaphores were previously removed,
> > > so this cleans up the remains.
> > > 
> > > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> > > 
> > 
> > Nice!
> > 
> > Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> 
> I'm wondering if I should have cleaned this area up more. I was 
> cleaning up the semaphores because the functions that locked those
> HOST_IF_MSG's has been removed.  However, the "handlers" for these 
> returned HOST_IF_MSG's still exist, as does the case in the 
> hostIFthread() switch.
> 
> They seem like artifacts, but then again, they could be needed for
> some pass-thru mode.
> 
> Leave it, clean it, or seek more info?

Maybe do one patch, and Cc the maintainers on that patch. From looking
at the history, I'm quite sure that this is also dead code.

My understanding is that this was originally part of an operating-system
independent library abstraction for a larger set of devices. The driver
is being stripped down to only support Linux and only a couple
of devices, and that has left tons of code that is never called.

	Arnd

[PATCH 2/3] staging: wilc1000: host_interface: replace semaphores with mutexes

[Alison Schofield]

Replace semaphores with mutexes in the host_interface driver.

This is a safe performance improvement because the usage model
meets the principle of ownership for mutexes: the thread that
locks is the same thread that unlocks.

Signed-off-by: Alison Schofield <amsfield22@gmail.com>
---
 drivers/staging/wilc1000/host_interface.c | 114 +++++++++++++++---------------
 drivers/staging/wilc1000/host_interface.h |  10 +--
 2 files changed, 63 insertions(+), 61 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index 66a0ac9..45e6ed3 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -240,10 +240,12 @@ bool wilc_optaining_ip;
 static u8 P2P_LISTEN_STATE;
 static struct task_struct *hif_thread_handler;
 static struct message_queue hif_msg_q;
-static struct semaphore hif_sema_thread;
-static struct semaphore hif_sema_driver;
-static struct semaphore hif_sema_wait_response;
-static struct semaphore hif_sema_deinit;
+
+static struct mutex hif_mutex_thread;	/* protect host interface thread    */
+static struct mutex hif_mutex_driver;	/* protect deinit of driver handler */
+static struct mutex hif_mutex_deinit;	/* protect host interface deinit    */
+static struct mutex hif_mutex_wait_response;	/* protect MSGs:	    */
+						/* macaddr,power,statistics */
 static struct timer_list periodic_rssi;
 
 u8 wilc_multicast_mac_addr_list[WILC_MULTICAST_TABLE_SIZE][ETH_ALEN];
@@ -332,7 +334,7 @@ static s32 handle_set_wfi_drv_handler(struct wilc_vif *vif,
 				      hif_drv_handler->handler);
 
 	if (!hif_drv_handler->handler)
-		up(&hif_sema_driver);
+		mutex_unlock(&hif_mutex_driver);
 
 	if (result) {
 		PRINT_ER("Failed to set driver handler\n");
@@ -357,7 +359,7 @@ static s32 handle_set_operation_mode(struct wilc_vif *vif,
 				      wilc_get_vif_idx(vif));
 
 	if ((hif_op_mode->mode) == IDLE_MODE)
-		up(&hif_sema_driver);
+		mutex_unlock(&hif_mutex_driver);
 
 	if (result) {
 		PRINT_ER("Failed to set driver handler\n");
@@ -471,7 +473,7 @@ static s32 handle_get_mac_address(struct wilc_vif *vif,
 		PRINT_ER("Failed to get mac address\n");
 		result = -EFAULT;
 	}
-	up(&hif_sema_wait_response);
+	mutex_unlock(&hif_mutex_wait_response);
 
 	return result;
 }
@@ -484,7 +486,7 @@ static s32 handle_cfg_param(struct wilc_vif *vif,
 	struct host_if_drv *hif_drv = vif->hif_drv;
 	u8 wid_cnt = 0;
 
-	down(&hif_drv->sem_cfg_values);
+	mutex_lock(&hif_drv->mutex_cfg_values);
 
 	if (cfg_param_attr->cfg_attr_info.flag & BSS_TYPE) {
 		if (cfg_param_attr->cfg_attr_info.bss_type < 6) {
@@ -765,14 +767,14 @@ static s32 handle_cfg_param(struct wilc_vif *vif,
 		PRINT_ER("Error in setting CFG params\n");
 
 ERRORHANDLER:
-	up(&hif_drv->sem_cfg_values);
+	mutex_unlock(&hif_drv->mutex_cfg_values);
 	return result;
 }
 
 static void Handle_wait_msg_q_empty(void)
 {
 	wilc_initialized = 0;
-	up(&hif_sema_wait_response);
+	mutex_unlock(&hif_mutex_wait_response);
 }
 
 static s32 Handle_ScanDone(struct wilc_vif *vif,
@@ -1708,7 +1710,7 @@ static int Handle_Key(struct wilc_vif *vif,
 						      &wid, 1,
 						      wilc_get_vif_idx(vif));
 		}
-		up(&hif_drv->sem_test_key_block);
+		mutex_unlock(&hif_drv->mutex_wep_key);
 		break;
 
 	case WPA_RX_GTK:
@@ -1743,7 +1745,7 @@ static int Handle_Key(struct wilc_vif *vif,
 						      wilc_get_vif_idx(vif));
 
 			kfree(pu8keybuf);
-			up(&hif_drv->sem_test_key_block);
+			mutex_unlock(&hif_drv->mutex_wep_key);
 		} else if (pstrHostIFkeyAttr->action & ADDKEY) {
 			pu8keybuf = kzalloc(RX_MIC_KEY_MSG_LEN, GFP_KERNEL);
 			if (pu8keybuf == NULL) {
@@ -1773,7 +1775,7 @@ static int Handle_Key(struct wilc_vif *vif,
 						      wilc_get_vif_idx(vif));
 
 			kfree(pu8keybuf);
-			up(&hif_drv->sem_test_key_block);
+			mutex_unlock(&hif_drv->mutex_wep_key);
 		}
 _WPARxGtk_end_case_:
 		kfree(pstrHostIFkeyAttr->attr.wpa.key);
@@ -1812,7 +1814,7 @@ _WPARxGtk_end_case_:
 						      strWIDList, 2,
 						      wilc_get_vif_idx(vif));
 			kfree(pu8keybuf);
-			up(&hif_drv->sem_test_key_block);
+			mutex_unlock(&hif_drv->mutex_wep_key);
 		} else if (pstrHostIFkeyAttr->action & ADDKEY) {
 			pu8keybuf = kmalloc(PTK_KEY_MSG_LEN, GFP_KERNEL);
 			if (!pu8keybuf) {
@@ -1835,7 +1837,7 @@ _WPARxGtk_end_case_:
 						      &wid, 1,
 						      wilc_get_vif_idx(vif));
 			kfree(pu8keybuf);
-			up(&hif_drv->sem_test_key_block);
+			mutex_unlock(&hif_drv->mutex_wep_key);
 		}
 
 _WPAPtk_end_case_:
@@ -1957,7 +1959,7 @@ static void Handle_Disconnect(struct wilc_vif *vif)
 		}
 	}
 
-	up(&hif_drv->sem_test_disconn_block);
+	mutex_unlock(&hif_drv->mutex_disconnect);
 }
 
 void wilc_resolve_disconnect_aberration(struct wilc_vif *vif)
@@ -2007,7 +2009,7 @@ static void Handle_GetRssi(struct wilc_vif *vif)
 		result = -EFAULT;
 	}
 
-	up(&vif->hif_drv->sem_get_rssi);
+	mutex_unlock(&vif->hif_drv->mutex_get_rssi);
 }
 
 static void Handle_GetLinkspeed(struct wilc_vif *vif)
@@ -2081,7 +2083,7 @@ static s32 Handle_GetStatistics(struct wilc_vif *vif,
 		wilc_enable_tcp_ack_filter(false);
 
 	if (pstrStatistics != &vif->wilc->dummy_statistics)
-		up(&hif_sema_wait_response);
+		mutex_unlock(&hif_mutex_wait_response);
 	return 0;
 }
 
@@ -2126,7 +2128,7 @@ static s32 Handle_Get_InActiveTime(struct wilc_vif *vif,
 
 	PRINT_D(CFG80211_DBG, "Getting inactive time : %d\n", inactive_time);
 
-	up(&hif_drv->sem_inactive_time);
+	mutex_unlock(&hif_drv->mutex_inactive_time);
 
 	return result;
 }
@@ -2319,7 +2321,7 @@ static void Handle_DelAllSta(struct wilc_vif *vif,
 ERRORHANDLER:
 	kfree(wid.val);
 
-	up(&hif_sema_wait_response);
+	mutex_unlock(&hif_mutex_wait_response);
 }
 
 static void Handle_DelStation(struct wilc_vif *vif,
@@ -2627,7 +2629,7 @@ static s32 Handle_DelAllRxBASessions(struct wilc_vif *vif,
 
 	kfree(wid.val);
 
-	up(&hif_sema_wait_response);
+	mutex_unlock(&hif_mutex_wait_response);
 
 	return result;
 }
@@ -2663,7 +2665,7 @@ static void handle_get_tx_pwr(struct wilc_vif *vif, u8 *tx_pwr)
 	if (ret)
 		netdev_err(vif->ndev, "Failed to get TX PWR\n");
 
-	up(&hif_sema_wait_response);
+	mutex_unlock(&hif_mutex_wait_response);
 }
 
 static int hostIFthread(void *pvArg)
@@ -2870,7 +2872,7 @@ static int hostIFthread(void *pvArg)
 		}
 	}
 
-	up(&hif_sema_thread);
+	mutex_unlock(&hif_mutex_thread);
 	return 0;
 }
 
@@ -2933,7 +2935,7 @@ int wilc_remove_wep_key(struct wilc_vif *vif, u8 index)
 	result = wilc_mq_send(&hif_msg_q, &msg, sizeof(struct host_if_msg));
 	if (result)
 		PRINT_ER("Error in sending message queue : Request to remove WEP key\n");
-	down(&hif_drv->sem_test_key_block);
+	mutex_lock(&hif_drv->mutex_wep_key);
 
 	return result;
 }
@@ -2961,7 +2963,7 @@ int wilc_set_wep_default_keyid(struct wilc_vif *vif, u8 index)
 	result = wilc_mq_send(&hif_msg_q, &msg, sizeof(struct host_if_msg));
 	if (result)
 		PRINT_ER("Error in sending message queue : Default key index\n");
-	down(&hif_drv->sem_test_key_block);
+	mutex_lock(&hif_drv->mutex_wep_key);
 
 	return result;
 }
@@ -2994,7 +2996,7 @@ int wilc_add_wep_key_bss_sta(struct wilc_vif *vif, const u8 *key, u8 len,
 	result = wilc_mq_send(&hif_msg_q, &msg, sizeof(struct host_if_msg));
 	if (result)
 		PRINT_ER("Error in sending message queue :WEP Key\n");
-	down(&hif_drv->sem_test_key_block);
+	mutex_lock(&hif_drv->mutex_wep_key);
 
 	return result;
 }
@@ -3035,7 +3037,7 @@ int wilc_add_wep_key_bss_ap(struct wilc_vif *vif, const u8 *key, u8 len,
 
 	if (result)
 		PRINT_ER("Error in sending message queue :WEP Key\n");
-	down(&hif_drv->sem_test_key_block);
+	mutex_lock(&hif_drv->mutex_wep_key);
 
 	return result;
 }
@@ -3101,7 +3103,7 @@ int wilc_add_ptk(struct wilc_vif *vif, const u8 *ptk, u8 ptk_key_len,
 	if (result)
 		PRINT_ER("Error in sending message queue:  PTK Key\n");
 
-	down(&hif_drv->sem_test_key_block);
+	mutex_lock(&hif_drv->mutex_wep_key);
 
 	return result;
 }
@@ -3169,7 +3171,7 @@ int wilc_add_rx_gtk(struct wilc_vif *vif, const u8 *rx_gtk, u8 gtk_key_len,
 	if (result)
 		PRINT_ER("Error in sending message queue:  RX GTK\n");
 
-	down(&hif_drv->sem_test_key_block);
+	mutex_lock(&hif_drv->mutex_wep_key);
 
 	return result;
 }
@@ -3225,7 +3227,7 @@ int wilc_get_mac_address(struct wilc_vif *vif, u8 *mac_addr)
 		return -EFAULT;
 	}
 
-	down(&hif_sema_wait_response);
+	mutex_lock(&hif_mutex_wait_response);
 	return result;
 }
 
@@ -3316,7 +3318,7 @@ int wilc_disconnect(struct wilc_vif *vif, u16 reason_code)
 	if (result)
 		PRINT_ER("Failed to send message queue: disconnect\n");
 
-	down(&hif_drv->sem_test_disconn_block);
+	mutex_lock(&hif_drv->mutex_disconnect);
 
 	return result;
 }
@@ -3438,7 +3440,7 @@ s32 wilc_get_inactive_time(struct wilc_vif *vif, const u8 *mac,
 	if (result)
 		PRINT_ER("Failed to send get host channel param's message queue ");
 
-	down(&hif_drv->sem_inactive_time);
+	mutex_lock(&hif_drv->mutex_inactive_time);
 
 	*pu32InactiveTime = inactive_time;
 
@@ -3461,7 +3463,7 @@ int wilc_get_rssi(struct wilc_vif *vif, s8 *rssi_level)
 		return -EFAULT;
 	}
 
-	down(&hif_drv->sem_get_rssi);
+	mutex_lock(&hif_drv->mutex_get_rssi);
 
 	if (!rssi_level) {
 		PRINT_ER("RSS pointer value is null");
@@ -3490,7 +3492,7 @@ int wilc_get_statistics(struct wilc_vif *vif, struct rf_info *stats)
 	}
 
 	if (stats != &vif->wilc->dummy_statistics)
-		down(&hif_sema_wait_response);
+		mutex_lock(&hif_mutex_wait_response);
 	return result;
 }
 
@@ -3600,7 +3602,7 @@ int wilc_init(struct net_device *dev, struct host_if_drv **hif_drv_handler)
 
 	scan_while_connected = false;
 
-	sema_init(&hif_sema_wait_response, 0);
+	mutex_init(&hif_mutex_wait_response);
 
 	hif_drv  = kzalloc(sizeof(struct host_if_drv), GFP_KERNEL);
 	if (!hif_drv) {
@@ -3617,15 +3619,15 @@ int wilc_init(struct net_device *dev, struct host_if_drv **hif_drv_handler)
 	wilc_optaining_ip = false;
 
 	if (clients_count == 0)	{
-		sema_init(&hif_sema_thread, 0);
-		sema_init(&hif_sema_driver, 0);
-		sema_init(&hif_sema_deinit, 1);
+		mutex_init(&hif_mutex_thread);
+		mutex_init(&hif_mutex_driver);
+		mutex_init(&hif_mutex_deinit);
 	}
 
-	sema_init(&hif_drv->sem_test_key_block, 0);
-	sema_init(&hif_drv->sem_test_disconn_block, 0);
-	sema_init(&hif_drv->sem_get_rssi, 0);
-	sema_init(&hif_drv->sem_inactive_time, 0);
+	mutex_init(&hif_drv->mutex_wep_key);
+	mutex_init(&hif_drv->mutex_disconnect);
+	mutex_init(&hif_drv->mutex_get_rssi);
+	mutex_init(&hif_drv->mutex_inactive_time);
 
 	if (clients_count == 0)	{
 		result = wilc_mq_create(&hif_msg_q);
@@ -3652,8 +3654,8 @@ int wilc_init(struct net_device *dev, struct host_if_drv **hif_drv_handler)
 	setup_timer(&hif_drv->connect_timer, TimerCB_Connect, 0);
 	setup_timer(&hif_drv->remain_on_ch_timer, ListenTimerCB, 0);
 
-	sema_init(&hif_drv->sem_cfg_values, 1);
-	down(&hif_drv->sem_cfg_values);
+	mutex_init(&hif_drv->mutex_cfg_values);
+	mutex_lock(&hif_drv->mutex_cfg_values);
 
 	hif_drv->hif_state = HOST_IF_IDLE;
 	hif_drv->cfg_values.site_survey_enabled = SITE_SURVEY_OFF;
@@ -3664,7 +3666,7 @@ int wilc_init(struct net_device *dev, struct host_if_drv **hif_drv_handler)
 
 	hif_drv->p2p_timeout = 0;
 
-	up(&hif_drv->sem_cfg_values);
+	mutex_unlock(&hif_drv->mutex_cfg_values);
 
 	clients_count++;
 
@@ -3687,7 +3689,7 @@ int wilc_deinit(struct wilc_vif *vif)
 		return -EFAULT;
 	}
 
-	down(&hif_sema_deinit);
+	mutex_lock(&hif_mutex_deinit);
 
 	terminated_handle = hif_drv;
 
@@ -3697,7 +3699,7 @@ int wilc_deinit(struct wilc_vif *vif)
 	del_timer_sync(&hif_drv->remain_on_ch_timer);
 
 	wilc_set_wfi_drv_handler(vif, 0, 0);
-	down(&hif_sema_driver);
+	mutex_lock(&hif_mutex_driver);
 
 	if (hif_drv->usr_scan_req.scan_result) {
 		hif_drv->usr_scan_req.scan_result(SCAN_EVENT_ABORTED, NULL,
@@ -3720,7 +3722,7 @@ int wilc_deinit(struct wilc_vif *vif)
 		if (result != 0)
 			PRINT_ER("Error in sending deinit's message queue message function: Error(%d)\n", result);
 
-		down(&hif_sema_thread);
+		mutex_lock(&hif_mutex_thread);
 
 		wilc_mq_destroy(&hif_msg_q);
 	}
@@ -3729,7 +3731,7 @@ int wilc_deinit(struct wilc_vif *vif)
 
 	clients_count--;
 	terminated_handle = NULL;
-	up(&hif_sema_deinit);
+	mutex_unlock(&hif_mutex_deinit);
 	return result;
 }
 
@@ -3776,25 +3778,25 @@ void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *pu8Buffer,
 	struct host_if_drv *hif_drv = NULL;
 	struct wilc_vif *vif;
 
-	down(&hif_sema_deinit);
+	mutex_lock(&hif_mutex_deinit);
 
 	id = ((pu8Buffer[u32Length - 4]) | (pu8Buffer[u32Length - 3] << 8) | (pu8Buffer[u32Length - 2] << 16) | (pu8Buffer[u32Length - 1] << 24));
 	vif = wilc_get_vif_from_idx(wilc, id);
 	if (!vif) {
-		up(&hif_sema_deinit);
+		mutex_unlock(&hif_mutex_deinit);
 		return;
 	}
 
 	hif_drv = vif->hif_drv;
 
 	if (!hif_drv || hif_drv == terminated_handle) {
-		up(&hif_sema_deinit);
+		mutex_unlock(&hif_mutex_deinit);
 		return;
 	}
 
 	if (!hif_drv->usr_conn_req.conn_result) {
 		PRINT_ER("Received mac status is not needed when there is no current Connect Reques\n");
-		up(&hif_sema_deinit);
+		mutex_unlock(&hif_mutex_deinit);
 		return;
 	}
 
@@ -3811,7 +3813,7 @@ void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *pu8Buffer,
 	if (result)
 		PRINT_ER("Error in sending message queue asynchronous message info: Error(%d)\n", result);
 
-	up(&hif_sema_deinit);
+	mutex_unlock(&hif_mutex_deinit);
 }
 
 void wilc_scan_complete_received(struct wilc *wilc, u8 *pu8Buffer,
@@ -4115,7 +4117,7 @@ int wilc_del_allstation(struct wilc_vif *vif, u8 mac_addr[][ETH_ALEN])
 	if (result)
 		PRINT_ER("wilc_mq_send fail\n");
 
-	down(&hif_sema_wait_response);
+	mutex_lock(&hif_mutex_wait_response);
 
 	return result;
 }
@@ -4448,7 +4450,7 @@ int wilc_get_tx_power(struct wilc_vif *vif, u8 *tx_power)
 	if (ret)
 		netdev_err(vif->ndev, "Failed to get TX PWR\n");
 
-	down(&hif_sema_wait_response);
+	mutex_lock(&hif_mutex_wait_response);
 	*tx_power = msg.body.tx_power.tx_pwr;
 
 	return ret;
diff --git a/drivers/staging/wilc1000/host_interface.h b/drivers/staging/wilc1000/host_interface.h
index 6135024..a438603 100644
--- a/drivers/staging/wilc1000/host_interface.h
+++ b/drivers/staging/wilc1000/host_interface.h
@@ -274,11 +274,11 @@ struct host_if_drv {
 	u8 assoc_bssid[ETH_ALEN];
 	struct cfg_param_val cfg_values;
 
-	struct semaphore sem_cfg_values;
-	struct semaphore sem_test_key_block;
-	struct semaphore sem_test_disconn_block;
-	struct semaphore sem_get_rssi;
-	struct semaphore sem_inactive_time;
+	struct mutex mutex_cfg_values;	   /* protect setting of config     */
+	struct mutex mutex_wep_key;	   /* protect WEP key actions       */
+	struct mutex mutex_disconnect;	   /* protect MSG Disconnect        */
+	struct mutex mutex_get_rssi;	   /* protect MSG Get RSSI          */
+	struct mutex mutex_inactive_time;  /* protect MSG Get Inactive Time */
 
 	struct timer_list scan_timer;
 	struct timer_list connect_timer;
-- 
2.1.4

Re: [Outreachy kernel] [PATCH 2/3] staging: wilc1000: host_interface: replace semaphores with mutexes

[Arnd Bergmann]

On Tuesday 16 February 2016 00:31:42 Alison Schofield wrote:
> Replace semaphores with mutexes in the host_interface driver.
> 
> This is a safe performance improvement because the usage model
> meets the principle of ownership for mutexes: the thread that
> locks is the same thread that unlocks.
> 
> Signed-off-by: Alison Schofield <amsfield22@gmail.com>

Good stuff in general, but it's slightly hard to review because you do
all at once. This might be more obvious if you did one patch per
semaphore.

Also, some of them look wrong:

>  
>  	if (clients_count == 0)	{
> -		sema_init(&hif_sema_thread, 0);
> -		sema_init(&hif_sema_driver, 0);
> -		sema_init(&hif_sema_deinit, 1);
> +		mutex_init(&hif_mutex_thread);
> +		mutex_init(&hif_mutex_driver);
> +		mutex_init(&hif_mutex_deinit);
>  	}
>  
> -	sema_init(&hif_drv->sem_test_key_block, 0);
> -	sema_init(&hif_drv->sem_test_disconn_block, 0);
> -	sema_init(&hif_drv->sem_get_rssi, 0);
> -	sema_init(&hif_drv->sem_inactive_time, 0);
> +	mutex_init(&hif_drv->mutex_wep_key);
> +	mutex_init(&hif_drv->mutex_disconnect);
> +	mutex_init(&hif_drv->mutex_get_rssi);
> +	mutex_init(&hif_drv->mutex_inactive_time);

Note that some of these are initialized to zero (locked), others are
initialized to one (unlocked). When you have a semaphore that starts
out as locked, you need to be extra careful to ensure the lock/unlock
comes in pairs, as it equates a

	mutex_init(&lock);
	mutex_lock(&lock);

and some of them might be unlocked elsewhere first. Looking at
how some of these are used, I think there are some that get
released in hostIFthread(), which is a kthread and taken in
a function waiting for the kthread. Those should be converted
to wait_for_completion() rather than a mutex.

	Arnd

Re: [Outreachy kernel] [PATCH 2/3] staging: wilc1000: host_interface: replace semaphores with mutexes

[Greg KH]

On Tue, Feb 16, 2016 at 12:31:42AM -0800, Alison Schofield wrote:
> Replace semaphores with mutexes in the host_interface driver.
> 
> This is a safe performance improvement because the usage model
> meets the principle of ownership for mutexes: the thread that
> locks is the same thread that unlocks.
> 
> Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> ---
>  drivers/staging/wilc1000/host_interface.c | 114 +++++++++++++++---------------
>  drivers/staging/wilc1000/host_interface.h |  10 +--
>  2 files changed, 63 insertions(+), 61 deletions(-)

These are hard patches for me to accept if you don't have the hardware
to test with.

Also I would argue that most of these locks can be removed, or made to
work on a per-interface basis, and shouldn't be "global" to the whole
driver.  Also note that the driver authors for this driver are working
really hard to clean up these types of issues, I would stay away from
this code at the moment unless you are just doing basic fixes as you
will end up conflicting with their changes happening at the same time (7
more changes came up just last night from them).

I'll take your first patch in the series, but this one I can't, sorry.

thanks,

greg k-h

Re: [PATCH 2/3] staging: wilc1000: host_interface: replace semaphores with mutexes

[Alison Schofield]

On Tue, Feb 16, 2016 at 12:31:42AM -0800, Alison Schofield wrote:
> Replace semaphores with mutexes in the host_interface driver.
> 
> This is a safe performance improvement because the usage model
> meets the principle of ownership for mutexes: the thread that
> locks is the same thread that unlocks.
> 
> Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> ---

Arnd,
Thanks for the review and redirection. I will use it to learn more
about the usage of semas, mutexes, and the wait_for_completion too.

Greg,
I hear you and will abandon this patch.  It was no easy task figuring
out the purpose of these locks (just to comment them), so that should
have been a red flag that perhaps this wasn't the wisest choice for
my first attempt at lock changes.  Learned some stuff and moving on...

alisons

Re: [Outreachy kernel] Re: [PATCH 2/3] staging: wilc1000: host_interface: replace semaphores with mutexes

[Arnd Bergmann]

On Tuesday 16 February 2016 10:16:07 Alison Schofield wrote:
> On Tue, Feb 16, 2016 at 12:31:42AM -0800, Alison Schofield wrote:
> > Replace semaphores with mutexes in the host_interface driver.
> > 
> > This is a safe performance improvement because the usage model
> > meets the principle of ownership for mutexes: the thread that
> > locks is the same thread that unlocks.
> > 
> > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> > ---
> 
> Arnd,
> Thanks for the review and redirection. I will use it to learn more
> about the usage of semas, mutexes, and the wait_for_completion too.

You're welcome. Feel free to ask me on IRC if you need more help
on this. I saw your question the other day, but you were gone when
I tried to reply about the down_timeout().

> Greg,
> I hear you and will abandon this patch.  It was no easy task figuring
> out the purpose of these locks (just to comment them), so that should
> have been a red flag that perhaps this wasn't the wisest choice for
> my first attempt at lock changes.  Learned some stuff and moving on...

For this specific driver, I've done a number a number of patches myself
and found that the maintainers at Atmel were rather responsive and
able to test my patches on their systems.

It might still be worthwhile to follow up doing one semaphore at a time,
I'll gladly do an detailed review of those patches.

There are also some easy unused semaphores in drivers/staging/rtl8712/
and drivers/staging/rtl8723au that you can take care of.

	Arnd

Re: [Outreachy kernel] Re: [PATCH 2/3] staging: wilc1000: host_interface: replace semaphores with mutexes

[Alison Schofield]

On Tue, Feb 16, 2016 at 10:34:11PM +0100, Arnd Bergmann wrote:
> On Tuesday 16 February 2016 10:16:07 Alison Schofield wrote:
> > On Tue, Feb 16, 2016 at 12:31:42AM -0800, Alison Schofield wrote:
> > > Replace semaphores with mutexes in the host_interface driver.
> > > 
> > > This is a safe performance improvement because the usage model
> > > meets the principle of ownership for mutexes: the thread that
> > > locks is the same thread that unlocks.
> > > 
> > > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> > > ---
> > 
> > Arnd,
> > Thanks for the review and redirection. I will use it to learn more
> > about the usage of semas, mutexes, and the wait_for_completion too.
> 
> You're welcome. Feel free to ask me on IRC if you need more help
> on this. I saw your question the other day, but you were gone when
> I tried to reply about the down_timeout().
> 
> > Greg,
> > I hear you and will abandon this patch.  It was no easy task figuring
> > out the purpose of these locks (just to comment them), so that should
> > have been a red flag that perhaps this wasn't the wisest choice for
> > my first attempt at lock changes.  Learned some stuff and moving on...
> 
> For this specific driver, I've done a number a number of patches myself
> and found that the maintainers at Atmel were rather responsive and
> able to test my patches on their systems.
> 
> It might still be worthwhile to follow up doing one semaphore at a time,
> I'll gladly do an detailed review of those patches.
> 
> There are also some easy unused semaphores in drivers/staging/rtl8712/
> and drivers/staging/rtl8723au that you can take care of.
> 
> 	Arnd

A new patch follows that replaces one semaphore with a completion.
This particular semaphore is part of a subset in the host_if_drv
structure that are essentially being used to communicate completion
of messages sent to the host interface.

My intent here would be to get this one reviewed to verify I'm on the
right track. Next, I'll ask Atmel maintainers if they would be
willing/able to test such a set of patches. If yes, I'd create the
patchset, get it reviewed, and send to Atmel for test.

(A review and update of those global semaphores the host_interface could
follow.)

alisons

[PATCH] staging: wilc1000: replace semaphore get_inactive_time with a completion

[Alison Schofield]

Semaphore get_inactive_time is used to signal completion of its host
interface message. Since the thread locking this semaphore will have
to wait, completions are the preferred mechanism and will offer a
performance improvement.

Signed-off-by: Alison Schofield <amsfield22@gmail.com>
---
 drivers/staging/wilc1000/host_interface.c | 7 ++++---
 drivers/staging/wilc1000/host_interface.h | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index d1eedfb..7cb8c28 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -2,6 +2,7 @@
 #include <linux/time.h>
 #include <linux/kthread.h>
 #include <linux/delay.h>
+#include <linux/completion.h>
 #include "host_interface.h"
 #include "coreconfigurator.h"
 #include "wilc_wlan.h"
@@ -2131,7 +2132,7 @@ static s32 Handle_Get_InActiveTime(struct wilc_vif *vif,
 
 	PRINT_D(CFG80211_DBG, "Getting inactive time : %d\n", inactive_time);
 
-	up(&hif_drv->sem_inactive_time);
+	complete(&hif_drv->comp_inactive_time);
 
 	return result;
 }
@@ -3443,7 +3444,7 @@ s32 wilc_get_inactive_time(struct wilc_vif *vif, const u8 *mac,
 	if (result)
 		PRINT_ER("Failed to send get host channel param's message queue ");
 
-	down(&hif_drv->sem_inactive_time);
+	wait_for_completion(&hif_drv->comp_inactive_time);
 
 	*pu32InactiveTime = inactive_time;
 
@@ -3632,7 +3633,7 @@ int wilc_init(struct net_device *dev, struct host_if_drv **hif_drv_handler)
 	sema_init(&hif_drv->sem_get_rssi, 0);
 	sema_init(&hif_drv->sem_get_link_speed, 0);
 	sema_init(&hif_drv->sem_get_chnl, 0);
-	sema_init(&hif_drv->sem_inactive_time, 0);
+	init_completion(&hif_drv->comp_inactive_time);
 
 	if (clients_count == 0)	{
 		result = wilc_mq_create(&hif_msg_q);
diff --git a/drivers/staging/wilc1000/host_interface.h b/drivers/staging/wilc1000/host_interface.h
index 5e65f2c..08229f6 100644
--- a/drivers/staging/wilc1000/host_interface.h
+++ b/drivers/staging/wilc1000/host_interface.h
@@ -280,7 +280,7 @@ struct host_if_drv {
 	struct semaphore sem_get_rssi;
 	struct semaphore sem_get_link_speed;
 	struct semaphore sem_get_chnl;
-	struct semaphore sem_inactive_time;
+	struct completion comp_inactive_time;
 
 	struct timer_list scan_timer;
 	struct timer_list connect_timer;
-- 
2.1.4

Re: [Outreachy kernel] [PATCH] staging: wilc1000: replace semaphore get_inactive_time with a completion

[Arnd Bergmann]

On Friday 19 February 2016 11:45:40 Alison Schofield wrote:
> Semaphore get_inactive_time is used to signal completion of its host
> interface message. Since the thread locking this semaphore will have
> to wait, completions are the preferred mechanism and will offer a
> performance improvement.
> 
> Signed-off-by: Alison Schofield <amsfield22@gmail.com>

Very nice patch!

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Re: [Outreachy kernel] [PATCH] staging: wilc1000: replace semaphore get_inactive_time with a completion

[Greg KH]

On Fri, Feb 19, 2016 at 11:45:40AM -0800, Alison Schofield wrote:
> Semaphore get_inactive_time is used to signal completion of its host
> interface message. Since the thread locking this semaphore will have
> to wait, completions are the preferred mechanism and will offer a
> performance improvement.
> 
> Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/staging/wilc1000/host_interface.c | 7 ++++---
>  drivers/staging/wilc1000/host_interface.h | 2 +-
>  2 files changed, 5 insertions(+), 4 deletions(-)

Doesn't apply after applying your other patch :(

Re: [Outreachy kernel] [PATCH] staging: wilc1000: replace semaphore get_inactive_time with a completion

[Alison Schofield]

On Fri, Feb 19, 2016 at 04:54:44PM -0800, Greg KH wrote:
> On Fri, Feb 19, 2016 at 11:45:40AM -0800, Alison Schofield wrote:
> > Semaphore get_inactive_time is used to signal completion of its host
> > interface message. Since the thread locking this semaphore will have
> > to wait, completions are the preferred mechanism and will offer a
> > performance improvement.
> > 
> > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> > Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  drivers/staging/wilc1000/host_interface.c | 7 ++++---
> >  drivers/staging/wilc1000/host_interface.h | 2 +-
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> Doesn't apply after applying your other patch :(
> 
I can rebase and resend a v2. 
I wasn't expecting you to take it, until I could get it tested.
Please advise.
Thanks,
alisons
it until I could get it tested.    

Re: [Outreachy kernel] [PATCH] staging: wilc1000: replace semaphore get_inactive_time with a completion

[Greg KH]

On Fri, Feb 19, 2016 at 05:06:08PM -0800, Alison Schofield wrote:
> On Fri, Feb 19, 2016 at 04:54:44PM -0800, Greg KH wrote:
> > On Fri, Feb 19, 2016 at 11:45:40AM -0800, Alison Schofield wrote:
> > > Semaphore get_inactive_time is used to signal completion of its host
> > > interface message. Since the thread locking this semaphore will have
> > > to wait, completions are the preferred mechanism and will offer a
> > > performance improvement.
> > > 
> > > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> > > Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> > > ---
> > >  drivers/staging/wilc1000/host_interface.c | 7 ++++---
> > >  drivers/staging/wilc1000/host_interface.h | 2 +-
> > >  2 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > Doesn't apply after applying your other patch :(
> > 
> I can rebase and resend a v2. 
> I wasn't expecting you to take it, until I could get it tested.

Please test and resend :)

thanks,

greg k-h

[PATCH 3/3] staging: wilc1000: host_interface: destroy mutexes at init/deinit

[Alison Schofield]

Use mutex_destroy() when driver initialization fails and during
deinitialization.  Move the init of per client mutexes later in
the init process to reduce the number of destroys needed on the
failure path.

Signed-off-by: Alison Schofield <amsfield22@gmail.com>
---
 drivers/staging/wilc1000/host_interface.c | 36 ++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
index 45e6ed3..cf1716d 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -3602,8 +3602,6 @@ int wilc_init(struct net_device *dev, struct host_if_drv **hif_drv_handler)
 
 	scan_while_connected = false;
 
-	mutex_init(&hif_mutex_wait_response);
-
 	hif_drv  = kzalloc(sizeof(struct host_if_drv), GFP_KERNEL);
 	if (!hif_drv) {
 		result = -ENOMEM;
@@ -3622,14 +3620,8 @@ int wilc_init(struct net_device *dev, struct host_if_drv **hif_drv_handler)
 		mutex_init(&hif_mutex_thread);
 		mutex_init(&hif_mutex_driver);
 		mutex_init(&hif_mutex_deinit);
-	}
+		mutex_init(&hif_mutex_wait_response);
 
-	mutex_init(&hif_drv->mutex_wep_key);
-	mutex_init(&hif_drv->mutex_disconnect);
-	mutex_init(&hif_drv->mutex_get_rssi);
-	mutex_init(&hif_drv->mutex_inactive_time);
-
-	if (clients_count == 0)	{
 		result = wilc_mq_create(&hif_msg_q);
 
 		if (result < 0) {
@@ -3654,7 +3646,12 @@ int wilc_init(struct net_device *dev, struct host_if_drv **hif_drv_handler)
 	setup_timer(&hif_drv->connect_timer, TimerCB_Connect, 0);
 	setup_timer(&hif_drv->remain_on_ch_timer, ListenTimerCB, 0);
 
+	mutex_init(&hif_drv->mutex_wep_key);
+	mutex_init(&hif_drv->mutex_disconnect);
+	mutex_init(&hif_drv->mutex_get_rssi);
+	mutex_init(&hif_drv->mutex_inactive_time);
 	mutex_init(&hif_drv->mutex_cfg_values);
+
 	mutex_lock(&hif_drv->mutex_cfg_values);
 
 	hif_drv->hif_state = HOST_IF_IDLE;
@@ -3675,6 +3672,12 @@ int wilc_init(struct net_device *dev, struct host_if_drv **hif_drv_handler)
 _fail_mq_:
 	wilc_mq_destroy(&hif_msg_q);
 _fail_:
+	if (clients_count == 0) {
+		mutex_destroy(&hif_mutex_thread);
+		mutex_destroy(&hif_mutex_driver);
+		mutex_destroy(&hif_mutex_deinit);
+		mutex_destroy(&hif_mutex_wait_response);
+	}
 	return result;
 }
 
@@ -3723,15 +3726,28 @@ int wilc_deinit(struct wilc_vif *vif)
 			PRINT_ER("Error in sending deinit's message queue message function: Error(%d)\n", result);
 
 		mutex_lock(&hif_mutex_thread);
-
 		wilc_mq_destroy(&hif_msg_q);
+		mutex_unlock(&hif_mutex_thread);
 	}
 
+	mutex_destroy(&hif_drv->mutex_wep_key);
+	mutex_destroy(&hif_drv->mutex_disconnect);
+	mutex_destroy(&hif_drv->mutex_get_rssi);
+	mutex_destroy(&hif_drv->mutex_inactive_time);
+	mutex_destroy(&hif_drv->mutex_cfg_values);
+
 	kfree(hif_drv);
 
 	clients_count--;
 	terminated_handle = NULL;
 	mutex_unlock(&hif_mutex_deinit);
+
+	if (clients_count == 0) {
+		mutex_destroy(&hif_mutex_thread);
+		mutex_destroy(&hif_mutex_driver);
+		mutex_destroy(&hif_mutex_deinit);
+		mutex_destroy(&hif_mutex_wait_response);
+	}
 	return result;
 }
 
-- 
2.1.4