Re: [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type fails

[Jon Hunter]

On 11/04/16 16:39, Marc Zyngier wrote:
> On 11/04/16 16:31, Jon Hunter wrote:
>> On 09/04/16 11:58, Marc Zyngier wrote:

[snip]

>>> I think we need to phase things. Let's start with warning people for a
>>> few kernel releases. Actively maintained platforms will quickly address
>>> the issue (fixing their DT). As I see it, this issue seems rather
>>> widespread (even kvmtool outputs a DT with the wrong triggering
>>> information).
>>>
>>> Once we've fixed the bulk of the platforms and virtual environments, we
>>> can start thinking about making it fail harder.
>>
>> Ok, so are you OK with this patch as-is? If so, can I add your ACK?
> 
> It depends where you plan to handle the error. Ideally, I'd keep on
> returning the error (because that's the right thing to do), and move the
> WARN_ON() into the core code. We'd keep on ignoring the error as we're
> doing today, but we'd scream about it.
> 
> After a couple of releases, we'd turn the WARN_ON into a hard fail.
> 
> Thoughts?

I agree that would be best/ideal, but looking at it, I don't believe it
is possible and this is why I have not done that so far.

If we were to add the WARN to the core code, then we would need to add a
warning everywhere __irq_set_trigger() is called. One of the places it
is called today is from __setup_irq() and today this does the right
thing and handle any error returned. The problem is that in
irq_create_fwspec_mapping() we have never checked the return code from
irq_set_irq_type() (which calls __irq_set_trigger()) or attempted to
handle any errors. So the problem is that depending on the path through
which the type is programmed, errors may or may not be detected. This is
the actual headache :-(

Given that this problem so far only pertains to GIC PPI interrupts and
that it is a not a catastrophic error (interrupts still work fine), I
was thinking we add the warning to the GIC driver.

May be a less severe change would be to only return an error if
configuring an SPI fails and if it is a PPI then simply WARN and
carry-on as we assume we cannot change it.

I hope this summarises the issue a bit further.

Cheers
Jon