On Thu, 25 Feb 2010 07:18:32 +0800 Andrew Morton <a...@linux-foundation.org> wrote:
> > ... > > > > +static int max3110_main_thread(void *_max) > > +{ > > + struct uart_max3110 *max = _max; > > + wait_queue_head_t *wq = &max->wq; > > + int ret = 0; > > + struct circ_buf *xmit = &max->con_xmit; > > + > > + init_waitqueue_head(wq); > > + pr_info(PR_FMT "start main thread\n"); > > + > > + do { > > + wait_event_interruptible(*wq, > > + max->flags || > > kthread_should_stop()); > > + set_bit(0, &max->mthread_up); > > + > > + if (use_irq && test_bit(M3110_IRQ_PENDING, > > &max->flags)) { > > + max3110_con_receive(max); > > + clear_bit(M3110_IRQ_PENDING, &max->flags); > > + } > > + > > + /* First handle console output */ > > + if (test_bit(M3110_CON_TX_NEED, &max->flags)) { > > + send_circ_buf(max, xmit); > > + clear_bit(M3110_CON_TX_NEED, &max->flags); > > + } > > + > > + /* Handle uart output */ > > + if (test_bit(M3110_UART_TX_NEED, &max->flags)) { > > + transmit_char(max); > > + clear_bit(M3110_UART_TX_NEED, &max->flags); > > + } > > + clear_bit(0, &max->mthread_up); > > + } while (!kthread_should_stop()); > > + > > + return ret; > > +} > > + > > +static irqreturn_t serial_m3110_irq(int irq, void *dev_id) > > +{ > > + struct uart_max3110 *max = dev_id; > > + > > + /* max3110's irq is a falling edge, not level triggered, > > + * so no need to disable the irq */ > > + set_bit(M3110_IRQ_PENDING, &max->flags); > > + > > + if (!test_bit(0, &max->mthread_up)) > > + wake_up_process(max->main_thread); > > + > > + return IRQ_HANDLED; > > +} > > The manipulation of mthread_up here (and in several other places) is > clearly quite racy. If you hit that race, the thread will not wake up > and the driver will sit there not doing anything until some other > event happens. > > This is perhaps fixable with test_and_set_bit() and > test_and_clear_bit() (need to think about that) or, of course, by > adding locking. > > But a simpler fix is just to delete mthread_up altogether. > wake_up_process() on a running process is an OK thing to do and > hopefully isn't terribly slow. Yes, wake_up_process won't harm a running process, our driver's case is a little special, the console's write() func may call wake_up_process() for every character in the buffer, thus we will try to avoid to call it. mthread_up can't be removed as it is also referenced in read_thread. I prefer to use the test_and_set/clear_bit for "mthread_up". Thanks, Feng diff --git a/drivers/serial/max3110.c b/drivers/serial/max3110.c index e8c44fa..d5bd71f 100644 --- a/drivers/serial/max3110.c +++ b/drivers/serial/max3110.c @@ -400,7 +400,7 @@ static int max3110_main_thread(void *_max) do { wait_event_interruptible(*wq, max->flags || kthread_should_stop()); - set_bit(0, &max->mthread_up); + test_and_set_bit(0, &max->mthread_up); if (use_irq && test_bit(M3110_IRQ_PENDING, &max->flags)) { max3110_con_receive(max); @@ -418,7 +418,7 @@ static int max3110_main_thread(void *_max) transmit_char(max); clear_bit(M3110_UART_TX_NEED, &max->flags); } - clear_bit(0, &max->mthread_up); + test_and_clear_bit(0, &max->mthread_up); } while (!kthread_should_stop()); return ret; ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev _______________________________________________ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general