lkml.org 
[lkml]   [2007]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] cpci_hotplug: Convert to use the kthread API
    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@xmission.com> - 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@lst.de>

    Acked-by: Scott Murray <scottm@somanetworks.com>

    > 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@somanetworks.com>"
    > @@ -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@somanetworks.com
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-04-28 00:13    [W:0.033 / U:92.092 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site