Re: [PATCH] cpci_hotplug: Convert to use the kthread API

[Scott Murray]

On Sun, 22 Apr 2007, Christoph Hellwig wrote:

> On Thu, Apr 19, 2007 at 12:55:29AM -0600, Eric W. Biederman wrote:
> > From: Eric W. Biederman <ebiederm@xxxxxxxxxxxx> - unquoted
> > 
> > kthread_run replaces the kernel_thread and  daemonize calls
> > during thread startup.
> > 
> > Calls to signal_pending were also removed as it is currently
> > impossible for the cpci_hotplug thread to receive signals.
> 
> This drivers thread are a bit of a miss, although a lot better than
> most other pci hotplug drivers :)
> 
> Below is more complete conversion to the kthread infrastructure +
> wake_up_process to wake the thread.  Note that we had to keep
> a thread_finished variable because the existing one had dual use.

Sorry, it took me a few days to get to testing this out.  It looks good,    
but I had to make a couple of tweaks to avoid a hang when rmmod'ing a
board driver.  The board drivers do:

cpci_hp_stop()
cpci_hp_unregister_controller(controller)

to shutdown, and the check in cpci_hp_unregister_controller if the thread
is running wasn't working due to a bit too much code being excised.  The
result was kthread_stop being called twice, which hangs.  I've indicated 
my changes to avoid this inline below.

Scott

> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Acked-by: Scott Murray <scottm@xxxxxxxxxxxxxxxx>

> Index: linux-2.6/drivers/pci/hotplug/cpci_hotplug_core.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/hotplug/cpci_hotplug_core.c	2007-04-22 12:54:17.000000000 +0200
> +++ linux-2.6/drivers/pci/hotplug/cpci_hotplug_core.c	2007-04-22 13:01:42.000000000 +0200
> @@ -35,6 +35,7 @@
>  #include <linux/smp_lock.h>
>  #include <asm/atomic.h>
>  #include <linux/delay.h>
> +#include <linux/kthread.h>
>  #include "cpci_hotplug.h"
>  
>  #define DRIVER_AUTHOR	"Scott Murray <scottm@xxxxxxxxxxxxxxxx>"
> @@ -59,9 +60,8 @@ static int slots;
>  static atomic_t extracting;
>  int cpci_debug;
>  static struct cpci_hp_controller *controller;
> -static struct semaphore event_semaphore;	/* mutex for process loop (up if something to process) */
> -static struct semaphore thread_exit;		/* guard ensure thread has exited before calling it quits */
> -static int thread_finished = 1;
> +static struct task_struct *cpci_thread;
> +static int thread_finished;
>  
>  static int enable_slot(struct hotplug_slot *slot);
>  static int disable_slot(struct hotplug_slot *slot);
> @@ -357,9 +357,7 @@ cpci_hp_intr(int irq, void *data)
>  	controller->ops->disable_irq();
>  
>  	/* Trigger processing by the event thread */
> -	dbg("Signal event_semaphore");
> -	up(&event_semaphore);
> -	dbg("exited cpci_hp_intr");
> +	wake_up_process(cpci_thread);
>  	return IRQ_HANDLED;
>  }
>  
> @@ -521,17 +519,12 @@ event_thread(void *data)
>  {
>  	int rc;
>  
> -	lock_kernel();
> -	daemonize("cpci_hp_eventd");
> -	unlock_kernel();
> -
>  	dbg("%s - event thread started", __FUNCTION__);
>  	while (1) {
>  		dbg("event thread sleeping");
> -		down_interruptible(&event_semaphore);
> -		dbg("event thread woken, thread_finished = %d",
> -		    thread_finished);
> -		if (thread_finished || signal_pending(current))
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		schedule();
> +		if (kthread_should_stop())
>  			break;
>  		do {
>  			rc = check_slots();
> @@ -541,18 +534,17 @@ event_thread(void *data)
>  			} else if (rc < 0) {
>  				dbg("%s - error checking slots", __FUNCTION__);
>  				thread_finished = 1;
> -				break;
> +				goto out;
>  			}
> -		} while (atomic_read(&extracting) && !thread_finished);
> -		if (thread_finished)
> +		} while (atomic_read(&extracting) && !kthread_should_stop());
> +		if (kthread_should_stop())
>  			break;
>  
>  		/* Re-enable ENUM# interrupt */
>  		dbg("%s - re-enabling irq", __FUNCTION__);
>  		controller->ops->enable_irq();
>  	}
> -	dbg("%s - event thread signals exit", __FUNCTION__);
> -	up(&thread_exit);
> + out:
>  	return 0;
>  }
>  
> @@ -562,12 +554,8 @@ poll_thread(void *data)
>  {
>  	int rc;
>  
> -	lock_kernel();
> -	daemonize("cpci_hp_polld");
> -	unlock_kernel();
> -
>  	while (1) {
> -		if (thread_finished || signal_pending(current))
> +		if (kthread_should_stop() || signal_pending(current))
>  			break;
>  		if (controller->ops->query_enum()) {
>  			do {
> @@ -578,48 +566,34 @@ poll_thread(void *data)
>  				} else if (rc < 0) {
>  					dbg("%s - error checking slots", __FUNCTION__);
>  					thread_finished = 1;
> -					break;
> +					goto out;
>  				}
> -			} while (atomic_read(&extracting) && !thread_finished);
> +			} while (atomic_read(&extracting) && !kthread_should_stop());
>  		}
>  		msleep(100);
>  	}
> -	dbg("poll thread signals exit");
> -	up(&thread_exit);
> + out:
>  	return 0;
>  }
>  
>  static int
>  cpci_start_thread(void)
>  {
> -	int pid;
> -
> -	/* initialize our semaphores */
> -	init_MUTEX_LOCKED(&event_semaphore);
> -	init_MUTEX_LOCKED(&thread_exit);
> -	thread_finished = 0;
> -
>  	if (controller->irq)
> -		pid = kernel_thread(event_thread, NULL, 0);
> +		cpci_thread = kthread_run(event_thread, NULL, "cpci_hp_eventd");
>  	else
> -		pid = kernel_thread(poll_thread, NULL, 0);
> -	if (pid < 0) {
> +		cpci_thread = kthread_run(poll_thread, NULL, "cpci_hp_polld");
> +	if (IS_ERR(cpci_thread)) {
>  		err("Can't start up our thread");
> -		return -1;
> +		return PTR_ERR(cpci_thread);
>  	}
> -	dbg("Our thread pid = %d", pid);

There still needs to be a:

        thread_finished = 0;

here, so that things work if a board driver is insmod'ed again after a 
rmmod (i.e. cpci_hp_start after a cpci_hp_stop).

>  	return 0;
>  }
>  
>  static void
>  cpci_stop_thread(void)
>  {
> -	thread_finished = 1;
> -	dbg("thread finish command given");
> -	if (controller->irq)
> -		up(&event_semaphore);
> -	dbg("wait for thread to exit");
> -	down(&thread_exit);
> +	kthread_stop(cpci_thread);

As well, there still needs to be a:

        thread_finished = 1;
                         
here to make the check in cpci_hp_unregister_controller work.

>  }
>  
>  int
 

-- 
Scott Murray
SOMA Networks, Inc.
Toronto, Ontario
e-mail: scottm@xxxxxxxxxxxxxxxx
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/