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

[Pierre Gondois]

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@xxxxxxx>
> >
> > 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.

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.

Regards,
Pierre


> [1] https://lore.kernel.org/r/e2ae06ba-de8f-2cae-60fa-fe9a215d779b@xxxxxxx
>
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215560
> > Signed-off-by: Pierre Gondois <pierre.gondois@xxxxxxx>
> > ---
> >   drivers/acpi/pci_link.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> > index 58647051c948..129e3e7e80ee 100644
> > --- a/drivers/acpi/pci_link.c
> > +++ b/drivers/acpi/pci_link.c
> > @@ -288,6 +288,13 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
> >   	if (!irq)
> >   		return -EINVAL;
> >   
> > +	if (!acpi_has_method(handle, METHOD_NAME__SRS)) {
> > +		if (link->irq.active == irq)
> > +			return 0;
> > +		acpi_handle_err(handle, "Unable to set IRQ %d: No _SRS.\n", irq);
> > +		return -ENODEV;
> > +	}
> > +
> >   	resource = kzalloc(sizeof(*resource) + 1, irqs_disabled() ? GFP_ATOMIC: GFP_KERNEL);
> >   	if (!resource)
> >   		return -ENOMEM;
> > --
> > 2.25.1