kthread: airo.c

PATCH In review Sukadev Bhattiprolu 4 participants 6 messages Jul 13, 2006 – Jul 26, 2006

Subsystems (3)

Networking Network drivers Networking drivers (wireless)

lore

Share & tools

lore

Sukadev Bhattiprolu

author

2006-07-13 20:53 UTC #0 lore raw

PATCH 1/1 kthread: airo.c

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

Suka

-----

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 <[email protected]>

drivers/net/wireless/airo.c +15 ~~-23~~

Index: linux-2.6.17.1/drivers/net/wireless/airo.c
===================================================================
@@ -47,6 +47,7 @@
 #include <linux/ioport.h>
 #include <linux/pci.h>
 #include <asm/uaccess.h>
+#include <linux/kthread.h>
 
 #include "airo.h"

@@ -1173,11 +1174,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;

@@ -1717,9 +1717,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 ? RID_BSSLISTFIRST : RID_BSSLISTNEXT,
 			    list, sizeof(*list), 1);

@@ -2381,8 +2381,7 @@ void stop_airo_card( struct net_device *
 		clear_bit(FLAG_REGISTERED, &ai->flags);
 	}
 	set_bit(JOB_DIE, &ai->flags);
-	kill_proc(ai->thr_pid, SIGTERM, 1);
-	wait_for_completion(&ai->thr_exited);
+	kthread_stop(ai->airo_thread_task);
 
 	/*
 	 * Clean out tx queue

@@ -2769,9 +2768,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 );

@@ -2876,8 +2874,7 @@ err_out_unlink:
 	del_airo_dev(dev);
 err_out_thr:
 	set_bit(JOB_DIE, &ai->flags);
-	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;

@@ -3009,13 +3006,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();

@@ -3043,7 +3034,7 @@ static int airo_thread(void *data) {
 						set_bit(JOB_AUTOWEP,&ai->flags);
 						break;
 					}
-					if (!signal_pending(current)) {
+					if (!kthread_should_stop()) {
 						unsigned long wake_at;
 						if (!ai->expires || !ai->scan_timeout) {
 							wake_at = max(ai->expires,

@@ -3055,7 +3046,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;
 				}

@@ -3100,7 +3091,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) {

@@ -3181,8 +3173,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);

Christoph Hellwig

2006-07-13 21:28 UTC #1 lore raw

Quoted 6 lines · Sukadev Bhattiprolu

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

I'm not sure it's that easy. I think it needs some more love:

- switch to wake_uo_process
- kill JOB_DIE
- cleanup a the convoluted mess in airo_thread a bit

Note that it's still reimplementing the single threaded workqueue functionality quite badly. So if someone could switch it over and while we're at it try to kill the idiociy of doing the trylock in the calling context and only then calling the thread by always calling the thread (which also solves the synchronization problem).

Anywhy, here's a small incremental patch ontop of yours to implement my above items:

drivers/net/wireless/airo.c +42 ~~-65~~

Index: linux-2.6/drivers/net/wireless/airo.c
===================================================================
@@ -1173,7 +1173,6 @@
 #define FLAG_FLASHING	15
 #define FLAG_WPA_CAPABLE	16
 	unsigned long flags;
-#define JOB_DIE	0
 #define JOB_XMIT	1
 #define JOB_XMIT11	2
 #define JOB_STATS	3

@@ -1191,7 +1190,6 @@
 	struct task_struct *list_bss_task;
 	struct task_struct *airo_thread_task;
 	struct semaphore sem;
-	wait_queue_head_t thr_wait;
 	unsigned long expires;
 	struct {
 		struct sk_buff *skb;

@@ -2182,7 +2180,7 @@
 		set_bit(FLAG_PENDING_XMIT, &priv->flags);
 		netif_stop_queue(dev);
 		set_bit(JOB_XMIT, &priv->jobs);
-		wake_up_interruptible(&priv->thr_wait);
+		wake_up_process(priv->airo_thread_task);
 	} else
 		airo_end_xmit(dev);
 	return 0;

@@ -2253,7 +2251,7 @@
 		set_bit(FLAG_PENDING_XMIT11, &priv->flags);
 		netif_stop_queue(dev);
 		set_bit(JOB_XMIT11, &priv->jobs);
-		wake_up_interruptible(&priv->thr_wait);
+		wake_up_process(priv->airo_thread_task);
 	} else
 		airo_end_xmit11(dev);
 	return 0;

@@ -2295,7 +2293,7 @@
 		/* Get stats out of the card if available */
 		if (down_trylock(&local->sem) != 0) {
 			set_bit(JOB_STATS, &local->jobs);
-			wake_up_interruptible(&local->thr_wait);
+			wake_up_process(local->airo_thread_task);
 		} else
 			airo_read_stats(local);
 	}

@@ -2322,7 +2320,7 @@
 		change_bit(FLAG_PROMISC, &ai->flags);
 		if (down_trylock(&ai->sem) != 0) {
 			set_bit(JOB_PROMISC, &ai->jobs);
-			wake_up_interruptible(&ai->thr_wait);
+			wake_up_process(ai->airo_thread_task);
 		} else
 			airo_set_promisc(ai);
 	}

@@ -2399,7 +2397,6 @@
 		}
 		clear_bit(FLAG_REGISTERED, &ai->flags);
 	}
-	set_bit(JOB_DIE, &ai->jobs);
 	kthread_stop(ai->airo_thread_task);
 
 	/*

@@ -2809,7 +2806,6 @@
 	sema_init(&ai->sem, 1);
 	ai->config.len = 0;
 	ai->pci = pci;
-	init_waitqueue_head (&ai->thr_wait);
 	ai->airo_thread_task = kthread_run(airo_thread, dev, dev->name);
 	if (IS_ERR(ai->airo_thread_task))
 		goto err_out_free;

@@ -2927,7 +2923,6 @@
 err_out_unlink:
 	del_airo_dev(dev);
 err_out_thr:
-	set_bit(JOB_DIE, &ai->jobs);
 	kthread_stop(ai->airo_thread_task);
 err_out_free:
 	free_netdev(dev);

@@ -3055,70 +3050,52 @@
 	wireless_send_event(ai->dev, SIOCGIWSCAN, &wrqu, NULL);
 }
 
-static int airo_thread(void *data) {
+static int airo_thread(void *data)
+{
 	struct net_device *dev = data;
 	struct airo_info *ai = dev->priv;
-	int locked;
 	
-	while(1) {
+	while (1) {
 		/* make swsusp happy with our thread */
 		try_to_freeze();
 
-		if (test_bit(JOB_DIE, &ai->jobs))
-			break;
+		for (;;) {
+			unsigned long wake_at;
 
-		if (ai->jobs) {
-			locked = down_interruptible(&ai->sem);
-		} else {
-			wait_queue_t wait;
+			set_current_state(TASK_INTERRUPTIBLE);
+			if (kthread_should_stop())
+				break;
+			if (ai->jobs)
+				break;
+			if (ai->scan_timeout &&
+			    time_after_eq(jiffies, ai->scan_timeout)) {
+				set_bit(JOB_SCAN_RESULTS, &ai->jobs);
+				break;
+			}
 
-			init_waitqueue_entry(&wait, current);
-			add_wait_queue(&ai->thr_wait, &wait);
-			for (;;) {
-				set_current_state(TASK_INTERRUPTIBLE);
-				if (ai->jobs)
-					break;
-				if (ai->expires || ai->scan_timeout) {
-					if (ai->scan_timeout &&
-							time_after_eq(jiffies,ai->scan_timeout)){
-						set_bit(JOB_SCAN_RESULTS, &ai->jobs);
-						break;
-					} else if (ai->expires &&
-							time_after_eq(jiffies,ai->expires)){
-						set_bit(JOB_AUTOWEP, &ai->jobs);
-						break;
-					}
-					if (!kthread_should_stop()) {
-						unsigned long wake_at;
-						if (!ai->expires || !ai->scan_timeout) {
-							wake_at = max(ai->expires,
-								ai->scan_timeout);
-						} else {
-							wake_at = min(ai->expires,
-								ai->scan_timeout);
-						}
-						schedule_timeout(wake_at - jiffies);
-						continue;
-					}
-				} else if (!kthread_should_stop()) {
-					schedule();
-					continue;
-				}
+			if (ai->expires &&
+			    time_after_eq(jiffies, ai->expires)){
+				set_bit(JOB_AUTOWEP, &ai->jobs);
 				break;
 			}
-			current->state = TASK_RUNNING;
-			remove_wait_queue(&ai->thr_wait, &wait);
-			locked = 1;
-		}
 
-		if (locked)
-			continue;
+			if (ai->expires && ai->scan_timeout)
+				wake_at = min(ai->expires, ai->scan_timeout);
+			else if (ai->expires || ai->scan_timeout)
+				wake_at = max(ai->expires, ai->scan_timeout);
+			else
+				wake_at = MAX_SCHEDULE_TIMEOUT;
 
-		if (test_bit(JOB_DIE, &ai->jobs)) {
-			up(&ai->sem);
-			break;
+			schedule_timeout(wake_at - jiffies);
 		}
 
+		__set_current_state(TASK_RUNNING);
+		if (kthread_should_stop())
+			break;
+
+		if (down_interruptible(&ai->sem)) 
+			continue;
+
 		if (ai->power.event || test_bit(FLAG_FLASHING, &ai->flags)) {
 			up(&ai->sem);
 			continue;

@@ -3180,7 +3157,7 @@
 			OUT4500( apriv, EVACK, EV_MIC );
 			if (test_bit(FLAG_MIC_CAPABLE, &apriv->flags)) {
 				set_bit(JOB_MIC, &apriv->jobs);
-				wake_up_interruptible(&apriv->thr_wait);
+				wake_up_process(apriv->airo_thread_task);
 			}
 		}
 		if ( status & EV_LINK ) {

@@ -3234,13 +3211,13 @@
 
 				if (down_trylock(&apriv->sem) != 0) {
 					set_bit(JOB_EVENT, &apriv->jobs);
-					wake_up_interruptible(&apriv->thr_wait);
+					wake_up_process(apriv->airo_thread_task);
 				} else
 					airo_send_event(dev);
 			} else if (!scan_forceloss) {
 				if (auto_wep && !apriv->expires) {
 					apriv->expires = RUN_AT(3*HZ);
-					wake_up_interruptible(&apriv->thr_wait);
+					wake_up_process(apriv->airo_thread_task);
 				}
 
 				/* Send event to user space */

@@ -3903,7 +3880,7 @@
 
 	if (auto_wep) {
 		ai->expires = RUN_AT(3*HZ);
-		wake_up_interruptible(&ai->thr_wait);
+		wake_up_process(ai->airo_thread_task);
 	}
 
 	return SUCCESS;

@@ -7179,7 +7156,7 @@
 out:
 	up(&ai->sem);
 	if (wake)
-		wake_up_interruptible(&ai->thr_wait);
+		wake_up_process(ai->airo_thread_task);
 	return 0;
 }

@@ -7677,7 +7654,7 @@
 		/* Get stats out of the card if available */
 		if (down_trylock(&local->sem) != 0) {
 			set_bit(JOB_WSTATS, &local->jobs);
-			wake_up_interruptible(&local->thr_wait);
+			wake_up_process(local->airo_thread_task);
 		} else
 			airo_read_wireless_stats(local);
 	}

Sukadev Bhattiprolu

author

2006-07-13 23:00 UTC #2 lore raw

Christoph Hellwig [[email protected]] wrote:

| On Thu, Jul 13, 2006 at 01:53:19PM -0700, Sukadev Bhattiprolu 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
| 
| I'm not sure it's that easy.  I think it needs some more love:
| 
|  - switch to wake_uo_process
|  - kill JOB_DIE
|  - cleanup a the convoluted mess in airo_thread a bit
| 
| Note that it's still reimplementing the single threaded workqueue
| functionality quite badly.  So if someone could switch it over and while
| we're at it try to kill the idiociy of doing the trylock in the calling
| context and only then calling the thread by always calling the thread
| (which also solves the synchronization problem).
| 
| Anywhy, here's a small incremental patch ontop of yours to implement my
| above items:

I had a quick look at your patch and looks fine to me. I agree we could do more to clean up the driver.

My inital goal was to replace kernel_thread() with kthread_*(). So can I assume you are ok with my patch and that it can go in as is ?

Suka

John W. Linville

2006-07-14 14:36 UTC #3 lore raw

Quoted 11 lines · Sukadev Bhattiprolu

Christoph Hellwig [[email protected]] wrote:

| On Thu, Jul 13, 2006 at 01:53:19PM -0700, Sukadev Bhattiprolu 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
| 
| I'm not sure it's that easy.  I think it needs some more love:

Quoted 2 lines

My inital goal was to replace kernel_thread() with kthread_*(). So can I assume you are ok with my patch and that it can go in as is ?

Could you please repost your (final) patch(es) to [email protected], and cc: me as well? I missed your original post, since it was (apparently) just to LKML.

Thanks,

John

Signature · 2 lines

John W. Linville
[email protected]

Sukadev Bhattiprolu

author

2006-07-24 18:13 UTC #4 lore raw

In reply to Sukadev Bhattiprolu

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.

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 <[email protected]>

Cc Javier Achirica <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc John Linville <[email protected]>

 drivers/net/wireless/airo.c |   38 +++++++++++++++-----------------------
 1 files changed, 15 insertions(+), 23 deletions(-)

drivers/net/wireless/airo.c +15 ~~-23~~

Index: linux-2.6.18-rc1-mm2/drivers/net/wireless/airo.c
===================================================================
@@ -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);

Andrew Morton

2006-07-26 06:19 UTC #5 lore raw

Quoted 33 lines · On Mon, 24 Jul 2006 11:13:09 -0700 Sukadev Bhattiprolu <[email protected]>…

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

Review static int readBSSListRid(struct airo_in · line 1741

Quoted 8 lines

(quoted diff) +2 -2

@@ -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:

drivers/net/wireless/airo.c renamed from drivers/net/wireless/airo.c~kthread-airoc-race-fix +1 -1

@@ -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;
 	}

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