linux-kernel - Re: Question about SPIs' interrupt trigger type restrictions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87ee0gn5rq.wl-maz@kernel.org>
Date:   Thu, 26 May 2022 07:54:01 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     richard clark <richard.xnu.clark@...il.com>
Cc:     Robin Murphy <robin.murphy@....com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: Question about SPIs' interrupt trigger type restrictions

On Thu, 26 May 2022 04:44:41 +0100,
richard clark <richard.xnu.clark@...il.com> wrote:
> 
> On Thu, May 26, 2022 at 3:14 AM Robin Murphy <robin.murphy@....com> wrote:
> >
> > On 2022-05-25 11:01, richard clark wrote:
> > > Hi Marc,
> > >
> > > For below code snippet about SPI interrupt trigger type:
> > >
> > > static int gic_set_type(struct irq_data *d, unsigned int type)
> > > {
> > >          ...
> > >          /* SPIs have restrictions on the supported types */
> > >          if ((range == SPI_RANGE || range == ESPI_RANGE) &&
> > >              type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> > >                  return -EINVAL;
> > >          ...
> > > }
> > >
> > > We have a device at hand whose interrupt type is SPI, Falling edge
> > > will trigger the interrupt. But the request_irq(50, handler,
> > > IRQ_TYPE_EDGE_FALLING, ...) will return -EINVAL.
> > >
> > > The question is, why must the SPI interrupt use IRQ_TYPE_EDGE_RISING
> > > instead of IRQ_TYPE_EDGE_FALLING?
> >
> > Because that's what the GIC architecture[1] says. From section 1.2.1
> > "Interrupt Types":
> >
> > "An interrupt that is edge-triggered has the following property:
> >         • It is asserted on detection of a rising edge of an interrupt signal
> 
> This rising edge detection is not true, it's also asserted by
> falling edge, just like the GICD_ICFGR register says: Changing the
> interrupt configuration between level-sensitive and *edge-triggered
> (in either direction)* at a time when there is a pending interrupt
> ...,

Let me finish the sentence for you:

<quote>
... will leave the interrupt in an UNKNOWN pending state.
</quote>

and the direction here is about the configuration bit, not the edge
direction.

> which has been confirmed by GIC-500 on my platform.

From the GIC500 r1p1 TRM, page 2-8:

<quote>
SPIs are generated either by wire inputs or by writes to the AXI4
slave programming interface.  The GIC-500 can support up to 960 SPIs
corresponding to the external spi[991:32] signal. The number of SPIs
available depends on the implemented configuration. The permitted
values are 32-960, in steps of 32. The first SPI has an ID number of
32. You can configure whether each SPI is triggered on a rising edge
or is active-HIGH level-sensitive.
</quote>

So I have no idea what you are talking about, but you definitely have
the wrong end of the stick. Both the architecture and the
implementations are aligned with what the GIC drivers do.

If your system behaves differently, this is because something is
inverting the signal, which is extremely common. Just describe this in
your device tree, or lie to the kernel, whichever way you want.

> 
> > and then, regardless of the state of the signal, remains asserted until
> > the interrupt is acknowledged by software."
> >
> > External signals with the wrong polarity may need external logic to
> 
> IMO, it's not wrong polarity for a device to interrupt the processor
> with a falling edge, it's normal. Actually, the GIC supports
> edge-trigger type:
> '0b10 Corresponding interrupt is edge-triggered', the
> IRQ_TYPE_EDGE_RISING check in gic_set_type(...) is just a sanity check
> from this point of view.

No, this is an architectural requirement, and the driver caters for
the architecture (and only that).

> I would more like to have below changes applied:
> 
> --- a/linux/drivers/irqchip/irq-gic-v3.c
> +++ b/linux/drivers/irqchip/irq-gic-v3.c
> 
> @@ -560,8 +560,7 @@ static int gic_set_type(struct irq_data *d,
> unsigned int type)
>                 return type != IRQ_TYPE_EDGE_RISING ? -EINVAL : 0;
>         /* SPIs have restrictions on the supported types */
> -       if ((range == SPI_RANGE || range == ESPI_RANGE) &&
> -           type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
> +       if ((range == SPI_RANGE || range == ESPI_RANGE) && !(type & 0xf))
>                 return -EINVAL;
> 

Not under my watch.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.