Re: [PATCH] kthread: airo.c

[Andrew Morton <akpm@osdl.org>]

On Mon, 24 Jul 2006 11:13:09 -0700
Sukadev Bhattiprolu <sukadev@us.ibm.com> wrote:

> Sukadev Bhattiprolu [sukadev@us.ibm.com] wrote:
> 
> | Andrew,
> | 
> | Javier Achirica, one of the major contributors to drivers/net/wireless/airo.c
> | took a look at this patch, and doesn't have any problems with it. It doesn't
> | fix any bugs and is just a cleanup, so it certainly isn't a candidate
> | for this mainline cycle
> 
> Here is the same patch, merged up to 2.6.18-rc2. Christoph's patch (see
> http://lkml.org/lkml/2006/7/13/332) still applies cleanly on top of this.
> 
> -----
> The airo driver is currently caching a pid for later use, but with the
> implementation of containers, pids themselves do not uniquely identify
> a task. The driver is also using kernel_thread() which is deprecated in
> drivers.
> 
> This patch essentially replaces the kernel_thread() with kthread_create().
> It also stores the task_struct of the airo_thread rather than its pid.
> Since this introduces a second task_struct in struct airo_info, the patch
> renames airo_info.task to airo_info.list_bss_task.
> 
> As an extension of these changes, the patch further:
> 
>          - replaces kill_proc() with kthread_stop()
>          - replaces signal_pending() with kthread_should_stop()
> 	 - removes thread completion synchronisation which is handled by
> 	   kthread_stop().
> 
> ..
>
> @@ -1736,9 +1736,9 @@ static int readBSSListRid(struct airo_in
>  		issuecommand(ai, &cmd, &rsp);
>  		up(&ai->sem);
>  		/* Let the command take effect */
> -		ai->task = current;
> +		ai->list_bss_task = current;
>  		ssleep(3);
> -		ai->task = NULL;
> +		ai->list_bss_task = NULL;

This looks a little racy to me.  It's relatively benign - a race will cause
us to sleep for too long.  But it's easy to fix:


--- a/drivers/net/wireless/airo.c~kthread-airoc-race-fix
+++ a/drivers/net/wireless/airo.c
@@ -1733,10 +1733,10 @@ static int readBSSListRid(struct airo_in
 		cmd.cmd=CMD_LISTBSS;
 		if (down_interruptible(&ai->sem))
 			return -ERESTARTSYS;
+		ai->list_bss_task = current;
 		issuecommand(ai, &cmd, &rsp);
 		up(&ai->sem);
 		/* Let the command take effect */
-		ai->list_bss_task = current;
 		ssleep(3);
 		ai->list_bss_task = NULL;
 	}
_

<looks more closely>

Actually, ssleep() ends up doing

        while (timeout)
                timeout = schedule_timeout_uninterruptible(timeout);

so if the intent of this code is to terminate the sleep early, when the
interrupt has happened then it isn't working right.  A fix would be to
convert the ssleep(3) into schedule_timeout_uninterruptible(3 * HZ).