On Wed, Feb 11, 2015 at 03:39:48PM +0000, Rafael J. Wysocki wrote:
> On Wednesday, February 11, 2015 04:03:17 PM Boris Brezillon wrote:
> > On Wed, 11 Feb 2015 16:17:20 +0100
> > "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx> wrote:
> >
> > > On Wednesday, February 11, 2015 02:43:45 PM Mark Rutland wrote:
> > > > [...]
> > > >
> > > > > > > > > +static irqreturn_t __handle_irq_event_percpu(unsigned int irq, struct irqaction *action)
> > > > > > > > > +{
> > > > > > > > > + /*
> > > > > > > > > + * During suspend we must not call potentially unsafe irq handlers.
> > > > > > > > > + * See suspend_suspendable_actions.
> > > > > > > > > + */
> > > > > > > > > + if (unlikely(action->flags & IRQF_NO_ACTION))
> > > > > > > > > + return IRQ_NONE;
> > > > > > > >
> > > > > > > > Thomas was trying to avoid any new conditional code in the interrupt
> > > > > > > > handling path, that's why I added a suspended_action list in my
> > > > > > > > proposal.
> > > > > > > > Even if your 'unlikely' statement make things better I'm pretty sure it
> > > > > > > > adds some latency.
> > > > > > >
> > > > > > > I can see that we don't want to add more code here to keep things
> > > > > > > clean/pure, but I find it hard to believe that a single bit test and
> > > > > > > branch (for data that should be hot in the cache) are going to add
> > > > > > > measurable latency to a path that does pointer chasing to get to the
> > > > > > > irqaction in the first place. I could be wrong though, and I'm happy to
> > > > > > > benchmark.
> > > > > >
> > > > > > Again, I don't have enough experience to say this is (or isn't)
> > > > > > impacting irq handling latency, I'm just reporting what Thomas told me.
> > > > > >
> > > > > > >
> > > > > > > It would be possible to go for your list shuffling approach here while
> > > > > > > still keeping the flag internal and all the logic hidden away in
> > > > > > > kernel/irq/pm.c. I wasn't sure how actions could be manipulated during
> > > > > > > suspend, which made me wary of moving them to a separate list.
> > > > > >
> > > > > > Moving them to a temporary list on suspend and restoring them on
> > > > > > resume should not be a problem.
> > > > > > The only drawback I see is that actions might be reordered after the
> > > > > > first resume (anyway, relying on shared irq action order is dangerous
> > > > > > IMHO).
> > > > >
> > > > > We considered doing that too and saw some drawbacks (in addition to the
> > > > > reordering of actions you've mentioned). It added just too much complexity
> > > > > to the IRQ suspend-resume code.
> > > > >
> > > > > I, personally, would be fine with adding an IRQ flag to silence the
> > > > > warning mentioned by Alexandre. Something like IRQD_TIMER_SHARED that would
> > > > > be set automatically if someone requested IRQF_TIMER | IRQF_SHARED.
> > > > >
> > > > > Thoughts?
> > > >
> > > > Even if the timer driver does that, we still require the other handlers
> > > > sharing the line to do the right thing across suspend, no? So either
> > > > their actions need to be masked at suspend time, or the handlers need to
> > > > detect when they're called during suspend and return early.
> > >
> > > Well, the issue at hand is about things that share an IRQ with a timer AFAICS.
> > >
> > > That is odd enough already and I'd say everyone in that situation needs to
> > > be prepared to take the pain (including having to check if the device is not
> > > suspended in their interrupt handlers).
> > >
> > > And quite frankly they need to do that already, because we've never suspended
> > > timer IRQs.
> > >
> > > > So for the flag at request time approach to work, all the drivers using
> > > > the interrupt would have to flag they're safe in that context.
> > >
> > > Something like IRQF_"I can share the line with a timer" I guess? That wouldn't
> > > hurt and can be checked at request time even.
> > >
> > > > I'm not averse to having that (only a few drivers shuold be affected and
> > > > we can sanity check them now).
> > >
> > > Right.
> >
> > Okay, if everyone agrees on this solution, then I'm fine with that too
> > (even if I'm a bit disappointed to have spent so much time on this
> > problem to eventually end-up with a simple IRQF_SHARED_TIMER flag :-().
>
> IRQD_SHARED_TIMER (that needs to be an IRQ flag, not an irqaction one).
Surely for the drivers to be able to announce that they can handle
suspend safely this has to be an irqaction flag?
For IRQD_SHARED_TIMER to work the irqchip would need to set this, and it
doesn't know anything about the drivers (or potentially what any of the
interrupts are if the block is reused).
> And spending time on things that never go in happens a lot in the core
> development. I've sent several versions of the wakeup interrupts handling
> rework and then Thomas wrote the final one himself. :-)
>
> The goal is to find a way that will address everyone's needs and that's
> not always starightforward.
Indeed!
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
