* Re: [PATCH] kthread: airo.c
[not found] <20060713205319.GA23594@us.ibm.com>
@ 2006-07-24 18:13 ` Sukadev Bhattiprolu
2006-07-26 6:18 ` Andrew Morton
0 siblings, 1 reply; 2+ messages in thread
From: Sukadev Bhattiprolu @ 2006-07-24 18:13 UTC (permalink / raw)
To: akpm
Cc: achirica, hch, linville, David C. Hansen, serue, clr, netdev,
linux-kernel
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().
Signed-off-by: Sukadev Bhattiprolu <sukadev@us•ibm.com>
Cc: Javier Achirica <achirica@gmail•com>
Cc: Christoph Hellwig <hch@infradead•org>
Cc: John Linville <linville@tuxdriver•com>
drivers/net/wireless/airo.c | 38 +++++++++++++++-----------------------
1 files changed, 15 insertions(+), 23 deletions(-)
Index: linux-2.6.18-rc1-mm2/drivers/net/wireless/airo.c
===================================================================
--- linux-2.6.18-rc1-mm2.orig/drivers/net/wireless/airo.c 2006-07-14 14:04:01.000000000 -0700
+++ linux-2.6.18-rc1-mm2/drivers/net/wireless/airo.c 2006-07-20 19:44:50.000000000 -0700
@@ -47,6 +47,7 @@
#include <linux/pci.h>
#include <asm/uaccess.h>
#include <net/ieee80211.h>
+#include <linux/kthread.h>
#include "airo.h"
@@ -1187,11 +1188,10 @@ struct airo_info {
int whichbap);
unsigned short *flash;
tdsRssiEntry *rssi;
- struct task_struct *task;
+ struct task_struct *list_bss_task;
+ struct task_struct *airo_thread_task;
struct semaphore sem;
- pid_t thr_pid;
wait_queue_head_t thr_wait;
- struct completion thr_exited;
unsigned long expires;
struct {
struct sk_buff *skb;
@@ -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;
}
rc = PC4500_readrid(ai, first ? ai->bssListFirst : ai->bssListNext,
list, ai->bssListRidLen, 1);
@@ -2400,8 +2400,7 @@ void stop_airo_card( struct net_device *
clear_bit(FLAG_REGISTERED, &ai->flags);
}
set_bit(JOB_DIE, &ai->jobs);
- kill_proc(ai->thr_pid, SIGTERM, 1);
- wait_for_completion(&ai->thr_exited);
+ kthread_stop(ai->airo_thread_task);
/*
* Clean out tx queue
@@ -2811,9 +2810,8 @@ static struct net_device *_init_airo_car
ai->config.len = 0;
ai->pci = pci;
init_waitqueue_head (&ai->thr_wait);
- init_completion (&ai->thr_exited);
- ai->thr_pid = kernel_thread(airo_thread, dev, CLONE_FS | CLONE_FILES);
- if (ai->thr_pid < 0)
+ ai->airo_thread_task = kthread_run(airo_thread, dev, dev->name);
+ if (IS_ERR(ai->airo_thread_task))
goto err_out_free;
ai->tfm = NULL;
rc = add_airo_dev( dev );
@@ -2930,8 +2928,7 @@ err_out_unlink:
del_airo_dev(dev);
err_out_thr:
set_bit(JOB_DIE, &ai->jobs);
- kill_proc(ai->thr_pid, SIGTERM, 1);
- wait_for_completion(&ai->thr_exited);
+ kthread_stop(ai->airo_thread_task);
err_out_free:
free_netdev(dev);
return NULL;
@@ -3063,13 +3060,7 @@ static int airo_thread(void *data) {
struct airo_info *ai = dev->priv;
int locked;
- daemonize("%s", dev->name);
- allow_signal(SIGTERM);
-
while(1) {
- if (signal_pending(current))
- flush_signals(current);
-
/* make swsusp happy with our thread */
try_to_freeze();
@@ -3097,7 +3088,7 @@ static int airo_thread(void *data) {
set_bit(JOB_AUTOWEP, &ai->jobs);
break;
}
- if (!signal_pending(current)) {
+ if (!kthread_should_stop()) {
unsigned long wake_at;
if (!ai->expires || !ai->scan_timeout) {
wake_at = max(ai->expires,
@@ -3109,7 +3100,7 @@ static int airo_thread(void *data) {
schedule_timeout(wake_at - jiffies);
continue;
}
- } else if (!signal_pending(current)) {
+ } else if (!kthread_should_stop()) {
schedule();
continue;
}
@@ -3154,7 +3145,8 @@ static int airo_thread(void *data) {
else /* Shouldn't get here, but we make sure to unlock */
up(&ai->sem);
}
- complete_and_exit (&ai->thr_exited, 0);
+
+ return 0;
}
static irqreturn_t airo_interrupt ( int irq, void* dev_id, struct pt_regs *regs) {
@@ -3235,8 +3227,8 @@ static irqreturn_t airo_interrupt ( int
if(newStatus == ASSOCIATED || newStatus == REASSOCIATED) {
if (auto_wep)
apriv->expires = 0;
- if (apriv->task)
- wake_up_process (apriv->task);
+ if (apriv->list_bss_task)
+ wake_up_process(apriv->list_bss_task);
set_bit(FLAG_UPDATE_UNI, &apriv->flags);
set_bit(FLAG_UPDATE_MULTI, &apriv->flags);
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] kthread: airo.c
2006-07-24 18:13 ` [PATCH] kthread: airo.c Sukadev Bhattiprolu
@ 2006-07-26 6:18 ` Andrew Morton
0 siblings, 0 replies; 2+ messages in thread
From: Andrew Morton @ 2006-07-26 6:18 UTC (permalink / raw)
To: Sukadev Bhattiprolu
Cc: achirica, hch, linville, haveblue, serue, clr, netdev,
linux-kernel
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).
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2006-07-26 6:19 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20060713205319.GA23594@us.ibm.com>
2006-07-24 18:13 ` [PATCH] kthread: airo.c Sukadev Bhattiprolu
2006-07-26 6:18 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox