Skip to contentSkip to conversation

staging: wilc1000: semaphores to mutexes in host_interface

PATCH In review Alison Schofield 3 participants 17 messages Feb 16, 2016 – Feb 20, 2016 Staging

lore
Share & tools
lore

Alison Schofield

author
#0 lore raw

cover staging: wilc1000: semaphores to mutexes in host_interface

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(-)
Signature · 1 line
2.1.4
author
#1 lore raw

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

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 <[email protected]>
---
 drivers/staging/wilc1000/host_interface.c | 7 -------
 drivers/staging/wilc1000/host_interface.h | 2 --
 2 files changed, 9 deletions(-)
drivers/staging/wilc1000/host_interface.c +0 -7
index d1eedfb..66a0ac9 100644
@@ -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)	{
drivers/staging/wilc1000/host_interface.h +0 -2
index 5e65f2c..6135024 100644
@@ -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;
Signature · 1 line
2.1.4
Reviewed-by on v1 1/3
#2 lore raw

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

Quoted 7 lines · 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 <[email protected]>

Nice!

Reviewed-by Arnd Bergmann <[email protected]>
author
#3 lore raw

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

Quoted 6 lines · Arnd Bergmann
Quoted 5 lines · 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.

Quoted 6 lines
Quoted 2 lines
Signed-off-by Alison Schofield <[email protected]>

Nice!

Reviewed-by Arnd Bergmann <[email protected]>

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

#4 lore raw

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

Quoted 20 lines · On Thursday 18 February 2016 22:45:53 Alison Schofield wrote:
Quoted 6 lines · Arnd Bergmann
Quoted 5 lines · 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.

Quoted 6 lines
Quoted 2 lines
Signed-off-by Alison Schofield <[email protected]>

Nice!

Reviewed-by Arnd Bergmann <[email protected]>

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.

Quoted 4 lines

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

author
#5 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/staging/wilc1000/host_interface.c | 114 +++++++++++++++---------------
 drivers/staging/wilc1000/host_interface.h |  10 +--
 2 files changed, 63 insertions(+), 61 deletions(-)
drivers/staging/wilc1000/host_interface.c +58 -56
index 66a0ac9..45e6ed3 100644
@@ -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;
drivers/staging/wilc1000/host_interface.h +5 -5
index 6135024..a438603 100644
@@ -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;
Signature · 1 line
2.1.4
#6 lore raw

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

Quoted 7 lines · 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 <[email protected]>

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:

Quoted 8 lines
drivers/staging/wilc1000/host_interface.c +3 -3
 	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);

drivers/staging/wilc1000/host_interface.c int wilc_init(struct net_device *dev, struct host_if_drv **hif_drv_handler) · line 3630

Quoted 10 lines
drivers/staging/wilc1000/host_interface.c +4 -4
 	}
 
-	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

maintainer
#7 lore raw

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

In reply to Alison Schofield
Quoted 11 lines · 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 <[email protected]>
---
 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

author
#8 lore raw

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

In reply to Alison Schofield
Quoted 8 lines · 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 <[email protected]>

---

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

#9 lore raw

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

Quoted 7 lines · On Tuesday 16 February 2016 10:16:07 Alison Schofield wrote:
Quoted 6 lines · 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.

Quoted 6 lines
Quoted 2 lines
Signed-off-by Alison Schofield <[email protected]>

---

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().

Quoted 5 lines

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

author
#10 lore raw

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

Quoted 29 lines · Arnd Bergmann
Quoted 7 lines · On Tuesday 16 February 2016 10:16:07 Alison Schofield wrote:
Quoted 6 lines · 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.

Quoted 6 lines
Quoted 2 lines
Signed-off-by Alison Schofield <[email protected]>

---

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().

Quoted 5 lines

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.

Quoted 7 lines

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

author
#11 lore raw

PATCH 1/1 staging: wilc1000: replace semaphore get_inactive_time with a completion

In reply to 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 <[email protected]>
---
 drivers/staging/wilc1000/host_interface.c | 7 ++++---
 drivers/staging/wilc1000/host_interface.h | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)
drivers/staging/wilc1000/host_interface.c +4 -3
index d1eedfb..7cb8c28 100644
@@ -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);
drivers/staging/wilc1000/host_interface.h +1 -1
index 5e65f2c..08229f6 100644
@@ -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;
Signature · 1 line
2.1.4
Reviewed-by on v1 1/1
#12 lore raw

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

Quoted 6 lines · 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 <[email protected]>

Very nice patch!

Reviewed-by Arnd Bergmann <[email protected]>
maintainer
#13 lore raw

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

In reply to Alison Schofield
Quoted 11 lines · 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 <[email protected]>
Reviewed-by Arnd Bergmann <[email protected]>
---
 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 :(

author
#14 lore raw

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

Quoted 6 lines · Greg KH
Quoted 5 lines · 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.

Quoted 9 lines
Quoted 6 lines
Signed-off-by Alison Schofield <[email protected]>
Reviewed-by Arnd Bergmann <[email protected]>
---
 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.

maintainer
#15 lore raw

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

Quoted 14 lines · Alison Schofield
Quoted 13 lines · Greg KH
Quoted 11 lines · 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 <[email protected]>
Reviewed-by Arnd Bergmann <[email protected]>
---
 drivers/staging/wilc1000/host_interface.c | 7 ++++---
 drivers/staging/wilc1000/host_interface.h | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)
Quoted 4 lines
Quoted 2 lines

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

author
#16 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/staging/wilc1000/host_interface.c | 36 ++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 10 deletions(-)
drivers/staging/wilc1000/host_interface.c +26 -10
index 45e6ed3..cf1716d 100644
@@ -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;
 }
Signature · 1 line
2.1.4