Re: [PATCH RESEND v1 1/2] ACPI/PCI: Make _SRS optional for link device

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Jul 06, 2022 at 11:52:56AM +0200, Pierre Gondois wrote:
> On 7/5/22 19:29, Bjorn Helgaas wrote:
> > On Fri, Jul 01, 2022 at 06:16:23PM +0200, Pierre Gondois wrote:
> > > From: Pierre Gondois <Pierre.Gondois@arm.com>
> > > 
> > > In ACPI 6.4, s6.2.13 "_PRT (PCI Routing Table)", PCI legacy
> > > interrupts can be described though a link device (first model).
> > >  From s6.2.16 "_SRS (Set Resource Settings)":
> > > "This optional control method [...]"
> > > 
> > > Make it optional to have a _SRS method for link devices.
> > 
> > I think it would be helpful to outline the reason for wanting these
> > changes in the commit log.  Otherwise we don't know the benefit and
> > it's harder to justify making the change since it's not an obvious
> > cleanup.
> > 
> > IIRC from [1] there *is* a good reason: you need to use Interrupt Link
> > devices so you can specify "level triggered, active high".
> > 
> > Without an Interrupt Link, you would get the default "level triggered,
> > active low" setting, which apparently isn't compatible with GICv2.
> > 
> > I assume this fixes a device that previously didn't work correctly,
> > but I don't see the details of that in the bugzilla.  I'm a little
> > confused about this.  Isn't GICv2 widely used already?  How are things
> > working now?  Or are there just a lot of broken devices?
> 
> It was unsure which of the 2 models described in ACPI 6.4, s6.2.13
> "_PRT (PCI Routing Table)" would be used for UEFI for kvmtool.
> 
> Remainder:
> The first model allows to accurately describe interrupts: level/edge
> triggered and active high/low. Interrupts are also configurable with
> _CRS/_PRS/_SRS/_DIS methods
> The second model allows to describe hardwired interrupts, and are
> by default level triggered, active low.
> 
> The kernel is aware that GivV2 interrupts are active high, so there
> was actually no need to accurately describe them. Thus the second
> model was used.
> While experimenting, we temporarily had a configuration using
> the first model, and only had a _CRS method (no _PRS/_SRS), which
> triggered some warnings.

OK, thanks.  So it sounds like there is some existing kernel code that
special-cases GICv2 interrupts to make them level/high, and that code
would not have been necessary if _PRS/_SRS had been optional from the
beginning.

I don't think we could ever *remove* that code because there's
firmware in the field that relies on it, and that firmware will never
be updated.

> So these patches are not fixes for existing platforms, but merely
> to make _PRS/_SRS methods optional.
> 
> In [1] I said I would submit patches to change that. If you think
> this is not necessary as the configuration is non-existing, I am
> perfectly fine to drop the patches.
> 
> Also as Rafael noted, the _DIS method should also be taken into
> consideration if _PRS/_SRS are made optional.

But that said, I'm not opposed to making _PRS/_SRS optional if that
makes legal and reasonable _PRT descriptions work, and if all the
considerations Rafael mentioned are taken care of.

Bjorn