lkml.org 
[lkml]   [2012]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [tip:irq/core] genirq: Flush the irq thread on synchronization
    On Wed, Mar 14, 2012 at 04:07:30AM -0700, tip-bot for Ido Yariv wrote:

    Ido, Thomas,

    Sorry for being late into the discussion.
    Have couple of concerns regarding this hunk..

    > @@ -783,39 +789,30 @@ static int irq_thread(void *data)
    > current->irq_thread = 1;
    >
    > while (!irq_wait_for_interrupt(action)) {
    > + irqreturn_t action_ret;
    >
    > irq_thread_check_affinity(desc, action);
    >
    > - atomic_inc(&desc->threads_active);
    > + action_ret = handler_fn(desc, action);
    > + if (!noirqdebug)
    > + note_interrupt(action->irq, desc, action_ret);
    >
    > - raw_spin_lock_irq(&desc->lock);
    > - if (unlikely(irqd_irq_disabled(&desc->irq_data))) {
    > - /*
    > - * CHECKME: We might need a dedicated
    > - * IRQ_THREAD_PENDING flag here, which
    > - * retriggers the thread in check_irq_resend()
    > - * but AFAICT IRQS_PENDING should be fine as it
    > - * retriggers the interrupt itself --- tglx
    > - */
    > - desc->istate |= IRQS_PENDING;
    > - raw_spin_unlock_irq(&desc->lock);
    > - } else {
    > - irqreturn_t action_ret;
    > -
    > - raw_spin_unlock_irq(&desc->lock);
    > - action_ret = handler_fn(desc, action);
    > - if (!noirqdebug)
    > - note_interrupt(action->irq, desc, action_ret);
    > - }
    > -
    > - wake = atomic_dec_and_test(&desc->threads_active);
    > -
    > - if (wake && waitqueue_active(&desc->wait_for_threads))
    > - wake_up(&desc->wait_for_threads);
    > + wake_threads_waitq(desc);
    > }
    >
    > - /* Prevent a stale desc->threads_oneshot */
    > - irq_finalize_oneshot(desc, action, true);
    > + /*
    > + * This is the regular exit path. __free_irq() is stopping the
    > + * thread via kthread_stop() after calling
    > + * synchronize_irq(). So neither IRQTF_RUNTHREAD nor the
    > + * oneshot mask bit should be set.
    > + *
    > + * Verify that this is true.
    > + */
    > + if (WARN_ON(test_and_clear_bit(IRQTF_RUNTHREAD, &action->thread_flags)))
    > + wake_threads_waitq(desc);

    If we hit this warning we do not know if this IRQTF_RUNTHREAD bit's count in
    desc->threads_active was decremented or not.

    Nevertheless, wake_threads_waitq() gets called and desc->threads_active gets
    decremented. As result, if desc->threads_active initially was decremented, we
    might wrongly wake up the queue while some threaded handler is still running.

    By contrast, if we choose not to wake up here, we might stuck in
    synchronize_irq(). Which is probably better than a fooling synchronize_irq().

    > +
    > + if (WARN_ON(desc->threads_oneshot & action->thread_mask))
    > + irq_finalize_oneshot(desc, action, true);

    This check is called when the action is already removed in __free_irq() and no
    desc->lock is held. Hence, a concurrent __setup_irq() could reallocate the very
    same bit in the meantime. So neither irq_finalize_oneshot() nor the warning
    are legitimate here.

    >
    > /*
    > * Clear irq_thread. Otherwise exit_irq_thread() would make

    --
    Regards,
    Alexander Gordeev
    agordeev@redhat.com


    \
     
     \ /
      Last update: 2012-03-15 20:09    [W:0.063 / U:0.284 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site