[PATCH RESEND v1 0/2] Make _PRS and _SRS methods optional

[PATCH RESEND v1 0/2] Make _PRS and _SRS methods optional

[Pierre Gondois]

The PCI legacy interrupts can be described with link devices,
cf ACPI 6.4, s6.2.13 "_PRT (PCI Routing Table)".
Link devices can have optional _SRS/_PRS methods to set the interrupt.

In PCI Firmware Specification Revision 3.3, s4.3.2.1. "Resource Setting":
"""
A non-configurable device only specifies _CRS. However, if they are
configurable, devices include _PRS to indicate the possible resource
setting and _SRS to allow OSPM to specify a new resource allocation
for the device.
"""

However, _PRS/_SRS methods are checked in drivers/acpi/pci_link.c,
and the driver aborts if they are absent.
E.g.: When _PRS is missing:
ACPI: \_SB_.PCI0.LNKA: _CRS 36 not found in _PRS
ACPI: \_SB_.PCI0.LNKA: No IRQ available. Try pci=noacpi or acpi=off

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215560

Pierre Gondois (2):
  ACPI/PCI: Make _SRS optional for link device
  ACPI/PCI: Make _PRS optional for link device

 drivers/acpi/pci_link.c | 46 ++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 14 deletions(-)

-- 
2.25.1

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

[Pierre Gondois]

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.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215560
Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---
 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

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

[Bjorn Helgaas]

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?

[1] https://lore.kernel.org/r/e2ae06ba-de8f-2cae-60fa-fe9a215d779b@arm.com

> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215560
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>  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
> 

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

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@arm.com
> 
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215560
>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
>> ---
>>   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
>>

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

[Bjorn Helgaas]

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

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

[Rafael J. Wysocki]

On Fri, Jul 1, 2022 at 6:17 PM Pierre Gondois <pierre.gondois@arm.com> 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.

Note that if _DIS is present, _SRS is necessary to enable the link and
acpi_pci_link_add() evaluates _DIS for all links.  So you need to
check both, not just one.

Moreover, it doesn't make much sense to provide _PRS without _SRS and
arguably _PRS is needed if _SRS is present, so this needs to be taken
into account too.

AFAICS, the only valid configuration in which _SRS and _PRS are not
present is when _DIS is not present too, so only _CRS is present and
the IRQ listed by it is actually in use.  However, in that case it is
hardly necessary to add a device object for the PCI link device at
all.

> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215560
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>  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
>

[PATCH RESEND v1 2/2] ACPI/PCI: Make _PRS optional for link device

[Pierre Gondois]

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.12 "_PRS (Possible Resource Settings)":
"This optional object evaluates [...]"

It is currently checked that the interrupt advertised in _CRS
is one of the interrupts available in _PRS.
Make this check conditional to the presence of _PRS.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=215560
Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
---
 drivers/acpi/pci_link.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index 129e3e7e80ee..b5a41866f135 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -532,19 +532,10 @@ int __init acpi_irq_penalty_init(void)
 
 static int acpi_irq_balance = -1;	/* 0: static, 1: balance */
 
-static int acpi_pci_link_allocate(struct acpi_pci_link *link)
+static int select_from_possible(struct acpi_pci_link *link)
 {
-	acpi_handle handle = link->device->handle;
-	int irq;
 	int i;
 
-	if (link->irq.initialized) {
-		if (link->refcnt == 0)
-			/* This means the link is disabled but initialized */
-			acpi_pci_link_set(link, link->irq.active);
-		return 0;
-	}
-
 	/*
 	 * search for active IRQ in list of possible IRQs.
 	 */
@@ -557,8 +548,9 @@ static int acpi_pci_link_allocate(struct acpi_pci_link *link)
 	 */
 	if (i == link->irq.possible_count) {
 		if (acpi_strict)
-			acpi_handle_warn(handle, "_CRS %d not found in _PRS\n",
-					 link->irq.active);
+			acpi_handle_warn(link->device->handle,
+					"_CRS %d not found in _PRS\n",
+					link->irq.active);
 		link->irq.active = 0;
 	}
 
@@ -566,9 +558,28 @@ static int acpi_pci_link_allocate(struct acpi_pci_link *link)
 	 * if active found, use it; else pick entry from end of possible list.
 	 */
 	if (link->irq.active)
-		irq = link->irq.active;
+		return link->irq.active;
+	else
+		return link->irq.possible[link->irq.possible_count - 1];
+}
+
+static int acpi_pci_link_allocate(struct acpi_pci_link *link)
+{
+	acpi_handle handle = link->device->handle;
+	int irq;
+	int i;
+
+	if (link->irq.initialized) {
+		if (link->refcnt == 0)
+			/* This means the link is disabled but initialized */
+			acpi_pci_link_set(link, link->irq.active);
+		return 0;
+	}
+
+	if (link->irq.possible_count)
+		irq = select_from_possible(link);
 	else
-		irq = link->irq.possible[link->irq.possible_count - 1];
+		irq = link->irq.active;
 
 	if (acpi_irq_balance || !link->irq.active) {
 		/*
-- 
2.25.1