Re: [PATCH] kthread: airo.c

Andrew Morton
2006-07-26T06:19:08Z

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