Re: [PATCH] kthread: airo.c
From: Andrew Morton <hidden>
Date: 2006-07-26 06:19:08
Also in:
lkml
On Mon, 24 Jul 2006 11:13:09 -0700 Sukadev Bhattiprolu [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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).