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

[Jeff Garzik <jgarzik@mandrakesoft.com>]

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/