[PATCH] kthread conversion: convert ieee1394 from kernel_thread

[PATCH] kthread conversion: convert ieee1394 from kernel_thread

[Serge E. Hallyn]

Convert ieee1394 from using deprecated kernel_thread to
kthread api.

Compiles fine, but unfortunately I am unable to test.

Signed-off-by: Serge Hallyn <serue@us.ibm.com>

---

 drivers/ieee1394/ieee1394_core.c |   20 +++++++++++---------
 drivers/ieee1394/nodemgr.c       |   12 +++++++-----
 2 files changed, 18 insertions(+), 14 deletions(-)

d1e631b1c58bc2cfe4dbcdb5138abce5060fed5a
diff --git a/drivers/ieee1394/ieee1394_core.c b/drivers/ieee1394/ieee1394_core.c
index be6854e..075ee3e 100644
--- a/drivers/ieee1394/ieee1394_core.c
+++ b/drivers/ieee1394/ieee1394_core.c
@@ -33,6 +33,7 @@
 #include <linux/kdev_t.h>
 #include <linux/skbuff.h>
 #include <linux/suspend.h>
+#include <linux/kthread.h>
 
 #include <asm/byteorder.h>
 #include <asm/semaphore.h>
@@ -997,7 +998,8 @@ void abort_timedouts(unsigned long __opa
  * packets that have a "complete" function are sent here. This way, the
  * completion is run out of kernel context, and doesn't block the rest of
  * the stack. */
-static int khpsbpkt_pid = -1, khpsbpkt_kill;
+static int khpsbpkt_kill;
+struct task_struct *khpsbpkt_task;
 static DECLARE_COMPLETION(khpsbpkt_complete);
 static struct sk_buff_head hpsbpkt_queue;
 static DECLARE_MUTEX_LOCKED(khpsbpkt_sig);
@@ -1065,8 +1067,8 @@ static int __init ieee1394_init(void)
 		HPSB_ERR("Some features may not be available\n");
 	}
 
-	khpsbpkt_pid = kernel_thread(hpsbpkt_thread, NULL, CLONE_KERNEL);
-	if (khpsbpkt_pid < 0) {
+	khpsbpkt_task = kthread_create(hpsbpkt_thread, NULL, "hpsbpkt");
+	if (IS_ERR(khpsbpkt_task)) {
 		HPSB_ERR("Failed to start hpsbpkt thread!\n");
 		ret = -ENOMEM;
 		goto exit_cleanup_config_roms;
@@ -1133,6 +1135,8 @@ static int __init ieee1394_init(void)
 		goto cleanup_csr;
 	}
 
+	wake_up_process(khpsbpkt_task);
+
 	return 0;
 
 cleanup_csr:
@@ -1148,10 +1152,8 @@ release_all_bus:
 release_chrdev:
 	unregister_chrdev_region(IEEE1394_CORE_DEV, 256);
 exit_release_kernel_thread:
-	if (khpsbpkt_pid >= 0) {
-		kill_proc(khpsbpkt_pid, SIGTERM, 1);
-		wait_for_completion(&khpsbpkt_complete);
-	}
+	if (khpsbpkt_task && !IS_ERR(khpsbpkt_task))
+		kthread_stop(khpsbpkt_task);
 exit_cleanup_config_roms:
 	hpsb_cleanup_config_roms();
 	return ret;
@@ -1172,11 +1174,11 @@ static void __exit ieee1394_cleanup(void
 		bus_remove_file(&ieee1394_bus_type, fw_bus_attrs[i]);
 	bus_unregister(&ieee1394_bus_type);
 
-	if (khpsbpkt_pid >= 0) {
+	if (khpsbpkt_task) {
 		khpsbpkt_kill = 1;
 		mb();
 		up(&khpsbpkt_sig);
-		wait_for_completion(&khpsbpkt_complete);
+		kthread_stop(khpsbpkt_task);
 	}
 
 	hpsb_cleanup_config_roms();
diff --git a/drivers/ieee1394/nodemgr.c b/drivers/ieee1394/nodemgr.c
index 082c7fd..83392af 100644
--- a/drivers/ieee1394/nodemgr.c
+++ b/drivers/ieee1394/nodemgr.c
@@ -19,6 +19,7 @@
 #include <linux/delay.h>
 #include <linux/pci.h>
 #include <linux/moduleparam.h>
+#include <linux/kthread.h>
 #include <asm/atomic.h>
 
 #include "ieee1394_types.h"
@@ -115,7 +116,7 @@ struct host_info {
 	struct list_head list;
 	struct completion exited;
 	struct semaphore reset_sem;
-	int pid;
+	struct task_struct *task;
 	char daemon_name[15];
 	int kill_me;
 };
@@ -1719,15 +1720,16 @@ static void nodemgr_add_host(struct hpsb
 
 	sprintf(hi->daemon_name, "knodemgrd_%d", host->id);
 
-	hi->pid = kernel_thread(nodemgr_host_thread, hi, CLONE_KERNEL);
+	hi->task = kthread_create(nodemgr_host_thread, hi, hi->daemon_name);
 
-	if (hi->pid < 0) {
+	if (IS_ERR(hi->task)) {
 		HPSB_ERR ("NodeMgr: failed to start %s thread for %s",
 			  hi->daemon_name, host->driver->name);
 		hpsb_destroy_hostinfo(&nodemgr_highlevel, host);
 		return;
 	}
 
+	wake_up_process(hi->task);
 	return;
 }
 
@@ -1749,11 +1751,11 @@ static void nodemgr_remove_host(struct h
 	struct host_info *hi = hpsb_get_hostinfo(&nodemgr_highlevel, host);
 
 	if (hi) {
-		if (hi->pid >= 0) {
+		if (hi->task && !IS_ERR(hi->task)) {
 			hi->kill_me = 1;
 			mb();
 			up(&hi->reset_sem);
-			wait_for_completion(&hi->exited);
+			kthread_stop(hi->task);
 			nodemgr_remove_host_dev(&host->device);
 		}
 	} else
-- 
1.1.6

Re: [PATCH] kthread conversion: convert ieee1394 from kernel_thread

[Christoph Hellwig]

On Sat, Jun 10, 2006 at 09:31:00AM -0500, Serge E. Hallyn wrote:
> Convert ieee1394 from using deprecated kernel_thread to
> kthread api.
> 
> Compiles fine, but unfortunately I am unable to test.

This patch does various things wrong or at least suboptimal.  See
'[PATCH] ieee1394_core: switch to kthread API' sent to the ieee1394-devel
list on the 14th of April for a patch the gets the formalisms right,
although I wasn't able to test it either.

Re: [PATCH] kthread conversion: convert ieee1394 from kernel_thread

[Stefan Richter]

Christoph Hellwig wrote:
> On Sat, Jun 10, 2006 at 09:31:00AM -0500, Serge E. Hallyn wrote:
> 
>>Convert ieee1394 from using deprecated kernel_thread to
>>kthread api.
>>
>>Compiles fine, but unfortunately I am unable to test.
> 
> 
> This patch does various things wrong or at least suboptimal.  See
> '[PATCH] ieee1394_core: switch to kthread API' sent to the ieee1394-devel
> list on the 14th of April for a patch the gets the formalisms right,
> although I wasn't able to test it either.

The patch Christoph is referring to, together with a small fix by Andrew 
Morton, is in -mm and is IMO ready to go into Linux 2.6.18. However this 
patch converts only the khpsbpkt (packet/ transactions handler), not the 
knodemgrd (bus management and protocol driver dispatcher).

Serge, could you reduce your patch to the nodemgr part and resubmit? A 
diff against current -mm would be most welcome. If you develop on top of 
Linus' tree, you could get current 1394 subsystem code which is almost 
identical to latest -mm here:
http://me.in-berlin.de/~s5r6/linux1394/updates/

Thanks,
-- 
Stefan Richter
-=====-=-==- -==- -=-=-
http://arcgraph.de/sr/

Re: [PATCH] kthread conversion: convert ieee1394 from kernel_thread

[Christoph Hellwig]

On Sat, Jun 10, 2006 at 05:11:42PM +0200, Stefan Richter wrote:
> Serge, could you reduce your patch to the nodemgr part and resubmit?

I'd prefer ieee1394 would just stop creating these thread entirely.
Sure, rescaning the bus might take some time, but so do pci or especially
scsi bus rescans.  A user should expect his thread to block when he
writes to an attribute caled rescan.

Re: [PATCH] kthread conversion: convert ieee1394 from kernel_thread

[Ben Collins]

On Sat, 2006-06-10 at 16:42 +0100, Christoph Hellwig wrote:
> On Sat, Jun 10, 2006 at 05:11:42PM +0200, Stefan Richter wrote:
> > Serge, could you reduce your patch to the nodemgr part and resubmit?
> 
> I'd prefer ieee1394 would just stop creating these thread entirely.
> Sure, rescaning the bus might take some time, but so do pci or especially
> scsi bus rescans.  A user should expect his thread to block when he
> writes to an attribute caled rescan.

1394 bus rescanning takes a _lot_ longer than a PCI rescan. If we don't
do this in a kthread, then we have to do it as a tasklet, and take a
chance of stalling for a few seconds (not ms), preventing other
tasklet's from running. Suboptimal, IMO.

Also, neither PCI nor SCSI rescans occur quite as often as 1394 rescans.
It's more like USB (which also uses a kthread, or at least used to).

-- 
Ubuntu     - http://www.ubuntu.com/
Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
SwissDisk  - http://www.swissdisk.com/

Re: [PATCH] kthread conversion: convert ieee1394 from kernel_thread

[Christoph Hellwig]

On Sat, Jun 10, 2006 at 12:34:46PM -0400, Ben Collins wrote:
> 1394 bus rescanning takes a _lot_ longer than a PCI rescan. If we don't
> do this in a kthread, then we have to do it as a tasklet, and take a
> chance of stalling for a few seconds (not ms), preventing other
> tasklet's from running. Suboptimal, IMO.

This is just user-initiated FC rescans.  And I doubt they take as long
as parallel scsi rescans which can go into the minutes range easily.
Nothing will be stalled by calling this except the caller, which would
usually be echo called from some shell, something the user can put in
the background using job control.

Re: [PATCH] kthread conversion: convert ieee1394 from kernel_thread

[Ben Collins]

On Sat, 2006-06-10 at 17:38 +0100, Christoph Hellwig wrote:
> On Sat, Jun 10, 2006 at 12:34:46PM -0400, Ben Collins wrote:
> > 1394 bus rescanning takes a _lot_ longer than a PCI rescan. If we don't
> > do this in a kthread, then we have to do it as a tasklet, and take a
> > chance of stalling for a few seconds (not ms), preventing other
> > tasklet's from running. Suboptimal, IMO.
> 
> This is just user-initiated FC rescans.  And I doubt they take as long
> as parallel scsi rescans which can go into the minutes range easily.
> Nothing will be stalled by calling this except the caller, which would
> usually be echo called from some shell, something the user can put in
> the background using job control.

Most rescans are initiated by a bus reset (usually caused by a
connect/disconnect of a device) that is detected in interrupt.
Obviously, we cannot initiate these rescans in interrupt, so a tasklet
or kthread is the only option.

The reason for handling user-initiated rescans (through some sysfs
interface?) and hardware-initiated rescans in the same place is code
simplicity, and synchronization.

I'm not sure what your implying about user-initiated rescans. The only
thing I can think of is device/driver binding, which isn't handled in
our kernel thread anyway (except where it's a new device being detected,
as opposed to a new driver being loaded).

-- 
Ubuntu     - http://www.ubuntu.com/
Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
SwissDisk  - http://www.swissdisk.com/

Re: [PATCH] kthread conversion: convert ieee1394 from kernel_thread

[Stefan Richter]

Christoph Hellwig wrote:
> On Sat, Jun 10, 2006 at 12:34:46PM -0400, Ben Collins wrote:
>>1394 bus rescanning takes a _lot_ longer than a PCI rescan. If we don't
>>do this in a kthread, then we have to do it as a tasklet, and take a
>>chance of stalling for a few seconds (not ms), preventing other
>>tasklet's from running. Suboptimal, IMO.
> 
> This is just user-initiated FC rescans.  And I doubt they take as long
> as parallel scsi rescans which can go into the minutes range easily.
> Nothing will be stalled by calling this except the caller, which would
> usually be echo called from some shell, something the user can put in
> the background using job control.

There is no such userspace caller yet.

Note, the task of nodemgr, or of a hypothetical userspace replacement, 
is not only to scan the ROMs of nodes and initiate attachment of 
protocol drivers but also
  - to rescan the ROMs of nodes after bus resets (necessary for protocol
    drivers to reconnect, which has restrictions WRT latency),
  - IEEE 1394 bus management (which has hard latency restrictions too).
We also started using nodemgr to determine the correct speed at which 
IEEE 1394b nodes can be reached, i.e. for fairly low-level tasks. And 
there are further plans for nodemgr like optimization of bus arbitration 
gaps, or parallelized node scanning.

I am not sure that this can really be loaded off to userspace. And if we 
would do so, deployment would become painful.
-- 
Stefan Richter
-=====-=-==- -==- -=-=-
http://arcgraph.de/sr/

Re: [PATCH] kthread conversion: convert ieee1394 from kernel_thread

[Christoph Hellwig]

On Sat, Jun 10, 2006 at 02:08:50PM -0400, Ben Collins wrote:
> Most rescans are initiated by a bus reset (usually caused by a
> connect/disconnect of a device) that is detected in interrupt.
> Obviously, we cannot initiate these rescans in interrupt, so a tasklet
> or kthread is the only option.
> 
> The reason for handling user-initiated rescans (through some sysfs
> interface?) and hardware-initiated rescans in the same place is code
> simplicity, and synchronization.
> 
> I'm not sure what your implying about user-initiated rescans. The only
> thing I can think of is device/driver binding, which isn't handled in
> our kernel thread anyway (except where it's a new device being detected,
> as opposed to a new driver being loaded).

Sorry, I was confused when looking at the code.  There are two calls
to kernel_thread in drivers/ieee1394/nodemgr.c.  The first one is from
fw_set_rescan, which handles a writeable sysfs attribute.  For that all
my comments in this thread stand, and I think it should not start a
thread.

The second one is the actual, real daemon you talked about most of the
time.  That one should of course stay a kernel thread!

Below is a draft patch to convert it to the kthread API and replace the
reset_sem with a simple wake_up_process scheme.  I removed the down_trylock
loop there which I think should be fine because we get the wakeup again
ASAP, but please double-check.


Index: linux-2.6/drivers/ieee1394/nodemgr.c
===================================================================
--- linux-2.6.orig/drivers/ieee1394/nodemgr.c	2006-06-10 20:25:14.000000000 +0200
+++ linux-2.6/drivers/ieee1394/nodemgr.c	2006-06-10 20:33:01.000000000 +0200
@@ -19,6 +19,7 @@
 #include <linux/delay.h>
 #include <linux/pci.h>
 #include <linux/moduleparam.h>
+#include <linux/kthread.h>
 #include <asm/atomic.h>
 
 #include "ieee1394_types.h"
@@ -113,11 +114,7 @@
 struct host_info {
 	struct hpsb_host *host;
 	struct list_head list;
-	struct completion exited;
-	struct semaphore reset_sem;
-	int pid;
-	char daemon_name[15];
-	int kill_me;
+	struct task_struct *thread;
 };
 
 static int nodemgr_bus_match(struct device * dev, struct device_driver * drv);
@@ -1567,9 +1564,6 @@
 	struct hpsb_host *host = hi->host;
 	int reset_cycles = 0;
 
-	/* No userlevel access needed */
-	daemonize(hi->daemon_name);
-
 	/* Setup our device-model entries */
 	nodemgr_create_host_dev_files(host);
 
@@ -1579,15 +1573,14 @@
 		unsigned int generation = 0;
 		int i;
 
-		if (down_interruptible(&hi->reset_sem) ||
-		    down_interruptible(&nodemgr_serialize)) {
+		if (down_interruptible(&nodemgr_serialize)) {
 			if (try_to_freeze())
 				continue;
 			printk("NodeMgr: received unexpected signal?!\n" );
 			break;
 		}
 
-		if (hi->kill_me) {
+		if (kthread_should_stop()) {
 			up(&nodemgr_serialize);
 			break;
 		}
@@ -1608,13 +1601,8 @@
 			 * returning bogus data. */
 			generation = get_hpsb_generation(host);
 
-			/* If we get a reset before we are done waiting, then
-			 * start the the waiting over again */
-			while (!down_trylock(&hi->reset_sem))
-				i = 0;
-
-			/* Check the kill_me again */
-			if (hi->kill_me) {
+			/* Check the whether we should stop again */
+			if (kthread_should_stop()) {
 				up(&nodemgr_serialize);
 				goto caught_signal;
 			}
@@ -1647,7 +1635,7 @@
 caught_signal:
 	HPSB_VERBOSE("NodeMgr: Exiting thread");
 
-	complete_and_exit(&hi->exited, 0);
+	return 0;
 }
 
 int nodemgr_for_each_host(void *__data, int (*cb)(struct hpsb_host *, void *))
@@ -1714,16 +1702,11 @@
 	}
 
 	hi->host = host;
-	init_completion(&hi->exited);
-        sema_init(&hi->reset_sem, 0);
+	hi->thread = kthread_create(nodemgr_host_thread, hi, "knodemgrd_%d", host->id);
 
-	sprintf(hi->daemon_name, "knodemgrd_%d", host->id);
-
-	hi->pid = kernel_thread(nodemgr_host_thread, hi, CLONE_KERNEL);
-
-	if (hi->pid < 0) {
-		HPSB_ERR ("NodeMgr: failed to start %s thread for %s",
-			  hi->daemon_name, host->driver->name);
+	if (IS_ERR(hi->thread)) {
+		HPSB_ERR ("NodeMgr: failed to start thread for %s",
+			  host->driver->name);
 		hpsb_destroy_hostinfo(&nodemgr_highlevel, host);
 		return;
 	}
@@ -1736,8 +1719,7 @@
 	struct host_info *hi = hpsb_get_hostinfo(&nodemgr_highlevel, host);
 
 	if (hi != NULL) {
-		HPSB_VERBOSE("NodeMgr: Processing host reset for %s", hi->daemon_name);
-		up(&hi->reset_sem);
+		wake_up_process(hi->thread);
 	} else
 		HPSB_ERR ("NodeMgr: could not process reset of unused host");
 
@@ -1749,13 +1731,8 @@
 	struct host_info *hi = hpsb_get_hostinfo(&nodemgr_highlevel, host);
 
 	if (hi) {
-		if (hi->pid >= 0) {
-			hi->kill_me = 1;
-			mb();
-			up(&hi->reset_sem);
-			wait_for_completion(&hi->exited);
-			nodemgr_remove_host_dev(&host->device);
-		}
+		kthread_stop(hi->thread);
+		nodemgr_remove_host_dev(&host->device);
 	} else
 		HPSB_ERR("NodeMgr: host %s does not exist, cannot remove",
 			 host->driver->name);

Re: [PATCH] kthread conversion: convert ieee1394 from kernel_thread

[Stefan Richter]

Christoph Hellwig wrote on 2006-06-17:
> Below is a draft patch to convert it to the kthread API and replace the
> reset_sem with a simple wake_up_process scheme.  I removed the down_trylock
> loop there which I think should be fine because we get the wakeup again
> ASAP, but please double-check.
[...]
[in nodemgr_host_thread(), top of event loop]
> @@ -1579,15 +1573,14 @@
>  		unsigned int generation = 0;
>  		int i;
>  
> -		if (down_interruptible(&hi->reset_sem) ||
> -		    down_interruptible(&nodemgr_serialize)) {
> +		if (down_interruptible(&nodemgr_serialize)) {

This won't work. "down_interruptible(&hi->reset_sem)" was there to put 
the thread to sleep after it did its work, until the next bus reset. Now 
the event loop would be entered continuously without an actual bus reset 
event.

(nodemgr_serialize is just a mutex disguised as a semaphore which 
prevents multiple nodemgr host threads to enter their event loop 
concurrently.)

>  			if (try_to_freeze())
>  				continue;
>  			printk("NodeMgr: received unexpected signal?!\n" );
>  			break;
>  		}
>  
> -		if (hi->kill_me) {
> +		if (kthread_should_stop()) {
>  			up(&nodemgr_serialize);
>  			break;
>  		}
> @@ -1608,13 +1601,8 @@
>  			 * returning bogus data. */
>  			generation = get_hpsb_generation(host);
>  
> -			/* If we get a reset before we are done waiting, then
> -			 * start the the waiting over again */
> -			while (!down_trylock(&hi->reset_sem))
> -				i = 0;
[...]

Another minor issue: This check cannot be removed without replacement. 
However we could implement this check easily without a counting 
semaphore. (We could check the bus generation before and after the sleep.)

I will try to rework this patch and split it into more patches: One 
which converts nodemgr to the kthread API but keeps the reset_sem, one 
patch which gets rid of the counting semaphore reset_sem, one patch 
which converts nodemgr_serialize to a mutex.

BTW, it may be possible to remove nodemgr_serialize too. There are other 
exclusion mechanisms in nodemgr.c which should already prevent most if 
not all undesirable concurrency, notably the semaphores 
dev->bus->subsys.rwsem and class->subsys.rwsem.
-- 
Stefan Richter
-=====-=-==- -==- =---=
http://arcgraph.de/sr/

Re: [PATCH] kthread conversion: convert ieee1394 from kernel_thread

[Stefan Richter]

I wrote:
> I will try to rework this patch

While doing so I discovered an old bug which happens with current code 
as well as with Linux 2.6.16.x and 2.6.15.x, maybe also on older kernels:

When "modprobe ohci1394" is followed shortly by "modprobe -r ohci1394" 
(say, 1 second after the previous modprobe finished), one of the 
following may happen:
  - kernel panic due to exception in interrupt
    (happened on 2.6.15.x preempt uniprocessor)
    or
  - modprobe -r hangs in D state, as does knodemgrd_0
    (happened on 2.6.16.x preempt SMP on a uniprocessor machine).

I have two FireWirew host adapters installed. The knodemgrd_1 slept 
interruptibly (S state) while the other slept uninterruptibly (D state) 
right after modprobe -r was issued. This happens with or without other 
nodes attached to the FireWire ports. Host adapters are based on a TI 
1394b chip (host 0) and on a VIA 1394a chip (host 1).

"All's right with the world" if a longer pause is put between modprobe 
and modprobe -r, say 4 seconds.

(I will add a bugzilla entry and try to resolve this eventually...)
-- 
Stefan Richter
-=====-=-==- -==- =--=-
http://arcgraph.de/sr/

[PATCH 2.6.17-rc6-mm2 0/6] ieee1394: nodemgr: misc API conversions

[Stefan Richter]

The following patches clean up and convert API usages in
ieee1394/nodemgr.c.  Parts of this patch series supersede the previously
posted variants of "[PATCH] kthread conversion: convert ieee1394 from
kernel_thread".

1/6 ieee1394: nodemgr: remove unnecessary includes
2/6 ieee1394: do not spawn a kernel_thread for user-initiated bus rescan
3/6 ieee1394: make module parameter ignore_drivers writable
4/6 ieee1394: nodemgr: switch to kthread API
5/6 ieee1394: nodemgr: replace reset semaphore
6/6 ieee1394: convert nodemgr_serialize semaphore to mutex

 drivers/ieee1394/nodemgr.c |  151 +++++++++++--------------------------
 1 files changed, 47 insertions(+), 104 deletions(-)

In case someone actually puts these patches to test and finds a lockup
or something more serious, please check if it isn't this older bug:
http://bugzilla.kernel.org/show_bug.cgi?id=6706
Two things which I could not test are behaviour on actual SMP machines
and the try_to_freeze() business.

Patch 3/6 is not an API conversion but is loosely related to patch 2/6.

After this patchset, all usages of the kernel_thread API are removed
from the ieee1394 subsystem.  Two usages of semaphores are replaced.
The following semaphores are still remaining in drivers/ieee1394:

dv1394.c:                video->sem       (simple mutex-type semaphore)
highlevel.c:             hl_drivers_sem   (RW semaphore)
ieee1394_transactions.c: tp->count        (counting semaphore)
nodemgr.c:               subsys.rwsem     (driver core's RW semaphores)
raw1394.c:               fi->complete_sem (completion semaphore)
-- 
Stefan Richter
-=====-=-==- -==- =--=-
http://arcgraph.de/sr/

[PATCH 2.6.17-rc6-mm2 1/6] ieee1394: nodemgr: remove unnecessary includes

[Stefan Richter]

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
Index: linux/drivers/ieee1394/nodemgr.c
===================================================================
--- linux.orig/drivers/ieee1394/nodemgr.c	2006-06-18 08:57:09.000000000 +0200
+++ linux/drivers/ieee1394/nodemgr.c	2006-06-18 09:11:18.000000000 +0200
@@ -12,12 +12,8 @@
 #include <linux/config.h>
 #include <linux/list.h>
 #include <linux/slab.h>
-#include <linux/smp_lock.h>
-#include <linux/interrupt.h>
-#include <linux/kmod.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
-#include <linux/pci.h>
 #include <linux/moduleparam.h>
 #include <asm/atomic.h>
 

[PATCH 2.6.17-rc6-mm2 2/6] ieee1394: do not spawn a kernel_thread for user-initiated bus rescan

[Stefan Richter]

nodemgr.c::fw_set_rescan() is used to re-run the driver core over
nodemgr's representation of unit directories in order to initiate
protocol driver probes.  It is initiated via write access to one of
nodemgr's sysfs attributes.  The purpose is to attach drivers to
units after switching a unit's ignore_driver attribute from 1 to 0.

It is not really necessary to fork a kernel_thread for this job.
The call to kernel_thread() can be eliminated to avoid the deprecated
API and to simplify the code a bit.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
Index: linux/drivers/ieee1394/nodemgr.c
===================================================================
--- linux.orig/drivers/ieee1394/nodemgr.c	2006-06-18 09:11:18.000000000 +0200
+++ linux/drivers/ieee1394/nodemgr.c	2006-06-18 09:15:45.000000000 +0200
@@ -402,26 +402,11 @@ static ssize_t fw_get_destroy_node(struc
 }
 static BUS_ATTR(destroy_node, S_IWUSR | S_IRUGO, fw_get_destroy_node, fw_set_destroy_node);
 
-static int nodemgr_rescan_bus_thread(void *__unused)
-{
-	/* No userlevel access needed */
-	daemonize("kfwrescan");
-
-	bus_rescan_devices(&ieee1394_bus_type);
-
-	return 0;
-}
 
 static ssize_t fw_set_rescan(struct bus_type *bus, const char *buf, size_t count)
 {
-	int state = simple_strtoul(buf, NULL, 10);
-
-	/* Don't wait for this, or care about errors. Root could do
-	 * something stupid and spawn this a lot of times, but that's
-	 * root's fault. */
-	if (state == 1)
-		kernel_thread(nodemgr_rescan_bus_thread, NULL, CLONE_KERNEL);
-
+	if (simple_strtoul(buf, NULL, 10) == 1)
+		bus_rescan_devices(&ieee1394_bus_type);
 	return count;
 }
 static ssize_t fw_get_rescan(struct bus_type *bus, char *buf)

[PATCH 2.6.17-rc6-mm2 3/6] ieee1394: make module parameter ignore_drivers writable

[Stefan Richter]

Nodemgr's ignore_drivers variable is exposed as a module load parameter
(therefore also as a sysfs attribute below /sys/module) and additionally
as an attribute below /sys/bus/ieee1394.  Since the latter is writable,
make the former writable too.

Note, the bus's attribute ignore_drivers is only relevant to newly added
units, not to present or suspended or resuming units.  Those have their
own attribute ignore_driver.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
Index: linux/drivers/ieee1394/nodemgr.c
===================================================================
--- linux.orig/drivers/ieee1394/nodemgr.c	2006-06-18 09:15:45.000000000 +0200
+++ linux/drivers/ieee1394/nodemgr.c	2006-06-18 11:27:13.000000000 +0200
@@ -27,7 +27,7 @@
 #include "nodemgr.h"
 
 static int ignore_drivers;
-module_param(ignore_drivers, int, 0444);
+module_param(ignore_drivers, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(ignore_drivers, "Disable automatic probing for drivers.");
 
 struct nodemgr_csr_info {

[PATCH 2.6.17-rc6-mm2 4/6] ieee1394: nodemgr: switch to kthread API

[Stefan Richter]

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
Index: linux/drivers/ieee1394/nodemgr.c
===================================================================
--- linux.orig/drivers/ieee1394/nodemgr.c	2006-06-18 09:15:51.000000000 +0200
+++ linux/drivers/ieee1394/nodemgr.c	2006-06-18 11:25:17.000000000 +0200
@@ -12,8 +12,8 @@
 #include <linux/config.h>
 #include <linux/list.h>
 #include <linux/slab.h>
-#include <linux/completion.h>
 #include <linux/delay.h>
+#include <linux/kthread.h>
 #include <linux/moduleparam.h>
 #include <asm/atomic.h>
 
@@ -162,11 +162,8 @@ static DECLARE_MUTEX(nodemgr_serialize);
 struct host_info {
 	struct hpsb_host *host;
 	struct list_head list;
-	struct completion exited;
 	struct semaphore reset_sem;
-	int pid;
-	char daemon_name[15];
-	int kill_me;
+	struct task_struct *thread;
 };
 
 static int nodemgr_bus_match(struct device * dev, struct device_driver * drv);
@@ -1601,10 +1598,8 @@ static int nodemgr_host_thread(void *__h
 {
 	struct host_info *hi = (struct host_info *)__hi;
 	struct hpsb_host *host = hi->host;
-	int reset_cycles = 0;
-
-	/* No userlevel access needed */
-	daemonize(hi->daemon_name);
+	unsigned int generation = 0;
+	int i, reset_cycles = 0;
 
 	/* Setup our device-model entries */
 	nodemgr_create_host_dev_files(host);
@@ -1612,30 +1607,22 @@ static int nodemgr_host_thread(void *__h
 	/* Sit and wait for a signal to probe the nodes on the bus. This
 	 * happens when we get a bus reset. */
 	while (1) {
-		unsigned int generation = 0;
-		int i;
-
 		if (down_interruptible(&hi->reset_sem) ||
 		    down_interruptible(&nodemgr_serialize)) {
 			if (try_to_freeze())
 				continue;
 			printk("NodeMgr: received unexpected signal?!\n" );
-			break;
+			goto exit;
 		}
 
-		if (hi->kill_me) {
-			up(&nodemgr_serialize);
-			break;
-		}
+		if (kthread_should_stop())
+			goto unlock_exit;
 
 		/* Pause for 1/4 second in 1/16 second intervals,
 		 * to make sure things settle down. */
 		for (i = 0; i < 4 ; i++) {
-			set_current_state(TASK_INTERRUPTIBLE);
-			if (msleep_interruptible(63)) {
-				up(&nodemgr_serialize);
-				goto caught_signal;
-			}
+			if (msleep_interruptible(63))
+				goto unlock_exit;
 
 			/* Now get the generation in which the node ID's we collect
 			 * are valid.  During the bus scan we will use this generation
@@ -1648,12 +1635,8 @@ static int nodemgr_host_thread(void *__h
 			 * start the the waiting over again */
 			while (!down_trylock(&hi->reset_sem))
 				i = 0;
-
-			/* Check the kill_me again */
-			if (hi->kill_me) {
-				up(&nodemgr_serialize);
-				goto caught_signal;
-			}
+			if (kthread_should_stop())
+				goto unlock_exit;
 		}
 
 		if (!nodemgr_check_irm_capability(host, reset_cycles) ||
@@ -1679,11 +1662,11 @@ static int nodemgr_host_thread(void *__h
 
 		up(&nodemgr_serialize);
 	}
-
-caught_signal:
+unlock_exit:
+	up(&nodemgr_serialize);
+exit:
 	HPSB_VERBOSE("NodeMgr: Exiting thread");
-
-	complete_and_exit(&hi->exited, 0);
+	return 0;
 }
 
 int nodemgr_for_each_host(void *__data, int (*cb)(struct hpsb_host *, void *))
@@ -1743,41 +1726,28 @@ static void nodemgr_add_host(struct hpsb
 	struct host_info *hi;
 
 	hi = hpsb_create_hostinfo(&nodemgr_highlevel, host, sizeof(*hi));
-
 	if (!hi) {
-		HPSB_ERR ("NodeMgr: out of memory in add host");
+		HPSB_ERR("NodeMgr: out of memory in add host");
 		return;
 	}
-
 	hi->host = host;
-	init_completion(&hi->exited);
-        sema_init(&hi->reset_sem, 0);
-
-	sprintf(hi->daemon_name, "knodemgrd_%d", host->id);
-
-	hi->pid = kernel_thread(nodemgr_host_thread, hi, CLONE_KERNEL);
-
-	if (hi->pid < 0) {
-		HPSB_ERR ("NodeMgr: failed to start %s thread for %s",
-			  hi->daemon_name, host->driver->name);
+	sema_init(&hi->reset_sem, 0);
+	hi->thread = kthread_run(nodemgr_host_thread, hi, "knodemgrd_%d",
+				 host->id);
+	if (IS_ERR(hi->thread)) {
+		HPSB_ERR("NodeMgr: cannot start thread for host %d", host->id);
 		hpsb_destroy_hostinfo(&nodemgr_highlevel, host);
-		return;
 	}
-
-	return;
 }
 
 static void nodemgr_host_reset(struct hpsb_host *host)
 {
 	struct host_info *hi = hpsb_get_hostinfo(&nodemgr_highlevel, host);
 
-	if (hi != NULL) {
-		HPSB_VERBOSE("NodeMgr: Processing host reset for %s", hi->daemon_name);
+	if (hi) {
+		HPSB_VERBOSE("NodeMgr: Processing reset for host %d", host->id);
 		up(&hi->reset_sem);
-	} else
-		HPSB_ERR ("NodeMgr: could not process reset of unused host");
-
-	return;
+	}
 }
 
 static void nodemgr_remove_host(struct hpsb_host *host)
@@ -1785,18 +1755,9 @@ static void nodemgr_remove_host(struct h
 	struct host_info *hi = hpsb_get_hostinfo(&nodemgr_highlevel, host);
 
 	if (hi) {
-		if (hi->pid >= 0) {
-			hi->kill_me = 1;
-			mb();
-			up(&hi->reset_sem);
-			wait_for_completion(&hi->exited);
-			nodemgr_remove_host_dev(&host->device);
-		}
-	} else
-		HPSB_ERR("NodeMgr: host %s does not exist, cannot remove",
-			 host->driver->name);
-
-	return;
+		kthread_stop_sem(hi->thread, &hi->reset_sem);
+		nodemgr_remove_host_dev(&host->device);
+	}
 }
 
 static struct hpsb_highlevel nodemgr_highlevel = {

[PATCH 2.6.17-rc6-mm2 5/6] ieee1394: nodemgr: replace reset semaphore

[Stefan Richter]

Convert knodemgrd's sleep/restart mechanism from a counting semaphore to
a schedule()/wake_up_process() scheme.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
Index: linux/drivers/ieee1394/nodemgr.c
===================================================================
--- linux.orig/drivers/ieee1394/nodemgr.c	2006-06-18 12:06:30.000000000 +0200
+++ linux/drivers/ieee1394/nodemgr.c	2006-06-18 16:34:45.000000000 +0200
@@ -162,7 +162,6 @@ static DECLARE_MUTEX(nodemgr_serialize);
 struct host_info {
 	struct hpsb_host *host;
 	struct list_head list;
-	struct semaphore reset_sem;
 	struct task_struct *thread;
 };
 
@@ -1468,9 +1467,8 @@ static void nodemgr_node_probe(struct ho
 	/* If we had a bus reset while we were scanning the bus, it is
 	 * possible that we did not probe all nodes.  In that case, we
 	 * skip the clean up for now, since we could remove nodes that
-	 * were still on the bus.  The bus reset increased hi->reset_sem,
-	 * so there's a bus scan pending which will do the clean up
-	 * eventually.
+	 * were still on the bus.  Another bus scan is pending which will
+	 * do the clean up eventually.
 	 *
 	 * Now let's tell the bus to rescan our devices. This may seem
 	 * like overhead, but the driver-model core will only scan a
@@ -1598,30 +1596,36 @@ static int nodemgr_host_thread(void *__h
 {
 	struct host_info *hi = (struct host_info *)__hi;
 	struct hpsb_host *host = hi->host;
-	unsigned int generation = 0;
+	unsigned int g, generation = get_hpsb_generation(host) - 1;
 	int i, reset_cycles = 0;
 
 	/* Setup our device-model entries */
 	nodemgr_create_host_dev_files(host);
 
-	/* Sit and wait for a signal to probe the nodes on the bus. This
-	 * happens when we get a bus reset. */
-	while (1) {
-		if (down_interruptible(&hi->reset_sem) ||
-		    down_interruptible(&nodemgr_serialize)) {
+	for (;;) {
+		/* Sleep until next bus reset */
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (get_hpsb_generation(host) == generation)
+			schedule();
+		__set_current_state(TASK_RUNNING);
+
+		/* Thread may have been woken up to freeze or to exit */
+		if (try_to_freeze())
+			continue;
+		if (kthread_should_stop())
+			goto exit;
+
+		if (down_interruptible(&nodemgr_serialize)) {
 			if (try_to_freeze())
 				continue;
-			printk("NodeMgr: received unexpected signal?!\n" );
 			goto exit;
 		}
 
-		if (kthread_should_stop())
-			goto unlock_exit;
-
 		/* Pause for 1/4 second in 1/16 second intervals,
 		 * to make sure things settle down. */
+		g = get_hpsb_generation(host);
 		for (i = 0; i < 4 ; i++) {
-			if (msleep_interruptible(63))
+			if (msleep_interruptible(63) || kthread_should_stop())
 				goto unlock_exit;
 
 			/* Now get the generation in which the node ID's we collect
@@ -1633,10 +1637,8 @@ static int nodemgr_host_thread(void *__h
 
 			/* If we get a reset before we are done waiting, then
 			 * start the the waiting over again */
-			while (!down_trylock(&hi->reset_sem))
-				i = 0;
-			if (kthread_should_stop())
-				goto unlock_exit;
+			if (generation != g)
+				g = generation, i = 0;
 		}
 
 		if (!nodemgr_check_irm_capability(host, reset_cycles) ||
@@ -1731,7 +1733,6 @@ static void nodemgr_add_host(struct hpsb
 		return;
 	}
 	hi->host = host;
-	sema_init(&hi->reset_sem, 0);
 	hi->thread = kthread_run(nodemgr_host_thread, hi, "knodemgrd_%d",
 				 host->id);
 	if (IS_ERR(hi->thread)) {
@@ -1746,7 +1747,7 @@ static void nodemgr_host_reset(struct hp
 
 	if (hi) {
 		HPSB_VERBOSE("NodeMgr: Processing reset for host %d", host->id);
-		up(&hi->reset_sem);
+		wake_up_process(hi->thread);
 	}
 }
 
@@ -1755,7 +1756,7 @@ static void nodemgr_remove_host(struct h
 	struct host_info *hi = hpsb_get_hostinfo(&nodemgr_highlevel, host);
 
 	if (hi) {
-		kthread_stop_sem(hi->thread, &hi->reset_sem);
+		kthread_stop(hi->thread);
 		nodemgr_remove_host_dev(&host->device);
 	}
 }

[PATCH 2.6.17-rc6-mm2 6/6] ieee1394: convert nodemgr_serialize semaphore to mutex

[Stefan Richter]

Another trivial sem2mutex conversion.

Side note:  nodemgr_serialize's purpose, when introduced in linux1394's
revision 529 in July 2002, was to protect several data structures which
are now largely handled by or together with Linux' driver core and are
now protected by the LDM's own mechanisms.  It may very well be possible
to get rid of this mutex entirely now.  But fully parallelized node
scanning is on our long-term TODO list anyway.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
Index: linux/drivers/ieee1394/nodemgr.c
===================================================================
--- linux.orig/drivers/ieee1394/nodemgr.c	2006-06-18 16:34:45.000000000 +0200
+++ linux/drivers/ieee1394/nodemgr.c	2006-06-18 17:28:48.000000000 +0200
@@ -157,7 +157,7 @@ static struct csr1212_bus_ops nodemgr_cs
  * but now we are much simpler because of the LDM.
  */
 
-static DECLARE_MUTEX(nodemgr_serialize);
+static DEFINE_MUTEX(nodemgr_serialize);
 
 struct host_info {
 	struct hpsb_host *host;
@@ -1615,7 +1615,7 @@ static int nodemgr_host_thread(void *__h
 		if (kthread_should_stop())
 			goto exit;
 
-		if (down_interruptible(&nodemgr_serialize)) {
+		if (mutex_lock_interruptible(&nodemgr_serialize)) {
 			if (try_to_freeze())
 				continue;
 			goto exit;
@@ -1644,7 +1644,7 @@ static int nodemgr_host_thread(void *__h
 		if (!nodemgr_check_irm_capability(host, reset_cycles) ||
 		    !nodemgr_do_irm_duties(host, reset_cycles)) {
 			reset_cycles++;
-			up(&nodemgr_serialize);
+			mutex_unlock(&nodemgr_serialize);
 			continue;
 		}
 		reset_cycles = 0;
@@ -1662,10 +1662,10 @@ static int nodemgr_host_thread(void *__h
 		/* Update some of our sysfs symlinks */
 		nodemgr_update_host_dev_links(host);
 
-		up(&nodemgr_serialize);
+		mutex_unlock(&nodemgr_serialize);
 	}
 unlock_exit:
-	up(&nodemgr_serialize);
+	mutex_unlock(&nodemgr_serialize);
 exit:
 	HPSB_VERBOSE("NodeMgr: Exiting thread");
 	return 0;