Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 6 Jul 2022 15:21:31 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Pierre Gondois <pierre.gondois@arm.com>
Cc:     linux-kernel@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <lenb@kernel.org>, linux-pci@vger.kernel.org,
        linux-acpi@vger.kernel.org
Subject: Re: [PATCH RESEND v1 1/2] ACPI/PCI: Make _SRS optional for link
 device
Message-ID: <20220706202131.GA218207@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <35de3948-d8f2-c2da-05f9-995eecf275ce@arm.com>
Precedence: bulk

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