Hi Thomas,
On Tue, 16 Dec 2014 11:03:55 +0100 (CET)
Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> 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.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
