Hi Thomas,

On Tue, 16 Dec 2014 11:03:55 +0100 (CET)
Thomas Gleixner [off-list ref] wrote:

On Tue, 16 Dec 2014, Boris Brezillon wrote:

quoted

On Mon, 15 Dec 2014 23:48:14 +0100
"Rafael J. Wysocki" [off-list ref] wrote:

quoted

Or even set IRFQ_NO_SUSPEND for all of the users of this interrupt and add
comments to them explaining why it is set.

Actually I thought about adding a new flag (let's call it
IRQF_DONT_COMPLAIN for now ;-)) to remove those warnings (or specifying
IRFQ_NO_SUSPEND in all peripherals sharing the IRQ with the init
timer), but after discussing the problem with Thomas I decided to go
for the approach described in my cover letter.

Thomas, correct me if I'm wrong, but your concern about the
IRQF_DONT_COMPLAIN approach was that it was leaving interrupt handlers
of suspended devices in an active state (meaning that they could be
called in "suspend" or "early resume" state), and such devices might
not properly handle interrupts while being in a suspended state (clocks
and regulators disabled).
In at91 specific case this should not be an issue thought.

We have the same problem when setting IRFQ_NO_SUSPEND on all peripherals
sharing the IRQ with the init timer.
Moreover, I'd like to keep the core automatically disabling the IRQ when
the PMC, RTC, watchdog or DBGU (UART) peripherals have their own
dedicated IRQ (which is the case on Atmel sama5 SoCs).
This implies testing for the SoC version in each of these drivers and
adapting the request_irq call accordingly.

But still all those drivers must disable the interrupts at the device
level on suspend, right?

quoted

Thomas, Rafael, if both of you think I should either introduce a new
flag or specify IRFQ_NO_SUSPEND in all shared IRQ users, then I can go
for one of this solution.

All of this really sucks. What about the following?

Install the timer interrupt as a demultiplexing interrupt.

Create a pseudo interrupt chip, which essentially does nothing, but
keeps track of the disabled state. Install handle_simple_irq as
handler for those "demux" interrupts. Then have:

struct data {
       u32 unmasked;
       u32 demuxavail;
};

static struct data demuxdata;

At init time you know how many of these demux interrupts are
available. So you set the demuxdata up, e.g. for 3 interrupts
connected:

	demuxdata.demuxavail = 0x07;

You install a pointer to demuxdata for all demux interrupts as irq
chip data and the simple handler.

And in the mask/unmask handlers you do:

mask(irqdata) {
       struct data *d = irq_data_get_irq_chip_data(irqdata);

       d->unmasked &= ~irqdata->mask;
       if (!d->unmasked)
       	  mask_demux_irq();
}

unmask(irqdata) {
       struct data *d = irq_data_get_irq_chip_data(irqdata);

       if (!d->unmasked)
       	  unmask_demux_irq();
       d->mask |= irqdata->mask;
}

Now the demuxhandler does:

    mask = demuxdata.demuxavail & demuxdata.unmasked;

    for_each_bit(bit, mask)
    	generic_handle_irq(demuxirq_start + bit);

So the handler wont be invoked for masked bits and handle_simple_irq()
will not call the device handler if the interrupt is marked disabled.

So in the suspend case all "demux" interrupts except those which are
marked NOSUSPEND are marked disabled and the handlers wont be invoked.

Locking and other details omitted.

That avoids the whole flag, action, whatever business for the price of
a really trivial demux mechanism. Everything just works. Even the irq
storm detector will just disable the parent interrupt once all
handlers return NONE often enough.

Still have one question regarding the spurious interrupt detection code.
AFAIU, it disables a specific IRQ handler if it triggers to often with
an IRQ_NONE return, right ?

In this dumb demuxer I can't tell which child interrupt should be
handled (there is no interrupt cause register), thus I call
generic_handle_irq on all unmasked IRQs.
Isn't there a risk to get some of my child interrupts disabled because
they always return IRQ_NONE (which is a normal use case for
IRQF_SHARED: we're only expecting at least one handler to return
IRQ_HANDLED or IRQ_WAKE_THREAD, not all of them) ?


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com