Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1374597 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type  fails Marc Zyngier <marc.zyngier@arm.com> - 2016-04-09 13:00 +0200
    Re: [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type  fails Marc Zyngier <marc.zyngier@arm.com> - 2016-04-11 17:40 +0200
      Re: [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type  fails Jon Hunter <jonathanh@nvidia.com> - 2016-04-12 11:00 +0200
        Re: [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type  fails Marc Zyngier <marc.zyngier@arm.com> - 2016-04-12 12:20 +0200
    Re: [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type  fails Jon Hunter <jonathanh@nvidia.com> - 2016-04-11 17:40 +0200

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

On Thu, 17 Mar 2016 15:04:01 +0000
Jon Hunter <jonathanh@nvidia.com> wrote:

> 
> On 17/03/16 14:51, Thomas Gleixner wrote:
> > On Thu, 17 Mar 2016, Jon Hunter wrote:
> > 
> >> Setting the interrupt type for private peripheral interrupts (PPIs) may
> >> not be supported by a given GIC because it is IMPLEMENTATION DEFINED
> >> whether this is allowed. There is no way to know if setting the type is
> >> supported for a given GIC and so the value written is read back to
> >> verify it matches the desired configuration. If it does not match then
> >> an error is return.
> >>
> >> There are cases where the interrupt configuration read from firmware
> >> (such as a device-tree blob), has been incorrect and hence
> >> gic_configure_irq() has returned an error. This error has gone
> >> undetected because the error code returned was ignored but the interrupt
> >> still worked fine because the configuration for the interrupt could not
> >> be overwritten.
> >>
> >> Given that this has done undetected and we should only fail to set the
> >> type for PPIs whose configuration cannot be changed anyway, don't return
> >> an error and simply WARN if this fails. This will allows us to fix up any
> >> places in the kernel where we should be checking the return status and
> >> maintain back compatibility with firmware images that may have incorrect
> >> interrupt configurations.
> > 
> > Though silently returning 0 is really the wrong thing to do. You can add the
> > warn, but why do you want to return success?
> 
> Yes that would be the correct thing to do I agree. However, the problem
> is that if we do this, then after the patch "irqdomain: Don't set type
> when mapping an IRQ" is applied, we may break interrupts for some
> existing device-tree binaries that have bad configuration (such as omap4
> and tegra20/30 ... see patches 1 and 2) that have gone unnoticed. So it
> is a back compatibility issue.
> 
> If you are wondering why these interrupts break after "irqdomain: Don't
> set type when mapping an IRQ", it is because today
> irq_create_fwspec_mapping() does not check the return code from setting
> the type, but if we defer setting the type until __setup_irq() which
> does check the return code, then all of a sudden interrupts that were
> working (even with bad configurations) start to fail.
> 
> The reason why I opted not to return an error code from
> gic_configure_irq() is it really can't fail. The failure being reported
> does not prevent the interrupt from working, but tells you your
> configuration does not match the hardware setting which you cannot
> overwrite.
> 
> So to maintain back compatibility and avoid any silent errors, I opted
> to make it a WARN and not return an error.
> 
> If people are ok with potentially breaking interrupts for device-tree
> binaries with bad settings, then I am ok to return an error here.

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.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

[toc] | [next] | [standalone]

#1376086

On 11/04/16 16:31, Jon Hunter wrote:
> Hi Mark,
> 
> On 09/04/16 11:58, Marc Zyngier wrote:
>> On Thu, 17 Mar 2016 15:04:01 +0000
>> Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>>>
>>> On 17/03/16 14:51, Thomas Gleixner wrote:
>>>> On Thu, 17 Mar 2016, Jon Hunter wrote:
>>>>
>>>>> Setting the interrupt type for private peripheral interrupts (PPIs) may
>>>>> not be supported by a given GIC because it is IMPLEMENTATION DEFINED
>>>>> whether this is allowed. There is no way to know if setting the type is
>>>>> supported for a given GIC and so the value written is read back to
>>>>> verify it matches the desired configuration. If it does not match then
>>>>> an error is return.
>>>>>
>>>>> There are cases where the interrupt configuration read from firmware
>>>>> (such as a device-tree blob), has been incorrect and hence
>>>>> gic_configure_irq() has returned an error. This error has gone
>>>>> undetected because the error code returned was ignored but the interrupt
>>>>> still worked fine because the configuration for the interrupt could not
>>>>> be overwritten.
>>>>>
>>>>> Given that this has done undetected and we should only fail to set the
>>>>> type for PPIs whose configuration cannot be changed anyway, don't return
>>>>> an error and simply WARN if this fails. This will allows us to fix up any
>>>>> places in the kernel where we should be checking the return status and
>>>>> maintain back compatibility with firmware images that may have incorrect
>>>>> interrupt configurations.
>>>>
>>>> Though silently returning 0 is really the wrong thing to do. You can add the
>>>> warn, but why do you want to return success?
>>>
>>> Yes that would be the correct thing to do I agree. However, the problem
>>> is that if we do this, then after the patch "irqdomain: Don't set type
>>> when mapping an IRQ" is applied, we may break interrupts for some
>>> existing device-tree binaries that have bad configuration (such as omap4
>>> and tegra20/30 ... see patches 1 and 2) that have gone unnoticed. So it
>>> is a back compatibility issue.
>>>
>>> If you are wondering why these interrupts break after "irqdomain: Don't
>>> set type when mapping an IRQ", it is because today
>>> irq_create_fwspec_mapping() does not check the return code from setting
>>> the type, but if we defer setting the type until __setup_irq() which
>>> does check the return code, then all of a sudden interrupts that were
>>> working (even with bad configurations) start to fail.
>>>
>>> The reason why I opted not to return an error code from
>>> gic_configure_irq() is it really can't fail. The failure being reported
>>> does not prevent the interrupt from working, but tells you your
>>> configuration does not match the hardware setting which you cannot
>>> overwrite.
>>>
>>> So to maintain back compatibility and avoid any silent errors, I opted
>>> to make it a WARN and not return an error.
>>>
>>> If people are ok with potentially breaking interrupts for device-tree
>>> binaries with bad settings, then I am ok to return an error here.
>>
>> 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?

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1376569

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

[toc] | [prev] | [next] | [standalone]

#1376633

On 12/04/16 09:50, Jon Hunter wrote:
> 
> 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'd take that. Limiting it to PPIs would be a minimal change, and the
warning would hopefully make people realize their DT is wrong. Failing
to program an SPI is really not expected, and should definitely explode.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1376100

Hi Mark,

On 09/04/16 11:58, Marc Zyngier wrote:
> On Thu, 17 Mar 2016 15:04:01 +0000
> Jon Hunter <jonathanh@nvidia.com> wrote:
> 
>>
>> On 17/03/16 14:51, Thomas Gleixner wrote:
>>> On Thu, 17 Mar 2016, Jon Hunter wrote:
>>>
>>>> Setting the interrupt type for private peripheral interrupts (PPIs) may
>>>> not be supported by a given GIC because it is IMPLEMENTATION DEFINED
>>>> whether this is allowed. There is no way to know if setting the type is
>>>> supported for a given GIC and so the value written is read back to
>>>> verify it matches the desired configuration. If it does not match then
>>>> an error is return.
>>>>
>>>> There are cases where the interrupt configuration read from firmware
>>>> (such as a device-tree blob), has been incorrect and hence
>>>> gic_configure_irq() has returned an error. This error has gone
>>>> undetected because the error code returned was ignored but the interrupt
>>>> still worked fine because the configuration for the interrupt could not
>>>> be overwritten.
>>>>
>>>> Given that this has done undetected and we should only fail to set the
>>>> type for PPIs whose configuration cannot be changed anyway, don't return
>>>> an error and simply WARN if this fails. This will allows us to fix up any
>>>> places in the kernel where we should be checking the return status and
>>>> maintain back compatibility with firmware images that may have incorrect
>>>> interrupt configurations.
>>>
>>> Though silently returning 0 is really the wrong thing to do. You can add the
>>> warn, but why do you want to return success?
>>
>> Yes that would be the correct thing to do I agree. However, the problem
>> is that if we do this, then after the patch "irqdomain: Don't set type
>> when mapping an IRQ" is applied, we may break interrupts for some
>> existing device-tree binaries that have bad configuration (such as omap4
>> and tegra20/30 ... see patches 1 and 2) that have gone unnoticed. So it
>> is a back compatibility issue.
>>
>> If you are wondering why these interrupts break after "irqdomain: Don't
>> set type when mapping an IRQ", it is because today
>> irq_create_fwspec_mapping() does not check the return code from setting
>> the type, but if we defer setting the type until __setup_irq() which
>> does check the return code, then all of a sudden interrupts that were
>> working (even with bad configurations) start to fail.
>>
>> The reason why I opted not to return an error code from
>> gic_configure_irq() is it really can't fail. The failure being reported
>> does not prevent the interrupt from working, but tells you your
>> configuration does not match the hardware setting which you cannot
>> overwrite.
>>
>> So to maintain back compatibility and avoid any silent errors, I opted
>> to make it a WARN and not return an error.
>>
>> If people are ok with potentially breaking interrupts for device-tree
>> binaries with bad settings, then I am ok to return an error here.
> 
> 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?

Cheers
Jon

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web