Re: [PATCH v2] genirq: don't sync irq thread if current happen tobe the very irq thread

[Thomas Gleixner]

On Sun, 20 May 2012, Yong Zhang wrote:
> On Sun, May 20, 2012 at 01:27:31PM +0800, Yong Zhang wrote:
> > --- a/kernel/irq/manage.c
> > +++ b/kernel/irq/manage.c
> > @@ -41,6 +41,7 @@ early_param("threadirqs", setup_forced_irqthreads);
> >  void synchronize_irq(unsigned int irq)
> >  {
> >  	struct irq_desc *desc = irq_to_desc(irq);
> > +	struct irqaction *action = desc->action;
> 
> Bad time for dereferencing *action.

You meant dereferencing *desc :)
 
>  	/*
>  	 * We made sure that no hardirq handler is running. Now verify
>  	 * that no threaded handlers are active.
> +	 * But for theaded irq, we don't sync if current happens to be
> +	 * the irq thread; otherwise we could deadlock.
>  	 */
> +	action = desc->action;

And dereferencing action w/o being protected by desc->lock is buggy.

+	while (action) {
> +		if (action->thread && action->thread == current)
> +			return;
> +		action = action->next;
> +	}
> +

Aside of that I really do not like that change. It'll hide real
deadlocks when disable_irq() is called from the interrupt handler.

Also this will not cure all problems of that MMC driver on RT or with
forced threaded interrupts.

Assume that tasklet code runs from the softirq thread so it will
schedule when desc->threads_active > 0. This will trigger a
"scheduling while atomic" warning.

The irq_enable/disable dance in that driver is amazing. I have no time
at the moment to grok the logic behind this, but it bet this can be
done way simpler and less horrible.

Thanks,

	tglx

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/