Date:	Sat, 18 Jul 2009 07:26:05 +0800
From:	Barry Song <21cnbao@...il.com>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	mingo@...hat.com, dahlmann.thomas@...or.de,
	LKML <linux-kernel@...r.kernel.org>,
	uclinux-dist-devel@...ckfin.uclinux.org,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH] Delete redundant IRQ_DISABLED check in irq_thread

Yes. The patch looks great and very intelligible. I think it is
acceptable and valid. But why not the case you said:

> Fact is that the interrupt can be disabled between the thread wake up
> and the handler thread calling the handler function. So it's a
> question of correctness not to call the handler when the irq is
> disabled for the following reason:

CPU 0                   CPU 1
hard irq
    -> thread is woken
                       disable_irq()
                       synchronize_irq() returns because
                       there is no thread running the handler

thread runs             code which assumes that no irq handler
    -> calls handler   can run continues.

> That would happen with your patch applied.

be changed to:
CPU 0                   CPU 1
hard irq
    -> thread is woken
                       disable_irq()

thread runs
    -> calls handler

                       synchronize_irq() returns because both hardirq
and thread is finished
                       run continues.

and

CPU 0                   CPU 1
hard irq
    -> thread is woken

thread runs
    -> calls handler
                        |
                        | disable_irq()
                        |
    thread fn return
                       synchronize_irq() returns because both hardirq
and thread is finished.
                       run continues.

Or synchronize_irq() still returns even thread is not executed or
running, but add an interface like flush_scheduled_work() for
work-queue to wait for the return of thread_fn like:
CPU 0                   CPU 1
hard irq
    -> thread is woken
                       disable_irq()
                       synchronize_irq() returns because
                       there is no thread running the handler

thread runs
    -> calls handler
                       synchronize_threaded_irq()
                       run continues.

I guess people call disable_irq to disable the whole execution,
specially to disable the hardirq(then both top and bottom are
blocked). It's really strange the disable operation only blocks the
bottom-half.

And an interrupt is generally a trigger source to trigger other
executions. And other executions often sync according to the
interrupt, and seldom control the interrupt enable/disable
asynchronously. So an asynchronous and unexpected disable_irq() is
generically called by close/remove/release and so on. Why not let it
wait for the finish of top and bottom?


2009/7/18, Thomas Gleixner <tglx@...utronix.de>:
> On Fri, 17 Jul 2009, Barry Song wrote:
>> 2009/7/17 Thomas Gleixner <tglx@...utronix.de>:
>> > The correct thing to do is disabling it at the device level if you
>> > need to prevent interrupt storms.
>>
>> For some devices which use level trigger and  buses like spi, i2c.
>> Checking the interrupt source and clear IRQ probably need to
>> read/write the registers of devices by i2c,spi. But i2c, spi access
>> maybe causes schedule based on their driver framework. So the checking
>> operation and clear operation can only be done in process switch. So
>> the only way to avoid interrupt storms is disabling the irq line at
>> hardirq, after clearing irq in the bottom-half by work-queue or
>> thread, then enable it again.
>> For example, a spi touchscreen is touched, then it gives a high level
>> in a GPIO irq line to CPU. CPU enter hardirq, but to clear the high
>> level, it must write the touchscreen by spi, how could it avoid the
>> interrupt storms betweem hardirq and bottom-half if it doesn't disable
>> the irq line since it can't write the spi in hardirq?
>
> Sigh, I probably don't need to understand why hardware designers come
> up with such crap. Using a level triggered interrupt for a device
> which cannot be shut up in the hard interrupt routine is simply
> moronic. But yeah, we have to live with that. :(
>
> Removing the disabled check to fix that stupidity and replacing it by
> a complex accounting mechanism is not an option.
>
> The patently untested patch below should solve your problem without
> adding horrible complexity.
>
> I think that's a straight forward solution to the problem and reflects
> the semantics of your use case very clearly while it does not touch
> the disabled logic. You really do not want to disable the interrupt,
> you want to keep it masked until you can unmask it again. That makes a
> huge difference.
>
> All you need to do is to set the irq flow handler of your GPIO pin to
> handle_level_oneshot_irq and the generic code will take care of it.
>
> Thanks,
>
> 	tglx
> ------
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index cb2e77a..5f22436 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -194,6 +194,8 @@ struct irq_desc {
>  #endif
>  	atomic_t		threads_active;
>  	wait_queue_head_t       wait_for_threads;
> +	void			(*thread_eoi)(unsigned int irq,
> +					      struct irq_desc *desc);
>  #ifdef CONFIG_PROC_FS
>  	struct proc_dir_entry	*dir;
>  #endif
> @@ -284,6 +286,7 @@ extern irqreturn_t handle_IRQ_event(unsigned int irq,
> struct irqaction *action);
>   * callable via desc->chip->handle_irq()
>   */
>  extern void handle_level_irq(unsigned int irq, struct irq_desc *desc);
> +extern void handle_level_oneshot_irq(unsigned int irq, struct irq_desc
> *desc);
>  extern void handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_edge_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_simple_irq(unsigned int irq, struct irq_desc *desc);
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 13c68e7..2aa072c 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -341,27 +341,15 @@ out_unlock:
>  	spin_unlock(&desc->lock);
>  }
>
> -/**
> - *	handle_level_irq - Level type irq handler
> - *	@irq:	the interrupt number
> - *	@desc:	the interrupt description structure for this irq
> - *
> - *	Level type interrupts are active as long as the hardware line has
> - *	the active level. This may require to mask the interrupt and unmask
> - *	it after the associated handler has acknowledged the device, so the
> - *	interrupt line is back to inactive.
> - */
> -void
> -handle_level_irq(unsigned int irq, struct irq_desc *desc)
> +static void __handle_level_irq(unsigned int irq, struct irq_desc *desc)
>  {
>  	struct irqaction *action;
>  	irqreturn_t action_ret;
>
> -	spin_lock(&desc->lock);
>  	mask_ack_irq(desc, irq);
>
>  	if (unlikely(desc->status & IRQ_INPROGRESS))
> -		goto out_unlock;
> +		return;
>  	desc->status &= ~(IRQ_REPLAY | IRQ_WAITING);
>  	kstat_incr_irqs_this_cpu(irq, desc);
>
> @@ -371,7 +359,7 @@ handle_level_irq(unsigned int irq, struct irq_desc
> *desc)
>  	 */
>  	action = desc->action;
>  	if (unlikely(!action || (desc->status & IRQ_DISABLED)))
> -		goto out_unlock;
> +		return;
>
>  	desc->status |= IRQ_INPROGRESS;
>  	spin_unlock(&desc->lock);
> @@ -382,14 +370,69 @@ handle_level_irq(unsigned int irq, struct irq_desc
> *desc)
>
>  	spin_lock(&desc->lock);
>  	desc->status &= ~IRQ_INPROGRESS;
> +}
> +
> +/**
> + *	handle_level_irq - Level type irq handler
> + *	@irq:	the interrupt number
> + *	@desc:	the interrupt description structure for this irq
> + *
> + *	Level type interrupts are active as long as the hardware line has
> + *	the active level. This may require to mask the interrupt and unmask
> + *	it after the associated handler has acknowledged the device, so the
> + *	interrupt line is back to inactive.
> + */
> +void
> +handle_level_irq(unsigned int irq, struct irq_desc *desc)
> +{
> +	spin_lock(&desc->lock);
> +	__handle_level_irq(irq, desc);
>  	if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
>  		desc->chip->unmask(irq);
> -out_unlock:
>  	spin_unlock(&desc->lock);
>  }
>  EXPORT_SYMBOL_GPL(handle_level_irq);
>
>  /**
> + *	handle_level_oneshot_irq - Level type oneshot irq handler
> + *	@irq:	the interrupt number
> + *	@desc:	the interrupt description structure for this irq
> + *
> + *	Level type interrupts are active as long as the hardware line has
> + *	the active level. This may require to mask the interrupt and unmask
> + *	it after the associated handler has acknowledged the device, so the
> + *	interrupt line is back to inactive. The oneshot variant keeps the
> + *	interrupt masked on return. This allows to use it with devices
> + *	where the interrupt can not be disabled or acknowledged on the
> + *	device level in the hard interrupt context (device is on i2c, spi..)
> + *	The unmask is done when the threaded interrupt has processed the
> + *	interrupt.
> + */
> +void
> +handle_level_oneshot_irq(unsigned int irq, struct irq_desc *desc)
> +{
> +	spin_lock(&desc->lock);
> +	__handle_level_irq(irq, desc);
> +	desc->status |= IRQ_MASKED;
> +	spin_unlock(&desc->lock);
> +}
> +EXPORT_SYMBOL_GPL(handle_level_oneshot_irq);
> +
> +/*
> + * One shot mode handlers do not unmask the irq line in the hard
> + * interrupt context. Unmask when the handler has finished.
> + */
> +static void unmask_oneshot_irq(unsigned int irq, struct irq_desc *desc)
> +{
> +	spin_lock_irq(&desc->lock);
> +	if (!(desc->status & IRQ_DISABLED) && (desc->status & IRQ_MASKED)) {
> +		desc->status &= ~IRQ_MASKED;
> +		desc->chip->unmask(irq);
> +	}
> +	spin_unlock_irq(&desc->lock);
> +}
> +
> +/**
>   *	handle_fasteoi_irq - irq handler for transparent controllers
>   *	@irq:	the interrupt number
>   *	@desc:	the interrupt description structure for this irq
> @@ -584,6 +627,9 @@ __set_irq_handler(unsigned int irq, irq_flow_handler_t
> handle, int is_chained,
>  	desc->handle_irq = handle;
>  	desc->name = name;
>
> +	if (handle == handle_level_oneshot_irq)
> +		desc->thread_eoi = unmask_oneshot_irq;
> +
>  	if (handle != handle_bad_irq && is_chained) {
>  		desc->status &= ~IRQ_DISABLED;
>  		desc->status |= IRQ_NOREQUEST | IRQ_NOPROBE;
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 50da676..d0115d4 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -475,6 +475,8 @@ static int irq_thread(void *data)
>  			spin_unlock_irq(&desc->lock);
>
>  			action->thread_fn(action->irq, action->dev_id);
> +			if (desc->thread_eoi)
> +				desc->thread_eoi(action->irq, desc);
>  		}
>
>  		wake = atomic_dec_and_test(&desc->threads_active);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists