On Thu, 25 Feb 2010 07:18:32 +0800
Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> 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;
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
