Re: [PATCH 0/5] ARM: at91: fix irq_pm_install_action WARNING

[Thomas Gleixner]

On Tue, 16 Dec 2014, Boris Brezillon wrote:
> On Mon, 15 Dec 2014 23:48:14 +0100
> "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx> wrote:
> > 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?

> 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.

Your device drivers just work fine, as long as they disable the
interrupt at the device level on suspend, but they have to do that in
any case. Aside of that detail they are completely oblivous whether
they are connected to this "demux" chip or to a seperate irq line on
other devices.

> BTW, have you heard about other platforms/drivers impacted by this
> WARN_ON addition, and if so how did they solve the problem ?

Nothing so far. Seems not many hardware folks are that crazy.

But I have no objections to make this generic infrastructure as long
as it can be compiled out.

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/