Re: [PATCH 2/2] ACPI / PCI: Account for ARI in _PRT lookups

[Don Dutile]

On 05/26/2015 01:54 PM, Alex Williamson wrote:
> The PCIe specification, rev 3.0, section 2.2.8.1, contains the
> following implementation note:
>
>      Virtual Wire Mapping for INTx Interrupts From ARI Devices
>
>      The implied Device Number for an ARI Device is 0. When ARI-aware
>      software (including BIOS and operating system) enables ARI
>      Forwarding in the Downstream Port immediately above an ARI Device
>      in order to access its Extended Functions, software must
>      comprehend that the Downstream Port will use Device Number 0 for
>      the virtual wire mappings of INTx interrupts coming from all
>      Functions of the ARI Device. If non-ARI-aware software attempts
>      to determine the virtual wire mappings for Extended Functions, it
>      can come up with incorrect mappings by examining the traditional
>      Device Number field and finding it to be non-0.
>
> We account for this in pci_swizzle_interrupt_pin(), but it looks like
> we miss it here, looking for a _PRT entry with a slot matching the
> ARI device slot number.  This can cause errors like:
>
> pcieport 0000:80:03.0: can't derive routing for PCI INT B
> sfc 0000:82:01.1: PCI INT B: no GSI
>
> pci_dev.irq is then invalid, resulting in errors for drivers that
> attempt to enable INTx on the device.  Fix by using slot 0 for ARI
> enabled devices.
>
> Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> ---
>   drivers/acpi/pci_irq.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
> index b1def41..65e83cd 100644
> --- a/drivers/acpi/pci_irq.c
> +++ b/drivers/acpi/pci_irq.c
> @@ -163,7 +163,7 @@ static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev,
>   {
>   	int segment = pci_domain_nr(dev->bus);
>   	int bus = dev->bus->number;
> -	int device = PCI_SLOT(dev->devfn);
> +	int device = pci_ari_enabled(dev->bus) ? 0 : PCI_SLOT(dev->devfn);
>   	struct acpi_prt_entry *entry;
>
>   	if (((prt->address >> 16) & 0xffff) != device ||
> @@ -181,7 +181,7 @@ static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev,
>   	 */
>   	entry->id.segment = segment;
>   	entry->id.bus = bus;
> -	entry->id.device = (prt->address >> 16) & 0xFFFF;
> +	entry->id.device = PCI_SLOT(dev->devfn);
I would expect that this should be = device, not PCI_SLOT(dev->devfn),
esp if used by ACPI core, since it'll be expecting a swizzle from device 0,
per above spec.

Additionally, if you look at the beginning of this function, this check is performed:
       if (((prt->address >> 16) & 0xffff) != device ||
            prt->pin + 1 != pin)
                return -ENODEV;

So, that implies you leave this assignment as is,
or set it to device -- six of one, half-dozen another.


>   	entry->pin = prt->pin + 1;
>
>   	do_prt_fixups(entry, prt);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/