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

[Arnd Bergmann <arnd@arndb.de>]

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