On Mon, 24 Jul 2006 11:13:09 -0700
Sukadev Bhattiprolu <[email protected]> wrote:
> Sukadev Bhattiprolu [[email protected]] 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).
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
