Re: [PATCH] pcmcia event thread. (fwd)

From: Jeff Garzik (jgarzik@mandrakesoft.com)
Date: Mon Nov 13 2000 - 08:48:40 EST

David Woodhouse wrote:
> I'm not sure why we changed from the existing state machine / timer setup to
> sleeping in the PCMCIA parse_events() routine,

I don't remember the exact reason, but Linus changed it for yenta...
maybe because CardBus Watcher always called the state machine from user
context...

> +static int pcmcia_event_thread(void * dummy)
> +{
> + DECLARE_WAITQUEUE(wait, current);
> +
> + current->session = 1;
> + current->pgrp = 1;
> + strcpy(current->comm, "kpcmciad");
> + current->tty = NULL;
> + spin_lock_irq(&current->sigmask_lock);
> + sigfillset(&current->blocked);
> + recalc_sigpending(current);
> + spin_unlock_irq(&current->sigmask_lock);
> + exit_mm(current);
> + exit_files(current);
> + exit_sighand(current);
> + exit_fs(current);

Replace most if not all of this crap w/ daemonize()

> +
> + while(!event_thread_leaving) {
> + void *active;
> +
> + set_current_state(TASK_INTERRUPTIBLE);
> + add_wait_queue(&event_thread_wq, &wait);
> +
> + /* Don't really need locking. But the implied mb() */
> + spin_lock(&tqueue_lock);
> + active = tq_pcmcia;
> + spin_unlock(&tqueue_lock);
> +
> + if (!active)
> + schedule();
> +
> + set_current_state(TASK_RUNNING);
> + remove_wait_queue(&event_thread_wq, &wait);
> +
> + run_task_queue(&tq_pcmcia);
> + }

it looks like the loop can be simplified to

while (1) {
        mb();
        active = tq_pcmcia;
        if (!active)
                interruptible_sleep_on(&event_thread_wq);
        if (signal_pending(current)
                break;
        run_task_queue(&tq_pcmcia);
}

> + /* Need up_and_exit() */
> + up(&event_thread_exit_sem);

Racy. Use waitpid() in the thread killer instead. You also need to
reap the process, just like in userland...

> + /* Start the thread for handling queued events for socket drivers */
> + kernel_thread (pcmcia_event_thread, NULL,
> + CLONE_FS | CLONE_FILES | CLONE_SIGHAND);

Why are you cloning _FS, _FILES, and _SIGHAND? I don't see why the
third arg should not be zero. man clone... 

-- 
Jeff Garzik             |
Building 1024           | The chief enemy of creativity is "good" sense
MandrakeSoft            |          -- Picasso
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

This archive was generated by hypermail 2b29 : Wed Nov 15 2000 - 21:00:23 EST