fix EH thread teardown

As Rolf Eike Beer noted we might not actually get to the kthread_should_stop() because we are waiting in the semaphore forever. I didn't get a rmmod hang because of this, but my instrumentation showed the thread defitiyly didn't exit.

So make sure to wake the EH thread before the kthread_stop, and to plug the reaming race check kthead_should_stop() a second time just before calling down_interruptible().

Index: scsi-misc-2.6/drivers/scsi/hosts.c
===================================================================
@@ -226,8 +226,10 @@
 	struct Scsi_Host *shost = dev_to_shost(dev);
 	struct device *parent = dev->parent;
 
-	if (shost->ehandler)
+	if (shost->ehandler) {
+		up(shost->eh_wait);
 		kthread_stop(shost->ehandler);
+	}
 	if (shost->work_q)
 		destroy_workqueue(shost->work_q);
 

Index: scsi-misc-2.6/drivers/scsi/scsi_error.c
===================================================================
@@ -1591,7 +1591,7 @@
 	SCSI_LOG_ERROR_RECOVERY(3, printk("Wake up parent of"
 					  " scsi_eh_%d\n",shost->host_no));
 
-	while (1) {
+	while (!kthread_should_stop()) {
 		/*
 		 * If we get a signal, it means we are supposed to go
 		 * away and die.  This typically happens if the user is

Wouldn't this hang the same way if rescheduling happens between the up() and the kthread_stop()?

Eike

Ok, we looked a bit closer on all this. http://lwn.net/Articles/65178/ tells that ktread_stop() will not send a signal, so the down_interruptible() will never return on kthread_stop(). But calling up() directly before kthread_stop() adds a race condition: if reschedule happens right after the up() we've won nothing and it will hang again.

Using this kind of semaphore for thread wakeup looks more dangerous everytime I think on it. down_interruptible() may be interrupted by someone with a signal and return without the semaphore locked. Better would be something like

i = down_interruptible();

if (kthread_should_stop())

	break;

if (i)

	continue;

Or we have to block all signals, then we can just ignore the _interruptible at all. What about something like this instead?

while(!kthread_should_stop()) {
	if (!down_trylock(&sem)) {
		if (kthread_should_stop())
			break;
		yield();
		continue;
	}

	/* something */
}

Eike