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 netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
