acpi: pci: don't ignore function ID of bridge device in _PRT

Message ID 20170407132222.28300-1-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel April 7, 2017, 1:22 p.m.
We currently derive legacy interrupt routing by matching _PRT
entries on the PCI device only, presumably under the assumption
that PRT entries always have a value of 0xffff in the function
field, meaning 'match all functions'.

This no longer holds for modern PCIe topologies, where the
legacy interrupts for different slots may be wired to different
functions on the same bridge device. For instance, on AMD Seattle,
we may have something like

-[0000:00]-+-00.0  Advanced Micro Devices, Inc. [AMD] Device 1a00
           +-02.0  Advanced Micro Devices, Inc. [AMD] Device 1a01
           +-02.2-[01]----00.0  Renesas uPD720202 USB 3.0 Host Controller
           \-02.3-[02]----00.0  Realtek RTL8169 PCIe Gigabit Ethernet

where the _PRT describes the legacy interrupt routing as

    Name (_PRT, Package ()  // _PRT: PCI Routing Table
    {
        // slot 1: dev 2 fn 1
        Package () { 0x20001, 0x0, 0x0, 0x140 },
        Package () { 0x20001, 0x1, 0x0, 0x141 },
        Package () { 0x20001, 0x2, 0x0, 0x142 },
        Package () { 0x20001, 0x3, 0x0, 0x143 },

        // slot 1: dev 2 fn 2
        Package () { 0x20002, 0x0, 0x0, 0x144 },
        Package () { 0x20002, 0x1, 0x0, 0x145 },
        Package () { 0x20002, 0x2, 0x0, 0x146 },
        Package () { 0x20002, 0x3, 0x0, 0x147 },

        // slot 1: dev 2 fn 3
        Package () { 0x20003, 0x0, 0x0, 0x148 },
        Package () { 0x20003, 0x1, 0x0, 0x149 },
        Package () { 0x20003, 0x2, 0x0, 0x14a },
        Package () { 0x20003, 0x3, 0x0, 0x14b }
    }) // _PRT

The current code always matches on the first entry when trying to
derive the legacy interrupt routing for the USB and Ethernet controllers
behind bridges 02.2 and 02.3, which results in the wrong entry to
be selected.

So fix this by matching the function ID to the value in the _PRT entry
if the _PRT entries are not using 0xffff in the lower word to match
all functions.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 drivers/acpi/pci_irq.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Bjorn Helgaas April 7, 2017, 5:06 p.m. | #1
Hi Ard,

On Fri, Apr 07, 2017 at 02:22:22PM +0100, Ard Biesheuvel wrote:
> We currently derive legacy interrupt routing by matching _PRT

> entries on the PCI device only, presumably under the assumption

> that PRT entries always have a value of 0xffff in the function

> field, meaning 'match all functions'.


The spec (ACPI v6.0, sec 6.2.13) contains a note that:

  The PCI function number in the Address field of the _PRT packages
  must be 0xFFFF, indicating "any" function number or "all functions".

If we need a patch like this, we need to somehow reconcile it with
that spec text to make sure firmware and OS folks have a common
understanding of how this is supposed to work.

> This no longer holds for modern PCIe topologies, where the

> legacy interrupts for different slots may be wired to different

> functions on the same bridge device. For instance, on AMD Seattle,

> we may have something like

> 

> -[0000:00]-+-00.0  Advanced Micro Devices, Inc. [AMD] Device 1a00

>            +-02.0  Advanced Micro Devices, Inc. [AMD] Device 1a01

>            +-02.2-[01]----00.0  Renesas uPD720202 USB 3.0 Host Controller

>            \-02.3-[02]----00.0  Realtek RTL8169 PCIe Gigabit Ethernet

> 

> where the _PRT describes the legacy interrupt routing as

> 

>     Name (_PRT, Package ()  // _PRT: PCI Routing Table

>     {

>         // slot 1: dev 2 fn 1

>         Package () { 0x20001, 0x0, 0x0, 0x140 },

>         Package () { 0x20001, 0x1, 0x0, 0x141 },

>         Package () { 0x20001, 0x2, 0x0, 0x142 },

>         Package () { 0x20001, 0x3, 0x0, 0x143 },

> 

>         // slot 1: dev 2 fn 2

>         Package () { 0x20002, 0x0, 0x0, 0x144 },

>         Package () { 0x20002, 0x1, 0x0, 0x145 },

>         Package () { 0x20002, 0x2, 0x0, 0x146 },

>         Package () { 0x20002, 0x3, 0x0, 0x147 },

> 

>         // slot 1: dev 2 fn 3

>         Package () { 0x20003, 0x0, 0x0, 0x148 },

>         Package () { 0x20003, 0x1, 0x0, 0x149 },

>         Package () { 0x20003, 0x2, 0x0, 0x14a },

>         Package () { 0x20003, 0x3, 0x0, 0x14b }

>     }) // _PRT


But I think this _PRT description is incorrect and we should change
the _PRT rather than the kernel.  My laptop has a basically identical
topology:

  -[0000:00]-+-00.0  Intel Corporation Sky Lake Host Bridge/DRAM Registers
             +-1c.0-[02]----00.0  Realtek Semiconductor Co., Ltd.  Device 525a
             +-1c.2-[04]----00.0  Intel Corporation Wireless 8260

and the ASL looks like this (paraphrased):

  Device (EXP1) {
    Name (_ADR, 0x001C0000)
    Name (_PRT) {
      Package () { 0xFFFF, 0x00, \_SB.LNKA, 0x00 },
      Package () { 0xFFFF, 0x01, \_SB.LNKB, 0x00 },
      ...
    }
  }
  Device (EXP3) {
    Name (_ADR, 0x001C0002)
    Name (_PRT) {
      Package () { 0xFFFF, 0x00, \_SB.LNKC, 0x00 },
      Package () { 0xFFFF, 0x01, \_SB.LNKD, 0x00 },
      ...
    }
  }

I assume the AMD Seattle device is on the motherboard, so I think this
topology could be described via two _PRTs, one for each root port.

> The current code always matches on the first entry when trying to

> derive the legacy interrupt routing for the USB and Ethernet controllers

> behind bridges 02.2 and 02.3, which results in the wrong entry to

> be selected.

> 

> So fix this by matching the function ID to the value in the _PRT entry

> if the _PRT entries are not using 0xffff in the lower word to match

> all functions.

>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  drivers/acpi/pci_irq.c | 2 ++

>  1 file changed, 2 insertions(+)

> 

> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c

> index c576a6fe4ebb..caac4ac94418 100644

> --- a/drivers/acpi/pci_irq.c

> +++ b/drivers/acpi/pci_irq.c

> @@ -160,6 +160,8 @@ static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev,

>  	struct acpi_prt_entry *entry;

>  

>  	if (((prt->address >> 16) & 0xffff) != device ||

> +	    ((prt->address & 0xffff) != 0xffff &&

> +	     (prt->address & 0xffff) != PCI_FUNC(dev->devfn)) ||

>  	    prt->pin + 1 != pin)

>  		return -ENODEV;

>  

> -- 

> 2.9.3

> 

> 

> _______________________________________________

> linux-arm-kernel mailing list

> linux-arm-kernel@lists.infradead.org

> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel April 7, 2017, 5:07 p.m. | #2
On 7 April 2017 at 14:22, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> We currently derive legacy interrupt routing by matching _PRT

> entries on the PCI device only, presumably under the assumption

> that PRT entries always have a value of 0xffff in the function

> field, meaning 'match all functions'.

>

> This no longer holds for modern PCIe topologies, where the

> legacy interrupts for different slots may be wired to different

> functions on the same bridge device. For instance, on AMD Seattle,

> we may have something like

>

> -[0000:00]-+-00.0  Advanced Micro Devices, Inc. [AMD] Device 1a00

>            +-02.0  Advanced Micro Devices, Inc. [AMD] Device 1a01

>            +-02.2-[01]----00.0  Renesas uPD720202 USB 3.0 Host Controller

>            \-02.3-[02]----00.0  Realtek RTL8169 PCIe Gigabit Ethernet

>

> where the _PRT describes the legacy interrupt routing as

>

>     Name (_PRT, Package ()  // _PRT: PCI Routing Table

>     {

>         // slot 1: dev 2 fn 1

>         Package () { 0x20001, 0x0, 0x0, 0x140 },

>         Package () { 0x20001, 0x1, 0x0, 0x141 },

>         Package () { 0x20001, 0x2, 0x0, 0x142 },

>         Package () { 0x20001, 0x3, 0x0, 0x143 },

>

>         // slot 1: dev 2 fn 2

>         Package () { 0x20002, 0x0, 0x0, 0x144 },

>         Package () { 0x20002, 0x1, 0x0, 0x145 },

>         Package () { 0x20002, 0x2, 0x0, 0x146 },

>         Package () { 0x20002, 0x3, 0x0, 0x147 },

>

>         // slot 1: dev 2 fn 3

>         Package () { 0x20003, 0x0, 0x0, 0x148 },

>         Package () { 0x20003, 0x1, 0x0, 0x149 },

>         Package () { 0x20003, 0x2, 0x0, 0x14a },

>         Package () { 0x20003, 0x3, 0x0, 0x14b }

>     }) // _PRT

>

> The current code always matches on the first entry when trying to

> derive the legacy interrupt routing for the USB and Ethernet controllers

> behind bridges 02.2 and 02.3, which results in the wrong entry to

> be selected.

>

> So fix this by matching the function ID to the value in the _PRT entry

> if the _PRT entries are not using 0xffff in the lower word to match

> all functions.

>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  drivers/acpi/pci_irq.c | 2 ++

>  1 file changed, 2 insertions(+)

>

> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c

> index c576a6fe4ebb..caac4ac94418 100644

> --- a/drivers/acpi/pci_irq.c

> +++ b/drivers/acpi/pci_irq.c

> @@ -160,6 +160,8 @@ static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev,

>         struct acpi_prt_entry *entry;

>

>         if (((prt->address >> 16) & 0xffff) != device ||

> +           ((prt->address & 0xffff) != 0xffff &&

> +            (prt->address & 0xffff) != PCI_FUNC(dev->devfn)) ||

>             prt->pin + 1 != pin)

>                 return -ENODEV;

>

> --

> 2.9.3

>


It has been pointed out to me by Alan that the current wording of the
ACPI spec requires the function field of the address to be 0xffff.

Section 6.2.13 of the ACPI spec has this note

Note: The PCI function number in the Address field of the _PRT
packages must be 0xFFFF, indicating
“any” function number or “all functions”

of which I wonder if it makes sense for bridges as well.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel April 7, 2017, 5:12 p.m. | #3
On 7 April 2017 at 18:06, Bjorn Helgaas <helgaas@kernel.org> wrote:
> Hi Ard,

>

> On Fri, Apr 07, 2017 at 02:22:22PM +0100, Ard Biesheuvel wrote:

>> We currently derive legacy interrupt routing by matching _PRT

>> entries on the PCI device only, presumably under the assumption

>> that PRT entries always have a value of 0xffff in the function

>> field, meaning 'match all functions'.

>

> The spec (ACPI v6.0, sec 6.2.13) contains a note that:

>

>   The PCI function number in the Address field of the _PRT packages

>   must be 0xFFFF, indicating "any" function number or "all functions".

>

> If we need a patch like this, we need to somehow reconcile it with

> that spec text to make sure firmware and OS folks have a common

> understanding of how this is supposed to work.

>

>> This no longer holds for modern PCIe topologies, where the

>> legacy interrupts for different slots may be wired to different

>> functions on the same bridge device. For instance, on AMD Seattle,

>> we may have something like

>>

>> -[0000:00]-+-00.0  Advanced Micro Devices, Inc. [AMD] Device 1a00

>>            +-02.0  Advanced Micro Devices, Inc. [AMD] Device 1a01

>>            +-02.2-[01]----00.0  Renesas uPD720202 USB 3.0 Host Controller

>>            \-02.3-[02]----00.0  Realtek RTL8169 PCIe Gigabit Ethernet

>>

>> where the _PRT describes the legacy interrupt routing as

>>

>>     Name (_PRT, Package ()  // _PRT: PCI Routing Table

>>     {

>>         // slot 1: dev 2 fn 1

>>         Package () { 0x20001, 0x0, 0x0, 0x140 },

>>         Package () { 0x20001, 0x1, 0x0, 0x141 },

>>         Package () { 0x20001, 0x2, 0x0, 0x142 },

>>         Package () { 0x20001, 0x3, 0x0, 0x143 },

>>

>>         // slot 1: dev 2 fn 2

>>         Package () { 0x20002, 0x0, 0x0, 0x144 },

>>         Package () { 0x20002, 0x1, 0x0, 0x145 },

>>         Package () { 0x20002, 0x2, 0x0, 0x146 },

>>         Package () { 0x20002, 0x3, 0x0, 0x147 },

>>

>>         // slot 1: dev 2 fn 3

>>         Package () { 0x20003, 0x0, 0x0, 0x148 },

>>         Package () { 0x20003, 0x1, 0x0, 0x149 },

>>         Package () { 0x20003, 0x2, 0x0, 0x14a },

>>         Package () { 0x20003, 0x3, 0x0, 0x14b }

>>     }) // _PRT

>

> But I think this _PRT description is incorrect and we should change

> the _PRT rather than the kernel.  My laptop has a basically identical

> topology:

>

>   -[0000:00]-+-00.0  Intel Corporation Sky Lake Host Bridge/DRAM Registers

>              +-1c.0-[02]----00.0  Realtek Semiconductor Co., Ltd.  Device 525a

>              +-1c.2-[04]----00.0  Intel Corporation Wireless 8260

>

> and the ASL looks like this (paraphrased):

>

>   Device (EXP1) {

>     Name (_ADR, 0x001C0000)

>     Name (_PRT) {

>       Package () { 0xFFFF, 0x00, \_SB.LNKA, 0x00 },

>       Package () { 0xFFFF, 0x01, \_SB.LNKB, 0x00 },

>       ...

>     }

>   }

>   Device (EXP3) {

>     Name (_ADR, 0x001C0002)

>     Name (_PRT) {

>       Package () { 0xFFFF, 0x00, \_SB.LNKC, 0x00 },

>       Package () { 0xFFFF, 0x01, \_SB.LNKD, 0x00 },

>       ...

>     }

>   }

>


Thanks for the explanation. But how is this wired up into the PNP0A08
device then? IOW, how does the ACPI code in Linux discover the
relation between these devices and my PCI root device?
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas April 7, 2017, 5:49 p.m. | #4
On Fri, Apr 07, 2017 at 06:12:05PM +0100, Ard Biesheuvel wrote:
> On 7 April 2017 at 18:06, Bjorn Helgaas <helgaas@kernel.org> wrote:

> > Hi Ard,

> >

> > On Fri, Apr 07, 2017 at 02:22:22PM +0100, Ard Biesheuvel wrote:

> >> We currently derive legacy interrupt routing by matching _PRT

> >> entries on the PCI device only, presumably under the assumption

> >> that PRT entries always have a value of 0xffff in the function

> >> field, meaning 'match all functions'.

> >

> > The spec (ACPI v6.0, sec 6.2.13) contains a note that:

> >

> >   The PCI function number in the Address field of the _PRT packages

> >   must be 0xFFFF, indicating "any" function number or "all functions".

> >

> > If we need a patch like this, we need to somehow reconcile it with

> > that spec text to make sure firmware and OS folks have a common

> > understanding of how this is supposed to work.

> >

> >> This no longer holds for modern PCIe topologies, where the

> >> legacy interrupts for different slots may be wired to different

> >> functions on the same bridge device. For instance, on AMD Seattle,

> >> we may have something like

> >>

> >> -[0000:00]-+-00.0  Advanced Micro Devices, Inc. [AMD] Device 1a00

> >>            +-02.0  Advanced Micro Devices, Inc. [AMD] Device 1a01

> >>            +-02.2-[01]----00.0  Renesas uPD720202 USB 3.0 Host Controller

> >>            \-02.3-[02]----00.0  Realtek RTL8169 PCIe Gigabit Ethernet

> >>

> >> where the _PRT describes the legacy interrupt routing as

> >>

> >>     Name (_PRT, Package ()  // _PRT: PCI Routing Table

> >>     {

> >>         // slot 1: dev 2 fn 1

> >>         Package () { 0x20001, 0x0, 0x0, 0x140 },

> >>         Package () { 0x20001, 0x1, 0x0, 0x141 },

> >>         Package () { 0x20001, 0x2, 0x0, 0x142 },

> >>         Package () { 0x20001, 0x3, 0x0, 0x143 },

> >>

> >>         // slot 1: dev 2 fn 2

> >>         Package () { 0x20002, 0x0, 0x0, 0x144 },

> >>         Package () { 0x20002, 0x1, 0x0, 0x145 },

> >>         Package () { 0x20002, 0x2, 0x0, 0x146 },

> >>         Package () { 0x20002, 0x3, 0x0, 0x147 },

> >>

> >>         // slot 1: dev 2 fn 3

> >>         Package () { 0x20003, 0x0, 0x0, 0x148 },

> >>         Package () { 0x20003, 0x1, 0x0, 0x149 },

> >>         Package () { 0x20003, 0x2, 0x0, 0x14a },

> >>         Package () { 0x20003, 0x3, 0x0, 0x14b }

> >>     }) // _PRT

> >

> > But I think this _PRT description is incorrect and we should change

> > the _PRT rather than the kernel.  My laptop has a basically identical

> > topology:

> >

> >   -[0000:00]-+-00.0  Intel Corporation Sky Lake Host Bridge/DRAM Registers

> >              +-1c.0-[02]----00.0  Realtek Semiconductor Co., Ltd.  Device 525a

> >              +-1c.2-[04]----00.0  Intel Corporation Wireless 8260

> >

> > and the ASL looks like this (paraphrased):

> >

> >   Device (EXP1) {

> >     Name (_ADR, 0x001C0000)

> >     Name (_PRT) {

> >       Package () { 0xFFFF, 0x00, \_SB.LNKA, 0x00 },

> >       Package () { 0xFFFF, 0x01, \_SB.LNKB, 0x00 },

> >       ...

> >     }

> >   }

> >   Device (EXP3) {

> >     Name (_ADR, 0x001C0002)

> >     Name (_PRT) {

> >       Package () { 0xFFFF, 0x00, \_SB.LNKC, 0x00 },

> >       Package () { 0xFFFF, 0x01, \_SB.LNKD, 0x00 },

> >       ...

> >     }

> >   }

> >

> 

> Thanks for the explanation. But how is this wired up into the PNP0A08

> device then? IOW, how does the ACPI code in Linux discover the

> relation between these devices and my PCI root device?


  Device (PCI0) {
    Name (_ADR, 0x00)
    Name (_HID, "PNP0A08")
    Device (EXP1) { ... }
    Device (EXP3) { ... }
  }
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi April 7, 2017, 6:06 p.m. | #5
On Fri, Apr 07, 2017 at 06:12:05PM +0100, Ard Biesheuvel wrote:
> On 7 April 2017 at 18:06, Bjorn Helgaas <helgaas@kernel.org> wrote:

> > Hi Ard,

> >

> > On Fri, Apr 07, 2017 at 02:22:22PM +0100, Ard Biesheuvel wrote:

> >> We currently derive legacy interrupt routing by matching _PRT

> >> entries on the PCI device only, presumably under the assumption

> >> that PRT entries always have a value of 0xffff in the function

> >> field, meaning 'match all functions'.

> >

> > The spec (ACPI v6.0, sec 6.2.13) contains a note that:

> >

> >   The PCI function number in the Address field of the _PRT packages

> >   must be 0xFFFF, indicating "any" function number or "all functions".

> >

> > If we need a patch like this, we need to somehow reconcile it with

> > that spec text to make sure firmware and OS folks have a common

> > understanding of how this is supposed to work.

> >

> >> This no longer holds for modern PCIe topologies, where the

> >> legacy interrupts for different slots may be wired to different

> >> functions on the same bridge device. For instance, on AMD Seattle,

> >> we may have something like

> >>

> >> -[0000:00]-+-00.0  Advanced Micro Devices, Inc. [AMD] Device 1a00

> >>            +-02.0  Advanced Micro Devices, Inc. [AMD] Device 1a01

> >>            +-02.2-[01]----00.0  Renesas uPD720202 USB 3.0 Host Controller

> >>            \-02.3-[02]----00.0  Realtek RTL8169 PCIe Gigabit Ethernet

> >>

> >> where the _PRT describes the legacy interrupt routing as

> >>

> >>     Name (_PRT, Package ()  // _PRT: PCI Routing Table

> >>     {

> >>         // slot 1: dev 2 fn 1

> >>         Package () { 0x20001, 0x0, 0x0, 0x140 },

> >>         Package () { 0x20001, 0x1, 0x0, 0x141 },

> >>         Package () { 0x20001, 0x2, 0x0, 0x142 },

> >>         Package () { 0x20001, 0x3, 0x0, 0x143 },

> >>

> >>         // slot 1: dev 2 fn 2

> >>         Package () { 0x20002, 0x0, 0x0, 0x144 },

> >>         Package () { 0x20002, 0x1, 0x0, 0x145 },

> >>         Package () { 0x20002, 0x2, 0x0, 0x146 },

> >>         Package () { 0x20002, 0x3, 0x0, 0x147 },

> >>

> >>         // slot 1: dev 2 fn 3

> >>         Package () { 0x20003, 0x0, 0x0, 0x148 },

> >>         Package () { 0x20003, 0x1, 0x0, 0x149 },

> >>         Package () { 0x20003, 0x2, 0x0, 0x14a },

> >>         Package () { 0x20003, 0x3, 0x0, 0x14b }

> >>     }) // _PRT

> >

> > But I think this _PRT description is incorrect and we should change

> > the _PRT rather than the kernel.  My laptop has a basically identical

> > topology:

> >

> >   -[0000:00]-+-00.0  Intel Corporation Sky Lake Host Bridge/DRAM Registers

> >              +-1c.0-[02]----00.0  Realtek Semiconductor Co., Ltd.  Device 525a

> >              +-1c.2-[04]----00.0  Intel Corporation Wireless 8260

> >

> > and the ASL looks like this (paraphrased):

> >

> >   Device (EXP1) {

> >     Name (_ADR, 0x001C0000)

> >     Name (_PRT) {

> >       Package () { 0xFFFF, 0x00, \_SB.LNKA, 0x00 },

> >       Package () { 0xFFFF, 0x01, \_SB.LNKB, 0x00 },

> >       ...

> >     }

> >   }

> >   Device (EXP3) {

> >     Name (_ADR, 0x001C0002)

> >     Name (_PRT) {

> >       Package () { 0xFFFF, 0x00, \_SB.LNKC, 0x00 },

> >       Package () { 0xFFFF, 0x01, \_SB.LNKD, 0x00 },

> >       ...

> >     }

> >   }

> >

> 

> Thanks for the explanation. But how is this wired up into the PNP0A08

> device then? IOW, how does the ACPI code in Linux discover the

> relation between these devices and my PCI root device?


You describe the PCI hierarchy starting from PNP0A08 at root and the
kernel assigns the ACPI companion through _ADR matching (see
acpi_pci_find_companion()) which is what is used by _PRT parsing
code to route IRQs IIUC.

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel April 7, 2017, 6:35 p.m. | #6
On 7 April 2017 at 19:06, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Fri, Apr 07, 2017 at 06:12:05PM +0100, Ard Biesheuvel wrote:

>> On 7 April 2017 at 18:06, Bjorn Helgaas <helgaas@kernel.org> wrote:

>> > Hi Ard,

>> >

>> > On Fri, Apr 07, 2017 at 02:22:22PM +0100, Ard Biesheuvel wrote:

>> >> We currently derive legacy interrupt routing by matching _PRT

>> >> entries on the PCI device only, presumably under the assumption

>> >> that PRT entries always have a value of 0xffff in the function

>> >> field, meaning 'match all functions'.

>> >

>> > The spec (ACPI v6.0, sec 6.2.13) contains a note that:

>> >

>> >   The PCI function number in the Address field of the _PRT packages

>> >   must be 0xFFFF, indicating "any" function number or "all functions".

>> >

>> > If we need a patch like this, we need to somehow reconcile it with

>> > that spec text to make sure firmware and OS folks have a common

>> > understanding of how this is supposed to work.

>> >

>> >> This no longer holds for modern PCIe topologies, where the

>> >> legacy interrupts for different slots may be wired to different

>> >> functions on the same bridge device. For instance, on AMD Seattle,

>> >> we may have something like

>> >>

>> >> -[0000:00]-+-00.0  Advanced Micro Devices, Inc. [AMD] Device 1a00

>> >>            +-02.0  Advanced Micro Devices, Inc. [AMD] Device 1a01

>> >>            +-02.2-[01]----00.0  Renesas uPD720202 USB 3.0 Host Controller

>> >>            \-02.3-[02]----00.0  Realtek RTL8169 PCIe Gigabit Ethernet

>> >>

>> >> where the _PRT describes the legacy interrupt routing as

>> >>

>> >>     Name (_PRT, Package ()  // _PRT: PCI Routing Table

>> >>     {

>> >>         // slot 1: dev 2 fn 1

>> >>         Package () { 0x20001, 0x0, 0x0, 0x140 },

>> >>         Package () { 0x20001, 0x1, 0x0, 0x141 },

>> >>         Package () { 0x20001, 0x2, 0x0, 0x142 },

>> >>         Package () { 0x20001, 0x3, 0x0, 0x143 },

>> >>

>> >>         // slot 1: dev 2 fn 2

>> >>         Package () { 0x20002, 0x0, 0x0, 0x144 },

>> >>         Package () { 0x20002, 0x1, 0x0, 0x145 },

>> >>         Package () { 0x20002, 0x2, 0x0, 0x146 },

>> >>         Package () { 0x20002, 0x3, 0x0, 0x147 },

>> >>

>> >>         // slot 1: dev 2 fn 3

>> >>         Package () { 0x20003, 0x0, 0x0, 0x148 },

>> >>         Package () { 0x20003, 0x1, 0x0, 0x149 },

>> >>         Package () { 0x20003, 0x2, 0x0, 0x14a },

>> >>         Package () { 0x20003, 0x3, 0x0, 0x14b }

>> >>     }) // _PRT

>> >

>> > But I think this _PRT description is incorrect and we should change

>> > the _PRT rather than the kernel.  My laptop has a basically identical

>> > topology:

>> >

>> >   -[0000:00]-+-00.0  Intel Corporation Sky Lake Host Bridge/DRAM Registers

>> >              +-1c.0-[02]----00.0  Realtek Semiconductor Co., Ltd.  Device 525a

>> >              +-1c.2-[04]----00.0  Intel Corporation Wireless 8260

>> >

>> > and the ASL looks like this (paraphrased):

>> >

>> >   Device (EXP1) {

>> >     Name (_ADR, 0x001C0000)

>> >     Name (_PRT) {

>> >       Package () { 0xFFFF, 0x00, \_SB.LNKA, 0x00 },

>> >       Package () { 0xFFFF, 0x01, \_SB.LNKB, 0x00 },

>> >       ...

>> >     }

>> >   }

>> >   Device (EXP3) {

>> >     Name (_ADR, 0x001C0002)

>> >     Name (_PRT) {

>> >       Package () { 0xFFFF, 0x00, \_SB.LNKC, 0x00 },

>> >       Package () { 0xFFFF, 0x01, \_SB.LNKD, 0x00 },

>> >       ...

>> >     }

>> >   }

>> >

>>

>> Thanks for the explanation. But how is this wired up into the PNP0A08

>> device then? IOW, how does the ACPI code in Linux discover the

>> relation between these devices and my PCI root device?

>

> You describe the PCI hierarchy starting from PNP0A08 at root and the

> kernel assigns the ACPI companion through _ADR matching (see

> acpi_pci_find_companion()) which is what is used by _PRT parsing

> code to route IRQs IIUC.

>


OK, I have changed my DSDT as follows:


Device (PCI0)
{
    Name (_ADR, 0x00)
    Name (_HID, "PNP0A08" /* PCI Express Bus */)  // _HID: Hardware ID
    Name (_CID, "PNP0A03" /* PCI Bus */)  // _CID: Compatible ID
    Name (_SEG, 0x00)  // _SEG: PCI Segment
    Name (_BBN, 0x00)  // _BBN: BIOS Bus Number
    Name (_CCA, 0x01)  // _CCA: Cache Coherency Attribute

    Device (EXP1)
    {
        Name (_ADR, 0x20001)  // _ADR: Address
        Name (_PRT, Package ()  // _PRT: PCI Routing Table
        {
            // slot 1: dev 2 fn 1
            Package () { 0xFFFF, 0x0, 0x0, 0x140 },
            Package () { 0xFFFF, 0x1, 0x0, 0x141 },
            Package () { 0xFFFF, 0x2, 0x0, 0x142 },
            Package () { 0xFFFF, 0x3, 0x0, 0x143 }
        }) // _PRT
    }
    Device (EXP2)
    {
        Name (_ADR, 0x20002)  // _ADR: Address
        Name (_PRT, Package ()  // _PRT: PCI Routing Table
        {
            // slot 2: dev 2 fn 2
            Package () { 0xFFFF, 0x0, 0x0, 0x144 },
            Package () { 0xFFFF, 0x1, 0x0, 0x145 },
            Package () { 0xFFFF, 0x2, 0x0, 0x146 },
            Package () { 0xFFFF, 0x3, 0x0, 0x147 }
        }) // _PRT
    }
    Device (EXP3)
    {
        Name (_ADR, 0x20003)  // _ADR: Address
        Name (_PRT, Package ()  // _PRT: PCI Routing Table
        {
            // slot 3: dev 2 fn 3
            Package () { 0xFFFF, 0x0, 0x0, 0x148 },
            Package () { 0xFFFF, 0x1, 0x0, 0x149 },
            Package () { 0xFFFF, 0x2, 0x0, 0x14a },
            Package () { 0xFFFF, 0x3, 0x0, 0x14b }
        }) // _PRT
    }

but it does not get picked up, and I am back to

[    3.357555] pcieport 0000:00:02.2: can't derive routing for PCI INT A
[    3.370477] pcieport 0000:00:02.2: PCI INT A: no GSI
[    3.380549] pcieport 0000:00:02.3: can't derive routing for PCI INT A
[    3.393476] pcieport 0000:00:02.3: PCI INT A: no GSI

Then I tried switching to

Device (SLT1)
{
    Name(_HID, EISAID("PNP0C0F"))
    Name(_UID, 0x1)
    Name(_PRS, ResourceTemplate() {
        Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x140 }
        Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x141 }
        Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x142 }
        Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x143 }
    })
    Method (_CRS, 0) { Return (_PRS) }
    Method (_SRS, 1) { }
    Method (_DIS) { }
}
Device (SLT2)
{
    Name(_HID, EISAID("PNP0C0F"))
    Name(_UID, 0x2)
    Name(_PRS, ResourceTemplate() {
        Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x144 }
        Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x145 }
        Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x146 }
        Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x147 }
    })
    Method (_CRS, 0) { Return (_PRS) }
    Method (_SRS, 1) { }
    Method (_DIS) { }
}
Device (SLT3)
{
    Name(_HID, EISAID("PNP0C0F"))
    Name(_UID, 0x3)
    Name(_PRS, ResourceTemplate() {
        Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x148 }
        Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x149 }
        Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x14A }
        Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x14B }
    })
    Method (_CRS, 0) { Return (_PRS) }
    Method (_SRS, 1) { }
    Method (_DIS) { }
}

//
// PCIe Root Bus
//
Device (PCI0)
{
    Name (_HID, "PNP0A08" /* PCI Express Bus */)  // _HID: Hardware ID
    Name (_CID, "PNP0A03" /* PCI Bus */)  // _CID: Compatible ID
    Name (_SEG, 0x00)  // _SEG: PCI Segment
    Name (_BBN, 0x00)  // _BBN: BIOS Bus Number
    Name (_CCA, 0x01)  // _CCA: Cache Coherency Attribute

    Device (EXP1)
    {
        Name (_ADR, 0x20001)  // _ADR: Address
        Name (_PRT, Package ()  // _PRT: PCI Routing Table
        {
            Package () { 0xFFFF, 0x0, \_SB.SLT1, 0x0 },
            Package () { 0xFFFF, 0x1, \_SB.SLT1, 0x1 },
            Package () { 0xFFFF, 0x2, \_SB.SLT1, 0x2 },
            Package () { 0xFFFF, 0x3, \_SB.SLT1, 0x3 }
        }) // _PRT
    }
    Device (EXP2)
    {
        Name (_ADR, 0x20002)  // _ADR: Address
        Name (_PRT, Package ()  // _PRT: PCI Routing Table
        {
            Package () { 0xFFFF, 0x0, \_SB.SLT2, 0x0 },
            Package () { 0xFFFF, 0x1, \_SB.SLT2, 0x1 },
            Package () { 0xFFFF, 0x2, \_SB.SLT2, 0x2 },
            Package () { 0xFFFF, 0x3, \_SB.SLT2, 0x3 }
        }) // _PRT
    }
    Device (EXP3)
    {
        Name (_ADR, 0x20003)  // _ADR: Address
        Name (_PRT, Package ()  // _PRT: PCI Routing Table
        {
            Package () { 0xFFFF, 0x0, \_SB.SLT3, 0x0 },
            Package () { 0xFFFF, 0x1, \_SB.SLT3, 0x1 },
            Package () { 0xFFFF, 0x2, \_SB.SLT3, 0x2 },
            Package () { 0xFFFF, 0x3, \_SB.SLT3, 0x3 }
        }) // _PRT
    }

with the same result.

So could we be missing anything in the arm64 implementation that
prevents the companion device from being found?
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi April 7, 2017, 9:36 p.m. | #7
On Fri, Apr 07, 2017 at 07:35:45PM +0100, Ard Biesheuvel wrote:
> On 7 April 2017 at 19:06, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:

> > On Fri, Apr 07, 2017 at 06:12:05PM +0100, Ard Biesheuvel wrote:

> >> On 7 April 2017 at 18:06, Bjorn Helgaas <helgaas@kernel.org> wrote:

> >> > Hi Ard,

> >> >

> >> > On Fri, Apr 07, 2017 at 02:22:22PM +0100, Ard Biesheuvel wrote:

> >> >> We currently derive legacy interrupt routing by matching _PRT

> >> >> entries on the PCI device only, presumably under the assumption

> >> >> that PRT entries always have a value of 0xffff in the function

> >> >> field, meaning 'match all functions'.

> >> >

> >> > The spec (ACPI v6.0, sec 6.2.13) contains a note that:

> >> >

> >> >   The PCI function number in the Address field of the _PRT packages

> >> >   must be 0xFFFF, indicating "any" function number or "all functions".

> >> >

> >> > If we need a patch like this, we need to somehow reconcile it with

> >> > that spec text to make sure firmware and OS folks have a common

> >> > understanding of how this is supposed to work.

> >> >

> >> >> This no longer holds for modern PCIe topologies, where the

> >> >> legacy interrupts for different slots may be wired to different

> >> >> functions on the same bridge device. For instance, on AMD Seattle,

> >> >> we may have something like

> >> >>

> >> >> -[0000:00]-+-00.0  Advanced Micro Devices, Inc. [AMD] Device 1a00

> >> >>            +-02.0  Advanced Micro Devices, Inc. [AMD] Device 1a01

> >> >>            +-02.2-[01]----00.0  Renesas uPD720202 USB 3.0 Host Controller

> >> >>            \-02.3-[02]----00.0  Realtek RTL8169 PCIe Gigabit Ethernet

> >> >>

> >> >> where the _PRT describes the legacy interrupt routing as

> >> >>

> >> >>     Name (_PRT, Package ()  // _PRT: PCI Routing Table

> >> >>     {

> >> >>         // slot 1: dev 2 fn 1

> >> >>         Package () { 0x20001, 0x0, 0x0, 0x140 },

> >> >>         Package () { 0x20001, 0x1, 0x0, 0x141 },

> >> >>         Package () { 0x20001, 0x2, 0x0, 0x142 },

> >> >>         Package () { 0x20001, 0x3, 0x0, 0x143 },

> >> >>

> >> >>         // slot 1: dev 2 fn 2

> >> >>         Package () { 0x20002, 0x0, 0x0, 0x144 },

> >> >>         Package () { 0x20002, 0x1, 0x0, 0x145 },

> >> >>         Package () { 0x20002, 0x2, 0x0, 0x146 },

> >> >>         Package () { 0x20002, 0x3, 0x0, 0x147 },

> >> >>

> >> >>         // slot 1: dev 2 fn 3

> >> >>         Package () { 0x20003, 0x0, 0x0, 0x148 },

> >> >>         Package () { 0x20003, 0x1, 0x0, 0x149 },

> >> >>         Package () { 0x20003, 0x2, 0x0, 0x14a },

> >> >>         Package () { 0x20003, 0x3, 0x0, 0x14b }

> >> >>     }) // _PRT

> >> >

> >> > But I think this _PRT description is incorrect and we should change

> >> > the _PRT rather than the kernel.  My laptop has a basically identical

> >> > topology:

> >> >

> >> >   -[0000:00]-+-00.0  Intel Corporation Sky Lake Host Bridge/DRAM Registers

> >> >              +-1c.0-[02]----00.0  Realtek Semiconductor Co., Ltd.  Device 525a

> >> >              +-1c.2-[04]----00.0  Intel Corporation Wireless 8260

> >> >

> >> > and the ASL looks like this (paraphrased):

> >> >

> >> >   Device (EXP1) {

> >> >     Name (_ADR, 0x001C0000)

> >> >     Name (_PRT) {

> >> >       Package () { 0xFFFF, 0x00, \_SB.LNKA, 0x00 },

> >> >       Package () { 0xFFFF, 0x01, \_SB.LNKB, 0x00 },

> >> >       ...

> >> >     }

> >> >   }

> >> >   Device (EXP3) {

> >> >     Name (_ADR, 0x001C0002)

> >> >     Name (_PRT) {

> >> >       Package () { 0xFFFF, 0x00, \_SB.LNKC, 0x00 },

> >> >       Package () { 0xFFFF, 0x01, \_SB.LNKD, 0x00 },

> >> >       ...

> >> >     }

> >> >   }

> >> >

> >>

> >> Thanks for the explanation. But how is this wired up into the PNP0A08

> >> device then? IOW, how does the ACPI code in Linux discover the

> >> relation between these devices and my PCI root device?

> >

> > You describe the PCI hierarchy starting from PNP0A08 at root and the

> > kernel assigns the ACPI companion through _ADR matching (see

> > acpi_pci_find_companion()) which is what is used by _PRT parsing

> > code to route IRQs IIUC.

> >

> 

> OK, I have changed my DSDT as follows:

> 

> 

> Device (PCI0)

> {

>     Name (_ADR, 0x00)

>     Name (_HID, "PNP0A08" /* PCI Express Bus */)  // _HID: Hardware ID

>     Name (_CID, "PNP0A03" /* PCI Bus */)  // _CID: Compatible ID

>     Name (_SEG, 0x00)  // _SEG: PCI Segment

>     Name (_BBN, 0x00)  // _BBN: BIOS Bus Number

>     Name (_CCA, 0x01)  // _CCA: Cache Coherency Attribute

> 

>     Device (EXP1)

>     {

>         Name (_ADR, 0x20001)  // _ADR: Address

>         Name (_PRT, Package ()  // _PRT: PCI Routing Table

>         {

>             // slot 1: dev 2 fn 1

>             Package () { 0xFFFF, 0x0, 0x0, 0x140 },

                           ^
                           0x2FFFF

Ditto for the other entries.

>             Package () { 0xFFFF, 0x1, 0x0, 0x141 },

>             Package () { 0xFFFF, 0x2, 0x0, 0x142 },

>             Package () { 0xFFFF, 0x3, 0x0, 0x143 }

>         }) // _PRT

>     }

>     Device (EXP2)

>     {

>         Name (_ADR, 0x20002)  // _ADR: Address

>         Name (_PRT, Package ()  // _PRT: PCI Routing Table

>         {

>             // slot 2: dev 2 fn 2

>             Package () { 0xFFFF, 0x0, 0x0, 0x144 },

>             Package () { 0xFFFF, 0x1, 0x0, 0x145 },

>             Package () { 0xFFFF, 0x2, 0x0, 0x146 },

>             Package () { 0xFFFF, 0x3, 0x0, 0x147 }

>         }) // _PRT

>     }

>     Device (EXP3)

>     {

>         Name (_ADR, 0x20003)  // _ADR: Address

>         Name (_PRT, Package ()  // _PRT: PCI Routing Table

>         {

>             // slot 3: dev 2 fn 3

>             Package () { 0xFFFF, 0x0, 0x0, 0x148 },

>             Package () { 0xFFFF, 0x1, 0x0, 0x149 },

>             Package () { 0xFFFF, 0x2, 0x0, 0x14a },

>             Package () { 0xFFFF, 0x3, 0x0, 0x14b }

>         }) // _PRT

>     }

> 

> but it does not get picked up, and I am back to

> 

> [    3.357555] pcieport 0000:00:02.2: can't derive routing for PCI INT A

> [    3.370477] pcieport 0000:00:02.2: PCI INT A: no GSI

> [    3.380549] pcieport 0000:00:02.3: can't derive routing for PCI INT A

> [    3.393476] pcieport 0000:00:02.3: PCI INT A: no GSI

> 

> Then I tried switching to

> 

> Device (SLT1)

> {

>     Name(_HID, EISAID("PNP0C0F"))

>     Name(_UID, 0x1)

>     Name(_PRS, ResourceTemplate() {

>         Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x140 }

>         Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x141 }

>         Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x142 }

>         Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x143 }

>     })

>     Method (_CRS, 0) { Return (_PRS) }

>     Method (_SRS, 1) { }

>     Method (_DIS) { }

> }

> Device (SLT2)

> {

>     Name(_HID, EISAID("PNP0C0F"))

>     Name(_UID, 0x2)

>     Name(_PRS, ResourceTemplate() {

>         Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x144 }

>         Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x145 }

>         Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x146 }

>         Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x147 }

>     })

>     Method (_CRS, 0) { Return (_PRS) }

>     Method (_SRS, 1) { }

>     Method (_DIS) { }

> }

> Device (SLT3)

> {

>     Name(_HID, EISAID("PNP0C0F"))

>     Name(_UID, 0x3)

>     Name(_PRS, ResourceTemplate() {

>         Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x148 }

>         Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x149 }

>         Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x14A }

>         Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive) { 0x14B }

>     })

>     Method (_CRS, 0) { Return (_PRS) }

>     Method (_SRS, 1) { }

>     Method (_DIS) { }

> }

> 

> //

> // PCIe Root Bus

> //

> Device (PCI0)

> {

>     Name (_HID, "PNP0A08" /* PCI Express Bus */)  // _HID: Hardware ID

>     Name (_CID, "PNP0A03" /* PCI Bus */)  // _CID: Compatible ID

>     Name (_SEG, 0x00)  // _SEG: PCI Segment

>     Name (_BBN, 0x00)  // _BBN: BIOS Bus Number

>     Name (_CCA, 0x01)  // _CCA: Cache Coherency Attribute

> 

>     Device (EXP1)

>     {

>         Name (_ADR, 0x20001)  // _ADR: Address

>         Name (_PRT, Package ()  // _PRT: PCI Routing Table

>         {

>             Package () { 0xFFFF, 0x0, \_SB.SLT1, 0x0 },

>             Package () { 0xFFFF, 0x1, \_SB.SLT1, 0x1 },

>             Package () { 0xFFFF, 0x2, \_SB.SLT1, 0x2 },

>             Package () { 0xFFFF, 0x3, \_SB.SLT1, 0x3 }

>         }) // _PRT

>     }

>     Device (EXP2)

>     {

>         Name (_ADR, 0x20002)  // _ADR: Address

>         Name (_PRT, Package ()  // _PRT: PCI Routing Table

>         {

>             Package () { 0xFFFF, 0x0, \_SB.SLT2, 0x0 },

>             Package () { 0xFFFF, 0x1, \_SB.SLT2, 0x1 },

>             Package () { 0xFFFF, 0x2, \_SB.SLT2, 0x2 },

>             Package () { 0xFFFF, 0x3, \_SB.SLT2, 0x3 }

>         }) // _PRT

>     }

>     Device (EXP3)

>     {

>         Name (_ADR, 0x20003)  // _ADR: Address

>         Name (_PRT, Package ()  // _PRT: PCI Routing Table

>         {

>             Package () { 0xFFFF, 0x0, \_SB.SLT3, 0x0 },

>             Package () { 0xFFFF, 0x1, \_SB.SLT3, 0x1 },

>             Package () { 0xFFFF, 0x2, \_SB.SLT3, 0x2 },

>             Package () { 0xFFFF, 0x3, \_SB.SLT3, 0x3 }

>         }) // _PRT

>     }

> 

> with the same result.

> 

> So could we be missing anything in the arm64 implementation that

> prevents the companion device from being found?


No and if there is that's a bug. I will help you debug it next week,
mind trying the change I suggest above please ? I *think* the devfn
parameter in the _PRT entry must match the device looked-up, in
particular the device bits.

Thanks !
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel April 8, 2017, 7:04 a.m. | #8
On 7 April 2017 at 22:36, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Fri, Apr 07, 2017 at 07:35:45PM +0100, Ard Biesheuvel wrote:

>> On 7 April 2017 at 19:06, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:

>> > On Fri, Apr 07, 2017 at 06:12:05PM +0100, Ard Biesheuvel wrote:

>> >> On 7 April 2017 at 18:06, Bjorn Helgaas <helgaas@kernel.org> wrote:

>> >> > Hi Ard,

>> >> >

>> >> > On Fri, Apr 07, 2017 at 02:22:22PM +0100, Ard Biesheuvel wrote:

>> >> >> We currently derive legacy interrupt routing by matching _PRT

>> >> >> entries on the PCI device only, presumably under the assumption

>> >> >> that PRT entries always have a value of 0xffff in the function

>> >> >> field, meaning 'match all functions'.

>> >> >

>> >> > The spec (ACPI v6.0, sec 6.2.13) contains a note that:

>> >> >

>> >> >   The PCI function number in the Address field of the _PRT packages

>> >> >   must be 0xFFFF, indicating "any" function number or "all functions".

>> >> >

>> >> > If we need a patch like this, we need to somehow reconcile it with

>> >> > that spec text to make sure firmware and OS folks have a common

>> >> > understanding of how this is supposed to work.

>> >> >

>> >> >> This no longer holds for modern PCIe topologies, where the

>> >> >> legacy interrupts for different slots may be wired to different

>> >> >> functions on the same bridge device. For instance, on AMD Seattle,

>> >> >> we may have something like

>> >> >>

>> >> >> -[0000:00]-+-00.0  Advanced Micro Devices, Inc. [AMD] Device 1a00

>> >> >>            +-02.0  Advanced Micro Devices, Inc. [AMD] Device 1a01

>> >> >>            +-02.2-[01]----00.0  Renesas uPD720202 USB 3.0 Host Controller

>> >> >>            \-02.3-[02]----00.0  Realtek RTL8169 PCIe Gigabit Ethernet

>> >> >>

>> >> >> where the _PRT describes the legacy interrupt routing as

>> >> >>

>> >> >>     Name (_PRT, Package ()  // _PRT: PCI Routing Table

>> >> >>     {

>> >> >>         // slot 1: dev 2 fn 1

>> >> >>         Package () { 0x20001, 0x0, 0x0, 0x140 },

>> >> >>         Package () { 0x20001, 0x1, 0x0, 0x141 },

>> >> >>         Package () { 0x20001, 0x2, 0x0, 0x142 },

>> >> >>         Package () { 0x20001, 0x3, 0x0, 0x143 },

>> >> >>

>> >> >>         // slot 1: dev 2 fn 2

>> >> >>         Package () { 0x20002, 0x0, 0x0, 0x144 },

>> >> >>         Package () { 0x20002, 0x1, 0x0, 0x145 },

>> >> >>         Package () { 0x20002, 0x2, 0x0, 0x146 },

>> >> >>         Package () { 0x20002, 0x3, 0x0, 0x147 },

>> >> >>

>> >> >>         // slot 1: dev 2 fn 3

>> >> >>         Package () { 0x20003, 0x0, 0x0, 0x148 },

>> >> >>         Package () { 0x20003, 0x1, 0x0, 0x149 },

>> >> >>         Package () { 0x20003, 0x2, 0x0, 0x14a },

>> >> >>         Package () { 0x20003, 0x3, 0x0, 0x14b }

>> >> >>     }) // _PRT

>> >> >

>> >> > But I think this _PRT description is incorrect and we should change

>> >> > the _PRT rather than the kernel.  My laptop has a basically identical

>> >> > topology:

>> >> >

>> >> >   -[0000:00]-+-00.0  Intel Corporation Sky Lake Host Bridge/DRAM Registers

>> >> >              +-1c.0-[02]----00.0  Realtek Semiconductor Co., Ltd.  Device 525a

>> >> >              +-1c.2-[04]----00.0  Intel Corporation Wireless 8260

>> >> >

>> >> > and the ASL looks like this (paraphrased):

>> >> >

>> >> >   Device (EXP1) {

>> >> >     Name (_ADR, 0x001C0000)

>> >> >     Name (_PRT) {

>> >> >       Package () { 0xFFFF, 0x00, \_SB.LNKA, 0x00 },

>> >> >       Package () { 0xFFFF, 0x01, \_SB.LNKB, 0x00 },

>> >> >       ...

>> >> >     }

>> >> >   }

>> >> >   Device (EXP3) {

>> >> >     Name (_ADR, 0x001C0002)

>> >> >     Name (_PRT) {

>> >> >       Package () { 0xFFFF, 0x00, \_SB.LNKC, 0x00 },

>> >> >       Package () { 0xFFFF, 0x01, \_SB.LNKD, 0x00 },

>> >> >       ...

>> >> >     }

>> >> >   }

>> >> >

>> >>

>> >> Thanks for the explanation. But how is this wired up into the PNP0A08

>> >> device then? IOW, how does the ACPI code in Linux discover the

>> >> relation between these devices and my PCI root device?

>> >

>> > You describe the PCI hierarchy starting from PNP0A08 at root and the

>> > kernel assigns the ACPI companion through _ADR matching (see

>> > acpi_pci_find_companion()) which is what is used by _PRT parsing

>> > code to route IRQs IIUC.

>> >

>>

>> OK, I have changed my DSDT as follows:

>>

>>

>> Device (PCI0)

>> {

>>     Name (_ADR, 0x00)

>>     Name (_HID, "PNP0A08" /* PCI Express Bus */)  // _HID: Hardware ID

>>     Name (_CID, "PNP0A03" /* PCI Bus */)  // _CID: Compatible ID

>>     Name (_SEG, 0x00)  // _SEG: PCI Segment

>>     Name (_BBN, 0x00)  // _BBN: BIOS Bus Number

>>     Name (_CCA, 0x01)  // _CCA: Cache Coherency Attribute

>>

>>     Device (EXP1)

>>     {

>>         Name (_ADR, 0x20001)  // _ADR: Address

>>         Name (_PRT, Package ()  // _PRT: PCI Routing Table

>>         {

>>             // slot 1: dev 2 fn 1

>>             Package () { 0xFFFF, 0x0, 0x0, 0x140 },

>                            ^

>                            0x2FFFF

>

> Ditto for the other entries.

>


I had already tried that as well, but it doesn't work.

Single stepping through the ACPI code, it seems like the _PRT entry is
never found by acpi_get_irq_routing_table(), and so whether the
contents are correct doesn't really matter at this point.

>>

>> So could we be missing anything in the arm64 implementation that

>> prevents the companion device from being found?

>

> No and if there is that's a bug. I will help you debug it next week,

> mind trying the change I suggest above please ? I *think* the devfn

> parameter in the _PRT entry must match the device looked-up, in

> particular the device bits.

>


Yes, let's look into this on Monday.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel April 8, 2017, 11:22 a.m. | #9
On 7 April 2017 at 19:35, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 7 April 2017 at 19:06, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:

>> On Fri, Apr 07, 2017 at 06:12:05PM +0100, Ard Biesheuvel wrote:

>>> On 7 April 2017 at 18:06, Bjorn Helgaas <helgaas@kernel.org> wrote:

[...]
>

> OK, I have changed my DSDT as follows:

>

>

> Device (PCI0)

> {

>     Name (_ADR, 0x00)

>     Name (_HID, "PNP0A08" /* PCI Express Bus */)  // _HID: Hardware ID

>     Name (_CID, "PNP0A03" /* PCI Bus */)  // _CID: Compatible ID

>     Name (_SEG, 0x00)  // _SEG: PCI Segment

>     Name (_BBN, 0x00)  // _BBN: BIOS Bus Number

>     Name (_CCA, 0x01)  // _CCA: Cache Coherency Attribute

>

>     Device (EXP1)

>     {

>         Name (_ADR, 0x20001)  // _ADR: Address

>         Name (_PRT, Package ()  // _PRT: PCI Routing Table

>         {

>             // slot 1: dev 2 fn 1

>             Package () { 0xFFFF, 0x0, 0x0, 0x140 },

>             Package () { 0xFFFF, 0x1, 0x0, 0x141 },

>             Package () { 0xFFFF, 0x2, 0x0, 0x142 },

>             Package () { 0xFFFF, 0x3, 0x0, 0x143 }

>         }) // _PRT

>     }

>     Device (EXP2)

>     {

>         Name (_ADR, 0x20002)  // _ADR: Address

>         Name (_PRT, Package ()  // _PRT: PCI Routing Table

>         {

>             // slot 2: dev 2 fn 2

>             Package () { 0xFFFF, 0x0, 0x0, 0x144 },

>             Package () { 0xFFFF, 0x1, 0x0, 0x145 },

>             Package () { 0xFFFF, 0x2, 0x0, 0x146 },

>             Package () { 0xFFFF, 0x3, 0x0, 0x147 }

>         }) // _PRT

>     }

>     Device (EXP3)

>     {

>         Name (_ADR, 0x20003)  // _ADR: Address

>         Name (_PRT, Package ()  // _PRT: PCI Routing Table

>         {

>             // slot 3: dev 2 fn 3

>             Package () { 0xFFFF, 0x0, 0x0, 0x148 },

>             Package () { 0xFFFF, 0x1, 0x0, 0x149 },

>             Package () { 0xFFFF, 0x2, 0x0, 0x14a },

>             Package () { 0xFFFF, 0x3, 0x0, 0x14b }

>         }) // _PRT

>     }

>

> but it does not get picked up, and I am back to

>

> [    3.357555] pcieport 0000:00:02.2: can't derive routing for PCI INT A

> [    3.370477] pcieport 0000:00:02.2: PCI INT A: no GSI

> [    3.380549] pcieport 0000:00:02.3: can't derive routing for PCI INT A

> [    3.393476] pcieport 0000:00:02.3: PCI INT A: no GSI

>


OK, this does appear to work in fact, for the devices behind the
bridges. These messages are from the pcieport driver trying to request
an IRQ for the bridge device itself, which no longer works now that
the _PRT methods have been moved out.

So that mostly solves the problem. I'll try adding back a _PRT in the
PCI root device itself, but i'm not sure if I can make any inferences
about the IRQ wiring from the data i have.

Thanks all,
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi April 8, 2017, 1:22 p.m. | #10
On Sat, Apr 08, 2017 at 12:22:15PM +0100, Ard Biesheuvel wrote:
> On 7 April 2017 at 19:35, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> > On 7 April 2017 at 19:06, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:

> >> On Fri, Apr 07, 2017 at 06:12:05PM +0100, Ard Biesheuvel wrote:

> >>> On 7 April 2017 at 18:06, Bjorn Helgaas <helgaas@kernel.org> wrote:

> [...]

> >

> > OK, I have changed my DSDT as follows:

> >

> >

> > Device (PCI0)

> > {

> >     Name (_ADR, 0x00)

> >     Name (_HID, "PNP0A08" /* PCI Express Bus */)  // _HID: Hardware ID

> >     Name (_CID, "PNP0A03" /* PCI Bus */)  // _CID: Compatible ID

> >     Name (_SEG, 0x00)  // _SEG: PCI Segment

> >     Name (_BBN, 0x00)  // _BBN: BIOS Bus Number

> >     Name (_CCA, 0x01)  // _CCA: Cache Coherency Attribute

> >

> >     Device (EXP1)

> >     {

> >         Name (_ADR, 0x20001)  // _ADR: Address

> >         Name (_PRT, Package ()  // _PRT: PCI Routing Table

> >         {

> >             // slot 1: dev 2 fn 1

> >             Package () { 0xFFFF, 0x0, 0x0, 0x140 },

> >             Package () { 0xFFFF, 0x1, 0x0, 0x141 },

> >             Package () { 0xFFFF, 0x2, 0x0, 0x142 },

> >             Package () { 0xFFFF, 0x3, 0x0, 0x143 }

> >         }) // _PRT

> >     }

> >     Device (EXP2)

> >     {

> >         Name (_ADR, 0x20002)  // _ADR: Address

> >         Name (_PRT, Package ()  // _PRT: PCI Routing Table

> >         {

> >             // slot 2: dev 2 fn 2

> >             Package () { 0xFFFF, 0x0, 0x0, 0x144 },

> >             Package () { 0xFFFF, 0x1, 0x0, 0x145 },

> >             Package () { 0xFFFF, 0x2, 0x0, 0x146 },

> >             Package () { 0xFFFF, 0x3, 0x0, 0x147 }

> >         }) // _PRT

> >     }

> >     Device (EXP3)

> >     {

> >         Name (_ADR, 0x20003)  // _ADR: Address

> >         Name (_PRT, Package ()  // _PRT: PCI Routing Table

> >         {

> >             // slot 3: dev 2 fn 3

> >             Package () { 0xFFFF, 0x0, 0x0, 0x148 },

> >             Package () { 0xFFFF, 0x1, 0x0, 0x149 },

> >             Package () { 0xFFFF, 0x2, 0x0, 0x14a },

> >             Package () { 0xFFFF, 0x3, 0x0, 0x14b }

> >         }) // _PRT

> >     }

> >

> > but it does not get picked up, and I am back to

> >

> > [    3.357555] pcieport 0000:00:02.2: can't derive routing for PCI INT A

> > [    3.370477] pcieport 0000:00:02.2: PCI INT A: no GSI

> > [    3.380549] pcieport 0000:00:02.3: can't derive routing for PCI INT A

> > [    3.393476] pcieport 0000:00:02.3: PCI INT A: no GSI

> >

> 

> OK, this does appear to work in fact, for the devices behind the

> bridges. These messages are from the pcieport driver trying to request

> an IRQ for the bridge device itself, which no longer works now that

> the _PRT methods have been moved out.

> 

> So that mostly solves the problem. I'll try adding back a _PRT in the

> PCI root device itself, but i'm not sure if I can make any inferences

> about the IRQ wiring from the data i have.


Yes and IIUC with the $SUBJECT patch applied, the endpoints would match
the _PRT entry under PNP0A08 through their bridge (PCIe port device = 2
fn = X) device entry, not their own (device = 0) (ie first look-up in
acpi_pci_irq_lookup() through acpi_pci_irq_find_prt_entry() should fail
for the endpoints, then the same look-up is carried out with the bridge
device instead and that succeeds, the _PRT entry routes IRQs for the
PCIeport AND their downstream endpoint with $SUBJECT patch applied).

If you add the _PRT under the PCIe port itself (table above) the PCIe
port can't find an ACPI parent device to route its own IRQx, that's my
reading of what's going on, I will debug it next week.

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel April 8, 2017, 2:02 p.m. | #11
> On 8 Apr 2017, at 14:22, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:

> 

>> On Sat, Apr 08, 2017 at 12:22:15PM +0100, Ard Biesheuvel wrote:

>>> On 7 April 2017 at 19:35, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>>>> On 7 April 2017 at 19:06, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:

>>>>> On Fri, Apr 07, 2017 at 06:12:05PM +0100, Ard Biesheuvel wrote:

>>>>>> On 7 April 2017 at 18:06, Bjorn Helgaas <helgaas@kernel.org> wrote:

>>> [...]

>>> 

>>> OK, I have changed my DSDT as follows:

>>> 

>>> 

>>> Device (PCI0)

>>> {

>>>    Name (_ADR, 0x00)

>>>    Name (_HID, "PNP0A08" /* PCI Express Bus */)  // _HID: Hardware ID

>>>    Name (_CID, "PNP0A03" /* PCI Bus */)  // _CID: Compatible ID

>>>    Name (_SEG, 0x00)  // _SEG: PCI Segment

>>>    Name (_BBN, 0x00)  // _BBN: BIOS Bus Number

>>>    Name (_CCA, 0x01)  // _CCA: Cache Coherency Attribute

>>> 

>>>    Device (EXP1)

>>>    {

>>>        Name (_ADR, 0x20001)  // _ADR: Address

>>>        Name (_PRT, Package ()  // _PRT: PCI Routing Table

>>>        {

>>>            // slot 1: dev 2 fn 1

>>>            Package () { 0xFFFF, 0x0, 0x0, 0x140 },

>>>            Package () { 0xFFFF, 0x1, 0x0, 0x141 },

>>>            Package () { 0xFFFF, 0x2, 0x0, 0x142 },

>>>            Package () { 0xFFFF, 0x3, 0x0, 0x143 }

>>>        }) // _PRT

>>>    }

>>>    Device (EXP2)

>>>    {

>>>        Name (_ADR, 0x20002)  // _ADR: Address

>>>        Name (_PRT, Package ()  // _PRT: PCI Routing Table

>>>        {

>>>            // slot 2: dev 2 fn 2

>>>            Package () { 0xFFFF, 0x0, 0x0, 0x144 },

>>>            Package () { 0xFFFF, 0x1, 0x0, 0x145 },

>>>            Package () { 0xFFFF, 0x2, 0x0, 0x146 },

>>>            Package () { 0xFFFF, 0x3, 0x0, 0x147 }

>>>        }) // _PRT

>>>    }

>>>    Device (EXP3)

>>>    {

>>>        Name (_ADR, 0x20003)  // _ADR: Address

>>>        Name (_PRT, Package ()  // _PRT: PCI Routing Table

>>>        {

>>>            // slot 3: dev 2 fn 3

>>>            Package () { 0xFFFF, 0x0, 0x0, 0x148 },

>>>            Package () { 0xFFFF, 0x1, 0x0, 0x149 },

>>>            Package () { 0xFFFF, 0x2, 0x0, 0x14a },

>>>            Package () { 0xFFFF, 0x3, 0x0, 0x14b }

>>>        }) // _PRT

>>>    }

>>> 

>>> but it does not get picked up, and I am back to

>>> 

>>> [    3.357555] pcieport 0000:00:02.2: can't derive routing for PCI INT A

>>> [    3.370477] pcieport 0000:00:02.2: PCI INT A: no GSI

>>> [    3.380549] pcieport 0000:00:02.3: can't derive routing for PCI INT A

>>> [    3.393476] pcieport 0000:00:02.3: PCI INT A: no GSI

>>> 

>> 

>> OK, this does appear to work in fact, for the devices behind the

>> bridges. These messages are from the pcieport driver trying to request

>> an IRQ for the bridge device itself, which no longer works now that

>> the _PRT methods have been moved out.

>> 

>> So that mostly solves the problem. I'll try adding back a _PRT in the

>> PCI root device itself, but i'm not sure if I can make any inferences

>> about the IRQ wiring from the data i have.

> 

> Yes and IIUC with the $SUBJECT patch applied, the endpoints would match

> the _PRT entry under PNP0A08 through their bridge (PCIe port device = 2

> fn = X) device entry, not their own (device = 0) (ie first look-up in

> acpi_pci_irq_lookup() through acpi_pci_irq_find_prt_entry() should fail

> for the endpoints, then the same look-up is carried out with the bridge

> device instead and that succeeds, the _PRT entry routes IRQs for the

> PCIeport AND their downstream endpoint with $SUBJECT patch applied).

> 

> If you add the _PRT under the PCIe port itself (table above) the PCIe

> port can't find an ACPI parent device to route its own IRQx, that's my

> reading of what's going on, I will debug it next week.

> 


This is my understanding as well, and i think there remains little to debug on the kernel side.

Moving the three banks of legacy interrupts to separate companion devices is the correct thing to do, and works as expected for the devices behind the bridge.

The only thing that is lacking now is a _PRT description in the pci root that describes how INTA is wired for the bridges themselves. I am not sure how this matters, and whether this pci h/w ever asserts that interrupt in the first place, so i am just going to map it to 0x140 for now until someone tells me otherwise.


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index c576a6fe4ebb..caac4ac94418 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -160,6 +160,8 @@  static int acpi_pci_irq_check_entry(acpi_handle handle, struct pci_dev *dev,
 	struct acpi_prt_entry *entry;
 
 	if (((prt->address >> 16) & 0xffff) != device ||
+	    ((prt->address & 0xffff) != 0xffff &&
+	     (prt->address & 0xffff) != PCI_FUNC(dev->devfn)) ||
 	    prt->pin + 1 != pin)
 		return -ENODEV;