LinuxLists.cc - [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Tue, Sep 15, 2020 at 11:49 PM Zhiqiang Hou <[email protected]> wrote:
>
> From: Hou Zhiqiang <[email protected]>
>
> On NXP Layerscape platforms, it results in SError in the
> enumeration of the PCIe controller, which is not connecting
> with an Endpoint device. And it doesn't make sense to
> enumerate the Endpoints when the PCIe link is down. So this
> patch added the link up check to avoid to fire configuration
> transactions on link down bus.

Michael reported the same issue as well.

What happens if the link goes down between the check and the access?
It's a racy check. I'd like to find an alternative solution. It's even
worse if Layerscape is used in ECAM mode. I looked at the EDK2 setup
for layerscape[1] and it looks like root ports are just skipped if
link is down. Maybe a link down just never happens once up, but if so,
then we only need to check it once and fail probe.

I've dug into this a bit more and am curious about the PCIE_ABSERR
register setting which is set to:

#define PCIE_ABSERR_SETTING 0x9401 /* Forward error of non-posted request */

It seems to me this is not what we want at least for config accesses,
but commit 84d897d6993 where this was added seems to say otherwise. Is
it not possible to configure the response per access type?

This appears to be a standard DWC register as the tegra driver defines
this address as PORT_LOGIC_AMBA_ERROR_RESPONSE_DEFAULT. Perhaps
someone can shed some light on what this register contains.

Rob

[1] https://git.linaro.org/leg/noupstream/edk2-platforms.git/tree/Silicon/NXP/Library/PciHostBridgeLib/PciHostBridgeLib.c?h=developer-box#n756

>
> [ 0.807773] SError Interrupt on CPU2, code 0xbf000002 -- SError
> [ 0.807775] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> [ 0.807776] Hardware name: LS1046A RDB Board (DT)
> [ 0.807777] pstate: 20000085 (nzCv daIf -PAN -UAO BTYPE=--)
> [ 0.807778] pc : pci_generic_config_read+0x3c/0xe0
> [ 0.807778] lr : pci_generic_config_read+0x24/0xe0
> [ 0.807779] sp : ffff80001003b7b0
> [ 0.807780] x29: ffff80001003b7b0 x28: ffff80001003ba74
> [ 0.807782] x27: ffff000971d96800 x26: ffff00096e77e0a8
> [ 0.807784] x25: ffff80001003b874 x24: ffff80001003b924
> [ 0.807786] x23: 0000000000000004 x22: 0000000000000000
> [ 0.807788] x21: 0000000000000000 x20: ffff80001003b874
> [ 0.807790] x19: 0000000000000004 x18: ffffffffffffffff
> [ 0.807791] x17: 00000000000000c0 x16: fffffe0025981840
> [ 0.807793] x15: ffffb94c75b69948 x14: 62203a383634203a
> [ 0.807795] x13: 666e6f635f726568 x12: 202c31203d207265
> [ 0.807797] x11: 626d756e3e2d7375 x10: 656877202c307830
> [ 0.807799] x9 : 203d206e66766564 x8 : 0000000000000908
> [ 0.807801] x7 : 0000000000000908 x6 : ffff800010900000
> [ 0.807802] x5 : ffff00096e77e080 x4 : 0000000000000000
> [ 0.807804] x3 : 0000000000000003 x2 : 84fa3440ff7e7000
> [ 0.807806] x1 : 0000000000000000 x0 : ffff800010034000
> [ 0.807808] Kernel panic - not syncing: Asynchronous SError Interrupt
> [ 0.807809] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> [ 0.807810] Hardware name: LS1046A RDB Board (DT)
> [ 0.807811] Call trace:
> [ 0.807812] dump_backtrace+0x0/0x1c0
> [ 0.807813] show_stack+0x18/0x28
> [ 0.807814] dump_stack+0xd8/0x134
> [ 0.807814] panic+0x180/0x398
> [ 0.807815] add_taint+0x0/0xb0
> [ 0.807816] arm64_serror_panic+0x78/0x88
> [ 0.807817] do_serror+0x68/0x180
> [ 0.807818] el1_error+0x84/0x100
> [ 0.807818] pci_generic_config_read+0x3c/0xe0
> [ 0.807819] dw_pcie_rd_other_conf+0x78/0x110
> [ 0.807820] pci_bus_read_config_dword+0x88/0xe8
> [ 0.807821] pci_bus_generic_read_dev_vendor_id+0x30/0x1b0
> [ 0.807822] pci_bus_read_dev_vendor_id+0x4c/0x78
> [ 0.807823] pci_scan_single_device+0x80/0x100
> [ 0.807824] pci_scan_slot+0x38/0x130
> [ 0.807825] pci_scan_child_bus_extend+0x54/0x2a0
> [ 0.807826] pci_scan_child_bus+0x14/0x20
> [ 0.807827] pci_scan_bridge_extend+0x230/0x570
> [ 0.807828] pci_scan_child_bus_extend+0x134/0x2a0
> [ 0.807829] pci_scan_root_bus_bridge+0x64/0xf0
> [ 0.807829] pci_host_probe+0x18/0xc8
> [ 0.807830] dw_pcie_host_init+0x220/0x378
> [ 0.807831] ls_pcie_probe+0x104/0x140
> [ 0.807832] platform_drv_probe+0x54/0xa8
> [ 0.807833] really_probe+0x118/0x3e0
> [ 0.807834] driver_probe_device+0x5c/0xc0
> [ 0.807835] device_driver_attach+0x74/0x80
> [ 0.807835] __driver_attach+0x8c/0xd8
> [ 0.807836] bus_for_each_dev+0x7c/0xd8
> [ 0.807837] driver_attach+0x24/0x30
> [ 0.807838] bus_add_driver+0x154/0x200
> [ 0.807839] driver_register+0x64/0x120
> [ 0.807839] __platform_driver_probe+0x7c/0x148
> [ 0.807840] ls_pcie_driver_init+0x24/0x30
> [ 0.807841] do_one_initcall+0x60/0x1d8
> [ 0.807842] kernel_init_freeable+0x1f4/0x24c
> [ 0.807843] kernel_init+0x14/0x118
> [ 0.807843] ret_from_fork+0x10/0x34
> [ 0.807854] SMP: stopping secondary CPUs
> [ 0.807855] Kernel Offset: 0x394c64080000 from 0xffff800010000000
> [ 0.807856] PHYS_OFFSET: 0xffff8bfd40000000
> [ 0.807856] CPU features: 0x0240022,21806000
> [ 0.807857] Memory Limit: none
>
> Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
> Signed-off-by: Hou Zhiqiang <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-designware-host.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index c01c9d2fb3f9..e82b518430c5 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -442,6 +442,9 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> struct pcie_port *pp = bus->sysdata;
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>
> + if (!dw_pcie_link_up(pci))
> + return NULL;
> +
> busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> PCIE_ATU_FUNC(PCI_FUNC(devfn));
>
> --
> 2.17.1
>

2020-09-18 11:04:04

[permalink] [raw]

Subject: RE: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

Hi Rob,

Thanks a lot for your comments!

> -----Original Message-----
> From: Rob Herring <[email protected]>
> Sent: 2020年9月17日 4:29
> To: Z.q. Hou <[email protected]>
> Cc: [email protected]; PCI <[email protected]>; Lorenzo
> Pieralisi <[email protected]>; Bjorn Helgaas
> <[email protected]>; Gustavo Pimentel
> <[email protected]>; Michael Walle <[email protected]>;
> Ard Biesheuvel <[email protected]>
> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> dw_child_pcie_ops
>
> On Tue, Sep 15, 2020 at 11:49 PM Zhiqiang Hou <[email protected]>
> wrote:
> >
> > From: Hou Zhiqiang <[email protected]>
> >
> > On NXP Layerscape platforms, it results in SError in the enumeration
> > of the PCIe controller, which is not connecting with an Endpoint
> > device. And it doesn't make sense to enumerate the Endpoints when the
> > PCIe link is down. So this patch added the link up check to avoid to
> > fire configuration transactions on link down bus.
>
> Michael reported the same issue as well.
>
> What happens if the link goes down between the check and the access?

This patch cannot cover this case, and will get the SError.
But I think it makes sense to avoid firing transactions on link down bus.

> It's a racy check. I'd like to find an alternative solution. It's even worse if
> Layerscape is used in ECAM mode. I looked at the EDK2 setup for
> layerscape[1] and it looks like root ports are just skipped if link is down.
> Maybe a link down just never happens once up, but if so, then we only need
> to check it once and fail probe.

Many customers connect the FPGA Endpoint, which may establish PCIe link
after the PCIe enumeration and then rescan the PCIe bus, so I think it should
not exit the probe of root port even if there is not link up during enumeration.

>
> I've dug into this a bit more and am curious about the PCIE_ABSERR register
> setting which is set to:
>
> #define PCIE_ABSERR_SETTING 0x9401 /* Forward error of non-posted
> request */
>
> It seems to me this is not what we want at least for config accesses, but
> commit 84d897d6993 where this was added seems to say otherwise. Is it not
> possible to configure the response per access type?

Thanks a lot for your investigation!
The story is like this: Some customers worry about these silent error (DWC PCIe
IP won't forward the error of outbound non-post request by default), so we
were pushed to enable the error forwarding to AXI in the commit
84d897d6993 as you saw. But it cannot differentiate the config transactions
from the MEM_rd, except the Vendor ID access, which is controlled by
a separate bit and it was set to not forward error of access of Vendor ID.
So we think it's okay to enable the error forwarding, the SError should not
occur, because after the enumeration it won't access the non-existent functions.

But now the SError is exactly caused by the first access of the non-existent
function, I dug into the kernel enumeration code and found it will fire a 4Byte
CFG read transaction to read the Vendor ID and Device ID together, so I suspect
the root cause is access the Device ID of a non-existent function triggers SError.

So the alternative solution seems to correct the PCIe enumeration, I will submit
a patch to let the first access only read the Vendor ID.

Thanks,
Zhiqiang

>
> This appears to be a standard DWC register as the tegra driver defines this
> address as PORT_LOGIC_AMBA_ERROR_RESPONSE_DEFAULT. Perhaps
> someone can shed some light on what this register contains.
>
> Rob
>
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.lin
> aro.org%2Fleg%2Fnoupstream%2Fedk2-platforms.git%2Ftree%2FSilicon%2F
> NXP%2FLibrary%2FPciHostBridgeLib%2FPciHostBridgeLib.c%3Fh%3Ddevelop
> er-box%23n756&data=02%7C01%7CZhiqiang.Hou%40nxp.com%7Cfa38
> f3e83d0c49983e0c08d85a7f2d5e%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C637358849549791506&sdata=bJ3uDUm%2FT%2FzC6qz
> GX7NNHQZdDxNFZ%2BBtcEQ3sLQIE4M%3D&reserved=0
>
>
> >
> > [ 0.807773] SError Interrupt on CPU2, code 0xbf000002 -- SError
> > [ 0.807775] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
> 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> > [ 0.807776] Hardware name: LS1046A RDB Board (DT)
> > [ 0.807777] pstate: 20000085 (nzCv daIf -PAN -UAO BTYPE=--)
> > [ 0.807778] pc : pci_generic_config_read+0x3c/0xe0
> > [ 0.807778] lr : pci_generic_config_read+0x24/0xe0
> > [ 0.807779] sp : ffff80001003b7b0
> > [ 0.807780] x29: ffff80001003b7b0 x28: ffff80001003ba74
> > [ 0.807782] x27: ffff000971d96800 x26: ffff00096e77e0a8
> > [ 0.807784] x25: ffff80001003b874 x24: ffff80001003b924
> > [ 0.807786] x23: 0000000000000004 x22: 0000000000000000
> > [ 0.807788] x21: 0000000000000000 x20: ffff80001003b874
> > [ 0.807790] x19: 0000000000000004 x18: ffffffffffffffff
> > [ 0.807791] x17: 00000000000000c0 x16: fffffe0025981840
> > [ 0.807793] x15: ffffb94c75b69948 x14: 62203a383634203a
> > [ 0.807795] x13: 666e6f635f726568 x12: 202c31203d207265
> > [ 0.807797] x11: 626d756e3e2d7375 x10: 656877202c307830
> > [ 0.807799] x9 : 203d206e66766564 x8 : 0000000000000908
> > [ 0.807801] x7 : 0000000000000908 x6 : ffff800010900000
> > [ 0.807802] x5 : ffff00096e77e080 x4 : 0000000000000000
> > [ 0.807804] x3 : 0000000000000003 x2 : 84fa3440ff7e7000
> > [ 0.807806] x1 : 0000000000000000 x0 : ffff800010034000
> > [ 0.807808] Kernel panic - not syncing: Asynchronous SError Interrupt
> > [ 0.807809] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
> 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> > [ 0.807810] Hardware name: LS1046A RDB Board (DT)
> > [ 0.807811] Call trace:
> > [ 0.807812] dump_backtrace+0x0/0x1c0
> > [ 0.807813] show_stack+0x18/0x28
> > [ 0.807814] dump_stack+0xd8/0x134
> > [ 0.807814] panic+0x180/0x398
> > [ 0.807815] add_taint+0x0/0xb0
> > [ 0.807816] arm64_serror_panic+0x78/0x88
> > [ 0.807817] do_serror+0x68/0x180
> > [ 0.807818] el1_error+0x84/0x100
> > [ 0.807818] pci_generic_config_read+0x3c/0xe0
> > [ 0.807819] dw_pcie_rd_other_conf+0x78/0x110
> > [ 0.807820] pci_bus_read_config_dword+0x88/0xe8
> > [ 0.807821] pci_bus_generic_read_dev_vendor_id+0x30/0x1b0
> > [ 0.807822] pci_bus_read_dev_vendor_id+0x4c/0x78
> > [ 0.807823] pci_scan_single_device+0x80/0x100
> > [ 0.807824] pci_scan_slot+0x38/0x130
> > [ 0.807825] pci_scan_child_bus_extend+0x54/0x2a0
> > [ 0.807826] pci_scan_child_bus+0x14/0x20
> > [ 0.807827] pci_scan_bridge_extend+0x230/0x570
> > [ 0.807828] pci_scan_child_bus_extend+0x134/0x2a0
> > [ 0.807829] pci_scan_root_bus_bridge+0x64/0xf0
> > [ 0.807829] pci_host_probe+0x18/0xc8
> > [ 0.807830] dw_pcie_host_init+0x220/0x378
> > [ 0.807831] ls_pcie_probe+0x104/0x140
> > [ 0.807832] platform_drv_probe+0x54/0xa8
> > [ 0.807833] really_probe+0x118/0x3e0
> > [ 0.807834] driver_probe_device+0x5c/0xc0
> > [ 0.807835] device_driver_attach+0x74/0x80
> > [ 0.807835] __driver_attach+0x8c/0xd8
> > [ 0.807836] bus_for_each_dev+0x7c/0xd8
> > [ 0.807837] driver_attach+0x24/0x30
> > [ 0.807838] bus_add_driver+0x154/0x200
> > [ 0.807839] driver_register+0x64/0x120
> > [ 0.807839] __platform_driver_probe+0x7c/0x148
> > [ 0.807840] ls_pcie_driver_init+0x24/0x30
> > [ 0.807841] do_one_initcall+0x60/0x1d8
> > [ 0.807842] kernel_init_freeable+0x1f4/0x24c
> > [ 0.807843] kernel_init+0x14/0x118
> > [ 0.807843] ret_from_fork+0x10/0x34
> > [ 0.807854] SMP: stopping secondary CPUs
> > [ 0.807855] Kernel Offset: 0x394c64080000 from 0xffff800010000000
> > [ 0.807856] PHYS_OFFSET: 0xffff8bfd40000000
> > [ 0.807856] CPU features: 0x0240022,21806000
> > [ 0.807857] Memory Limit: none
> >
> > Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
> > Signed-off-by: Hou Zhiqiang <[email protected]>
> > ---
> > drivers/pci/controller/dwc/pcie-designware-host.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index c01c9d2fb3f9..e82b518430c5 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -442,6 +442,9 @@ static void __iomem
> *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> > struct pcie_port *pp = bus->sysdata;
> > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >
> > + if (!dw_pcie_link_up(pci))
> > + return NULL;
> > +
> > busdev = PCIE_ATU_BUS(bus->number) |
> PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> > PCIE_ATU_FUNC(PCI_FUNC(devfn));
> >
> > --
> > 2.17.1
> >

2020-09-18 11:16:01

by Michael Walle

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

Hi Zhiqiang,

> So the alternative solution seems to correct the PCIe enumeration, I
> will submit
> a patch to let the first access only read the Vendor ID.

Please put me on CC of that patch.

Thanks,
-michael

2020-09-18 12:49:28

by Bjorn Helgaas

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Fri, Sep 18, 2020 at 11:02:07AM +0000, Z.q. Hou wrote:

> But now the SError is exactly caused by the first access of the
> non-existent function, I dug into the kernel enumeration code and
> found it will fire a 4Byte CFG read transaction to read the Vendor
> ID and Device ID together, so I suspect the root cause is access the
> Device ID of a non-existent function triggers SError.
>
> So the alternative solution seems to correct the PCIe enumeration, I
> will submit a patch to let the first access only read the Vendor ID.

If it is incorrect for the first access to be a 32-bit read of both
the Vendor and the Device ID, please cite the relevant section of the
spec in your patch.

I don't like to make changes to generic code to accommodate specific
pieces of hardware because then we restrict future changes based on
some device that will soon be obsolete and forgotten.

I'm pretty sure the spec language about CRS handling is careful to
talk about "reads that *include* Vendor ID", not just "reads of Vendor
ID", so the implication is that it covers 32-bit reads as well as
16-bit reads.

Bjorn

2020-09-18 15:29:50

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Fri, Sep 18, 2020 at 5:02 AM Z.q. Hou <[email protected]> wrote:
>
> Hi Rob,
>
> Thanks a lot for your comments!
>
> > -----Original Message-----
> > From: Rob Herring <[email protected]>
> > Sent: 2020年9月17日 4:29
> > To: Z.q. Hou <[email protected]>
> > Cc: [email protected]; PCI <[email protected]>; Lorenzo
> > Pieralisi <[email protected]>; Bjorn Helgaas
> > <[email protected]>; Gustavo Pimentel
> > <[email protected]>; Michael Walle <[email protected]>;
> > Ard Biesheuvel <[email protected]>
> > Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> > dw_child_pcie_ops
> >
> > On Tue, Sep 15, 2020 at 11:49 PM Zhiqiang Hou <[email protected]>
> > wrote:
> > >
> > > From: Hou Zhiqiang <[email protected]>
> > >
> > > On NXP Layerscape platforms, it results in SError in the enumeration
> > > of the PCIe controller, which is not connecting with an Endpoint
> > > device. And it doesn't make sense to enumerate the Endpoints when the
> > > PCIe link is down. So this patch added the link up check to avoid to
> > > fire configuration transactions on link down bus.
> >
> > Michael reported the same issue as well.
> >
> > What happens if the link goes down between the check and the access?
>
> This patch cannot cover this case, and will get the SError.
> But I think it makes sense to avoid firing transactions on link down bus.

That's impossible to do without a race even in h/w.

> > It's a racy check. I'd like to find an alternative solution. It's even worse if
> > Layerscape is used in ECAM mode. I looked at the EDK2 setup for
> > layerscape[1] and it looks like root ports are just skipped if link is down.
> > Maybe a link down just never happens once up, but if so, then we only need
> > to check it once and fail probe.
>
> Many customers connect the FPGA Endpoint, which may establish PCIe link
> after the PCIe enumeration and then rescan the PCIe bus, so I think it should
> not exit the probe of root port even if there is not link up during enumeration.

That's a good reason. I want to unify the behavior here as it varies
per platform currently and wasn't sure which way to go.

> > I've dug into this a bit more and am curious about the PCIE_ABSERR register
> > setting which is set to:
> >
> > #define PCIE_ABSERR_SETTING 0x9401 /* Forward error of non-posted
> > request */
> >
> > It seems to me this is not what we want at least for config accesses, but
> > commit 84d897d6993 where this was added seems to say otherwise. Is it not
> > possible to configure the response per access type?
>
> Thanks a lot for your investigation!
> The story is like this: Some customers worry about these silent error (DWC PCIe
> IP won't forward the error of outbound non-post request by default), so we
> were pushed to enable the error forwarding to AXI in the commit
> 84d897d6993 as you saw. But it cannot differentiate the config transactions
> from the MEM_rd, except the Vendor ID access, which is controlled by
> a separate bit and it was set to not forward error of access of Vendor ID.
> So we think it's okay to enable the error forwarding, the SError should not
> occur, because after the enumeration it won't access the non-existent functions.

We've rejected upstream support for platforms aborting on config
accesses[1]. I think there's clear consensus that aborting is the
wrong behavior.

Do MEM_wr errors get forwarded? Seems like that would be enough. Also,
wouldn't page faults catch most OOB accesses anyways? You need things
page aligned anyways with an IOMMU and doing userspace access or guest
assignment.

Here's another idea, how about only enabling forwarding errors if the
link is up? If really would need to be configured any time the link
state changes rather than just at probe. I'm not sure if you have a
way to disable it on link down though.

Rob

2020-09-21 14:51:57

[permalink] [raw]

Subject: RE: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

Hi Michael,

Thanks a lot for your comments!

> -----Original Message-----
> From: Michael Walle <[email protected]>
> Sent: 2020??9??18?? 19:14
> To: Z.q. Hou <[email protected]>
> Cc: Rob Herring <[email protected]>; [email protected]; PCI
> <[email protected]>; Lorenzo Pieralisi <[email protected]>;
> Bjorn Helgaas <[email protected]>; Gustavo Pimentel
> <[email protected]>; Ard Biesheuvel <[email protected]>
> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> dw_child_pcie_ops
>
> Hi Zhiqiang,
>
> > So the alternative solution seems to correct the PCIe enumeration, I
> > will submit a patch to let the first access only read the Vendor ID.
>
> Please put me on CC of that patch.

Saw more comments on this, I'll discuss more with Rob and Bjorn, and must act prudently.

Regards,
Zhiqiang

>
> Thanks,
> -michael

2020-09-21 16:32:54

[permalink] [raw]

Subject: RE: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

Hi Bjorn,

Thanks a lot for your comments!

> -----Original Message-----
> From: Bjorn Helgaas <[email protected]>
> Sent: 2020??9??18?? 20:47
> To: Z.q. Hou <[email protected]>
> Cc: Rob Herring <[email protected]>; [email protected]; PCI
> <[email protected]>; Lorenzo Pieralisi <[email protected]>;
> Bjorn Helgaas <[email protected]>; Gustavo Pimentel
> <[email protected]>; Michael Walle <[email protected]>;
> Ard Biesheuvel <[email protected]>
> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> dw_child_pcie_ops
>
> On Fri, Sep 18, 2020 at 11:02:07AM +0000, Z.q. Hou wrote:
>
> > But now the SError is exactly caused by the first access of the
> > non-existent function, I dug into the kernel enumeration code and
> > found it will fire a 4Byte CFG read transaction to read the Vendor ID
> > and Device ID together, so I suspect the root cause is access the
> > Device ID of a non-existent function triggers SError.
> >
> > So the alternative solution seems to correct the PCIe enumeration, I
> > will submit a patch to let the first access only read the Vendor ID.
>
> If it is incorrect for the first access to be a 32-bit read of both the Vendor
> and the Device ID, please cite the relevant section of the spec in your patch.
>
> I don't like to make changes to generic code to accommodate specific pieces
> of hardware because then we restrict future changes based on some device
> that will soon be obsolete and forgotten.
>
> I'm pretty sure the spec language about CRS handling is careful to talk about
> "reads that *include* Vendor ID", not just "reads of Vendor ID", so the
> implication is that it covers 32-bit reads as well as 16-bit reads.
>

Yes, I agree with you that we must be carful with the generic code.
NXP Layerscape SError aside, it turns out to be more complex, limiting the first CFG access to 16-bit Vendor ID also causes SError, the hardware behavior seems not the same as the described of the register PCIE_ABSERR.

For the PCIe enumeration, I found the descriptions of Vendor ID and Device ID in the PCI Express Base Specification, Rev. 4.0 Version 1.0 (pasted below), it recommends to read Vendor ID to determine the presentence of a function and use the Device ID (with Vendor ID and Revision ID) to determine the driver needed. But in section 2.3.2 Completion Handling Rules, it seems, as you said, not limit to 16-bit Vendor ID, so I want to hear your and Rob's suggestion on this change.

7.5.1.1.1 Vendor ID Register (Offset 00h)
The Vendor ID register is HwInit and the value in this register identifies the manufacturer of the
Function. In keeping with PCI-SIG procedures, valid vendor identifiers must be allocated by the
PCI-SIG to ensure uniqueness. Each vendor must have at least one Vendor ID. It is recommended
that software read the Vendor ID register to determine if a Function is present, where a value of
FFFFh indicates that no Function is present.
7.5.1.1.2 Device ID Register (Offset 02h)
The Device ID register is HwInit and the value in this register identifies the particular Function.
The Device ID must be allocated by the vendor. The Device ID, in conjunction with the Vendor ID
and Revision ID, are used as one mechanism for software to determine which driver should be
loaded. The vendor must ensure that the chosen values do not result in the use of an incompatible
device driver.

Regards,
Zhiqiang

> Bjorn

2020-09-24 04:28:55

[permalink] [raw]

Subject: RE: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

Hi Rob,

Thanks a lot for your comments!

> -----Original Message-----
> From: Rob Herring <[email protected]>
> Sent: 2020年9月18日 23:28
> To: Z.q. Hou <[email protected]>
> Cc: [email protected]; PCI <[email protected]>; Lorenzo
> Pieralisi <[email protected]>; Bjorn Helgaas
> <[email protected]>; Gustavo Pimentel
> <[email protected]>; Michael Walle <[email protected]>;
> Ard Biesheuvel <[email protected]>
> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> dw_child_pcie_ops
>
> On Fri, Sep 18, 2020 at 5:02 AM Z.q. Hou <[email protected]> wrote:
> >
> > Hi Rob,
> >
> > Thanks a lot for your comments!
> >
> > > -----Original Message-----
> > > From: Rob Herring <[email protected]>
> > > Sent: 2020年9月17日 4:29
> > > To: Z.q. Hou <[email protected]>
> > > Cc: [email protected]; PCI <[email protected]>;
> > > Lorenzo Pieralisi <[email protected]>; Bjorn Helgaas
> > > <[email protected]>; Gustavo Pimentel
> > > <[email protected]>; Michael Walle
> <[email protected]>;
> > > Ard Biesheuvel <[email protected]>
> > > Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> > > dw_child_pcie_ops
> > >
> > > On Tue, Sep 15, 2020 at 11:49 PM Zhiqiang Hou
> <[email protected]>
> > > wrote:
> > > >
> > > > From: Hou Zhiqiang <[email protected]>
> > > >
> > > > On NXP Layerscape platforms, it results in SError in the
> > > > enumeration of the PCIe controller, which is not connecting with
> > > > an Endpoint device. And it doesn't make sense to enumerate the
> > > > Endpoints when the PCIe link is down. So this patch added the link
> > > > up check to avoid to fire configuration transactions on link down bus.
> > >
> > > Michael reported the same issue as well.
> > >
> > > What happens if the link goes down between the check and the access?
> >
> > This patch cannot cover this case, and will get the SError.
> > But I think it makes sense to avoid firing transactions on link down bus.
>
> That's impossible to do without a race even in h/w.

Agree.

>
> > > It's a racy check. I'd like to find an alternative solution. It's
> > > even worse if Layerscape is used in ECAM mode. I looked at the EDK2
> > > setup for layerscape[1] and it looks like root ports are just skipped if link
> is down.
> > > Maybe a link down just never happens once up, but if so, then we
> > > only need to check it once and fail probe.
> >
> > Many customers connect the FPGA Endpoint, which may establish PCIe
> > link after the PCIe enumeration and then rescan the PCIe bus, so I
> > think it should not exit the probe of root port even if there is not link up
> during enumeration.
>
> That's a good reason. I want to unify the behavior here as it varies per
> platform currently and wasn't sure which way to go.
>
>
> > > I've dug into this a bit more and am curious about the PCIE_ABSERR
> > > register setting which is set to:
> > >
> > > #define PCIE_ABSERR_SETTING 0x9401 /* Forward error of non-posted
> > > request */
> > >
> > > It seems to me this is not what we want at least for config
> > > accesses, but commit 84d897d6993 where this was added seems to say
> > > otherwise. Is it not possible to configure the response per access type?
> >
> > Thanks a lot for your investigation!
> > The story is like this: Some customers worry about these silent error
> > (DWC PCIe IP won't forward the error of outbound non-post request by
> > default), so we were pushed to enable the error forwarding to AXI in
> > the commit
> > 84d897d6993 as you saw. But it cannot differentiate the config
> > transactions from the MEM_rd, except the Vendor ID access, which is
> > controlled by a separate bit and it was set to not forward error of access
> of Vendor ID.
> > So we think it's okay to enable the error forwarding, the SError
> > should not occur, because after the enumeration it won't access the
> non-existent functions.
>
> We've rejected upstream support for platforms aborting on config
> accesses[1]. I think there's clear consensus that aborting is the wrong
> behavior.
>
> Do MEM_wr errors get forwarded? Seems like that would be enough. Also,
> wouldn't page faults catch most OOB accesses anyways? You need things
> page aligned anyways with an IOMMU and doing userspace access or guest
> assignment.

Yes, errors of MEM_wr can be forwarded.

>
> Here's another idea, how about only enabling forwarding errors if the link is
> up? If really would need to be configured any time the link state changes
> rather than just at probe. I'm not sure if you have a way to disable it on link
> down though.

Dug deeper into this issue and found the setting of not forwarding error of non-existent Vender ID access counts on the link partner:
1. When there is a link partner (namely link up), it will return 0xffff when read non-existent function Vendor ID and won't forward error to AXI.
2. When no link partner (link down), it will forward the error of reading non-existent function Vendor ID to AXI and result in SError.

I think this is a DWC PCIe IP specific issue but not get feedback from design team.
I'm thinking to disable this error forwarding just like other platforms, since when these errors (UR, CA and CT) are detected, AER driver can also report the error and try to recover.

Thanks,
Zhiqiang

>
> Rob

2020-09-28 09:40:33

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Thu, Sep 24, 2020 at 04:24:47AM +0000, Z.q. Hou wrote:
> Hi Rob,
>
> Thanks a lot for your comments!
>
> > -----Original Message-----
> > From: Rob Herring <[email protected]>
> > Sent: 2020年9月18日 23:28
> > To: Z.q. Hou <[email protected]>
> > Cc: [email protected]; PCI <[email protected]>; Lorenzo
> > Pieralisi <[email protected]>; Bjorn Helgaas
> > <[email protected]>; Gustavo Pimentel
> > <[email protected]>; Michael Walle <[email protected]>;
> > Ard Biesheuvel <[email protected]>
> > Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> > dw_child_pcie_ops
> >
> > On Fri, Sep 18, 2020 at 5:02 AM Z.q. Hou <[email protected]> wrote:
> > >
> > > Hi Rob,
> > >
> > > Thanks a lot for your comments!
> > >
> > > > -----Original Message-----
> > > > From: Rob Herring <[email protected]>
> > > > Sent: 2020年9月17日 4:29
> > > > To: Z.q. Hou <[email protected]>
> > > > Cc: [email protected]; PCI <[email protected]>;
> > > > Lorenzo Pieralisi <[email protected]>; Bjorn Helgaas
> > > > <[email protected]>; Gustavo Pimentel
> > > > <[email protected]>; Michael Walle
> > <[email protected]>;
> > > > Ard Biesheuvel <[email protected]>
> > > > Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> > > > dw_child_pcie_ops
> > > >
> > > > On Tue, Sep 15, 2020 at 11:49 PM Zhiqiang Hou
> > <[email protected]>
> > > > wrote:
> > > > >
> > > > > From: Hou Zhiqiang <[email protected]>
> > > > >
> > > > > On NXP Layerscape platforms, it results in SError in the
> > > > > enumeration of the PCIe controller, which is not connecting with
> > > > > an Endpoint device. And it doesn't make sense to enumerate the
> > > > > Endpoints when the PCIe link is down. So this patch added the link
> > > > > up check to avoid to fire configuration transactions on link down bus.
> > > >
> > > > Michael reported the same issue as well.
> > > >
> > > > What happens if the link goes down between the check and the access?
> > >
> > > This patch cannot cover this case, and will get the SError.
> > > But I think it makes sense to avoid firing transactions on link down bus.
> >
> > That's impossible to do without a race even in h/w.
>
> Agree.
>
> >
> > > > It's a racy check. I'd like to find an alternative solution. It's
> > > > even worse if Layerscape is used in ECAM mode. I looked at the EDK2
> > > > setup for layerscape[1] and it looks like root ports are just skipped if link
> > is down.
> > > > Maybe a link down just never happens once up, but if so, then we
> > > > only need to check it once and fail probe.
> > >
> > > Many customers connect the FPGA Endpoint, which may establish PCIe
> > > link after the PCIe enumeration and then rescan the PCIe bus, so I
> > > think it should not exit the probe of root port even if there is not link up
> > during enumeration.
> >
> > That's a good reason. I want to unify the behavior here as it varies per
> > platform currently and wasn't sure which way to go.
> >
> >
> > > > I've dug into this a bit more and am curious about the PCIE_ABSERR
> > > > register setting which is set to:
> > > >
> > > > #define PCIE_ABSERR_SETTING 0x9401 /* Forward error of non-posted
> > > > request */
> > > >
> > > > It seems to me this is not what we want at least for config
> > > > accesses, but commit 84d897d6993 where this was added seems to say
> > > > otherwise. Is it not possible to configure the response per access type?
> > >
> > > Thanks a lot for your investigation!
> > > The story is like this: Some customers worry about these silent error
> > > (DWC PCIe IP won't forward the error of outbound non-post request by
> > > default), so we were pushed to enable the error forwarding to AXI in
> > > the commit
> > > 84d897d6993 as you saw. But it cannot differentiate the config
> > > transactions from the MEM_rd, except the Vendor ID access, which is
> > > controlled by a separate bit and it was set to not forward error of access
> > of Vendor ID.
> > > So we think it's okay to enable the error forwarding, the SError
> > > should not occur, because after the enumeration it won't access the
> > non-existent functions.
> >
> > We've rejected upstream support for platforms aborting on config
> > accesses[1]. I think there's clear consensus that aborting is the wrong
> > behavior.
> >
> > Do MEM_wr errors get forwarded? Seems like that would be enough. Also,
> > wouldn't page faults catch most OOB accesses anyways? You need things
> > page aligned anyways with an IOMMU and doing userspace access or guest
> > assignment.
>
> Yes, errors of MEM_wr can be forwarded.
>
> >
> > Here's another idea, how about only enabling forwarding errors if the link is
> > up? If really would need to be configured any time the link state changes
> > rather than just at probe. I'm not sure if you have a way to disable it on link
> > down though.
>
> Dug deeper into this issue and found the setting of not forwarding
> error of non-existent Vender ID access counts on the link partner: 1.
> When there is a link partner (namely link up), it will return 0xffff
> when read non-existent function Vendor ID and won't forward error to
> AXI. 2. When no link partner (link down), it will forward the error
> of reading non-existent function Vendor ID to AXI and result in
> SError.
>
> I think this is a DWC PCIe IP specific issue but not get feedback from
> design team. I'm thinking to disable this error forwarding just like
> other platforms, since when these errors (UR, CA and CT) are detected,
> AER driver can also report the error and try to recover.

I take this as you shall send a patch to fix this issue shortly,
is this correct ?

Thanks,
Lorenzo

2020-09-29 04:08:32

[permalink] [raw]

Subject: RE: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

Hi Lorenzo,

Thanks a lot for your comments!

> -----Original Message-----
> From: Lorenzo Pieralisi <[email protected]>
> Sent: 2020年9月28日 17:39
> To: Z.q. Hou <[email protected]>
> Cc: Rob Herring <[email protected]>; [email protected]; PCI
> <[email protected]>; Bjorn Helgaas <[email protected]>;
> Gustavo Pimentel <[email protected]>; Michael Walle
> <[email protected]>; Ard Biesheuvel <[email protected]>
> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> dw_child_pcie_ops
>
> On Thu, Sep 24, 2020 at 04:24:47AM +0000, Z.q. Hou wrote:
> > Hi Rob,
> >
> > Thanks a lot for your comments!
> >
> > > -----Original Message-----
> > > From: Rob Herring <[email protected]>
> > > Sent: 2020年9月18日 23:28
> > > To: Z.q. Hou <[email protected]>
> > > Cc: [email protected]; PCI <[email protected]>;
> > > Lorenzo Pieralisi <[email protected]>; Bjorn Helgaas
> > > <[email protected]>; Gustavo Pimentel
> > > <[email protected]>; Michael Walle
> <[email protected]>;
> > > Ard Biesheuvel <[email protected]>
> > > Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> > > dw_child_pcie_ops
> > >
> > > On Fri, Sep 18, 2020 at 5:02 AM Z.q. Hou <[email protected]>
> wrote:
> > > >
> > > > Hi Rob,
> > > >
> > > > Thanks a lot for your comments!
> > > >
> > > > > -----Original Message-----
> > > > > From: Rob Herring <[email protected]>
> > > > > Sent: 2020年9月17日 4:29
> > > > > To: Z.q. Hou <[email protected]>
> > > > > Cc: [email protected]; PCI
> > > > > <[email protected]>; Lorenzo Pieralisi
> > > > > <[email protected]>; Bjorn Helgaas
> > > > > <[email protected]>; Gustavo Pimentel
> > > > > <[email protected]>; Michael Walle
> > > <[email protected]>;
> > > > > Ard Biesheuvel <[email protected]>
> > > > > Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> > > > > dw_child_pcie_ops
> > > > >
> > > > > On Tue, Sep 15, 2020 at 11:49 PM Zhiqiang Hou
> > > <[email protected]>
> > > > > wrote:
> > > > > >
> > > > > > From: Hou Zhiqiang <[email protected]>
> > > > > >
> > > > > > On NXP Layerscape platforms, it results in SError in the
> > > > > > enumeration of the PCIe controller, which is not connecting
> > > > > > with an Endpoint device. And it doesn't make sense to
> > > > > > enumerate the Endpoints when the PCIe link is down. So this
> > > > > > patch added the link up check to avoid to fire configuration
> transactions on link down bus.
> > > > >
> > > > > Michael reported the same issue as well.
> > > > >
> > > > > What happens if the link goes down between the check and the
> access?
> > > >
> > > > This patch cannot cover this case, and will get the SError.
> > > > But I think it makes sense to avoid firing transactions on link down bus.
> > >
> > > That's impossible to do without a race even in h/w.
> >
> > Agree.
> >
> > >
> > > > > It's a racy check. I'd like to find an alternative solution.
> > > > > It's even worse if Layerscape is used in ECAM mode. I looked at
> > > > > the EDK2 setup for layerscape[1] and it looks like root ports
> > > > > are just skipped if link
> > > is down.
> > > > > Maybe a link down just never happens once up, but if so, then we
> > > > > only need to check it once and fail probe.
> > > >
> > > > Many customers connect the FPGA Endpoint, which may establish PCIe
> > > > link after the PCIe enumeration and then rescan the PCIe bus, so I
> > > > think it should not exit the probe of root port even if there is
> > > > not link up
> > > during enumeration.
> > >
> > > That's a good reason. I want to unify the behavior here as it varies
> > > per platform currently and wasn't sure which way to go.
> > >
> > >
> > > > > I've dug into this a bit more and am curious about the
> > > > > PCIE_ABSERR register setting which is set to:
> > > > >
> > > > > #define PCIE_ABSERR_SETTING 0x9401 /* Forward error of
> > > > > non-posted request */
> > > > >
> > > > > It seems to me this is not what we want at least for config
> > > > > accesses, but commit 84d897d6993 where this was added seems to
> > > > > say otherwise. Is it not possible to configure the response per access
> type?
> > > >
> > > > Thanks a lot for your investigation!
> > > > The story is like this: Some customers worry about these silent
> > > > error (DWC PCIe IP won't forward the error of outbound non-post
> > > > request by default), so we were pushed to enable the error
> > > > forwarding to AXI in the commit
> > > > 84d897d6993 as you saw. But it cannot differentiate the config
> > > > transactions from the MEM_rd, except the Vendor ID access, which
> > > > is controlled by a separate bit and it was set to not forward
> > > > error of access
> > > of Vendor ID.
> > > > So we think it's okay to enable the error forwarding, the SError
> > > > should not occur, because after the enumeration it won't access
> > > > the
> > > non-existent functions.
> > >
> > > We've rejected upstream support for platforms aborting on config
> > > accesses[1]. I think there's clear consensus that aborting is the
> > > wrong behavior.
> > >
> > > Do MEM_wr errors get forwarded? Seems like that would be enough.
> > > Also, wouldn't page faults catch most OOB accesses anyways? You need
> > > things page aligned anyways with an IOMMU and doing userspace access
> > > or guest assignment.
> >
> > Yes, errors of MEM_wr can be forwarded.
> >
> > >
> > > Here's another idea, how about only enabling forwarding errors if
> > > the link is up? If really would need to be configured any time the
> > > link state changes rather than just at probe. I'm not sure if you
> > > have a way to disable it on link down though.
> >
> > Dug deeper into this issue and found the setting of not forwarding
> > error of non-existent Vender ID access counts on the link partner: 1.
> > When there is a link partner (namely link up), it will return 0xffff
> > when read non-existent function Vendor ID and won't forward error to
> > AXI. 2. When no link partner (link down), it will forward the error
> > of reading non-existent function Vendor ID to AXI and result in
> > SError.
> >
> > I think this is a DWC PCIe IP specific issue but not get feedback from
> > design team. I'm thinking to disable this error forwarding just like
> > other platforms, since when these errors (UR, CA and CT) are detected,
> > AER driver can also report the error and try to recover.
>
> I take this as you shall send a patch to fix this issue shortly, is this correct ?

The issue becomes complex:
I reviewed the DWC PCIe databook of verion 4.40a which is used on Layerscape platforms, and it said that " Your RC application should not generate CFG requests until it has confirmed that the link is up by sampling the smlh_link_up and rmlh_link_up outputs".
So, the link up checking should not be remove before each outbound CFG access.
Gustavo, can you share more details on the link up checking? Does it only exist in the 4.40a?

For the feature of error forwarding to AXI that enabled on Layerscape PCIe, I will send a patch to disable it today.

Thanks,
Zhiqiang

>
> Thanks,
> Lorenzo

2020-09-29 15:27:10

by Gustavo Pimentel

[permalink] [raw]

Subject: RE: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Tue, Sep 29, 2020 at 5:5:41, Z.q. Hou <[email protected]> wrote:

> Hi Lorenzo,
>
> Thanks a lot for your comments!
>
> > -----Original Message-----
> > From: Lorenzo Pieralisi <[email protected]>
> > Sent: 2020年9月28日 17:39
> > To: Z.q. Hou <[email protected]>
> > Cc: Rob Herring <[email protected]>; [email protected]; PCI
> > <[email protected]>; Bjorn Helgaas <[email protected]>;
> > Gustavo Pimentel <[email protected]>; Michael Walle
> > <[email protected]>; Ard Biesheuvel <[email protected]>
> > Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> > dw_child_pcie_ops
> >
> > On Thu, Sep 24, 2020 at 04:24:47AM +0000, Z.q. Hou wrote:
> > > Hi Rob,
> > >
> > > Thanks a lot for your comments!
> > >
> > > > -----Original Message-----
> > > > From: Rob Herring <[email protected]>
> > > > Sent: 2020年9月18日 23:28
> > > > To: Z.q. Hou <[email protected]>
> > > > Cc: [email protected]; PCI <[email protected]>;
> > > > Lorenzo Pieralisi <[email protected]>; Bjorn Helgaas
> > > > <[email protected]>; Gustavo Pimentel
> > > > <[email protected]>; Michael Walle
> > <[email protected]>;
> > > > Ard Biesheuvel <[email protected]>
> > > > Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> > > > dw_child_pcie_ops
> > > >
> > > > On Fri, Sep 18, 2020 at 5:02 AM Z.q. Hou <[email protected]>
> > wrote:
> > > > >
> > > > > Hi Rob,
> > > > >
> > > > > Thanks a lot for your comments!
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Rob Herring <[email protected]>
> > > > > > Sent: 2020年9月17日 4:29
> > > > > > To: Z.q. Hou <[email protected]>
> > > > > > Cc: [email protected]; PCI
> > > > > > <[email protected]>; Lorenzo Pieralisi
> > > > > > <[email protected]>; Bjorn Helgaas
> > > > > > <[email protected]>; Gustavo Pimentel
> > > > > > <[email protected]>; Michael Walle
> > > > <[email protected]>;
> > > > > > Ard Biesheuvel <[email protected]>
> > > > > > Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> > > > > > dw_child_pcie_ops
> > > > > >
> > > > > > On Tue, Sep 15, 2020 at 11:49 PM Zhiqiang Hou
> > > > <[email protected]>
> > > > > > wrote:
> > > > > > >
> > > > > > > From: Hou Zhiqiang <[email protected]>
> > > > > > >
> > > > > > > On NXP Layerscape platforms, it results in SError in the
> > > > > > > enumeration of the PCIe controller, which is not connecting
> > > > > > > with an Endpoint device. And it doesn't make sense to
> > > > > > > enumerate the Endpoints when the PCIe link is down. So this
> > > > > > > patch added the link up check to avoid to fire configuration
> > transactions on link down bus.
> > > > > >
> > > > > > Michael reported the same issue as well.
> > > > > >
> > > > > > What happens if the link goes down between the check and the
> > access?
> > > > >
> > > > > This patch cannot cover this case, and will get the SError.
> > > > > But I think it makes sense to avoid firing transactions on link down bus.
> > > >
> > > > That's impossible to do without a race even in h/w.
> > >
> > > Agree.
> > >
> > > >
> > > > > > It's a racy check. I'd like to find an alternative solution.
> > > > > > It's even worse if Layerscape is used in ECAM mode. I looked at
> > > > > > the EDK2 setup for layerscape[1] and it looks like root ports
> > > > > > are just skipped if link
> > > > is down.
> > > > > > Maybe a link down just never happens once up, but if so, then we
> > > > > > only need to check it once and fail probe.
> > > > >
> > > > > Many customers connect the FPGA Endpoint, which may establish PCIe
> > > > > link after the PCIe enumeration and then rescan the PCIe bus, so I
> > > > > think it should not exit the probe of root port even if there is
> > > > > not link up
> > > > during enumeration.
> > > >
> > > > That's a good reason. I want to unify the behavior here as it varies
> > > > per platform currently and wasn't sure which way to go.
> > > >
> > > >
> > > > > > I've dug into this a bit more and am curious about the
> > > > > > PCIE_ABSERR register setting which is set to:
> > > > > >
> > > > > > #define PCIE_ABSERR_SETTING 0x9401 /* Forward error of
> > > > > > non-posted request */
> > > > > >
> > > > > > It seems to me this is not what we want at least for config
> > > > > > accesses, but commit 84d897d6993 where this was added seems to
> > > > > > say otherwise. Is it not possible to configure the response per access
> > type?
> > > > >
> > > > > Thanks a lot for your investigation!
> > > > > The story is like this: Some customers worry about these silent
> > > > > error (DWC PCIe IP won't forward the error of outbound non-post
> > > > > request by default), so we were pushed to enable the error
> > > > > forwarding to AXI in the commit
> > > > > 84d897d6993 as you saw. But it cannot differentiate the config
> > > > > transactions from the MEM_rd, except the Vendor ID access, which
> > > > > is controlled by a separate bit and it was set to not forward
> > > > > error of access
> > > > of Vendor ID.
> > > > > So we think it's okay to enable the error forwarding, the SError
> > > > > should not occur, because after the enumeration it won't access
> > > > > the
> > > > non-existent functions.
> > > >
> > > > We've rejected upstream support for platforms aborting on config
> > > > accesses[1]. I think there's clear consensus that aborting is the
> > > > wrong behavior.
> > > >
> > > > Do MEM_wr errors get forwarded? Seems like that would be enough.
> > > > Also, wouldn't page faults catch most OOB accesses anyways? You need
> > > > things page aligned anyways with an IOMMU and doing userspace access
> > > > or guest assignment.
> > >
> > > Yes, errors of MEM_wr can be forwarded.
> > >
> > > >
> > > > Here's another idea, how about only enabling forwarding errors if
> > > > the link is up? If really would need to be configured any time the
> > > > link state changes rather than just at probe. I'm not sure if you
> > > > have a way to disable it on link down though.
> > >
> > > Dug deeper into this issue and found the setting of not forwarding
> > > error of non-existent Vender ID access counts on the link partner: 1.
> > > When there is a link partner (namely link up), it will return 0xffff
> > > when read non-existent function Vendor ID and won't forward error to
> > > AXI. 2. When no link partner (link down), it will forward the error
> > > of reading non-existent function Vendor ID to AXI and result in
> > > SError.
> > >
> > > I think this is a DWC PCIe IP specific issue but not get feedback from
> > > design team. I'm thinking to disable this error forwarding just like
> > > other platforms, since when these errors (UR, CA and CT) are detected,
> > > AER driver can also report the error and try to recover.
> >
> > I take this as you shall send a patch to fix this issue shortly, is this correct ?
>
> The issue becomes complex:
> I reviewed the DWC PCIe databook of verion 4.40a which is used on Layerscape platforms, and it said that " Your RC application should not generate CFG requests until it has confirmed that the link is up by sampling the smlh_link_up and rmlh_link_up outputs".
> So, the link up checking should not be remove before each outbound CFG access.
> Gustavo, can you share more details on the link up checking? Does it only exist in the 4.40a?

Hi Zhiqiang,

According to the information that I got from the IP team you are correct,
the same requirement still exists on the newer IP versions.

-Gustavo

>
> For the feature of error forwarding to AXI that enabled on Layerscape PCIe, I will send a patch to disable it today.
>
> Thanks,
> Zhiqiang
>
> >
> > Thanks,
> > Lorenzo

2020-09-29 17:13:32

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Tue, Sep 29, 2020 at 10:24 AM Gustavo Pimentel
<[email protected]> wrote:
>
> On Tue, Sep 29, 2020 at 5:5:41, Z.q. Hou <[email protected]> wrote:
>
> > Hi Lorenzo,
> >
> > Thanks a lot for your comments!
> >
> > > -----Original Message-----
> > > From: Lorenzo Pieralisi <[email protected]>
> > > Sent: 2020年9月28日 17:39
> > > To: Z.q. Hou <[email protected]>
> > > Cc: Rob Herring <[email protected]>; [email protected]; PCI
> > > <[email protected]>; Bjorn Helgaas <[email protected]>;
> > > Gustavo Pimentel <[email protected]>; Michael Walle
> > > <[email protected]>; Ard Biesheuvel <[email protected]>
> > > Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> > > dw_child_pcie_ops
> > >
> > > On Thu, Sep 24, 2020 at 04:24:47AM +0000, Z.q. Hou wrote:
> > > > Hi Rob,
> > > >
> > > > Thanks a lot for your comments!
> > > >
> > > > > -----Original Message-----
> > > > > From: Rob Herring <[email protected]>
> > > > > Sent: 2020年9月18日 23:28
> > > > > To: Z.q. Hou <[email protected]>
> > > > > Cc: [email protected]; PCI <[email protected]>;
> > > > > Lorenzo Pieralisi <[email protected]>; Bjorn Helgaas
> > > > > <[email protected]>; Gustavo Pimentel
> > > > > <[email protected]>; Michael Walle
> > > <[email protected]>;
> > > > > Ard Biesheuvel <[email protected]>
> > > > > Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> > > > > dw_child_pcie_ops
> > > > >
> > > > > On Fri, Sep 18, 2020 at 5:02 AM Z.q. Hou <[email protected]>
> > > wrote:
> > > > > >
> > > > > > Hi Rob,
> > > > > >
> > > > > > Thanks a lot for your comments!
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Rob Herring <[email protected]>
> > > > > > > Sent: 2020年9月17日 4:29
> > > > > > > To: Z.q. Hou <[email protected]>
> > > > > > > Cc: [email protected]; PCI
> > > > > > > <[email protected]>; Lorenzo Pieralisi
> > > > > > > <[email protected]>; Bjorn Helgaas
> > > > > > > <[email protected]>; Gustavo Pimentel
> > > > > > > <[email protected]>; Michael Walle
> > > > > <[email protected]>;
> > > > > > > Ard Biesheuvel <[email protected]>
> > > > > > > Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> > > > > > > dw_child_pcie_ops
> > > > > > >
> > > > > > > On Tue, Sep 15, 2020 at 11:49 PM Zhiqiang Hou
> > > > > <[email protected]>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > From: Hou Zhiqiang <[email protected]>
> > > > > > > >
> > > > > > > > On NXP Layerscape platforms, it results in SError in the
> > > > > > > > enumeration of the PCIe controller, which is not connecting
> > > > > > > > with an Endpoint device. And it doesn't make sense to
> > > > > > > > enumerate the Endpoints when the PCIe link is down. So this
> > > > > > > > patch added the link up check to avoid to fire configuration
> > > transactions on link down bus.
> > > > > > >
> > > > > > > Michael reported the same issue as well.
> > > > > > >
> > > > > > > What happens if the link goes down between the check and the
> > > access?
> > > > > >
> > > > > > This patch cannot cover this case, and will get the SError.
> > > > > > But I think it makes sense to avoid firing transactions on link down bus.
> > > > >
> > > > > That's impossible to do without a race even in h/w.
> > > >
> > > > Agree.
> > > >
> > > > >
> > > > > > > It's a racy check. I'd like to find an alternative solution.
> > > > > > > It's even worse if Layerscape is used in ECAM mode. I looked at
> > > > > > > the EDK2 setup for layerscape[1] and it looks like root ports
> > > > > > > are just skipped if link
> > > > > is down.
> > > > > > > Maybe a link down just never happens once up, but if so, then we
> > > > > > > only need to check it once and fail probe.
> > > > > >
> > > > > > Many customers connect the FPGA Endpoint, which may establish PCIe
> > > > > > link after the PCIe enumeration and then rescan the PCIe bus, so I
> > > > > > think it should not exit the probe of root port even if there is
> > > > > > not link up
> > > > > during enumeration.
> > > > >
> > > > > That's a good reason. I want to unify the behavior here as it varies
> > > > > per platform currently and wasn't sure which way to go.
> > > > >
> > > > >
> > > > > > > I've dug into this a bit more and am curious about the
> > > > > > > PCIE_ABSERR register setting which is set to:
> > > > > > >
> > > > > > > #define PCIE_ABSERR_SETTING 0x9401 /* Forward error of
> > > > > > > non-posted request */
> > > > > > >
> > > > > > > It seems to me this is not what we want at least for config
> > > > > > > accesses, but commit 84d897d6993 where this was added seems to
> > > > > > > say otherwise. Is it not possible to configure the response per access
> > > type?
> > > > > >
> > > > > > Thanks a lot for your investigation!
> > > > > > The story is like this: Some customers worry about these silent
> > > > > > error (DWC PCIe IP won't forward the error of outbound non-post
> > > > > > request by default), so we were pushed to enable the error
> > > > > > forwarding to AXI in the commit
> > > > > > 84d897d6993 as you saw. But it cannot differentiate the config
> > > > > > transactions from the MEM_rd, except the Vendor ID access, which
> > > > > > is controlled by a separate bit and it was set to not forward
> > > > > > error of access
> > > > > of Vendor ID.
> > > > > > So we think it's okay to enable the error forwarding, the SError
> > > > > > should not occur, because after the enumeration it won't access
> > > > > > the
> > > > > non-existent functions.
> > > > >
> > > > > We've rejected upstream support for platforms aborting on config
> > > > > accesses[1]. I think there's clear consensus that aborting is the
> > > > > wrong behavior.
> > > > >
> > > > > Do MEM_wr errors get forwarded? Seems like that would be enough.
> > > > > Also, wouldn't page faults catch most OOB accesses anyways? You need
> > > > > things page aligned anyways with an IOMMU and doing userspace access
> > > > > or guest assignment.
> > > >
> > > > Yes, errors of MEM_wr can be forwarded.
> > > >
> > > > >
> > > > > Here's another idea, how about only enabling forwarding errors if
> > > > > the link is up? If really would need to be configured any time the
> > > > > link state changes rather than just at probe. I'm not sure if you
> > > > > have a way to disable it on link down though.
> > > >
> > > > Dug deeper into this issue and found the setting of not forwarding
> > > > error of non-existent Vender ID access counts on the link partner: 1.
> > > > When there is a link partner (namely link up), it will return 0xffff
> > > > when read non-existent function Vendor ID and won't forward error to
> > > > AXI. 2. When no link partner (link down), it will forward the error
> > > > of reading non-existent function Vendor ID to AXI and result in
> > > > SError.
> > > >
> > > > I think this is a DWC PCIe IP specific issue but not get feedback from
> > > > design team. I'm thinking to disable this error forwarding just like
> > > > other platforms, since when these errors (UR, CA and CT) are detected,
> > > > AER driver can also report the error and try to recover.
> > >
> > > I take this as you shall send a patch to fix this issue shortly, is this correct ?
> >
> > The issue becomes complex:
> > I reviewed the DWC PCIe databook of verion 4.40a which is used on Layerscape platforms, and it said that " Your RC application should not generate CFG requests until it has confirmed that the link is up by sampling the smlh_link_up and rmlh_link_up outputs".
> > So, the link up checking should not be remove before each outbound CFG access.
> > Gustavo, can you share more details on the link up checking? Does it only exist in the 4.40a?
>
> Hi Zhiqiang,
>
> According to the information that I got from the IP team you are correct,
> the same requirement still exists on the newer IP versions.

How is that possible in a race free way?

Testing on meson and layerscape (with the forwarding of errors
disabled) shows a link check is not needed. But then dra7xx seems to
need one (or has some f/w setup).

Rob

2020-09-30 13:26:35

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

Hi,

On 29/09/20 10:41 pm, Rob Herring wrote:
> On Tue, Sep 29, 2020 at 10:24 AM Gustavo Pimentel
> <[email protected]> wrote:
>>
>> On Tue, Sep 29, 2020 at 5:5:41, Z.q. Hou <[email protected]> wrote:
>>
>>> Hi Lorenzo,
>>>
>>> Thanks a lot for your comments!
>>>
>>>> -----Original Message-----
>>>> From: Lorenzo Pieralisi <[email protected]>
>>>> Sent: 2020年9月28日 17:39
>>>> To: Z.q. Hou <[email protected]>
>>>> Cc: Rob Herring <[email protected]>; [email protected]; PCI
>>>> <[email protected]>; Bjorn Helgaas <[email protected]>;
>>>> Gustavo Pimentel <[email protected]>; Michael Walle
>>>> <[email protected]>; Ard Biesheuvel <[email protected]>
>>>> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
>>>> dw_child_pcie_ops
>>>>
>>>> On Thu, Sep 24, 2020 at 04:24:47AM +0000, Z.q. Hou wrote:
>>>>> Hi Rob,
>>>>>
>>>>> Thanks a lot for your comments!
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Rob Herring <[email protected]>
>>>>>> Sent: 2020年9月18日 23:28
>>>>>> To: Z.q. Hou <[email protected]>
>>>>>> Cc: [email protected]; PCI <[email protected]>;
>>>>>> Lorenzo Pieralisi <[email protected]>; Bjorn Helgaas
>>>>>> <[email protected]>; Gustavo Pimentel
>>>>>> <[email protected]>; Michael Walle
>>>> <[email protected]>;
>>>>>> Ard Biesheuvel <[email protected]>
>>>>>> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
>>>>>> dw_child_pcie_ops
>>>>>>
>>>>>> On Fri, Sep 18, 2020 at 5:02 AM Z.q. Hou <[email protected]>
>>>> wrote:
>>>>>>>
>>>>>>> Hi Rob,
>>>>>>>
>>>>>>> Thanks a lot for your comments!
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Rob Herring <[email protected]>
>>>>>>>> Sent: 2020年9月17日 4:29
>>>>>>>> To: Z.q. Hou <[email protected]>
>>>>>>>> Cc: [email protected]; PCI
>>>>>>>> <[email protected]>; Lorenzo Pieralisi
>>>>>>>> <[email protected]>; Bjorn Helgaas
>>>>>>>> <[email protected]>; Gustavo Pimentel
>>>>>>>> <[email protected]>; Michael Walle
>>>>>> <[email protected]>;
>>>>>>>> Ard Biesheuvel <[email protected]>
>>>>>>>> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
>>>>>>>> dw_child_pcie_ops
>>>>>>>>
>>>>>>>> On Tue, Sep 15, 2020 at 11:49 PM Zhiqiang Hou
>>>>>> <[email protected]>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> From: Hou Zhiqiang <[email protected]>
>>>>>>>>>
>>>>>>>>> On NXP Layerscape platforms, it results in SError in the
>>>>>>>>> enumeration of the PCIe controller, which is not connecting
>>>>>>>>> with an Endpoint device. And it doesn't make sense to
>>>>>>>>> enumerate the Endpoints when the PCIe link is down. So this
>>>>>>>>> patch added the link up check to avoid to fire configuration
>>>> transactions on link down bus.
>>>>>>>>
>>>>>>>> Michael reported the same issue as well.
>>>>>>>>
>>>>>>>> What happens if the link goes down between the check and the
>>>> access?
>>>>>>>
>>>>>>> This patch cannot cover this case, and will get the SError.
>>>>>>> But I think it makes sense to avoid firing transactions on link down bus.
>>>>>>
>>>>>> That's impossible to do without a race even in h/w.
>>>>>
>>>>> Agree.
>>>>>
>>>>>>
>>>>>>>> It's a racy check. I'd like to find an alternative solution.
>>>>>>>> It's even worse if Layerscape is used in ECAM mode. I looked at
>>>>>>>> the EDK2 setup for layerscape[1] and it looks like root ports
>>>>>>>> are just skipped if link
>>>>>> is down.
>>>>>>>> Maybe a link down just never happens once up, but if so, then we
>>>>>>>> only need to check it once and fail probe.
>>>>>>>
>>>>>>> Many customers connect the FPGA Endpoint, which may establish PCIe
>>>>>>> link after the PCIe enumeration and then rescan the PCIe bus, so I
>>>>>>> think it should not exit the probe of root port even if there is
>>>>>>> not link up
>>>>>> during enumeration.
>>>>>>
>>>>>> That's a good reason. I want to unify the behavior here as it varies
>>>>>> per platform currently and wasn't sure which way to go.
>>>>>>
>>>>>>
>>>>>>>> I've dug into this a bit more and am curious about the
>>>>>>>> PCIE_ABSERR register setting which is set to:
>>>>>>>>
>>>>>>>> #define PCIE_ABSERR_SETTING 0x9401 /* Forward error of
>>>>>>>> non-posted request */
>>>>>>>>
>>>>>>>> It seems to me this is not what we want at least for config
>>>>>>>> accesses, but commit 84d897d6993 where this was added seems to
>>>>>>>> say otherwise. Is it not possible to configure the response per access
>>>> type?
>>>>>>>
>>>>>>> Thanks a lot for your investigation!
>>>>>>> The story is like this: Some customers worry about these silent
>>>>>>> error (DWC PCIe IP won't forward the error of outbound non-post
>>>>>>> request by default), so we were pushed to enable the error
>>>>>>> forwarding to AXI in the commit
>>>>>>> 84d897d6993 as you saw. But it cannot differentiate the config
>>>>>>> transactions from the MEM_rd, except the Vendor ID access, which
>>>>>>> is controlled by a separate bit and it was set to not forward
>>>>>>> error of access
>>>>>> of Vendor ID.
>>>>>>> So we think it's okay to enable the error forwarding, the SError
>>>>>>> should not occur, because after the enumeration it won't access
>>>>>>> the
>>>>>> non-existent functions.
>>>>>>
>>>>>> We've rejected upstream support for platforms aborting on config
>>>>>> accesses[1]. I think there's clear consensus that aborting is the
>>>>>> wrong behavior.
>>>>>>
>>>>>> Do MEM_wr errors get forwarded? Seems like that would be enough.
>>>>>> Also, wouldn't page faults catch most OOB accesses anyways? You need
>>>>>> things page aligned anyways with an IOMMU and doing userspace access
>>>>>> or guest assignment.
>>>>>
>>>>> Yes, errors of MEM_wr can be forwarded.
>>>>>
>>>>>>
>>>>>> Here's another idea, how about only enabling forwarding errors if
>>>>>> the link is up? If really would need to be configured any time the
>>>>>> link state changes rather than just at probe. I'm not sure if you
>>>>>> have a way to disable it on link down though.
>>>>>
>>>>> Dug deeper into this issue and found the setting of not forwarding
>>>>> error of non-existent Vender ID access counts on the link partner: 1.
>>>>> When there is a link partner (namely link up), it will return 0xffff
>>>>> when read non-existent function Vendor ID and won't forward error to
>>>>> AXI. 2. When no link partner (link down), it will forward the error
>>>>> of reading non-existent function Vendor ID to AXI and result in
>>>>> SError.
>>>>>
>>>>> I think this is a DWC PCIe IP specific issue but not get feedback from
>>>>> design team. I'm thinking to disable this error forwarding just like
>>>>> other platforms, since when these errors (UR, CA and CT) are detected,
>>>>> AER driver can also report the error and try to recover.
>>>>
>>>> I take this as you shall send a patch to fix this issue shortly, is this correct ?
>>>
>>> The issue becomes complex:
>>> I reviewed the DWC PCIe databook of verion 4.40a which is used on Layerscape platforms, and it said that " Your RC application should not generate CFG requests until it has confirmed that the link is up by sampling the smlh_link_up and rmlh_link_up outputs".
>>> So, the link up checking should not be remove before each outbound CFG access.
>>> Gustavo, can you share more details on the link up checking? Does it only exist in the 4.40a?
>>
>> Hi Zhiqiang,
>>
>> According to the information that I got from the IP team you are correct,
>> the same requirement still exists on the newer IP versions.
>
> How is that possible in a race free way?
>
> Testing on meson and layerscape (with the forwarding of errors
> disabled) shows a link check is not needed. But then dra7xx seems to
> need one (or has some f/w setup).

Yeah, I don't see any registers in the DRA7x PCIe wrapper for disabling
error forwarding.

Thanks
Kishon

2020-09-30 15:03:47

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Wed, Sep 30, 2020 at 8:22 AM Kishon Vijay Abraham I <[email protected]> wrote:
>
> Hi,
>
> On 29/09/20 10:41 pm, Rob Herring wrote:
> > On Tue, Sep 29, 2020 at 10:24 AM Gustavo Pimentel
> > <[email protected]> wrote:
> >>
> >> On Tue, Sep 29, 2020 at 5:5:41, Z.q. Hou <[email protected]> wrote:
> >>
> >>> Hi Lorenzo,
> >>>
> >>> Thanks a lot for your comments!
> >>>
> >>>> -----Original Message-----
> >>>> From: Lorenzo Pieralisi <[email protected]>
> >>>> Sent: 2020年9月28日 17:39
> >>>> To: Z.q. Hou <[email protected]>
> >>>> Cc: Rob Herring <[email protected]>; [email protected]; PCI
> >>>> <[email protected]>; Bjorn Helgaas <[email protected]>;
> >>>> Gustavo Pimentel <[email protected]>; Michael Walle
> >>>> <[email protected]>; Ard Biesheuvel <[email protected]>
> >>>> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> >>>> dw_child_pcie_ops
> >>>>
> >>>> On Thu, Sep 24, 2020 at 04:24:47AM +0000, Z.q. Hou wrote:
> >>>>> Hi Rob,
> >>>>>
> >>>>> Thanks a lot for your comments!
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Rob Herring <[email protected]>
> >>>>>> Sent: 2020年9月18日 23:28
> >>>>>> To: Z.q. Hou <[email protected]>
> >>>>>> Cc: [email protected]; PCI <[email protected]>;
> >>>>>> Lorenzo Pieralisi <[email protected]>; Bjorn Helgaas
> >>>>>> <[email protected]>; Gustavo Pimentel
> >>>>>> <[email protected]>; Michael Walle
> >>>> <[email protected]>;
> >>>>>> Ard Biesheuvel <[email protected]>
> >>>>>> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> >>>>>> dw_child_pcie_ops
> >>>>>>
> >>>>>> On Fri, Sep 18, 2020 at 5:02 AM Z.q. Hou <[email protected]>
> >>>> wrote:
> >>>>>>>
> >>>>>>> Hi Rob,
> >>>>>>>
> >>>>>>> Thanks a lot for your comments!
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Rob Herring <[email protected]>
> >>>>>>>> Sent: 2020年9月17日 4:29
> >>>>>>>> To: Z.q. Hou <[email protected]>
> >>>>>>>> Cc: [email protected]; PCI
> >>>>>>>> <[email protected]>; Lorenzo Pieralisi
> >>>>>>>> <[email protected]>; Bjorn Helgaas
> >>>>>>>> <[email protected]>; Gustavo Pimentel
> >>>>>>>> <[email protected]>; Michael Walle
> >>>>>> <[email protected]>;
> >>>>>>>> Ard Biesheuvel <[email protected]>
> >>>>>>>> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> >>>>>>>> dw_child_pcie_ops
> >>>>>>>>
> >>>>>>>> On Tue, Sep 15, 2020 at 11:49 PM Zhiqiang Hou
> >>>>>> <[email protected]>
> >>>>>>>> wrote:
> >>>>>>>>>
> >>>>>>>>> From: Hou Zhiqiang <[email protected]>
> >>>>>>>>>
> >>>>>>>>> On NXP Layerscape platforms, it results in SError in the
> >>>>>>>>> enumeration of the PCIe controller, which is not connecting
> >>>>>>>>> with an Endpoint device. And it doesn't make sense to
> >>>>>>>>> enumerate the Endpoints when the PCIe link is down. So this
> >>>>>>>>> patch added the link up check to avoid to fire configuration
> >>>> transactions on link down bus.
> >>>>>>>>
> >>>>>>>> Michael reported the same issue as well.
> >>>>>>>>
> >>>>>>>> What happens if the link goes down between the check and the
> >>>> access?
> >>>>>>>
> >>>>>>> This patch cannot cover this case, and will get the SError.
> >>>>>>> But I think it makes sense to avoid firing transactions on link down bus.
> >>>>>>
> >>>>>> That's impossible to do without a race even in h/w.
> >>>>>
> >>>>> Agree.
> >>>>>
> >>>>>>
> >>>>>>>> It's a racy check. I'd like to find an alternative solution.
> >>>>>>>> It's even worse if Layerscape is used in ECAM mode. I looked at
> >>>>>>>> the EDK2 setup for layerscape[1] and it looks like root ports
> >>>>>>>> are just skipped if link
> >>>>>> is down.
> >>>>>>>> Maybe a link down just never happens once up, but if so, then we
> >>>>>>>> only need to check it once and fail probe.
> >>>>>>>
> >>>>>>> Many customers connect the FPGA Endpoint, which may establish PCIe
> >>>>>>> link after the PCIe enumeration and then rescan the PCIe bus, so I
> >>>>>>> think it should not exit the probe of root port even if there is
> >>>>>>> not link up
> >>>>>> during enumeration.
> >>>>>>
> >>>>>> That's a good reason. I want to unify the behavior here as it varies
> >>>>>> per platform currently and wasn't sure which way to go.
> >>>>>>
> >>>>>>
> >>>>>>>> I've dug into this a bit more and am curious about the
> >>>>>>>> PCIE_ABSERR register setting which is set to:
> >>>>>>>>
> >>>>>>>> #define PCIE_ABSERR_SETTING 0x9401 /* Forward error of
> >>>>>>>> non-posted request */
> >>>>>>>>
> >>>>>>>> It seems to me this is not what we want at least for config
> >>>>>>>> accesses, but commit 84d897d6993 where this was added seems to
> >>>>>>>> say otherwise. Is it not possible to configure the response per access
> >>>> type?
> >>>>>>>
> >>>>>>> Thanks a lot for your investigation!
> >>>>>>> The story is like this: Some customers worry about these silent
> >>>>>>> error (DWC PCIe IP won't forward the error of outbound non-post
> >>>>>>> request by default), so we were pushed to enable the error
> >>>>>>> forwarding to AXI in the commit
> >>>>>>> 84d897d6993 as you saw. But it cannot differentiate the config
> >>>>>>> transactions from the MEM_rd, except the Vendor ID access, which
> >>>>>>> is controlled by a separate bit and it was set to not forward
> >>>>>>> error of access
> >>>>>> of Vendor ID.
> >>>>>>> So we think it's okay to enable the error forwarding, the SError
> >>>>>>> should not occur, because after the enumeration it won't access
> >>>>>>> the
> >>>>>> non-existent functions.
> >>>>>>
> >>>>>> We've rejected upstream support for platforms aborting on config
> >>>>>> accesses[1]. I think there's clear consensus that aborting is the
> >>>>>> wrong behavior.
> >>>>>>
> >>>>>> Do MEM_wr errors get forwarded? Seems like that would be enough.
> >>>>>> Also, wouldn't page faults catch most OOB accesses anyways? You need
> >>>>>> things page aligned anyways with an IOMMU and doing userspace access
> >>>>>> or guest assignment.
> >>>>>
> >>>>> Yes, errors of MEM_wr can be forwarded.
> >>>>>
> >>>>>>
> >>>>>> Here's another idea, how about only enabling forwarding errors if
> >>>>>> the link is up? If really would need to be configured any time the
> >>>>>> link state changes rather than just at probe. I'm not sure if you
> >>>>>> have a way to disable it on link down though.
> >>>>>
> >>>>> Dug deeper into this issue and found the setting of not forwarding
> >>>>> error of non-existent Vender ID access counts on the link partner: 1.
> >>>>> When there is a link partner (namely link up), it will return 0xffff
> >>>>> when read non-existent function Vendor ID and won't forward error to
> >>>>> AXI. 2. When no link partner (link down), it will forward the error
> >>>>> of reading non-existent function Vendor ID to AXI and result in
> >>>>> SError.
> >>>>>
> >>>>> I think this is a DWC PCIe IP specific issue but not get feedback from
> >>>>> design team. I'm thinking to disable this error forwarding just like
> >>>>> other platforms, since when these errors (UR, CA and CT) are detected,
> >>>>> AER driver can also report the error and try to recover.
> >>>>
> >>>> I take this as you shall send a patch to fix this issue shortly, is this correct ?
> >>>
> >>> The issue becomes complex:
> >>> I reviewed the DWC PCIe databook of verion 4.40a which is used on Layerscape platforms, and it said that " Your RC application should not generate CFG requests until it has confirmed that the link is up by sampling the smlh_link_up and rmlh_link_up outputs".
> >>> So, the link up checking should not be remove before each outbound CFG access.
> >>> Gustavo, can you share more details on the link up checking? Does it only exist in the 4.40a?
> >>
> >> Hi Zhiqiang,
> >>
> >> According to the information that I got from the IP team you are correct,
> >> the same requirement still exists on the newer IP versions.
> >
> > How is that possible in a race free way?
> >
> > Testing on meson and layerscape (with the forwarding of errors
> > disabled) shows a link check is not needed. But then dra7xx seems to
> > need one (or has some f/w setup).
>
> Yeah, I don't see any registers in the DRA7x PCIe wrapper for disabling
> error forwarding.

It's a DWC port logic register AFAICT, but perhaps not present in all versions.

Rob

2020-10-01 13:36:30

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

Hi Rob,

On 30/09/20 8:31 pm, Rob Herring wrote:
> On Wed, Sep 30, 2020 at 8:22 AM Kishon Vijay Abraham I <[email protected]> wrote:
>>
>> Hi,
>>
>> On 29/09/20 10:41 pm, Rob Herring wrote:
>>> On Tue, Sep 29, 2020 at 10:24 AM Gustavo Pimentel
>>> <[email protected]> wrote:
>>>>
>>>> On Tue, Sep 29, 2020 at 5:5:41, Z.q. Hou <[email protected]> wrote:
>>>>
>>>>> Hi Lorenzo,
>>>>>
>>>>> Thanks a lot for your comments!
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Lorenzo Pieralisi <[email protected]>
>>>>>> Sent: 2020年9月28日 17:39
>>>>>> To: Z.q. Hou <[email protected]>
>>>>>> Cc: Rob Herring <[email protected]>; [email protected]; PCI
>>>>>> <[email protected]>; Bjorn Helgaas <[email protected]>;
>>>>>> Gustavo Pimentel <[email protected]>; Michael Walle
>>>>>> <[email protected]>; Ard Biesheuvel <[email protected]>
>>>>>> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
>>>>>> dw_child_pcie_ops
>>>>>>
>>>>>> On Thu, Sep 24, 2020 at 04:24:47AM +0000, Z.q. Hou wrote:
>>>>>>> Hi Rob,
>>>>>>>
>>>>>>> Thanks a lot for your comments!
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Rob Herring <[email protected]>
>>>>>>>> Sent: 2020年9月18日 23:28
>>>>>>>> To: Z.q. Hou <[email protected]>
>>>>>>>> Cc: [email protected]; PCI <[email protected]>;
>>>>>>>> Lorenzo Pieralisi <[email protected]>; Bjorn Helgaas
>>>>>>>> <[email protected]>; Gustavo Pimentel
>>>>>>>> <[email protected]>; Michael Walle
>>>>>> <[email protected]>;
>>>>>>>> Ard Biesheuvel <[email protected]>
>>>>>>>> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
>>>>>>>> dw_child_pcie_ops
>>>>>>>>
>>>>>>>> On Fri, Sep 18, 2020 at 5:02 AM Z.q. Hou <[email protected]>
>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> Hi Rob,
>>>>>>>>>
>>>>>>>>> Thanks a lot for your comments!
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Rob Herring <[email protected]>
>>>>>>>>>> Sent: 2020年9月17日 4:29
>>>>>>>>>> To: Z.q. Hou <[email protected]>
>>>>>>>>>> Cc: [email protected]; PCI
>>>>>>>>>> <[email protected]>; Lorenzo Pieralisi
>>>>>>>>>> <[email protected]>; Bjorn Helgaas
>>>>>>>>>> <[email protected]>; Gustavo Pimentel
>>>>>>>>>> <[email protected]>; Michael Walle
>>>>>>>> <[email protected]>;
>>>>>>>>>> Ard Biesheuvel <[email protected]>
>>>>>>>>>> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
>>>>>>>>>> dw_child_pcie_ops
>>>>>>>>>>
>>>>>>>>>> On Tue, Sep 15, 2020 at 11:49 PM Zhiqiang Hou
>>>>>>>> <[email protected]>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> From: Hou Zhiqiang <[email protected]>
>>>>>>>>>>>
>>>>>>>>>>> On NXP Layerscape platforms, it results in SError in the
>>>>>>>>>>> enumeration of the PCIe controller, which is not connecting
>>>>>>>>>>> with an Endpoint device. And it doesn't make sense to
>>>>>>>>>>> enumerate the Endpoints when the PCIe link is down. So this
>>>>>>>>>>> patch added the link up check to avoid to fire configuration
>>>>>> transactions on link down bus.
>>>>>>>>>>
>>>>>>>>>> Michael reported the same issue as well.
>>>>>>>>>>
>>>>>>>>>> What happens if the link goes down between the check and the
>>>>>> access?
>>>>>>>>>
>>>>>>>>> This patch cannot cover this case, and will get the SError.
>>>>>>>>> But I think it makes sense to avoid firing transactions on link down bus.
>>>>>>>>
>>>>>>>> That's impossible to do without a race even in h/w.
>>>>>>>
>>>>>>> Agree.
>>>>>>>
>>>>>>>>
>>>>>>>>>> It's a racy check. I'd like to find an alternative solution.
>>>>>>>>>> It's even worse if Layerscape is used in ECAM mode. I looked at
>>>>>>>>>> the EDK2 setup for layerscape[1] and it looks like root ports
>>>>>>>>>> are just skipped if link
>>>>>>>> is down.
>>>>>>>>>> Maybe a link down just never happens once up, but if so, then we
>>>>>>>>>> only need to check it once and fail probe.
>>>>>>>>>
>>>>>>>>> Many customers connect the FPGA Endpoint, which may establish PCIe
>>>>>>>>> link after the PCIe enumeration and then rescan the PCIe bus, so I
>>>>>>>>> think it should not exit the probe of root port even if there is
>>>>>>>>> not link up
>>>>>>>> during enumeration.
>>>>>>>>
>>>>>>>> That's a good reason. I want to unify the behavior here as it varies
>>>>>>>> per platform currently and wasn't sure which way to go.
>>>>>>>>
>>>>>>>>
>>>>>>>>>> I've dug into this a bit more and am curious about the
>>>>>>>>>> PCIE_ABSERR register setting which is set to:
>>>>>>>>>>
>>>>>>>>>> #define PCIE_ABSERR_SETTING 0x9401 /* Forward error of
>>>>>>>>>> non-posted request */
>>>>>>>>>>
>>>>>>>>>> It seems to me this is not what we want at least for config
>>>>>>>>>> accesses, but commit 84d897d6993 where this was added seems to
>>>>>>>>>> say otherwise. Is it not possible to configure the response per access
>>>>>> type?
>>>>>>>>>
>>>>>>>>> Thanks a lot for your investigation!
>>>>>>>>> The story is like this: Some customers worry about these silent
>>>>>>>>> error (DWC PCIe IP won't forward the error of outbound non-post
>>>>>>>>> request by default), so we were pushed to enable the error
>>>>>>>>> forwarding to AXI in the commit
>>>>>>>>> 84d897d6993 as you saw. But it cannot differentiate the config
>>>>>>>>> transactions from the MEM_rd, except the Vendor ID access, which
>>>>>>>>> is controlled by a separate bit and it was set to not forward
>>>>>>>>> error of access
>>>>>>>> of Vendor ID.
>>>>>>>>> So we think it's okay to enable the error forwarding, the SError
>>>>>>>>> should not occur, because after the enumeration it won't access
>>>>>>>>> the
>>>>>>>> non-existent functions.
>>>>>>>>
>>>>>>>> We've rejected upstream support for platforms aborting on config
>>>>>>>> accesses[1]. I think there's clear consensus that aborting is the
>>>>>>>> wrong behavior.
>>>>>>>>
>>>>>>>> Do MEM_wr errors get forwarded? Seems like that would be enough.
>>>>>>>> Also, wouldn't page faults catch most OOB accesses anyways? You need
>>>>>>>> things page aligned anyways with an IOMMU and doing userspace access
>>>>>>>> or guest assignment.
>>>>>>>
>>>>>>> Yes, errors of MEM_wr can be forwarded.
>>>>>>>
>>>>>>>>
>>>>>>>> Here's another idea, how about only enabling forwarding errors if
>>>>>>>> the link is up? If really would need to be configured any time the
>>>>>>>> link state changes rather than just at probe. I'm not sure if you
>>>>>>>> have a way to disable it on link down though.
>>>>>>>
>>>>>>> Dug deeper into this issue and found the setting of not forwarding
>>>>>>> error of non-existent Vender ID access counts on the link partner: 1.
>>>>>>> When there is a link partner (namely link up), it will return 0xffff
>>>>>>> when read non-existent function Vendor ID and won't forward error to
>>>>>>> AXI. 2. When no link partner (link down), it will forward the error
>>>>>>> of reading non-existent function Vendor ID to AXI and result in
>>>>>>> SError.
>>>>>>>
>>>>>>> I think this is a DWC PCIe IP specific issue but not get feedback from
>>>>>>> design team. I'm thinking to disable this error forwarding just like
>>>>>>> other platforms, since when these errors (UR, CA and CT) are detected,
>>>>>>> AER driver can also report the error and try to recover.
>>>>>>
>>>>>> I take this as you shall send a patch to fix this issue shortly, is this correct ?
>>>>>
>>>>> The issue becomes complex:
>>>>> I reviewed the DWC PCIe databook of verion 4.40a which is used on Layerscape platforms, and it said that " Your RC application should not generate CFG requests until it has confirmed that the link is up by sampling the smlh_link_up and rmlh_link_up outputs".
>>>>> So, the link up checking should not be remove before each outbound CFG access.
>>>>> Gustavo, can you share more details on the link up checking? Does it only exist in the 4.40a?
>>>>
>>>> Hi Zhiqiang,
>>>>
>>>> According to the information that I got from the IP team you are correct,
>>>> the same requirement still exists on the newer IP versions.
>>>
>>> How is that possible in a race free way?
>>>
>>> Testing on meson and layerscape (with the forwarding of errors
>>> disabled) shows a link check is not needed. But then dra7xx seems to
>>> need one (or has some f/w setup).
>>
>> Yeah, I don't see any registers in the DRA7x PCIe wrapper for disabling
>> error forwarding.
>
> It's a DWC port logic register AFAICT, but perhaps not present in all versions.

Okay. I see there's a register PCIECTRL_PL_AXIS_SLV_ERR_RESP which has a
reset value of 0.

It has four bit-fields, RESET_TIMEOUT_ERR_MAP, NO_VID_ERR_MAP,
DBI_ERR_MAP and SLAVE_ERR_MAP. I'm not seeing any difference in behavior
if I set all these bits. Maybe it requires platform support too. I'll
check this with our design team.

Meanwhile would it be okay to add linkup check atleast for DRA7X so that
we could have it booting in linux-next?

Thanks
Kishon

2020-10-01 16:48:34

by Michael Walle

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

Am 2020-10-01 15:32, schrieb Kishon Vijay Abraham I:

> Meanwhile would it be okay to add linkup check atleast for DRA7X so
> that
> we could have it booting in linux-next?

Layerscape SoCs (at least the LS1028A) are also still broken in
linux-next,
did I miss something here?

-michael

2020-10-02 09:33:39

by Naresh Kamboju

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Thu, 1 Oct 2020 at 22:16, Michael Walle <[email protected]> wrote:
>
> Am 2020-10-01 15:32, schrieb Kishon Vijay Abraham I:
>
> > Meanwhile would it be okay to add linkup check atleast for DRA7X so
> > that
> > we could have it booting in linux-next?
>
> Layerscape SoCs (at least the LS1028A) are also still broken in
> linux-next,
> did I miss something here?

I have been monitoring linux next boot and functional testing on nxp devices
for more than two week and still the problem exists on nxp-ls2088.

Do you mind checking the possibilities to revert bad patches on linux next tree
and continue to work on fixes please ?

suspected bad commit: [ I have not bisected this problem ]
c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")

crash log snippet:
[ 1.563008] SError Interrupt on CPU5, code 0xbf000002 -- SError
[ 1.563010] CPU: 5 PID: 1 Comm: swapper/0 Not tainted
5.9.0-rc7-next-20201001 #1
[ 1.563011] Hardware name: Freescale Layerscape 2088A RDB Board (DT)
[ 1.563013] pstate: 20000085 (nzCv daIf -PAN -UAO -TCO BTYPE=--)
[ 1.563014] pc : pci_generic_config_read+0x44/0xe8
[ 1.563015] lr : pci_generic_config_read+0x2c/0xe8

full boot log and its link,
----------------------------------
[ 0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd082]
[ 0.000000] Linux version 5.9.0-rc7-next-20201001
(TuxBuild@40858153859f) (aarch64-linux-gnu-gcc (Debian 9.3.0-8) 9.3.0,
GNU ld (GNU Binutils for Debian) 2.34) #1 SMP PREEMPT Thu Oct 1
14:14:17 UTC 2020
[ 0.000000] Machine model: Freescale Layerscape 2088A RDB Board
[ 0.000000] earlycon: uart8250 at MMIO 0x00000000021c0600 (options '')
[ 0.000000] printk: bootconsole [uart8250] enabled
[ 0.000000] efi: UEFI not found.
[ 0.000000] [Firmware Bug]: Kernel image misaligned at boot, please
fix your bootloader!
[ 0.000000] cma: Reserved 32 MiB at 0x00000000f9c00000
[ 0.000000] NUMA: No NUMA configuration found
[ 0.000000] NUMA: Faking a node at [mem
0x0000000080000000-0x000000837fffffff]
[ 0.000000] NUMA: NODE_DATA [mem 0x837e3fb100-0x837e3fcfff]
[ 0.000000] Zone ranges:
[ 0.000000] DMA [mem 0x0000000080000000-0x00000000bfffffff]
[ 0.000000] DMA32 [mem 0x00000000c0000000-0x00000000ffffffff]
[ 0.000000] Normal [mem 0x0000000100000000-0x000000837fffffff]
[ 0.000000] Movable zone start for each node
[ 0.000000] Early memory node ranges
[ 0.000000] node 0: [mem 0x0000000080000000-0x00000000fbdfffff]
[ 0.000000] node 0: [mem 0x0000008080000000-0x000000837fffffff]
[ 0.000000] Initmem setup node 0 [mem 0x0000000080000000-0x000000837fffffff]
[ 0.000000] psci: probing for conduit method from DT.
[ 0.000000] psci: PSCIv1.1 detected in firmware.
[ 0.000000] psci: Using standard PSCI v0.2 function IDs
[ 0.000000] psci: MIGRATE_INFO_TYPE not supported.
[ 0.000000] psci: SMC Calling Convention v1.1
[ 0.000000] percpu: Embedded 31 pages/cpu s89624 r8192 d29160 u126976
[ 0.000000] Detected PIPT I-cache on CPU0
[ 0.000000] CPU features: detected: GIC system register CPU interface
[ 0.000000] CPU features: detected: EL2 vector hardening
[ 0.000000] CPU features: kernel page table isolation forced ON by KASLR
[ 0.000000] CPU features: detected: Kernel page table isolation (KPTI)
[ 0.000000] CPU features: detected: Spectre-v2
[ 0.000000] CPU features: detected: ARM errata 1165522, 1319367, or 1530923
[ 0.000000] Built 1 zonelists, mobility grouping on. Total pages: 3596040
[ 0.000000] Policy zone: Normal
[ 0.000000] Kernel command line: console=ttyS1,115200n8
root=/dev/nfs rw
nfsroot=59.144.98.45:/var/lib/lava/dispatcher/tmp/90794/extract-nfsrootfs-r9w7i8h0,tcp,hard,v3
earlycon=uart8250,mmio,0x21c0600 nousb default_hugepagesz=2m
hugepagesz=2m hugepages=256 arm-smmu-mod.disable_bypass=n
arm-smmu.disable_bypass=n ip=dhcp
[ 0.000000] Dentry cache hash table entries: 2097152 (order: 12,
16777216 bytes, linear)
[ 0.000000] Inode-cache hash table entries: 1048576 (order: 11,
8388608 bytes, linear)
[ 0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off
[ 0.000000] software IO TLB: mapped [mem
0x00000000bbfff000-0x00000000bffff000] (64MB)
[ 0.000000] Memory: 14189284K/14612480K available (17468K kernel
code, 3990K rwdata, 9344K rodata, 9344K init, 566K bss, 390428K
reserved, 32768K cma-reserved)
[ 0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=8, Nodes=1
[ 0.000000] ftrace: allocating 59411 entries in 233 pages
[ 0.000000] ftrace: allocated 233 pages with 5 groups
[ 0.000000] rcu: Preemptible hierarchical RCU implementation.
[ 0.000000] rcu: RCU event tracing is enabled.
[ 0.000000] rcu: RCU restricting CPUs from NR_CPUS=256 to nr_cpu_ids=8.
[ 0.000000] Trampoline variant of Tasks RCU enabled.
[ 0.000000] Rude variant of Tasks RCU enabled.
[ 0.000000] rcu: RCU calculated value of scheduler-enlistment delay
is 25 jiffies.
[ 0.000000] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=8
[ 0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
[ 0.000000] GICv3: GIC: Using split EOI/Deactivate mode
[ 0.000000] GICv3: 256 SPIs implemented
[ 0.000000] GICv3: 0 Extended SPIs implemented
[ 0.000000] GICv3: Distributor has no Range Selector support
[ 0.000000] GICv3: 16 PPIs implemented
[ 0.000000] GICv3: CPU0: found redistributor 0 region 0:0x0000000006100000
[ 0.000000] ITS [mem 0x06020000-0x0603ffff]
[ 0.000000] ITS@0x0000000006020000: allocated 8192 Devices
@836e5d0000 (flat, esz 8, psz 64K, shr 0)
[ 0.000000] ITS: using cache flushing for cmd queue
[ 0.000000] GICv3: using LPI property table @0x000000836e5e0000
[ 0.000000] GIC: using cache flushing for LPI property table
[ 0.000000] GICv3: CPU0: using allocated LPI pending table
@0x000000836e5f0000
[ 0.000000] random: get_random_bytes called from
start_kernel+0x39c/0x56c with crng_init=0
[ 0.000000] arch_timer: Enabling global workaround for Freescale
erratum a005858
[ 0.000000] arch_timer: CPU0: Trapping CNTVCT access
[ 0.000000] arch_timer: cp15 timer(s) running at 25.00MHz (phys).
[ 0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff
max_cycles: 0x5c409fb33, max_idle_ns: 440795203156 ns
[ 0.000002] sched_clock: 56 bits at 25MHz, resolution 39ns, wraps
every 4398046511103ns
[ 0.008260] Console: colour dummy device 80x25
[ 0.012772] Calibrating delay loop (skipped), value calculated
using timer frequency.. 50.00 BogoMIPS (lpj=100000)
[ 0.023192] pid_max: default: 32768 minimum: 301
[ 0.027878] LSM: Security Framework initializing
[ 0.032588] Mount-cache hash table entries: 32768 (order: 6, 262144
bytes, linear)
[ 0.040245] Mountpoint-cache hash table entries: 32768 (order: 6,
262144 bytes, linear)
[ 0.049245] rcu: Hierarchical SRCU implementation.
[ 0.054670] Platform MSI: gic-its@6020000 domain created
[ 0.060101] PCI/MSI: /interrupt-controller@6000000/gic-its@6020000
domain created
[ 0.067682] fsl-mc MSI: gic-its@6020000 domain created
[ 0.073366] EFI services will not be available.
[ 0.078210] smp: Bringing up secondary CPUs ...
[ 0.083166] Detected PIPT I-cache on CPU1
[ 0.083182] GICv3: CPU1: found redistributor 1 region 0:0x0000000006120000
[ 0.083190] GICv3: CPU1: using allocated LPI pending table
@0x000000836e600000
[ 0.083206] arch_timer: CPU1: Trapping CNTVCT access
[ 0.083216] CPU1: Booted secondary processor 0x0000000001 [0x410fd082]
[ 0.083656] Detected PIPT I-cache on CPU2
[ 0.083671] GICv3: CPU2: found redistributor 100 region 0:0x0000000006140000
[ 0.083679] GICv3: CPU2: using allocated LPI pending table
@0x000000836e610000
[ 0.083694] arch_timer: CPU2: Trapping CNTVCT access
[ 0.083703] CPU2: Booted secondary processor 0x0000000100 [0x410fd082]
[ 0.084154] Detected PIPT I-cache on CPU3
[ 0.084164] GICv3: CPU3: found redistributor 101 region 0:0x0000000006160000
[ 0.084171] GICv3: CPU3: using allocated LPI pending table
@0x000000836e620000
[ 0.084181] arch_timer: CPU3: Trapping CNTVCT access
[ 0.084188] CPU3: Booted secondary processor 0x0000000101 [0x410fd082]
[ 0.084608] Detected PIPT I-cache on CPU4
[ 0.084624] GICv3: CPU4: found redistributor 200 region 0:0x0000000006180000
[ 0.084632] GICv3: CPU4: using allocated LPI pending table
@0x000000836e630000
[ 0.084647] arch_timer: CPU4: Trapping CNTVCT access
[ 0.084656] CPU4: Booted secondary processor 0x0000000200 [0x410fd082]
[ 0.085098] Detected PIPT I-cache on CPU5
[ 0.085109] GICv3: CPU5: found redistributor 201 region 0:0x00000000061a0000
[ 0.085116] GICv3: CPU5: using allocated LPI pending table
@0x000000836e640000
[ 0.085126] arch_timer: CPU5: Trapping CNTVCT access
[ 0.085134] CPU5: Booted secondary processor 0x0000000201 [0x410fd082]
[ 0.085563] Detected PIPT I-cache on CPU6
[ 0.085581] GICv3: CPU6: found redistributor 300 region 0:0x00000000061c0000
[ 0.085589] GICv3: CPU6: using allocated LPI pending table
@0x000000836e650000
[ 0.085604] arch_timer: CPU6: Trapping CNTVCT access
[ 0.085615] CPU6: Booted secondary processor 0x0000000300 [0x410fd082]
[ 0.086050] Detected PIPT I-cache on CPU7
[ 0.086061] GICv3: CPU7: found redistributor 301 region 0:0x00000000061e0000
[ 0.086068] GICv3: CPU7: using allocated LPI pending table
@0x000000836e660000
[ 0.086079] arch_timer: CPU7: Trapping CNTVCT access
[ 0.086087] CPU7: Booted secondary processor 0x0000000301 [0x410fd082]
[ 0.086162] smp: Brought up 1 node, 8 CPUs
[ 0.299613] SMP: Total of 8 processors activated.
[ 0.304348] CPU features: detected: 32-bit EL0 Support
[ 0.309537] CPU features: detected: CRC32 instructions
[ 0.314711] CPU features: detected: 32-bit EL1 Support
[ 0.339004] CPU: All CPU(s) started at EL2
[ 0.343157] alternatives: patching kernel code
[ 0.348551] devtmpfs: initialized
[ 0.353789] KASLR enabled
[ 0.356755] clocksource: jiffies: mask: 0xffffffff max_cycles:
0xffffffff, max_idle_ns: 7645041785100000 ns
[ 0.366571] futex hash table entries: 2048 (order: 5, 131072 bytes, linear)
[ 0.374031] pinctrl core: initialized pinctrl subsystem
[ 0.379863] DMI not present or invalid.
[ 0.384013] NET: Registered protocol family 16
[ 0.389659] DMA: preallocated 2048 KiB GFP_KERNEL pool for atomic allocations
[ 0.397124] DMA: preallocated 2048 KiB GFP_KERNEL|GFP_DMA pool for
atomic allocations
[ 0.405317] DMA: preallocated 2048 KiB GFP_KERNEL|GFP_DMA32 pool
for atomic allocations
[ 0.413398] audit: initializing netlink subsys (disabled)
[ 0.418940] audit: type=2000 audit(0.280:1): state=initialized
audit_enabled=0 res=1
[ 0.419382] thermal_sys: Registered thermal governor 'step_wise'
[ 0.426741] thermal_sys: Registered thermal governor 'power_allocator'
[ 0.433373] cpuidle: using governor menu
[ 0.444057] hw-breakpoint: found 6 breakpoint and 4 watchpoint registers.
[ 0.451014] ASID allocator initialised with 32768 entries
[ 0.457115] Serial: AMBA PL011 UART driver
[ 0.482622] Machine: Freescale Layerscape 2088A RDB Board
[ 0.488055] SoC family: QorIQ LS2088A
[ 0.491733] SoC ID: svr:0x87090010, Revision: 1.0
[ 0.511230] HugeTLB registered 2.00 MiB page size, pre-allocated 256 pages
[ 0.518172] HugeTLB registered 1.00 GiB page size, pre-allocated 0 pages
[ 0.524921] HugeTLB registered 32.0 MiB page size, pre-allocated 0 pages
[ 0.531661] HugeTLB registered 64.0 KiB page size, pre-allocated 0 pages
[ 0.541027] cryptd: max_cpu_qlen set to 1000
[ 0.550747] ACPI: Interpreter disabled.
[ 0.556228] iommu: Default domain type: Translated
[ 0.561321] vgaarb: loaded
[ 0.564177] SCSI subsystem initialized
[ 0.568186] usbcore: registered new interface driver usbfs
[ 0.573730] usbcore: registered new interface driver hub
[ 0.579135] usbcore: registered new device driver usb
[ 0.584676] imx-i2c 2000000.i2c: can't get pinctrl, bus recovery
not supported
[ 0.592140] i2c i2c-0: IMX I2C adapter registered
[ 0.597181] mc: Linux media interface: v0.10
[ 0.601505] videodev: Linux video capture interface: v2.00
[ 0.607040] pps_core: LinuxPPS API ver. 1 registered
[ 0.612031] pps_core: Software ver. 5.3.6 - Copyright 2005-2007
Rodolfo Giometti <[email protected]>
[ 0.621226] PTP clock support registered
[ 0.625279] EDAC MC: Ver: 3.0.0
[ 0.629525] fsl-ifc 2240000.ifc: Freescale Integrated Flash Controller
[ 0.636130] fsl-ifc 2240000.ifc: IFC version 2.0, 8 banks
[ 0.641716] FPGA manager framework
[ 0.645165] Advanced Linux Sound Architecture Driver Initialized.
[ 0.651768] clocksource: Switched to clocksource arch_sys_counter
[ 1.001279] VFS: Disk quotas dquot_6.6.0
[ 1.005269] VFS: Dquot-cache hash table entries: 512 (order 0, 4096 bytes)
[ 1.012344] pnp: PnP ACPI: disabled
[ 1.019574] NET: Registered protocol family 2
[ 1.024205] tcp_listen_portaddr_hash hash table entries: 8192
(order: 5, 131072 bytes, linear)
[ 1.032962] TCP established hash table entries: 131072 (order: 8,
1048576 bytes, linear)
[ 1.041546] TCP bind hash table entries: 65536 (order: 8, 1048576
bytes, linear)
[ 1.049454] TCP: Hash tables configured (established 131072 bind 65536)
[ 1.056243] UDP hash table entries: 8192 (order: 6, 262144 bytes, linear)
[ 1.063249] UDP-Lite hash table entries: 8192 (order: 6, 262144
bytes, linear)
[ 1.070792] NET: Registered protocol family 1
[ 1.075466] RPC: Registered named UNIX socket transport module.
[ 1.081446] RPC: Registered udp transport module.
[ 1.086178] RPC: Registered tcp transport module.
[ 1.090909] RPC: Registered tcp NFSv4.1 backchannel transport module.
[ 1.097393] PCI: CLS 0 bytes, default 64
[ 1.102116] hw perfevents: enabled with armv8_pmuv3 PMU driver, 7
counters available
[ 1.110121] kvm [1]: IPA Size Limit: 44 bits
[ 1.115035] kvm [1]: vgic-v2@c0e0000
[ 1.118651] kvm [1]: GIC system register CPU interface enabled
[ 1.124671] kvm [1]: vgic interrupt IRQ9
[ 1.128787] kvm [1]: Hyp mode initialized successfully
[ 1.137122] Initialise system trusted keyrings
[ 1.141697] workingset: timestamp_bits=44 max_order=22 bucket_order=0
[ 1.150888] squashfs: version 4.0 (2009/01/31) Phillip Lougher
[ 1.157123] NFS: Registering the id_resolver key type
[ 1.162230] Key type id_resolver registered
[ 1.166438] Key type id_legacy registered
[ 1.170508] nfs4filelayout_init: NFSv4 File Layout Driver Registering...
[ 1.177326] 9p: Installing v9fs 9p2000 file system support
[ 1.204782] NET: Registered protocol family 38
[ 1.209256] Key type asymmetric registered
[ 1.213377] Asymmetric key parser 'x509' registered
[ 1.218299] Block layer SCSI generic (bsg) driver version 0.4
loaded (major 244)
[ 1.225741] io scheduler mq-deadline registered
[ 1.230299] io scheduler kyber registered
[ 1.244691] layerscape-pcie 3600000.pcie: host bridge
/soc/pcie@3600000 ranges:
[ 1.252091] layerscape-pcie 3600000.pcie: IO
0x3000010000..0x300001ffff -> 0x0000000000
[ 1.260693] layerscape-pcie 3600000.pcie: MEM
0x3040000000..0x307fffffff -> 0x0040000000
[ 1.269382] layerscape-pcie 3600000.pcie: PCI host bridge to bus 0000:00
[ 1.276126] pci_bus 0000:00: root bus resource [bus 00-ff]
[ 1.281644] pci_bus 0000:00: root bus resource [io 0x0000-0xffff]
[ 1.287862] pci_bus 0000:00: root bus resource [mem
0x3040000000-0x307fffffff] (bus address [0x40000000-0x7fffffff])
[ 1.298475] pci 0000:00:00.0: [1957:8240] type 01 class 0x060400
[ 1.304533] pci 0000:00:00.0: reg 0x10: [mem 0x00000000-0x00ffffff]
[ 1.310844] pci 0000:00:00.0: reg 0x14: [mem 0x00000000-0x03ffffff]
[ 1.317155] pci 0000:00:00.0: reg 0x38: [mem 0x3048000000-0x3048ffffff pref]
[ 1.324323] pci 0000:00:00.0: supports D1 D2
[ 1.328616] pci 0000:00:00.0: PME# supported from D0 D1 D2 D3hot
[ 1.335444] pci 0000:01:00.0: [8086:10d3] type 00 class 0x020000
[ 1.341558] pci 0000:01:00.0: reg 0x10: [mem 0x3049000000-0x304901ffff]
[ 1.348250] pci 0000:01:00.0: reg 0x14: [mem 0x3049080000-0x30490fffff]
[ 1.354941] pci 0000:01:00.0: reg 0x18: [io 0x1000-0x101f]
[ 1.360585] pci 0000:01:00.0: reg 0x1c: [mem 0x3049100000-0x3049103fff]
[ 1.367360] pci 0000:01:00.0: reg 0x30: [mem 0x3049140000-0x304917ffff pref]
[ 1.374722] pci 0000:01:00.0: PME# supported from D0 D3hot D3cold
[ 1.392335] pci 0000:00:00.0: BAR 1: assigned [mem 0x3040000000-0x3043ffffff]
[ 1.399519] pci 0000:00:00.0: BAR 0: assigned [mem 0x3044000000-0x3044ffffff]
[ 1.406703] pci 0000:00:00.0: BAR 6: assigned [mem
0x3045000000-0x3045ffffff pref]
[ 1.414321] pci 0000:00:00.0: BAR 14: assigned [mem
0x3046000000-0x30460fffff]
[ 1.421588] pci 0000:00:00.0: BAR 13: assigned [io 0x1000-0x1fff]
[ 1.427806] pci 0000:01:00.0: BAR 1: assigned [mem 0x3046000000-0x304607ffff]
[ 1.434996] pci 0000:01:00.0: BAR 6: assigned [mem
0x3046080000-0x30460bffff pref]
[ 1.442613] pci 0000:01:00.0: BAR 0: assigned [mem 0x30460c0000-0x30460dffff]
[ 1.449802] pci 0000:01:00.0: BAR 3: assigned [mem 0x30460e0000-0x30460e3fff]
[ 1.456991] pci 0000:01:00.0: BAR 2: assigned [io 0x1000-0x101f]
[ 1.463132] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
[ 1.468388] pci 0000:00:00.0: bridge window [io 0x1000-0x1fff]
[ 1.474519] pci 0000:00:00.0: bridge window [mem 0x3046000000-0x30460fffff]
[ 1.481942] layerscape-pcie 3700000.pcie: host bridge
/soc/pcie@3700000 ranges:
[ 1.489357] layerscape-pcie 3700000.pcie: IO
0x3800010000..0x380001ffff -> 0x0000000000
[ 1.497969] layerscape-pcie 3700000.pcie: MEM
0x3840000000..0x387fffffff -> 0x0040000000
[ 1.506647] layerscape-pcie 3700000.pcie: PCI host bridge to bus 0001:00
[ 1.513390] pci_bus 0001:00: root bus resource [bus 00-ff]
[ 1.518910] pci_bus 0001:00: root bus resource [io
0x10000-0x1ffff] (bus address [0x0000-0xffff])
[ 1.527925] pci_bus 0001:00: root bus resource [mem
0x3840000000-0x387fffffff] (bus address [0x40000000-0x7fffffff])
[ 1.538539] pci 0001:00:00.0: [1957:8240] type 01 class 0x060400
[ 1.544609] pci 0001:00:00.0: reg 0x38: [mem 0x3840000000-0x38400007ff pref]
[ 1.551773] pci 0001:00:00.0: supports D1 D2
[ 1.556066] pci 0001:00:00.0: PME# supported from D0 D1 D2 D3hot
[ 1.563008] SError Interrupt on CPU5, code 0xbf000002 -- SError
[ 1.563010] CPU: 5 PID: 1 Comm: swapper/0 Not tainted
5.9.0-rc7-next-20201001 #1
[ 1.563011] Hardware name: Freescale Layerscape 2088A RDB Board (DT)
[ 1.563013] pstate: 20000085 (nzCv daIf -PAN -UAO -TCO BTYPE=--)
[ 1.563014] pc : pci_generic_config_read+0x44/0xe8
[ 1.563015] lr : pci_generic_config_read+0x2c/0xe8
[ 1.563016] sp : ffff80001005b7d0
[ 1.563017] x29: ffff80001005b7d0 x28: 0000000000000001
[ 1.563020] x27: 0000000000000000 x26: ffff0082eddfb800
[ 1.563022] x25: ffffc428e3e1dde8 x24: 0000000000000000
[ 1.563025] x23: ffff80001005b924 x22: 0000000000000087
[ 1.563027] x21: ffff0082eddfb800 x20: 0000000000000004
[ 1.563029] x19: ffff80001005b864 x18: 0000000000000000
[ 1.563032] x17: 00000000be711609 x16: 000000006455f136
[ 1.563034] x15: ffff0082ee750480 x14: ffffffffffffffff
[ 1.563036] x13: ffff0082cd313a1c x12: ffff0082cd313293
[ 1.563039] x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f
[ 1.563041] x9 : ffffc428e1d4a53c x8 : 0000000000000004
[ 1.563043] x7 : ffff800011200000 x6 : ffff800011200000
[ 1.563046] x5 : 0000000000000003 x4 : 0000000000000004
[ 1.563048] x3 : 0000000000000004 x2 : dda16212b714c600
[ 1.563050] x1 : 0000000000000000 x0 : ffff800010202000
[ 1.563053] Kernel panic - not syncing: Asynchronous SError Interrupt
[ 1.563054] CPU: 5 PID: 1 Comm: swapper/0 Not tainted
5.9.0-rc7-next-20201001 #1
[ 1.563056] Hardware name: Freescale Layerscape 2088A RDB Board (DT)
[ 1.563057] Call trace:
[ 1.563058] dump_backtrace+0x0/0x1d8
[ 1.563059] show_stack+0x20/0x70
[ 1.563059] dump_stack+0xf8/0x168
[ 1.563060] panic+0x184/0x390
[ 1.563061] nmi_panic+0x94/0x98
[ 1.563062] arm64_serror_panic+0x88/0x94
[ 1.563063] do_serror+0xac/0x1c8
[ 1.563064] el1_error+0x88/0x104
[ 1.563065] pci_generic_config_read+0x44/0xe8
[ 1.563066] dw_pcie_rd_other_conf+0x20/0x78
[ 1.563067] pci_bus_read_config_dword+0x88/0xe0
[ 1.563068] pci_bus_generic_read_dev_vendor_id+0x3c/0x1b8
[ 1.563070] pci_bus_read_dev_vendor_id+0x54/0x78
[ 1.563071] pci_scan_single_device+0x84/0xf8
[ 1.563072] pci_scan_slot+0x48/0x128
[ 1.563073] pci_scan_child_bus_extend+0x60/0x340
[ 1.563074] pci_scan_child_bus+0x1c/0x28
[ 1.563075] pci_scan_bridge_extend+0x168/0x5a8
[ 1.563076] pci_scan_child_bus_extend+0x138/0x340
[ 1.563077] pci_scan_root_bus_bridge+0x6c/0xe0
[ 1.563078] pci_host_probe+0x20/0xd0
[ 1.563079] dw_pcie_host_init+0x1b0/0x320
[ 1.563080] ls_pcie_probe+0x108/0x140
[ 1.563081] platform_drv_probe+0x5c/0xb0
[ 1.563082] really_probe+0xf0/0x4d8
[ 1.563083] driver_probe_device+0xfc/0x168
[ 1.563084] device_driver_attach+0x7c/0x88
[ 1.563085] __driver_attach+0xac/0x178
[ 1.563086] bus_for_each_dev+0x78/0xc8
[ 1.563087] driver_attach+0x2c/0x38
[ 1.563088] bus_add_driver+0x14c/0x230
[ 1.563089] driver_register+0x6c/0x128
[ 1.563090] __platform_driver_probe+0x80/0x148
[ 1.563091] ls_pcie_driver_init+0x2c/0x38
[ 1.563092] do_one_initcall+0x4c/0x2d0
[ 1.563093] kernel_init_freeable+0x214/0x280
[ 1.563094] kernel_init+0x1c/0x128
[ 1.563095] ret_from_fork+0x10/0x30
[ 1.563111] SMP: stopping secondary CPUs
[ 1.563112] Kernel Offset: 0x4428d1680000 from 0xffff800010000000
[ 1.563113] PHYS_OFFSET: 0xffffd10300000000
[ 1.563114] CPU features: 0x0240022,21806008
[ 1.563115] Memory Limit: none

link,
https://lavalab.nxp.com/scheduler/job/90794#L791

--
Linaro LKFT
https://lkft.linaro.org

2020-10-08 15:56:10

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Thu, Oct 01, 2020 at 07:02:04PM +0530, Kishon Vijay Abraham I wrote:

[...]

> >> Yeah, I don't see any registers in the DRA7x PCIe wrapper for disabling
> >> error forwarding.
> >
> > It's a DWC port logic register AFAICT, but perhaps not present in all versions.
>
> Okay. I see there's a register PCIECTRL_PL_AXIS_SLV_ERR_RESP which has a
> reset value of 0.
>
> It has four bit-fields, RESET_TIMEOUT_ERR_MAP, NO_VID_ERR_MAP,
> DBI_ERR_MAP and SLAVE_ERR_MAP. I'm not seeing any difference in behavior
> if I set all these bits. Maybe it requires platform support too. I'll
> check this with our design team.
>
> Meanwhile would it be okay to add linkup check atleast for DRA7X so that
> we could have it booting in linux-next?

Do you mind sending a patch on top of my pci/dwc please ?

Thanks,
Lorenzo

2020-10-08 15:56:18

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Thu, Oct 8, 2020 at 9:47 AM Naresh Kamboju <[email protected]> wrote:
>
> On Fri, 2 Oct 2020 at 14:59, Naresh Kamboju <[email protected]> wrote:
> >
> > On Thu, 1 Oct 2020 at 22:16, Michael Walle <[email protected]> wrote:
> > >
> > > Am 2020-10-01 15:32, schrieb Kishon Vijay Abraham I:
> > >
> > > > Meanwhile would it be okay to add linkup check atleast for DRA7X so
> > > > that
> > > > we could have it booting in linux-next?
> > >
> > > Layerscape SoCs (at least the LS1028A) are also still broken in
> > > linux-next,
> > > did I miss something here?
> >
> > I have been monitoring linux next boot and functional testing on nxp devices
> > for more than two week and still the problem exists on nxp-ls2088.
> >
> > Do you mind checking the possibilities to revert bad patches on linux next tree
> > and continue to work on fixes please ?
> >
> > suspected bad commit: [ I have not bisected this problem ]
> > c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
> >
> > crash log snippet:
> > [ 1.563008] SError Interrupt on CPU5, code 0xbf000002 -- SError
> > [ 1.563010] CPU: 5 PID: 1 Comm: swapper/0 Not tainted
> > 5.9.0-rc7-next-20201001 #1
> > [ 1.563011] Hardware name: Freescale Layerscape 2088A RDB Board (DT)
> > [ 1.563013] pstate: 20000085 (nzCv daIf -PAN -UAO -TCO BTYPE=--)
> > [ 1.563014] pc : pci_generic_config_read+0x44/0xe8
> > [ 1.563015] lr : pci_generic_config_read+0x2c/0xe8
>
>
> This reported issue is gone now on Linux next master branch.
> I am not sure which is a fix commit.

There isn't one, better double check that. We're still waiting on
respinning of the revert patch.

BTW, why is the kernelci NXP lab almost always down? I have a branch
now to test things and I'm not done breaking the DWC driver. :)

Rob

2020-10-08 16:44:28

by Naresh Kamboju

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Fri, 2 Oct 2020 at 14:59, Naresh Kamboju <[email protected]> wrote:
>
> On Thu, 1 Oct 2020 at 22:16, Michael Walle <[email protected]> wrote:
> >
> > Am 2020-10-01 15:32, schrieb Kishon Vijay Abraham I:
> >
> > > Meanwhile would it be okay to add linkup check atleast for DRA7X so
> > > that
> > > we could have it booting in linux-next?
> >
> > Layerscape SoCs (at least the LS1028A) are also still broken in
> > linux-next,
> > did I miss something here?
>
> I have been monitoring linux next boot and functional testing on nxp devices
> for more than two week and still the problem exists on nxp-ls2088.
>
> Do you mind checking the possibilities to revert bad patches on linux next tree
> and continue to work on fixes please ?
>
> suspected bad commit: [ I have not bisected this problem ]
> c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
>
> crash log snippet:
> [ 1.563008] SError Interrupt on CPU5, code 0xbf000002 -- SError
> [ 1.563010] CPU: 5 PID: 1 Comm: swapper/0 Not tainted
> 5.9.0-rc7-next-20201001 #1
> [ 1.563011] Hardware name: Freescale Layerscape 2088A RDB Board (DT)
> [ 1.563013] pstate: 20000085 (nzCv daIf -PAN -UAO -TCO BTYPE=--)
> [ 1.563014] pc : pci_generic_config_read+0x44/0xe8
> [ 1.563015] lr : pci_generic_config_read+0x2c/0xe8

This reported issue is gone now on Linux next master branch.
I am not sure which is a fix commit.

- Naresh

2020-10-08 18:25:13

by Naresh Kamboju

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Thu, 8 Oct 2020 at 20:42, Rob Herring <[email protected]> wrote:
>
> On Thu, Oct 8, 2020 at 9:47 AM Naresh Kamboju <[email protected]> wrote:
> >
> > On Fri, 2 Oct 2020 at 14:59, Naresh Kamboju <[email protected]> wrote:
> > >
> > > On Thu, 1 Oct 2020 at 22:16, Michael Walle <[email protected]> wrote:
> > > >
> > > > Am 2020-10-01 15:32, schrieb Kishon Vijay Abraham I:
> > > >
> > > > > Meanwhile would it be okay to add linkup check atleast for DRA7X so
> > > > > that
> > > > > we could have it booting in linux-next?
> > > >
> > > > Layerscape SoCs (at least the LS1028A) are also still broken in
> > > > linux-next,
> > > > did I miss something here?
> > >
> > > I have been monitoring linux next boot and functional testing on nxp devices
> > > for more than two week and still the problem exists on nxp-ls2088.
> > >
> > > Do you mind checking the possibilities to revert bad patches on linux next tree
> > > and continue to work on fixes please ?
> > >
> > > suspected bad commit: [ I have not bisected this problem ]
> > > c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
> > >
> > > crash log snippet:
> > > [ 1.563008] SError Interrupt on CPU5, code 0xbf000002 -- SError
> > > [ 1.563010] CPU: 5 PID: 1 Comm: swapper/0 Not tainted
> > > 5.9.0-rc7-next-20201001 #1
> > > [ 1.563011] Hardware name: Freescale Layerscape 2088A RDB Board (DT)
> > > [ 1.563013] pstate: 20000085 (nzCv daIf -PAN -UAO -TCO BTYPE=--)
> > > [ 1.563014] pc : pci_generic_config_read+0x44/0xe8
> > > [ 1.563015] lr : pci_generic_config_read+0x2c/0xe8
> >
> >
> > This reported issue is gone now on Linux next master branch.

I am taking this verdict back.
I have seen the boot pass and the reported issue is gone but after checking
all 20 test jobs the 4 boot passes without the above reported error.
and later the 16 boot failed with the reported error.

> > I am not sure which is a fix commit.
>
> There isn't one, better double check that. We're still waiting on
> respinning of the revert patch.
>
> BTW, why is the kernelci NXP lab almost always down? I have a branch
> now to test things and I'm not done breaking the DWC driver. :)

Now the NXP LAVA lab is back online after upgrade.
LKFT running daily testing on device "nxp-ls2088" [1]

You can monitor and check results on Linaro Linux next project [2]

[1] https://lavalab.nxp.com/scheduler/device_type/nxp-ls2088
[2] https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20201008/?results_layout=table&failures_only=false#!?details=995,999#test-results

- Naresh

2020-10-12 04:44:41

[permalink] [raw]

Subject: RE: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

> -----Original Message-----
> From: Rob Herring <[email protected]>
> Sent: 2020年9月30日 1:11
> To: Gustavo Pimentel <[email protected]>
> Cc: Z.q. Hou <[email protected]>; Lorenzo Pieralisi
> <[email protected]>; [email protected]; PCI
> <[email protected]>; Bjorn Helgaas <[email protected]>;
> Michael Walle <[email protected]>; Ard Biesheuvel <[email protected]>
> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> dw_child_pcie_ops
>
> On Tue, Sep 29, 2020 at 10:24 AM Gustavo Pimentel
> <[email protected]> wrote:
> >
> > On Tue, Sep 29, 2020 at 5:5:41, Z.q. Hou <[email protected]> wrote:
> >
> > > Hi Lorenzo,
> > >
> > > Thanks a lot for your comments!
> > >
> > > > -----Original Message-----
> > > > From: Lorenzo Pieralisi <[email protected]>
> > > > Sent: 2020年9月28日 17:39
> > > > To: Z.q. Hou <[email protected]>
> > > > Cc: Rob Herring <[email protected]>; [email protected];
> > > > PCI <[email protected]>; Bjorn Helgaas
> > > > <[email protected]>; Gustavo Pimentel
> > > > <[email protected]>; Michael Walle
> <[email protected]>;
> > > > Ard Biesheuvel <[email protected]>
> > > > Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> > > > dw_child_pcie_ops
> > > >
> > > > On Thu, Sep 24, 2020 at 04:24:47AM +0000, Z.q. Hou wrote:
> > > > > Hi Rob,
> > > > >
> > > > > Thanks a lot for your comments!
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Rob Herring <[email protected]>
> > > > > > Sent: 2020年9月18日 23:28
> > > > > > To: Z.q. Hou <[email protected]>
> > > > > > Cc: [email protected]; PCI
> > > > > > <[email protected]>; Lorenzo Pieralisi
> > > > > > <[email protected]>; Bjorn Helgaas
> > > > > > <[email protected]>; Gustavo Pimentel
> > > > > > <[email protected]>; Michael Walle
> > > > <[email protected]>;
> > > > > > Ard Biesheuvel <[email protected]>
> > > > > > Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus
> > > > > > of dw_child_pcie_ops
> > > > > >
> > > > > > On Fri, Sep 18, 2020 at 5:02 AM Z.q. Hou
> > > > > > <[email protected]>
> > > > wrote:
> > > > > > >
> > > > > > > Hi Rob,
> > > > > > >
> > > > > > > Thanks a lot for your comments!
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Rob Herring <[email protected]>
> > > > > > > > Sent: 2020年9月17日 4:29
> > > > > > > > To: Z.q. Hou <[email protected]>
> > > > > > > > Cc: [email protected]; PCI
> > > > > > > > <[email protected]>; Lorenzo Pieralisi
> > > > > > > > <[email protected]>; Bjorn Helgaas
> > > > > > > > <[email protected]>; Gustavo Pimentel
> > > > > > > > <[email protected]>; Michael Walle
> > > > > > <[email protected]>;
> > > > > > > > Ard Biesheuvel <[email protected]>
> > > > > > > > Subject: Re: [PATCH] PCI: dwc: Added link up check in
> > > > > > > > map_bus of dw_child_pcie_ops
> > > > > > > >
> > > > > > > > On Tue, Sep 15, 2020 at 11:49 PM Zhiqiang Hou
> > > > > > <[email protected]>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > From: Hou Zhiqiang <[email protected]>
> > > > > > > > >
> > > > > > > > > On NXP Layerscape platforms, it results in SError in the
> > > > > > > > > enumeration of the PCIe controller, which is not
> > > > > > > > > connecting with an Endpoint device. And it doesn't make
> > > > > > > > > sense to enumerate the Endpoints when the PCIe link is
> > > > > > > > > down. So this patch added the link up check to avoid to
> > > > > > > > > fire configuration
> > > > transactions on link down bus.
> > > > > > > >
> > > > > > > > Michael reported the same issue as well.
> > > > > > > >
> > > > > > > > What happens if the link goes down between the check and
> > > > > > > > the
> > > > access?
> > > > > > >
> > > > > > > This patch cannot cover this case, and will get the SError.
> > > > > > > But I think it makes sense to avoid firing transactions on link down
> bus.
> > > > > >
> > > > > > That's impossible to do without a race even in h/w.
> > > > >
> > > > > Agree.
> > > > >
> > > > > >
> > > > > > > > It's a racy check. I'd like to find an alternative solution.
> > > > > > > > It's even worse if Layerscape is used in ECAM mode. I
> > > > > > > > looked at the EDK2 setup for layerscape[1] and it looks
> > > > > > > > like root ports are just skipped if link
> > > > > > is down.
> > > > > > > > Maybe a link down just never happens once up, but if so,
> > > > > > > > then we only need to check it once and fail probe.
> > > > > > >
> > > > > > > Many customers connect the FPGA Endpoint, which may
> > > > > > > establish PCIe link after the PCIe enumeration and then
> > > > > > > rescan the PCIe bus, so I think it should not exit the probe
> > > > > > > of root port even if there is not link up
> > > > > > during enumeration.
> > > > > >
> > > > > > That's a good reason. I want to unify the behavior here as it
> > > > > > varies per platform currently and wasn't sure which way to go.
> > > > > >
> > > > > >
> > > > > > > > I've dug into this a bit more and am curious about the
> > > > > > > > PCIE_ABSERR register setting which is set to:
> > > > > > > >
> > > > > > > > #define PCIE_ABSERR_SETTING 0x9401 /* Forward error of
> > > > > > > > non-posted request */
> > > > > > > >
> > > > > > > > It seems to me this is not what we want at least for
> > > > > > > > config accesses, but commit 84d897d6993 where this was
> > > > > > > > added seems to say otherwise. Is it not possible to
> > > > > > > > configure the response per access
> > > > type?
> > > > > > >
> > > > > > > Thanks a lot for your investigation!
> > > > > > > The story is like this: Some customers worry about these
> > > > > > > silent error (DWC PCIe IP won't forward the error of
> > > > > > > outbound non-post request by default), so we were pushed to
> > > > > > > enable the error forwarding to AXI in the commit
> > > > > > > 84d897d6993 as you saw. But it cannot differentiate the
> > > > > > > config transactions from the MEM_rd, except the Vendor ID
> > > > > > > access, which is controlled by a separate bit and it was set
> > > > > > > to not forward error of access
> > > > > > of Vendor ID.
> > > > > > > So we think it's okay to enable the error forwarding, the
> > > > > > > SError should not occur, because after the enumeration it
> > > > > > > won't access the
> > > > > > non-existent functions.
> > > > > >
> > > > > > We've rejected upstream support for platforms aborting on
> > > > > > config accesses[1]. I think there's clear consensus that
> > > > > > aborting is the wrong behavior.
> > > > > >
> > > > > > Do MEM_wr errors get forwarded? Seems like that would be
> enough.
> > > > > > Also, wouldn't page faults catch most OOB accesses anyways?
> > > > > > You need things page aligned anyways with an IOMMU and doing
> > > > > > userspace access or guest assignment.
> > > > >
> > > > > Yes, errors of MEM_wr can be forwarded.
> > > > >
> > > > > >
> > > > > > Here's another idea, how about only enabling forwarding errors
> > > > > > if the link is up? If really would need to be configured any
> > > > > > time the link state changes rather than just at probe. I'm not
> > > > > > sure if you have a way to disable it on link down though.
> > > > >
> > > > > Dug deeper into this issue and found the setting of not
> > > > > forwarding error of non-existent Vender ID access counts on the link
> partner: 1.
> > > > > When there is a link partner (namely link up), it will return
> > > > > 0xffff when read non-existent function Vendor ID and won't
> > > > > forward error to AXI. 2. When no link partner (link down), it
> > > > > will forward the error of reading non-existent function Vendor
> > > > > ID to AXI and result in SError.
> > > > >
> > > > > I think this is a DWC PCIe IP specific issue but not get
> > > > > feedback from design team. I'm thinking to disable this error
> > > > > forwarding just like other platforms, since when these errors
> > > > > (UR, CA and CT) are detected, AER driver can also report the error
> and try to recover.
> > > >
> > > > I take this as you shall send a patch to fix this issue shortly, is this
> correct ?
> > >
> > > The issue becomes complex:
> > > I reviewed the DWC PCIe databook of verion 4.40a which is used on
> Layerscape platforms, and it said that " Your RC application should not
> generate CFG requests until it has confirmed that the link is up by sampling
> the smlh_link_up and rmlh_link_up outputs".
> > > So, the link up checking should not be remove before each outbound CFG
> access.
> > > Gustavo, can you share more details on the link up checking? Does it only
> exist in the 4.40a?
> >
> > Hi Zhiqiang,
> >
> > According to the information that I got from the IP team you are
> > correct, the same requirement still exists on the newer IP versions.
>
> How is that possible in a race free way?
>
> Testing on meson and layerscape (with the forwarding of errors
> disabled) shows a link check is not needed. But then dra7xx seems to need
> one (or has some f/w setup).

Seems i.MX PCIe driver also has to check link up, otherwise the SError will occur.
Actually I found when the link is not up, the i.MX PCIe driver will probe fail and not continue to enumeration, so I'm wondering how it happens on i.MX. I have email i.MX owner to query the detail, but seems he's still on Chinese National Day holiday and does not feedback.

Thanks,
Zhiqiang

>
> Rob

2020-10-12 04:50:03

[permalink] [raw]

Subject: RE: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

Hi Kishon,

> -----Original Message-----
> From: Kishon Vijay Abraham I <[email protected]>
> Sent: 2020年10月1日 21:32
> To: Rob Herring <[email protected]>
> Cc: Gustavo Pimentel <[email protected]>; Z.q. Hou
> <[email protected]>; Lorenzo Pieralisi <[email protected]>;
> [email protected]; PCI <[email protected]>; Bjorn
> Helgaas <[email protected]>; Michael Walle <[email protected]>; Ard
> Biesheuvel <[email protected]>
> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> dw_child_pcie_ops
>
> Hi Rob,
>
> On 30/09/20 8:31 pm, Rob Herring wrote:
> > On Wed, Sep 30, 2020 at 8:22 AM Kishon Vijay Abraham I <[email protected]>
> wrote:
> >>
> >> Hi,
> >>
> >> On 29/09/20 10:41 pm, Rob Herring wrote:
> >>> On Tue, Sep 29, 2020 at 10:24 AM Gustavo Pimentel
> >>> <[email protected]> wrote:
> >>>>
> >>>> On Tue, Sep 29, 2020 at 5:5:41, Z.q. Hou <[email protected]>
> wrote:
> >>>>
> >>>>> Hi Lorenzo,
> >>>>>
> >>>>> Thanks a lot for your comments!
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Lorenzo Pieralisi <[email protected]>
> >>>>>> Sent: 2020年9月28日 17:39
> >>>>>> To: Z.q. Hou <[email protected]>
> >>>>>> Cc: Rob Herring <[email protected]>; [email protected];
> >>>>>> PCI <[email protected]>; Bjorn Helgaas
> >>>>>> <[email protected]>; Gustavo Pimentel
> >>>>>> <[email protected]>; Michael Walle
> >>>>>> <[email protected]>; Ard Biesheuvel <[email protected]>
> >>>>>> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> >>>>>> dw_child_pcie_ops
> >>>>>>
> >>>>>> On Thu, Sep 24, 2020 at 04:24:47AM +0000, Z.q. Hou wrote:
> >>>>>>> Hi Rob,
> >>>>>>>
> >>>>>>> Thanks a lot for your comments!
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Rob Herring <[email protected]>
> >>>>>>>> Sent: 2020年9月18日 23:28
> >>>>>>>> To: Z.q. Hou <[email protected]>
> >>>>>>>> Cc: [email protected]; PCI
> >>>>>>>> <[email protected]>; Lorenzo Pieralisi
> >>>>>>>> <[email protected]>; Bjorn Helgaas
> >>>>>>>> <[email protected]>; Gustavo Pimentel
> >>>>>>>> <[email protected]>; Michael Walle
> >>>>>> <[email protected]>;
> >>>>>>>> Ard Biesheuvel <[email protected]>
> >>>>>>>> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus
> >>>>>>>> of dw_child_pcie_ops
> >>>>>>>>
> >>>>>>>> On Fri, Sep 18, 2020 at 5:02 AM Z.q. Hou
> <[email protected]>
> >>>>>> wrote:
> >>>>>>>>>
> >>>>>>>>> Hi Rob,
> >>>>>>>>>
> >>>>>>>>> Thanks a lot for your comments!
> >>>>>>>>>
> >>>>>>>>>> -----Original Message-----
> >>>>>>>>>> From: Rob Herring <[email protected]>
> >>>>>>>>>> Sent: 2020年9月17日 4:29
> >>>>>>>>>> To: Z.q. Hou <[email protected]>
> >>>>>>>>>> Cc: [email protected]; PCI
> >>>>>>>>>> <[email protected]>; Lorenzo Pieralisi
> >>>>>>>>>> <[email protected]>; Bjorn Helgaas
> >>>>>>>>>> <[email protected]>; Gustavo Pimentel
> >>>>>>>>>> <[email protected]>; Michael Walle
> >>>>>>>> <[email protected]>;
> >>>>>>>>>> Ard Biesheuvel <[email protected]>
> >>>>>>>>>> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus
> >>>>>>>>>> of dw_child_pcie_ops
> >>>>>>>>>>
> >>>>>>>>>> On Tue, Sep 15, 2020 at 11:49 PM Zhiqiang Hou
> >>>>>>>> <[email protected]>
> >>>>>>>>>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> From: Hou Zhiqiang <[email protected]>
> >>>>>>>>>>>
> >>>>>>>>>>> On NXP Layerscape platforms, it results in SError in the
> >>>>>>>>>>> enumeration of the PCIe controller, which is not connecting
> >>>>>>>>>>> with an Endpoint device. And it doesn't make sense to
> >>>>>>>>>>> enumerate the Endpoints when the PCIe link is down. So this
> >>>>>>>>>>> patch added the link up check to avoid to fire configuration
> >>>>>> transactions on link down bus.
> >>>>>>>>>>
> >>>>>>>>>> Michael reported the same issue as well.
> >>>>>>>>>>
> >>>>>>>>>> What happens if the link goes down between the check and the
> >>>>>> access?
> >>>>>>>>>
> >>>>>>>>> This patch cannot cover this case, and will get the SError.
> >>>>>>>>> But I think it makes sense to avoid firing transactions on link
> down bus.
> >>>>>>>>
> >>>>>>>> That's impossible to do without a race even in h/w.
> >>>>>>>
> >>>>>>> Agree.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>>> It's a racy check. I'd like to find an alternative solution.
> >>>>>>>>>> It's even worse if Layerscape is used in ECAM mode. I looked
> >>>>>>>>>> at the EDK2 setup for layerscape[1] and it looks like root
> >>>>>>>>>> ports are just skipped if link
> >>>>>>>> is down.
> >>>>>>>>>> Maybe a link down just never happens once up, but if so, then
> >>>>>>>>>> we only need to check it once and fail probe.
> >>>>>>>>>
> >>>>>>>>> Many customers connect the FPGA Endpoint, which may
> establish
> >>>>>>>>> PCIe link after the PCIe enumeration and then rescan the PCIe
> >>>>>>>>> bus, so I think it should not exit the probe of root port even
> >>>>>>>>> if there is not link up
> >>>>>>>> during enumeration.
> >>>>>>>>
> >>>>>>>> That's a good reason. I want to unify the behavior here as it
> >>>>>>>> varies per platform currently and wasn't sure which way to go.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>> I've dug into this a bit more and am curious about the
> >>>>>>>>>> PCIE_ABSERR register setting which is set to:
> >>>>>>>>>>
> >>>>>>>>>> #define PCIE_ABSERR_SETTING 0x9401 /* Forward error of
> >>>>>>>>>> non-posted request */
> >>>>>>>>>>
> >>>>>>>>>> It seems to me this is not what we want at least for config
> >>>>>>>>>> accesses, but commit 84d897d6993 where this was added
> seems
> >>>>>>>>>> to say otherwise. Is it not possible to configure the
> >>>>>>>>>> response per access
> >>>>>> type?
> >>>>>>>>>
> >>>>>>>>> Thanks a lot for your investigation!
> >>>>>>>>> The story is like this: Some customers worry about these
> >>>>>>>>> silent error (DWC PCIe IP won't forward the error of outbound
> >>>>>>>>> non-post request by default), so we were pushed to enable the
> >>>>>>>>> error forwarding to AXI in the commit
> >>>>>>>>> 84d897d6993 as you saw. But it cannot differentiate the config
> >>>>>>>>> transactions from the MEM_rd, except the Vendor ID access,
> >>>>>>>>> which is controlled by a separate bit and it was set to not
> >>>>>>>>> forward error of access
> >>>>>>>> of Vendor ID.
> >>>>>>>>> So we think it's okay to enable the error forwarding, the
> >>>>>>>>> SError should not occur, because after the enumeration it
> >>>>>>>>> won't access the
> >>>>>>>> non-existent functions.
> >>>>>>>>
> >>>>>>>> We've rejected upstream support for platforms aborting on
> >>>>>>>> config accesses[1]. I think there's clear consensus that
> >>>>>>>> aborting is the wrong behavior.
> >>>>>>>>
> >>>>>>>> Do MEM_wr errors get forwarded? Seems like that would be
> enough.
> >>>>>>>> Also, wouldn't page faults catch most OOB accesses anyways?
> You
> >>>>>>>> need things page aligned anyways with an IOMMU and doing
> >>>>>>>> userspace access or guest assignment.
> >>>>>>>
> >>>>>>> Yes, errors of MEM_wr can be forwarded.
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Here's another idea, how about only enabling forwarding errors
> >>>>>>>> if the link is up? If really would need to be configured any
> >>>>>>>> time the link state changes rather than just at probe. I'm not
> >>>>>>>> sure if you have a way to disable it on link down though.
> >>>>>>>
> >>>>>>> Dug deeper into this issue and found the setting of not
> >>>>>>> forwarding error of non-existent Vender ID access counts on the
> link partner: 1.
> >>>>>>> When there is a link partner (namely link up), it will return
> >>>>>>> 0xffff when read non-existent function Vendor ID and won't
> >>>>>>> forward error to AXI. 2. When no link partner (link down), it
> >>>>>>> will forward the error of reading non-existent function Vendor
> >>>>>>> ID to AXI and result in SError.
> >>>>>>>
> >>>>>>> I think this is a DWC PCIe IP specific issue but not get
> >>>>>>> feedback from design team. I'm thinking to disable this error
> >>>>>>> forwarding just like other platforms, since when these errors
> >>>>>>> (UR, CA and CT) are detected, AER driver can also report the error
> and try to recover.
> >>>>>>
> >>>>>> I take this as you shall send a patch to fix this issue shortly, is this
> correct ?
> >>>>>
> >>>>> The issue becomes complex:
> >>>>> I reviewed the DWC PCIe databook of verion 4.40a which is used on
> Layerscape platforms, and it said that " Your RC application should not
> generate CFG requests until it has confirmed that the link is up by sampling
> the smlh_link_up and rmlh_link_up outputs".
> >>>>> So, the link up checking should not be remove before each outbound
> CFG access.
> >>>>> Gustavo, can you share more details on the link up checking? Does it
> only exist in the 4.40a?
> >>>>
> >>>> Hi Zhiqiang,
> >>>>
> >>>> According to the information that I got from the IP team you are
> >>>> correct, the same requirement still exists on the newer IP versions.
> >>>
> >>> How is that possible in a race free way?
> >>>
> >>> Testing on meson and layerscape (with the forwarding of errors
> >>> disabled) shows a link check is not needed. But then dra7xx seems to
> >>> need one (or has some f/w setup).
> >>
> >> Yeah, I don't see any registers in the DRA7x PCIe wrapper for
> >> disabling error forwarding.
> >
> > It's a DWC port logic register AFAICT, but perhaps not present in all
> versions.
>
> Okay. I see there's a register PCIECTRL_PL_AXIS_SLV_ERR_RESP which has a
> reset value of 0.
>
> It has four bit-fields, RESET_TIMEOUT_ERR_MAP, NO_VID_ERR_MAP,
> DBI_ERR_MAP and SLAVE_ERR_MAP. I'm not seeing any difference in
> behavior if I set all these bits. Maybe it requires platform support too. I'll
> check this with our design team.

In DWC v4.40a databook, there is a bit AMBA_ERROR_RESPONSE_GLOBAL which controls if enable the error forwarding. The *MAP bits only determine which error (SLVERR or DECERR) will be forwarded to AXI/AHB bus.

Thanks,
Zhiqiang

>
> Meanwhile would it be okay to add linkup check atleast for DRA7X so that
> we could have it booting in linux-next?
>
> Thanks
> Kishon

2020-10-13 07:30:16

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

Hi Lorenzo,

On 08/10/20 8:38 pm, Lorenzo Pieralisi wrote:
> On Thu, Oct 01, 2020 at 07:02:04PM +0530, Kishon Vijay Abraham I wrote:
>
> [...]
>
>>>> Yeah, I don't see any registers in the DRA7x PCIe wrapper for disabling
>>>> error forwarding.
>>>
>>> It's a DWC port logic register AFAICT, but perhaps not present in all versions.
>>
>> Okay. I see there's a register PCIECTRL_PL_AXIS_SLV_ERR_RESP which has a
>> reset value of 0.
>>
>> It has four bit-fields, RESET_TIMEOUT_ERR_MAP, NO_VID_ERR_MAP,
>> DBI_ERR_MAP and SLAVE_ERR_MAP. I'm not seeing any difference in behavior
>> if I set all these bits. Maybe it requires platform support too. I'll
>> check this with our design team.
>>
>> Meanwhile would it be okay to add linkup check atleast for DRA7X so that
>> we could have it booting in linux-next?
>
> Do you mind sending a patch on top of my pci/dwc please ?

I just tried applying this on your pci/dwc branch and it applied without
any conflicts. Please let me know if you still want me or Hou to resend
the patch.

Thank you,
Kishon

2020-10-13 12:03:08

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Wed, Sep 16, 2020 at 01:41:30PM +0800, Zhiqiang Hou wrote:
> From: Hou Zhiqiang <[email protected]>
>
> On NXP Layerscape platforms, it results in SError in the
> enumeration of the PCIe controller, which is not connecting
> with an Endpoint device. And it doesn't make sense to
> enumerate the Endpoints when the PCIe link is down. So this
> patch added the link up check to avoid to fire configuration
> transactions on link down bus.
>
> [ 0.807773] SError Interrupt on CPU2, code 0xbf000002 -- SError
> [ 0.807775] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> [ 0.807776] Hardware name: LS1046A RDB Board (DT)
> [ 0.807777] pstate: 20000085 (nzCv daIf -PAN -UAO BTYPE=--)
> [ 0.807778] pc : pci_generic_config_read+0x3c/0xe0
> [ 0.807778] lr : pci_generic_config_read+0x24/0xe0
> [ 0.807779] sp : ffff80001003b7b0
> [ 0.807780] x29: ffff80001003b7b0 x28: ffff80001003ba74
> [ 0.807782] x27: ffff000971d96800 x26: ffff00096e77e0a8
> [ 0.807784] x25: ffff80001003b874 x24: ffff80001003b924
> [ 0.807786] x23: 0000000000000004 x22: 0000000000000000
> [ 0.807788] x21: 0000000000000000 x20: ffff80001003b874
> [ 0.807790] x19: 0000000000000004 x18: ffffffffffffffff
> [ 0.807791] x17: 00000000000000c0 x16: fffffe0025981840
> [ 0.807793] x15: ffffb94c75b69948 x14: 62203a383634203a
> [ 0.807795] x13: 666e6f635f726568 x12: 202c31203d207265
> [ 0.807797] x11: 626d756e3e2d7375 x10: 656877202c307830
> [ 0.807799] x9 : 203d206e66766564 x8 : 0000000000000908
> [ 0.807801] x7 : 0000000000000908 x6 : ffff800010900000
> [ 0.807802] x5 : ffff00096e77e080 x4 : 0000000000000000
> [ 0.807804] x3 : 0000000000000003 x2 : 84fa3440ff7e7000
> [ 0.807806] x1 : 0000000000000000 x0 : ffff800010034000
> [ 0.807808] Kernel panic - not syncing: Asynchronous SError Interrupt
> [ 0.807809] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> [ 0.807810] Hardware name: LS1046A RDB Board (DT)
> [ 0.807811] Call trace:
> [ 0.807812] dump_backtrace+0x0/0x1c0
> [ 0.807813] show_stack+0x18/0x28
> [ 0.807814] dump_stack+0xd8/0x134
> [ 0.807814] panic+0x180/0x398
> [ 0.807815] add_taint+0x0/0xb0
> [ 0.807816] arm64_serror_panic+0x78/0x88
> [ 0.807817] do_serror+0x68/0x180
> [ 0.807818] el1_error+0x84/0x100
> [ 0.807818] pci_generic_config_read+0x3c/0xe0
> [ 0.807819] dw_pcie_rd_other_conf+0x78/0x110
> [ 0.807820] pci_bus_read_config_dword+0x88/0xe8
> [ 0.807821] pci_bus_generic_read_dev_vendor_id+0x30/0x1b0
> [ 0.807822] pci_bus_read_dev_vendor_id+0x4c/0x78
> [ 0.807823] pci_scan_single_device+0x80/0x100
> [ 0.807824] pci_scan_slot+0x38/0x130
> [ 0.807825] pci_scan_child_bus_extend+0x54/0x2a0
> [ 0.807826] pci_scan_child_bus+0x14/0x20
> [ 0.807827] pci_scan_bridge_extend+0x230/0x570
> [ 0.807828] pci_scan_child_bus_extend+0x134/0x2a0
> [ 0.807829] pci_scan_root_bus_bridge+0x64/0xf0
> [ 0.807829] pci_host_probe+0x18/0xc8
> [ 0.807830] dw_pcie_host_init+0x220/0x378
> [ 0.807831] ls_pcie_probe+0x104/0x140
> [ 0.807832] platform_drv_probe+0x54/0xa8
> [ 0.807833] really_probe+0x118/0x3e0
> [ 0.807834] driver_probe_device+0x5c/0xc0
> [ 0.807835] device_driver_attach+0x74/0x80
> [ 0.807835] __driver_attach+0x8c/0xd8
> [ 0.807836] bus_for_each_dev+0x7c/0xd8
> [ 0.807837] driver_attach+0x24/0x30
> [ 0.807838] bus_add_driver+0x154/0x200
> [ 0.807839] driver_register+0x64/0x120
> [ 0.807839] __platform_driver_probe+0x7c/0x148
> [ 0.807840] ls_pcie_driver_init+0x24/0x30
> [ 0.807841] do_one_initcall+0x60/0x1d8
> [ 0.807842] kernel_init_freeable+0x1f4/0x24c
> [ 0.807843] kernel_init+0x14/0x118
> [ 0.807843] ret_from_fork+0x10/0x34
> [ 0.807854] SMP: stopping secondary CPUs
> [ 0.807855] Kernel Offset: 0x394c64080000 from 0xffff800010000000
> [ 0.807856] PHYS_OFFSET: 0xffff8bfd40000000
> [ 0.807856] CPU features: 0x0240022,21806000
> [ 0.807857] Memory Limit: none
>
> Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
> Signed-off-by: Hou Zhiqiang <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-designware-host.c | 6 ++++++
> 1 file changed, 6 insertions(+)

Rob,

do you mind if I squash this in with the commit it is fixing ?

Please let me know.

Thanks,
Lorenzo

> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index c01c9d2fb3f9..e82b518430c5 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -442,6 +442,9 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> struct pcie_port *pp = bus->sysdata;
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>
> + if (!dw_pcie_link_up(pci))
> + return NULL;
> +
> busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> PCIE_ATU_FUNC(PCI_FUNC(devfn));
>
> --
> 2.17.1
>

2020-10-14 21:14:10

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Wed, Oct 14, 2020 at 6:13 AM Lorenzo Pieralisi
<[email protected]> wrote:
>
> On Wed, Sep 16, 2020 at 01:41:30PM +0800, Zhiqiang Hou wrote:
> > From: Hou Zhiqiang <[email protected]>
> >
> > On NXP Layerscape platforms, it results in SError in the
> > enumeration of the PCIe controller, which is not connecting
> > with an Endpoint device. And it doesn't make sense to
> > enumerate the Endpoints when the PCIe link is down. So this
> > patch added the link up check to avoid to fire configuration
> > transactions on link down bus.
> >
> > [ 0.807773] SError Interrupt on CPU2, code 0xbf000002 -- SError
> > [ 0.807775] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> > [ 0.807776] Hardware name: LS1046A RDB Board (DT)
> > [ 0.807777] pstate: 20000085 (nzCv daIf -PAN -UAO BTYPE=--)
> > [ 0.807778] pc : pci_generic_config_read+0x3c/0xe0
> > [ 0.807778] lr : pci_generic_config_read+0x24/0xe0
> > [ 0.807779] sp : ffff80001003b7b0
> > [ 0.807780] x29: ffff80001003b7b0 x28: ffff80001003ba74
> > [ 0.807782] x27: ffff000971d96800 x26: ffff00096e77e0a8
> > [ 0.807784] x25: ffff80001003b874 x24: ffff80001003b924
> > [ 0.807786] x23: 0000000000000004 x22: 0000000000000000
> > [ 0.807788] x21: 0000000000000000 x20: ffff80001003b874
> > [ 0.807790] x19: 0000000000000004 x18: ffffffffffffffff
> > [ 0.807791] x17: 00000000000000c0 x16: fffffe0025981840
> > [ 0.807793] x15: ffffb94c75b69948 x14: 62203a383634203a
> > [ 0.807795] x13: 666e6f635f726568 x12: 202c31203d207265
> > [ 0.807797] x11: 626d756e3e2d7375 x10: 656877202c307830
> > [ 0.807799] x9 : 203d206e66766564 x8 : 0000000000000908
> > [ 0.807801] x7 : 0000000000000908 x6 : ffff800010900000
> > [ 0.807802] x5 : ffff00096e77e080 x4 : 0000000000000000
> > [ 0.807804] x3 : 0000000000000003 x2 : 84fa3440ff7e7000
> > [ 0.807806] x1 : 0000000000000000 x0 : ffff800010034000
> > [ 0.807808] Kernel panic - not syncing: Asynchronous SError Interrupt
> > [ 0.807809] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> > [ 0.807810] Hardware name: LS1046A RDB Board (DT)
> > [ 0.807811] Call trace:
> > [ 0.807812] dump_backtrace+0x0/0x1c0
> > [ 0.807813] show_stack+0x18/0x28
> > [ 0.807814] dump_stack+0xd8/0x134
> > [ 0.807814] panic+0x180/0x398
> > [ 0.807815] add_taint+0x0/0xb0
> > [ 0.807816] arm64_serror_panic+0x78/0x88
> > [ 0.807817] do_serror+0x68/0x180
> > [ 0.807818] el1_error+0x84/0x100
> > [ 0.807818] pci_generic_config_read+0x3c/0xe0
> > [ 0.807819] dw_pcie_rd_other_conf+0x78/0x110
> > [ 0.807820] pci_bus_read_config_dword+0x88/0xe8
> > [ 0.807821] pci_bus_generic_read_dev_vendor_id+0x30/0x1b0
> > [ 0.807822] pci_bus_read_dev_vendor_id+0x4c/0x78
> > [ 0.807823] pci_scan_single_device+0x80/0x100
> > [ 0.807824] pci_scan_slot+0x38/0x130
> > [ 0.807825] pci_scan_child_bus_extend+0x54/0x2a0
> > [ 0.807826] pci_scan_child_bus+0x14/0x20
> > [ 0.807827] pci_scan_bridge_extend+0x230/0x570
> > [ 0.807828] pci_scan_child_bus_extend+0x134/0x2a0
> > [ 0.807829] pci_scan_root_bus_bridge+0x64/0xf0
> > [ 0.807829] pci_host_probe+0x18/0xc8
> > [ 0.807830] dw_pcie_host_init+0x220/0x378
> > [ 0.807831] ls_pcie_probe+0x104/0x140
> > [ 0.807832] platform_drv_probe+0x54/0xa8
> > [ 0.807833] really_probe+0x118/0x3e0
> > [ 0.807834] driver_probe_device+0x5c/0xc0
> > [ 0.807835] device_driver_attach+0x74/0x80
> > [ 0.807835] __driver_attach+0x8c/0xd8
> > [ 0.807836] bus_for_each_dev+0x7c/0xd8
> > [ 0.807837] driver_attach+0x24/0x30
> > [ 0.807838] bus_add_driver+0x154/0x200
> > [ 0.807839] driver_register+0x64/0x120
> > [ 0.807839] __platform_driver_probe+0x7c/0x148
> > [ 0.807840] ls_pcie_driver_init+0x24/0x30
> > [ 0.807841] do_one_initcall+0x60/0x1d8
> > [ 0.807842] kernel_init_freeable+0x1f4/0x24c
> > [ 0.807843] kernel_init+0x14/0x118
> > [ 0.807843] ret_from_fork+0x10/0x34
> > [ 0.807854] SMP: stopping secondary CPUs
> > [ 0.807855] Kernel Offset: 0x394c64080000 from 0xffff800010000000
> > [ 0.807856] PHYS_OFFSET: 0xffff8bfd40000000
> > [ 0.807856] CPU features: 0x0240022,21806000
> > [ 0.807857] Memory Limit: none
> >
> > Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
>
> Hi Rob,
>
> can I squash this patch into the commit above ?

Okay on applying, but better to not squash it so we can see why it is needed.

Rob

2020-10-14 23:02:41

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Wed, Sep 16, 2020 at 01:41:30PM +0800, Zhiqiang Hou wrote:
> From: Hou Zhiqiang <[email protected]>
>
> On NXP Layerscape platforms, it results in SError in the
> enumeration of the PCIe controller, which is not connecting
> with an Endpoint device. And it doesn't make sense to
> enumerate the Endpoints when the PCIe link is down. So this
> patch added the link up check to avoid to fire configuration
> transactions on link down bus.
>
> [ 0.807773] SError Interrupt on CPU2, code 0xbf000002 -- SError
> [ 0.807775] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> [ 0.807776] Hardware name: LS1046A RDB Board (DT)
> [ 0.807777] pstate: 20000085 (nzCv daIf -PAN -UAO BTYPE=--)
> [ 0.807778] pc : pci_generic_config_read+0x3c/0xe0
> [ 0.807778] lr : pci_generic_config_read+0x24/0xe0
> [ 0.807779] sp : ffff80001003b7b0
> [ 0.807780] x29: ffff80001003b7b0 x28: ffff80001003ba74
> [ 0.807782] x27: ffff000971d96800 x26: ffff00096e77e0a8
> [ 0.807784] x25: ffff80001003b874 x24: ffff80001003b924
> [ 0.807786] x23: 0000000000000004 x22: 0000000000000000
> [ 0.807788] x21: 0000000000000000 x20: ffff80001003b874
> [ 0.807790] x19: 0000000000000004 x18: ffffffffffffffff
> [ 0.807791] x17: 00000000000000c0 x16: fffffe0025981840
> [ 0.807793] x15: ffffb94c75b69948 x14: 62203a383634203a
> [ 0.807795] x13: 666e6f635f726568 x12: 202c31203d207265
> [ 0.807797] x11: 626d756e3e2d7375 x10: 656877202c307830
> [ 0.807799] x9 : 203d206e66766564 x8 : 0000000000000908
> [ 0.807801] x7 : 0000000000000908 x6 : ffff800010900000
> [ 0.807802] x5 : ffff00096e77e080 x4 : 0000000000000000
> [ 0.807804] x3 : 0000000000000003 x2 : 84fa3440ff7e7000
> [ 0.807806] x1 : 0000000000000000 x0 : ffff800010034000
> [ 0.807808] Kernel panic - not syncing: Asynchronous SError Interrupt
> [ 0.807809] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> [ 0.807810] Hardware name: LS1046A RDB Board (DT)
> [ 0.807811] Call trace:
> [ 0.807812] dump_backtrace+0x0/0x1c0
> [ 0.807813] show_stack+0x18/0x28
> [ 0.807814] dump_stack+0xd8/0x134
> [ 0.807814] panic+0x180/0x398
> [ 0.807815] add_taint+0x0/0xb0
> [ 0.807816] arm64_serror_panic+0x78/0x88
> [ 0.807817] do_serror+0x68/0x180
> [ 0.807818] el1_error+0x84/0x100
> [ 0.807818] pci_generic_config_read+0x3c/0xe0
> [ 0.807819] dw_pcie_rd_other_conf+0x78/0x110
> [ 0.807820] pci_bus_read_config_dword+0x88/0xe8
> [ 0.807821] pci_bus_generic_read_dev_vendor_id+0x30/0x1b0
> [ 0.807822] pci_bus_read_dev_vendor_id+0x4c/0x78
> [ 0.807823] pci_scan_single_device+0x80/0x100
> [ 0.807824] pci_scan_slot+0x38/0x130
> [ 0.807825] pci_scan_child_bus_extend+0x54/0x2a0
> [ 0.807826] pci_scan_child_bus+0x14/0x20
> [ 0.807827] pci_scan_bridge_extend+0x230/0x570
> [ 0.807828] pci_scan_child_bus_extend+0x134/0x2a0
> [ 0.807829] pci_scan_root_bus_bridge+0x64/0xf0
> [ 0.807829] pci_host_probe+0x18/0xc8
> [ 0.807830] dw_pcie_host_init+0x220/0x378
> [ 0.807831] ls_pcie_probe+0x104/0x140
> [ 0.807832] platform_drv_probe+0x54/0xa8
> [ 0.807833] really_probe+0x118/0x3e0
> [ 0.807834] driver_probe_device+0x5c/0xc0
> [ 0.807835] device_driver_attach+0x74/0x80
> [ 0.807835] __driver_attach+0x8c/0xd8
> [ 0.807836] bus_for_each_dev+0x7c/0xd8
> [ 0.807837] driver_attach+0x24/0x30
> [ 0.807838] bus_add_driver+0x154/0x200
> [ 0.807839] driver_register+0x64/0x120
> [ 0.807839] __platform_driver_probe+0x7c/0x148
> [ 0.807840] ls_pcie_driver_init+0x24/0x30
> [ 0.807841] do_one_initcall+0x60/0x1d8
> [ 0.807842] kernel_init_freeable+0x1f4/0x24c
> [ 0.807843] kernel_init+0x14/0x118
> [ 0.807843] ret_from_fork+0x10/0x34
> [ 0.807854] SMP: stopping secondary CPUs
> [ 0.807855] Kernel Offset: 0x394c64080000 from 0xffff800010000000
> [ 0.807856] PHYS_OFFSET: 0xffff8bfd40000000
> [ 0.807856] CPU features: 0x0240022,21806000
> [ 0.807857] Memory Limit: none
>
> Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")

Hi Rob,

can I squash this patch into the commit above ?

Thanks,
Lorenzo

> Signed-off-by: Hou Zhiqiang <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-designware-host.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index c01c9d2fb3f9..e82b518430c5 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -442,6 +442,9 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> struct pcie_port *pp = bus->sysdata;
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>
> + if (!dw_pcie_link_up(pci))
> + return NULL;
> +
> busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> PCIE_ATU_FUNC(PCI_FUNC(devfn));
>
> --
> 2.17.1
>

2020-10-16 02:04:36

by Bjorn Helgaas

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Wed, Sep 16, 2020 at 01:41:30PM +0800, Zhiqiang Hou wrote:
> From: Hou Zhiqiang <[email protected]>
>
> On NXP Layerscape platforms, it results in SError in the
> enumeration of the PCIe controller, which is not connecting
> with an Endpoint device. And it doesn't make sense to
> enumerate the Endpoints when the PCIe link is down. So this
> patch added the link up check to avoid to fire configuration
> transactions on link down bus.

Lorenzo already applied this, but a couple questions:

You call out NXP Layerscape specifically, but doesn't this affect
other DWC-based platforms, too? You later mentioned imx6, Kishon
mentioned dra7xx, Michael mentioned ls1028a, Naresh mentioned ls2088
(probably both the same as your "NXP Layerscape").

The backtrace below contains a bunch of irrelevant info. The
timestamps are pointless. The backtrace past
pci_scan_single_device+0x80/0x100 or so really doesn't add anything
either.

It'd be nice to have a comment in the code because the code *looks*
wrong and racy. Without a hint, everybody who sees it will have to
dig through the history to see why we tolerate the race.

> [ 0.807773] SError Interrupt on CPU2, code 0xbf000002 -- SError
> [ 0.807775] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> [ 0.807776] Hardware name: LS1046A RDB Board (DT)
> [ 0.807777] pstate: 20000085 (nzCv daIf -PAN -UAO BTYPE=--)
> [ 0.807778] pc : pci_generic_config_read+0x3c/0xe0
> [ 0.807778] lr : pci_generic_config_read+0x24/0xe0
> [ 0.807779] sp : ffff80001003b7b0
> [ 0.807780] x29: ffff80001003b7b0 x28: ffff80001003ba74
> [ 0.807782] x27: ffff000971d96800 x26: ffff00096e77e0a8
> [ 0.807784] x25: ffff80001003b874 x24: ffff80001003b924
> [ 0.807786] x23: 0000000000000004 x22: 0000000000000000
> [ 0.807788] x21: 0000000000000000 x20: ffff80001003b874
> [ 0.807790] x19: 0000000000000004 x18: ffffffffffffffff
> [ 0.807791] x17: 00000000000000c0 x16: fffffe0025981840
> [ 0.807793] x15: ffffb94c75b69948 x14: 62203a383634203a
> [ 0.807795] x13: 666e6f635f726568 x12: 202c31203d207265
> [ 0.807797] x11: 626d756e3e2d7375 x10: 656877202c307830
> [ 0.807799] x9 : 203d206e66766564 x8 : 0000000000000908
> [ 0.807801] x7 : 0000000000000908 x6 : ffff800010900000
> [ 0.807802] x5 : ffff00096e77e080 x4 : 0000000000000000
> [ 0.807804] x3 : 0000000000000003 x2 : 84fa3440ff7e7000
> [ 0.807806] x1 : 0000000000000000 x0 : ffff800010034000
> [ 0.807808] Kernel panic - not syncing: Asynchronous SError Interrupt
> [ 0.807809] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> [ 0.807810] Hardware name: LS1046A RDB Board (DT)
> [ 0.807811] Call trace:
> [ 0.807812] dump_backtrace+0x0/0x1c0
> [ 0.807813] show_stack+0x18/0x28
> [ 0.807814] dump_stack+0xd8/0x134
> [ 0.807814] panic+0x180/0x398
> [ 0.807815] add_taint+0x0/0xb0
> [ 0.807816] arm64_serror_panic+0x78/0x88
> [ 0.807817] do_serror+0x68/0x180
> [ 0.807818] el1_error+0x84/0x100
> [ 0.807818] pci_generic_config_read+0x3c/0xe0
> [ 0.807819] dw_pcie_rd_other_conf+0x78/0x110
> [ 0.807820] pci_bus_read_config_dword+0x88/0xe8
> [ 0.807821] pci_bus_generic_read_dev_vendor_id+0x30/0x1b0
> [ 0.807822] pci_bus_read_dev_vendor_id+0x4c/0x78
> [ 0.807823] pci_scan_single_device+0x80/0x100
> [ 0.807824] pci_scan_slot+0x38/0x130
> [ 0.807825] pci_scan_child_bus_extend+0x54/0x2a0
> [ 0.807826] pci_scan_child_bus+0x14/0x20
> [ 0.807827] pci_scan_bridge_extend+0x230/0x570
> [ 0.807828] pci_scan_child_bus_extend+0x134/0x2a0
> [ 0.807829] pci_scan_root_bus_bridge+0x64/0xf0
> [ 0.807829] pci_host_probe+0x18/0xc8
> [ 0.807830] dw_pcie_host_init+0x220/0x378
> [ 0.807831] ls_pcie_probe+0x104/0x140
> [ 0.807832] platform_drv_probe+0x54/0xa8
> [ 0.807833] really_probe+0x118/0x3e0
> [ 0.807834] driver_probe_device+0x5c/0xc0
> [ 0.807835] device_driver_attach+0x74/0x80
> [ 0.807835] __driver_attach+0x8c/0xd8
> [ 0.807836] bus_for_each_dev+0x7c/0xd8
> [ 0.807837] driver_attach+0x24/0x30
> [ 0.807838] bus_add_driver+0x154/0x200
> [ 0.807839] driver_register+0x64/0x120
> [ 0.807839] __platform_driver_probe+0x7c/0x148
> [ 0.807840] ls_pcie_driver_init+0x24/0x30
> [ 0.807841] do_one_initcall+0x60/0x1d8
> [ 0.807842] kernel_init_freeable+0x1f4/0x24c
> [ 0.807843] kernel_init+0x14/0x118
> [ 0.807843] ret_from_fork+0x10/0x34
> [ 0.807854] SMP: stopping secondary CPUs
> [ 0.807855] Kernel Offset: 0x394c64080000 from 0xffff800010000000
> [ 0.807856] PHYS_OFFSET: 0xffff8bfd40000000
> [ 0.807856] CPU features: 0x0240022,21806000
> [ 0.807857] Memory Limit: none
>
> Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
> Signed-off-by: Hou Zhiqiang <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-designware-host.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index c01c9d2fb3f9..e82b518430c5 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -442,6 +442,9 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> struct pcie_port *pp = bus->sysdata;
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>
> + if (!dw_pcie_link_up(pci))
> + return NULL;
> +
> busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> PCIE_ATU_FUNC(PCI_FUNC(devfn));
>
> --
> 2.17.1
>

2020-10-16 14:45:51

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Thu, Oct 15, 2020 at 05:47:38PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 16, 2020 at 01:41:30PM +0800, Zhiqiang Hou wrote:
> > From: Hou Zhiqiang <[email protected]>
> >
> > On NXP Layerscape platforms, it results in SError in the
> > enumeration of the PCIe controller, which is not connecting
> > with an Endpoint device. And it doesn't make sense to
> > enumerate the Endpoints when the PCIe link is down. So this
> > patch added the link up check to avoid to fire configuration
> > transactions on link down bus.
>
> Lorenzo already applied this, but a couple questions:

Happy to drop it - I merged it because it is a regression but
that's certainly not a proper fix either.

> You call out NXP Layerscape specifically, but doesn't this affect
> other DWC-based platforms, too? You later mentioned imx6, Kishon
> mentioned dra7xx, Michael mentioned ls1028a, Naresh mentioned ls2088
> (probably both the same as your "NXP Layerscape").
>
> The backtrace below contains a bunch of irrelevant info. The
> timestamps are pointless. The backtrace past
> pci_scan_single_device+0x80/0x100 or so really doesn't add anything
> either.

I can trim the log message easily but I certainly agree with you
the problem in this patch is the usual racy link check that still
nobody can explains and that we were trying to remove.

It would be very good if the respective platform maintainers went
the extra mile to help here, Rob and I don't have this HW.

Shall we drop this patch and investigate further (possibly hitting
-rc1 with a fix containing a decent explanation ?)

>
> It'd be nice to have a comment in the code because the code *looks*
> wrong and racy. Without a hint, everybody who sees it will have to
> dig through the history to see why we tolerate the race.

+1, see above, ready to drop it.

Lorenzo

> > [ 0.807773] SError Interrupt on CPU2, code 0xbf000002 -- SError
> > [ 0.807775] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> > [ 0.807776] Hardware name: LS1046A RDB Board (DT)
> > [ 0.807777] pstate: 20000085 (nzCv daIf -PAN -UAO BTYPE=--)
> > [ 0.807778] pc : pci_generic_config_read+0x3c/0xe0
> > [ 0.807778] lr : pci_generic_config_read+0x24/0xe0
> > [ 0.807779] sp : ffff80001003b7b0
> > [ 0.807780] x29: ffff80001003b7b0 x28: ffff80001003ba74
> > [ 0.807782] x27: ffff000971d96800 x26: ffff00096e77e0a8
> > [ 0.807784] x25: ffff80001003b874 x24: ffff80001003b924
> > [ 0.807786] x23: 0000000000000004 x22: 0000000000000000
> > [ 0.807788] x21: 0000000000000000 x20: ffff80001003b874
> > [ 0.807790] x19: 0000000000000004 x18: ffffffffffffffff
> > [ 0.807791] x17: 00000000000000c0 x16: fffffe0025981840
> > [ 0.807793] x15: ffffb94c75b69948 x14: 62203a383634203a
> > [ 0.807795] x13: 666e6f635f726568 x12: 202c31203d207265
> > [ 0.807797] x11: 626d756e3e2d7375 x10: 656877202c307830
> > [ 0.807799] x9 : 203d206e66766564 x8 : 0000000000000908
> > [ 0.807801] x7 : 0000000000000908 x6 : ffff800010900000
> > [ 0.807802] x5 : ffff00096e77e080 x4 : 0000000000000000
> > [ 0.807804] x3 : 0000000000000003 x2 : 84fa3440ff7e7000
> > [ 0.807806] x1 : 0000000000000000 x0 : ffff800010034000
> > [ 0.807808] Kernel panic - not syncing: Asynchronous SError Interrupt
> > [ 0.807809] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> > [ 0.807810] Hardware name: LS1046A RDB Board (DT)
> > [ 0.807811] Call trace:
> > [ 0.807812] dump_backtrace+0x0/0x1c0
> > [ 0.807813] show_stack+0x18/0x28
> > [ 0.807814] dump_stack+0xd8/0x134
> > [ 0.807814] panic+0x180/0x398
> > [ 0.807815] add_taint+0x0/0xb0
> > [ 0.807816] arm64_serror_panic+0x78/0x88
> > [ 0.807817] do_serror+0x68/0x180
> > [ 0.807818] el1_error+0x84/0x100
> > [ 0.807818] pci_generic_config_read+0x3c/0xe0
> > [ 0.807819] dw_pcie_rd_other_conf+0x78/0x110
> > [ 0.807820] pci_bus_read_config_dword+0x88/0xe8
> > [ 0.807821] pci_bus_generic_read_dev_vendor_id+0x30/0x1b0
> > [ 0.807822] pci_bus_read_dev_vendor_id+0x4c/0x78
> > [ 0.807823] pci_scan_single_device+0x80/0x100
> > [ 0.807824] pci_scan_slot+0x38/0x130
> > [ 0.807825] pci_scan_child_bus_extend+0x54/0x2a0
> > [ 0.807826] pci_scan_child_bus+0x14/0x20
> > [ 0.807827] pci_scan_bridge_extend+0x230/0x570
> > [ 0.807828] pci_scan_child_bus_extend+0x134/0x2a0
> > [ 0.807829] pci_scan_root_bus_bridge+0x64/0xf0
> > [ 0.807829] pci_host_probe+0x18/0xc8
> > [ 0.807830] dw_pcie_host_init+0x220/0x378
> > [ 0.807831] ls_pcie_probe+0x104/0x140
> > [ 0.807832] platform_drv_probe+0x54/0xa8
> > [ 0.807833] really_probe+0x118/0x3e0
> > [ 0.807834] driver_probe_device+0x5c/0xc0
> > [ 0.807835] device_driver_attach+0x74/0x80
> > [ 0.807835] __driver_attach+0x8c/0xd8
> > [ 0.807836] bus_for_each_dev+0x7c/0xd8
> > [ 0.807837] driver_attach+0x24/0x30
> > [ 0.807838] bus_add_driver+0x154/0x200
> > [ 0.807839] driver_register+0x64/0x120
> > [ 0.807839] __platform_driver_probe+0x7c/0x148
> > [ 0.807840] ls_pcie_driver_init+0x24/0x30
> > [ 0.807841] do_one_initcall+0x60/0x1d8
> > [ 0.807842] kernel_init_freeable+0x1f4/0x24c
> > [ 0.807843] kernel_init+0x14/0x118
> > [ 0.807843] ret_from_fork+0x10/0x34
> > [ 0.807854] SMP: stopping secondary CPUs
> > [ 0.807855] Kernel Offset: 0x394c64080000 from 0xffff800010000000
> > [ 0.807856] PHYS_OFFSET: 0xffff8bfd40000000
> > [ 0.807856] CPU features: 0x0240022,21806000
> > [ 0.807857] Memory Limit: none
> >
> > Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
> > Signed-off-by: Hou Zhiqiang <[email protected]>
> > ---
> > drivers/pci/controller/dwc/pcie-designware-host.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index c01c9d2fb3f9..e82b518430c5 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -442,6 +442,9 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> > struct pcie_port *pp = bus->sysdata;
> > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >
> > + if (!dw_pcie_link_up(pci))
> > + return NULL;
> > +
> > busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> > PCIE_ATU_FUNC(PCI_FUNC(devfn));
> >
> > --
> > 2.17.1
> >

2020-10-19 10:19:35

[permalink] [raw]

Subject: RE: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

Hello Bjorn,

Thanks a lot for your comments!

> -----Original Message-----
> From: Bjorn Helgaas <[email protected]>
> Sent: 2020??10??16?? 6:48
> To: Z.q. Hou <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> dw_child_pcie_ops
>
> On Wed, Sep 16, 2020 at 01:41:30PM +0800, Zhiqiang Hou wrote:
> > From: Hou Zhiqiang <[email protected]>
> >
> > On NXP Layerscape platforms, it results in SError in the enumeration
> > of the PCIe controller, which is not connecting with an Endpoint
> > device. And it doesn't make sense to enumerate the Endpoints when the
> > PCIe link is down. So this patch added the link up check to avoid to
> > fire configuration transactions on link down bus.
>
> Lorenzo already applied this, but a couple questions:
>
> You call out NXP Layerscape specifically, but doesn't this affect other
> DWC-based platforms, too? You later mentioned imx6, Kishon mentioned
> dra7xx, Michael mentioned ls1028a, Naresh mentioned ls2088 (probably
> both the same as your "NXP Layerscape").

For NXP Layerscape platforms (the ls1028a and ls2088a are also NXP Layerscape platform), as the error response to AXI/AHB was enabled, it will get UR error and trigger SError on AXI bus when it accesses a non-existent BDF on a link down bus. I'm not clear about how it happens on dra7xxx and imx6, since they doesn't enable the error response to AXI/AHB.

>
> The backtrace below contains a bunch of irrelevant info. The timestamps
> are pointless. The backtrace past
> pci_scan_single_device+0x80/0x100 or so really doesn't add anything either.
>
> It'd be nice to have a comment in the code because the code *looks* wrong
> and racy. Without a hint, everybody who sees it will have to dig through
> the history to see why we tolerate the race.

Yes, agree, but seems the cause of the SError on dra7xx and imx6 is different from Layerscape platforms, we need to make it clear first.

Thanks,
Zhiqiang

>
> > [ 0.807773] SError Interrupt on CPU2, code 0xbf000002 -- SError
> > [ 0.807775] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
> 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> > [ 0.807776] Hardware name: LS1046A RDB Board (DT)
> > [ 0.807777] pstate: 20000085 (nzCv daIf -PAN -UAO BTYPE=--)
> > [ 0.807778] pc : pci_generic_config_read+0x3c/0xe0
> > [ 0.807778] lr : pci_generic_config_read+0x24/0xe0
> > [ 0.807779] sp : ffff80001003b7b0
> > [ 0.807780] x29: ffff80001003b7b0 x28: ffff80001003ba74
> > [ 0.807782] x27: ffff000971d96800 x26: ffff00096e77e0a8
> > [ 0.807784] x25: ffff80001003b874 x24: ffff80001003b924
> > [ 0.807786] x23: 0000000000000004 x22: 0000000000000000
> > [ 0.807788] x21: 0000000000000000 x20: ffff80001003b874
> > [ 0.807790] x19: 0000000000000004 x18: ffffffffffffffff
> > [ 0.807791] x17: 00000000000000c0 x16: fffffe0025981840
> > [ 0.807793] x15: ffffb94c75b69948 x14: 62203a383634203a
> > [ 0.807795] x13: 666e6f635f726568 x12: 202c31203d207265
> > [ 0.807797] x11: 626d756e3e2d7375 x10: 656877202c307830
> > [ 0.807799] x9 : 203d206e66766564 x8 : 0000000000000908
> > [ 0.807801] x7 : 0000000000000908 x6 : ffff800010900000
> > [ 0.807802] x5 : ffff00096e77e080 x4 : 0000000000000000
> > [ 0.807804] x3 : 0000000000000003 x2 : 84fa3440ff7e7000
> > [ 0.807806] x1 : 0000000000000000 x0 : ffff800010034000
> > [ 0.807808] Kernel panic - not syncing: Asynchronous SError Interrupt
> > [ 0.807809] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
> 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> > [ 0.807810] Hardware name: LS1046A RDB Board (DT)
> > [ 0.807811] Call trace:
> > [ 0.807812] dump_backtrace+0x0/0x1c0
> > [ 0.807813] show_stack+0x18/0x28
> > [ 0.807814] dump_stack+0xd8/0x134
> > [ 0.807814] panic+0x180/0x398
> > [ 0.807815] add_taint+0x0/0xb0
> > [ 0.807816] arm64_serror_panic+0x78/0x88
> > [ 0.807817] do_serror+0x68/0x180
> > [ 0.807818] el1_error+0x84/0x100
> > [ 0.807818] pci_generic_config_read+0x3c/0xe0
> > [ 0.807819] dw_pcie_rd_other_conf+0x78/0x110
> > [ 0.807820] pci_bus_read_config_dword+0x88/0xe8
> > [ 0.807821] pci_bus_generic_read_dev_vendor_id+0x30/0x1b0
> > [ 0.807822] pci_bus_read_dev_vendor_id+0x4c/0x78
> > [ 0.807823] pci_scan_single_device+0x80/0x100
> > [ 0.807824] pci_scan_slot+0x38/0x130
> > [ 0.807825] pci_scan_child_bus_extend+0x54/0x2a0
> > [ 0.807826] pci_scan_child_bus+0x14/0x20
> > [ 0.807827] pci_scan_bridge_extend+0x230/0x570
> > [ 0.807828] pci_scan_child_bus_extend+0x134/0x2a0
> > [ 0.807829] pci_scan_root_bus_bridge+0x64/0xf0
> > [ 0.807829] pci_host_probe+0x18/0xc8
> > [ 0.807830] dw_pcie_host_init+0x220/0x378
> > [ 0.807831] ls_pcie_probe+0x104/0x140
> > [ 0.807832] platform_drv_probe+0x54/0xa8
> > [ 0.807833] really_probe+0x118/0x3e0
> > [ 0.807834] driver_probe_device+0x5c/0xc0
> > [ 0.807835] device_driver_attach+0x74/0x80
> > [ 0.807835] __driver_attach+0x8c/0xd8
> > [ 0.807836] bus_for_each_dev+0x7c/0xd8
> > [ 0.807837] driver_attach+0x24/0x30
> > [ 0.807838] bus_add_driver+0x154/0x200
> > [ 0.807839] driver_register+0x64/0x120
> > [ 0.807839] __platform_driver_probe+0x7c/0x148
> > [ 0.807840] ls_pcie_driver_init+0x24/0x30
> > [ 0.807841] do_one_initcall+0x60/0x1d8
> > [ 0.807842] kernel_init_freeable+0x1f4/0x24c
> > [ 0.807843] kernel_init+0x14/0x118
> > [ 0.807843] ret_from_fork+0x10/0x34
> > [ 0.807854] SMP: stopping secondary CPUs
> > [ 0.807855] Kernel Offset: 0x394c64080000 from 0xffff800010000000
> > [ 0.807856] PHYS_OFFSET: 0xffff8bfd40000000
> > [ 0.807856] CPU features: 0x0240022,21806000
> > [ 0.807857] Memory Limit: none
> >
> > Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
> > Signed-off-by: Hou Zhiqiang <[email protected]>
> > ---
> > drivers/pci/controller/dwc/pcie-designware-host.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index c01c9d2fb3f9..e82b518430c5 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -442,6 +442,9 @@ static void __iomem
> *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> > struct pcie_port *pp = bus->sysdata;
> > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >
> > + if (!dw_pcie_link_up(pci))
> > + return NULL;
> > +
> > busdev = PCIE_ATU_BUS(bus->number) |
> PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> > PCIE_ATU_FUNC(PCI_FUNC(devfn));
> >
> > --
> > 2.17.1
> >

2020-10-19 10:19:58

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

Hi Hou,

On 19/10/20 10:54 am, Z.q. Hou wrote:
> Hello Bjorn,
>
> Thanks a lot for your comments!
>
>> -----Original Message-----
>> From: Bjorn Helgaas <[email protected]>
>> Sent: 2020??10??16?? 6:48
>> To: Z.q. Hou <[email protected]>
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]
>> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
>> dw_child_pcie_ops
>>
>> On Wed, Sep 16, 2020 at 01:41:30PM +0800, Zhiqiang Hou wrote:
>>> From: Hou Zhiqiang <[email protected]>
>>>
>>> On NXP Layerscape platforms, it results in SError in the enumeration
>>> of the PCIe controller, which is not connecting with an Endpoint
>>> device. And it doesn't make sense to enumerate the Endpoints when the
>>> PCIe link is down. So this patch added the link up check to avoid to
>>> fire configuration transactions on link down bus.
>>
>> Lorenzo already applied this, but a couple questions:
>>
>> You call out NXP Layerscape specifically, but doesn't this affect other
>> DWC-based platforms, too? You later mentioned imx6, Kishon mentioned
>> dra7xx, Michael mentioned ls1028a, Naresh mentioned ls2088 (probably
>> both the same as your "NXP Layerscape").
>
> For NXP Layerscape platforms (the ls1028a and ls2088a are also NXP Layerscape platform), as the error response to AXI/AHB was enabled, it will get UR error and trigger SError on AXI bus when it accesses a non-existent BDF on a link down bus. I'm not clear about how it happens on dra7xxx and imx6, since they doesn't enable the error response to AXI/AHB.

That's exactly the case with DRA7xx as the error response is enabled by
default in the platform integration.

Thanks
Kishon

>
>>
>> The backtrace below contains a bunch of irrelevant info. The timestamps
>> are pointless. The backtrace past
>> pci_scan_single_device+0x80/0x100 or so really doesn't add anything either.
>>
>> It'd be nice to have a comment in the code because the code *looks* wrong
>> and racy. Without a hint, everybody who sees it will have to dig through
>> the history to see why we tolerate the race.
>
> Yes, agree, but seems the cause of the SError on dra7xx and imx6 is different from Layerscape platforms, we need to make it clear first.
>
> Thanks,
> Zhiqiang
>
>>
>>> [ 0.807773] SError Interrupt on CPU2, code 0xbf000002 -- SError
>>> [ 0.807775] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
>> 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
>>> [ 0.807776] Hardware name: LS1046A RDB Board (DT)
>>> [ 0.807777] pstate: 20000085 (nzCv daIf -PAN -UAO BTYPE=--)
>>> [ 0.807778] pc : pci_generic_config_read+0x3c/0xe0
>>> [ 0.807778] lr : pci_generic_config_read+0x24/0xe0
>>> [ 0.807779] sp : ffff80001003b7b0
>>> [ 0.807780] x29: ffff80001003b7b0 x28: ffff80001003ba74
>>> [ 0.807782] x27: ffff000971d96800 x26: ffff00096e77e0a8
>>> [ 0.807784] x25: ffff80001003b874 x24: ffff80001003b924
>>> [ 0.807786] x23: 0000000000000004 x22: 0000000000000000
>>> [ 0.807788] x21: 0000000000000000 x20: ffff80001003b874
>>> [ 0.807790] x19: 0000000000000004 x18: ffffffffffffffff
>>> [ 0.807791] x17: 00000000000000c0 x16: fffffe0025981840
>>> [ 0.807793] x15: ffffb94c75b69948 x14: 62203a383634203a
>>> [ 0.807795] x13: 666e6f635f726568 x12: 202c31203d207265
>>> [ 0.807797] x11: 626d756e3e2d7375 x10: 656877202c307830
>>> [ 0.807799] x9 : 203d206e66766564 x8 : 0000000000000908
>>> [ 0.807801] x7 : 0000000000000908 x6 : ffff800010900000
>>> [ 0.807802] x5 : ffff00096e77e080 x4 : 0000000000000000
>>> [ 0.807804] x3 : 0000000000000003 x2 : 84fa3440ff7e7000
>>> [ 0.807806] x1 : 0000000000000000 x0 : ffff800010034000
>>> [ 0.807808] Kernel panic - not syncing: Asynchronous SError Interrupt
>>> [ 0.807809] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
>> 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
>>> [ 0.807810] Hardware name: LS1046A RDB Board (DT)
>>> [ 0.807811] Call trace:
>>> [ 0.807812] dump_backtrace+0x0/0x1c0
>>> [ 0.807813] show_stack+0x18/0x28
>>> [ 0.807814] dump_stack+0xd8/0x134
>>> [ 0.807814] panic+0x180/0x398
>>> [ 0.807815] add_taint+0x0/0xb0
>>> [ 0.807816] arm64_serror_panic+0x78/0x88
>>> [ 0.807817] do_serror+0x68/0x180
>>> [ 0.807818] el1_error+0x84/0x100
>>> [ 0.807818] pci_generic_config_read+0x3c/0xe0
>>> [ 0.807819] dw_pcie_rd_other_conf+0x78/0x110
>>> [ 0.807820] pci_bus_read_config_dword+0x88/0xe8
>>> [ 0.807821] pci_bus_generic_read_dev_vendor_id+0x30/0x1b0
>>> [ 0.807822] pci_bus_read_dev_vendor_id+0x4c/0x78
>>> [ 0.807823] pci_scan_single_device+0x80/0x100
>>> [ 0.807824] pci_scan_slot+0x38/0x130
>>> [ 0.807825] pci_scan_child_bus_extend+0x54/0x2a0
>>> [ 0.807826] pci_scan_child_bus+0x14/0x20
>>> [ 0.807827] pci_scan_bridge_extend+0x230/0x570
>>> [ 0.807828] pci_scan_child_bus_extend+0x134/0x2a0
>>> [ 0.807829] pci_scan_root_bus_bridge+0x64/0xf0
>>> [ 0.807829] pci_host_probe+0x18/0xc8
>>> [ 0.807830] dw_pcie_host_init+0x220/0x378
>>> [ 0.807831] ls_pcie_probe+0x104/0x140
>>> [ 0.807832] platform_drv_probe+0x54/0xa8
>>> [ 0.807833] really_probe+0x118/0x3e0
>>> [ 0.807834] driver_probe_device+0x5c/0xc0
>>> [ 0.807835] device_driver_attach+0x74/0x80
>>> [ 0.807835] __driver_attach+0x8c/0xd8
>>> [ 0.807836] bus_for_each_dev+0x7c/0xd8
>>> [ 0.807837] driver_attach+0x24/0x30
>>> [ 0.807838] bus_add_driver+0x154/0x200
>>> [ 0.807839] driver_register+0x64/0x120
>>> [ 0.807839] __platform_driver_probe+0x7c/0x148
>>> [ 0.807840] ls_pcie_driver_init+0x24/0x30
>>> [ 0.807841] do_one_initcall+0x60/0x1d8
>>> [ 0.807842] kernel_init_freeable+0x1f4/0x24c
>>> [ 0.807843] kernel_init+0x14/0x118
>>> [ 0.807843] ret_from_fork+0x10/0x34
>>> [ 0.807854] SMP: stopping secondary CPUs
>>> [ 0.807855] Kernel Offset: 0x394c64080000 from 0xffff800010000000
>>> [ 0.807856] PHYS_OFFSET: 0xffff8bfd40000000
>>> [ 0.807856] CPU features: 0x0240022,21806000
>>> [ 0.807857] Memory Limit: none
>>>
>>> Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
>>> Signed-off-by: Hou Zhiqiang <[email protected]>
>>> ---
>>> drivers/pci/controller/dwc/pcie-designware-host.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
>>> b/drivers/pci/controller/dwc/pcie-designware-host.c
>>> index c01c9d2fb3f9..e82b518430c5 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>> @@ -442,6 +442,9 @@ static void __iomem
>> *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
>>> struct pcie_port *pp = bus->sysdata;
>>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>
>>> + if (!dw_pcie_link_up(pci))
>>> + return NULL;
>>> +
>>> busdev = PCIE_ATU_BUS(bus->number) |
>> PCIE_ATU_DEV(PCI_SLOT(devfn)) |
>>> PCIE_ATU_FUNC(PCI_FUNC(devfn));
>>>
>>> --
>>> 2.17.1
>>>

2020-10-19 16:15:30

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Mon, Oct 12, 2020 at 04:41:11AM +0000, Z.q. Hou wrote:

[...]

> > >> Yeah, I don't see any registers in the DRA7x PCIe wrapper for
> > >> disabling error forwarding.
> > >
> > > It's a DWC port logic register AFAICT, but perhaps not present in all
> > versions.
> >
> > Okay. I see there's a register PCIECTRL_PL_AXIS_SLV_ERR_RESP which has a
> > reset value of 0.
> >
> > It has four bit-fields, RESET_TIMEOUT_ERR_MAP, NO_VID_ERR_MAP,
> > DBI_ERR_MAP and SLAVE_ERR_MAP. I'm not seeing any difference in
> > behavior if I set all these bits. Maybe it requires platform support too. I'll
> > check this with our design team.
>
> In DWC v4.40a databook, there is a bit AMBA_ERROR_RESPONSE_GLOBAL
> which controls if enable the error forwarding. The *MAP bits only
> determine which error (SLVERR or DECERR) will be forwarded to AXI/AHB
> bus.

I have not seen a follow-up to this but I would like to, still keen
on avoiding this patch if possible - if this is port logic it should
be common across controllers implementations I assume.

Gustavo, Kishon ?

Thanks,
Lorenzo

> Thanks,
> Zhiqiang
>
> >
> > Meanwhile would it be okay to add linkup check atleast for DRA7X so that
> > we could have it booting in linux-next?
> >
> > Thanks
> > Kishon

2020-10-20 02:33:53

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Thu, Oct 15, 2020 at 05:47:38PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 16, 2020 at 01:41:30PM +0800, Zhiqiang Hou wrote:
> > From: Hou Zhiqiang <[email protected]>
> >
> > On NXP Layerscape platforms, it results in SError in the
> > enumeration of the PCIe controller, which is not connecting
> > with an Endpoint device. And it doesn't make sense to
> > enumerate the Endpoints when the PCIe link is down. So this
> > patch added the link up check to avoid to fire configuration
> > transactions on link down bus.
>
> Lorenzo already applied this, but a couple questions:
>
> You call out NXP Layerscape specifically, but doesn't this affect
> other DWC-based platforms, too? You later mentioned imx6, Kishon
> mentioned dra7xx, Michael mentioned ls1028a, Naresh mentioned ls2088
> (probably both the same as your "NXP Layerscape").
>
> The backtrace below contains a bunch of irrelevant info. The
> timestamps are pointless. The backtrace past
> pci_scan_single_device+0x80/0x100 or so really doesn't add anything
> either.
>
> It'd be nice to have a comment in the code because the code *looks*
> wrong and racy. Without a hint, everybody who sees it will have to
> dig through the history to see why we tolerate the race.

I can add the comment myself, I improved the backtrace and I can
try to summarize why error forwarding can't be disabled even though
I would appreciate more help from DWC maintainers - as I don't have
access to DWC specs.

This patch is a workaround at best but I am not sure we went all the way
to prevent it - I am quite tempted to drop it and postpone it to -rc*.

Lorenzo

>
> > [ 0.807773] SError Interrupt on CPU2, code 0xbf000002 -- SError
> > [ 0.807775] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> > [ 0.807776] Hardware name: LS1046A RDB Board (DT)
> > [ 0.807777] pstate: 20000085 (nzCv daIf -PAN -UAO BTYPE=--)
> > [ 0.807778] pc : pci_generic_config_read+0x3c/0xe0
> > [ 0.807778] lr : pci_generic_config_read+0x24/0xe0
> > [ 0.807779] sp : ffff80001003b7b0
> > [ 0.807780] x29: ffff80001003b7b0 x28: ffff80001003ba74
> > [ 0.807782] x27: ffff000971d96800 x26: ffff00096e77e0a8
> > [ 0.807784] x25: ffff80001003b874 x24: ffff80001003b924
> > [ 0.807786] x23: 0000000000000004 x22: 0000000000000000
> > [ 0.807788] x21: 0000000000000000 x20: ffff80001003b874
> > [ 0.807790] x19: 0000000000000004 x18: ffffffffffffffff
> > [ 0.807791] x17: 00000000000000c0 x16: fffffe0025981840
> > [ 0.807793] x15: ffffb94c75b69948 x14: 62203a383634203a
> > [ 0.807795] x13: 666e6f635f726568 x12: 202c31203d207265
> > [ 0.807797] x11: 626d756e3e2d7375 x10: 656877202c307830
> > [ 0.807799] x9 : 203d206e66766564 x8 : 0000000000000908
> > [ 0.807801] x7 : 0000000000000908 x6 : ffff800010900000
> > [ 0.807802] x5 : ffff00096e77e080 x4 : 0000000000000000
> > [ 0.807804] x3 : 0000000000000003 x2 : 84fa3440ff7e7000
> > [ 0.807806] x1 : 0000000000000000 x0 : ffff800010034000
> > [ 0.807808] Kernel panic - not syncing: Asynchronous SError Interrupt
> > [ 0.807809] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> > [ 0.807810] Hardware name: LS1046A RDB Board (DT)
> > [ 0.807811] Call trace:
> > [ 0.807812] dump_backtrace+0x0/0x1c0
> > [ 0.807813] show_stack+0x18/0x28
> > [ 0.807814] dump_stack+0xd8/0x134
> > [ 0.807814] panic+0x180/0x398
> > [ 0.807815] add_taint+0x0/0xb0
> > [ 0.807816] arm64_serror_panic+0x78/0x88
> > [ 0.807817] do_serror+0x68/0x180
> > [ 0.807818] el1_error+0x84/0x100
> > [ 0.807818] pci_generic_config_read+0x3c/0xe0
> > [ 0.807819] dw_pcie_rd_other_conf+0x78/0x110
> > [ 0.807820] pci_bus_read_config_dword+0x88/0xe8
> > [ 0.807821] pci_bus_generic_read_dev_vendor_id+0x30/0x1b0
> > [ 0.807822] pci_bus_read_dev_vendor_id+0x4c/0x78
> > [ 0.807823] pci_scan_single_device+0x80/0x100
> > [ 0.807824] pci_scan_slot+0x38/0x130
> > [ 0.807825] pci_scan_child_bus_extend+0x54/0x2a0
> > [ 0.807826] pci_scan_child_bus+0x14/0x20
> > [ 0.807827] pci_scan_bridge_extend+0x230/0x570
> > [ 0.807828] pci_scan_child_bus_extend+0x134/0x2a0
> > [ 0.807829] pci_scan_root_bus_bridge+0x64/0xf0
> > [ 0.807829] pci_host_probe+0x18/0xc8
> > [ 0.807830] dw_pcie_host_init+0x220/0x378
> > [ 0.807831] ls_pcie_probe+0x104/0x140
> > [ 0.807832] platform_drv_probe+0x54/0xa8
> > [ 0.807833] really_probe+0x118/0x3e0
> > [ 0.807834] driver_probe_device+0x5c/0xc0
> > [ 0.807835] device_driver_attach+0x74/0x80
> > [ 0.807835] __driver_attach+0x8c/0xd8
> > [ 0.807836] bus_for_each_dev+0x7c/0xd8
> > [ 0.807837] driver_attach+0x24/0x30
> > [ 0.807838] bus_add_driver+0x154/0x200
> > [ 0.807839] driver_register+0x64/0x120
> > [ 0.807839] __platform_driver_probe+0x7c/0x148
> > [ 0.807840] ls_pcie_driver_init+0x24/0x30
> > [ 0.807841] do_one_initcall+0x60/0x1d8
> > [ 0.807842] kernel_init_freeable+0x1f4/0x24c
> > [ 0.807843] kernel_init+0x14/0x118
> > [ 0.807843] ret_from_fork+0x10/0x34
> > [ 0.807854] SMP: stopping secondary CPUs
> > [ 0.807855] Kernel Offset: 0x394c64080000 from 0xffff800010000000
> > [ 0.807856] PHYS_OFFSET: 0xffff8bfd40000000
> > [ 0.807856] CPU features: 0x0240022,21806000
> > [ 0.807857] Memory Limit: none
> >
> > Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
> > Signed-off-by: Hou Zhiqiang <[email protected]>
> > ---
> > drivers/pci/controller/dwc/pcie-designware-host.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index c01c9d2fb3f9..e82b518430c5 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -442,6 +442,9 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> > struct pcie_port *pp = bus->sysdata;
> > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >
> > + if (!dw_pcie_link_up(pci))
> > + return NULL;
> > +
> > busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> > PCIE_ATU_FUNC(PCI_FUNC(devfn));
> >
> > --
> > 2.17.1
> >

2020-10-20 10:17:07

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

Hi Lorenzo,

On 19/10/20 9:43 pm, Lorenzo Pieralisi wrote:
> On Mon, Oct 12, 2020 at 04:41:11AM +0000, Z.q. Hou wrote:
>
> [...]
>
>>>>> Yeah, I don't see any registers in the DRA7x PCIe wrapper for
>>>>> disabling error forwarding.
>>>>
>>>> It's a DWC port logic register AFAICT, but perhaps not present in all
>>> versions.
>>>
>>> Okay. I see there's a register PCIECTRL_PL_AXIS_SLV_ERR_RESP which has a
>>> reset value of 0.
>>>
>>> It has four bit-fields, RESET_TIMEOUT_ERR_MAP, NO_VID_ERR_MAP,
>>> DBI_ERR_MAP and SLAVE_ERR_MAP. I'm not seeing any difference in
>>> behavior if I set all these bits. Maybe it requires platform support too. I'll
>>> check this with our design team.
>>
>> In DWC v4.40a databook, there is a bit AMBA_ERROR_RESPONSE_GLOBAL
>> which controls if enable the error forwarding. The *MAP bits only
>> determine which error (SLVERR or DECERR) will be forwarded to AXI/AHB
>> bus.
>
> I have not seen a follow-up to this but I would like to, still keen
> on avoiding this patch if possible - if this is port logic it should
> be common across controllers implementations I assume.
>
> Gustavo, Kishon ?

Atleast in the TI DRA7 TRM, I could see only
PCIECTRL_PL_AXIS_SLV_ERR_RESP and PCIECTRL_PL_AXIS_SLV_TIMEOUT register
but no global error response bit. I'd have expected configuring
SLV_ERR_RESP would have disabled error forwarding, but I don't see any
change in behavior if I modify the value of PCIECTRL_PL_AXIS_SLV_ERR_RESP.

TI PCIe controller in DRA7 is not directly connected to AXI/AHB but
there is an intermediary bridge. So I suspect there is some issue on how
the controller is integrated in TI platform. Since the board hangs, I
couldn't get lot of visibility of controller state.

Thanks
Kishon

2020-10-20 11:25:08

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Fri, Sep 18, 2020 at 09:27:40AM -0600, Rob Herring wrote:

[...]

> > > Maybe a link down just never happens once up, but if so, then we only need
> > > to check it once and fail probe.
> >
> > Many customers connect the FPGA Endpoint, which may establish PCIe link
> > after the PCIe enumeration and then rescan the PCIe bus, so I think it should
> > not exit the probe of root port even if there is not link up during enumeration.
>
> That's a good reason. I want to unify the behavior here as it varies
> per platform currently and wasn't sure which way to go.

We don't need to fail probe - just skip enumeration. Is there an IRQ
event associated with link coming up ? Scanning the bus can be done
upon link-up IRQ.

For platforms that forward the link down as an SError this still does
not solve the problem (if the link goes down unexpectedly) but I
question their design in the first place, this patch does not fix their
behaviour regardless.

Lorenzo

2020-10-20 11:25:37

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

2020-10-20 11:28:46

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Thu, Oct 15, 2020 at 05:47:38PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 16, 2020 at 01:41:30PM +0800, Zhiqiang Hou wrote:
> > From: Hou Zhiqiang <[email protected]>
> >
> > On NXP Layerscape platforms, it results in SError in the
> > enumeration of the PCIe controller, which is not connecting
> > with an Endpoint device. And it doesn't make sense to
> > enumerate the Endpoints when the PCIe link is down. So this
> > patch added the link up check to avoid to fire configuration
> > transactions on link down bus.
>
> Lorenzo already applied this, but a couple questions:
>
> You call out NXP Layerscape specifically, but doesn't this affect
> other DWC-based platforms, too? You later mentioned imx6, Kishon
> mentioned dra7xx, Michael mentioned ls1028a, Naresh mentioned ls2088
> (probably both the same as your "NXP Layerscape").
>
> The backtrace below contains a bunch of irrelevant info. The
> timestamps are pointless. The backtrace past
> pci_scan_single_device+0x80/0x100 or so really doesn't add anything
> either.
>
> It'd be nice to have a comment in the code because the code *looks*
> wrong and racy. Without a hint, everybody who sees it will have to
> dig through the history to see why we tolerate the race.

I have updated the commit log the best I could with the information
I have, pci/dwc branch. Please all have a look and report any issue
you may find.

This link-up check is nonetheless broken and we should find a long
term solution that is not relying on these shaky fixes.

Lorenzo

> > [ 0.807773] SError Interrupt on CPU2, code 0xbf000002 -- SError
> > [ 0.807775] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> > [ 0.807776] Hardware name: LS1046A RDB Board (DT)
> > [ 0.807777] pstate: 20000085 (nzCv daIf -PAN -UAO BTYPE=--)
> > [ 0.807778] pc : pci_generic_config_read+0x3c/0xe0
> > [ 0.807778] lr : pci_generic_config_read+0x24/0xe0
> > [ 0.807779] sp : ffff80001003b7b0
> > [ 0.807780] x29: ffff80001003b7b0 x28: ffff80001003ba74
> > [ 0.807782] x27: ffff000971d96800 x26: ffff00096e77e0a8
> > [ 0.807784] x25: ffff80001003b874 x24: ffff80001003b924
> > [ 0.807786] x23: 0000000000000004 x22: 0000000000000000
> > [ 0.807788] x21: 0000000000000000 x20: ffff80001003b874
> > [ 0.807790] x19: 0000000000000004 x18: ffffffffffffffff
> > [ 0.807791] x17: 00000000000000c0 x16: fffffe0025981840
> > [ 0.807793] x15: ffffb94c75b69948 x14: 62203a383634203a
> > [ 0.807795] x13: 666e6f635f726568 x12: 202c31203d207265
> > [ 0.807797] x11: 626d756e3e2d7375 x10: 656877202c307830
> > [ 0.807799] x9 : 203d206e66766564 x8 : 0000000000000908
> > [ 0.807801] x7 : 0000000000000908 x6 : ffff800010900000
> > [ 0.807802] x5 : ffff00096e77e080 x4 : 0000000000000000
> > [ 0.807804] x3 : 0000000000000003 x2 : 84fa3440ff7e7000
> > [ 0.807806] x1 : 0000000000000000 x0 : ffff800010034000
> > [ 0.807808] Kernel panic - not syncing: Asynchronous SError Interrupt
> > [ 0.807809] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> > [ 0.807810] Hardware name: LS1046A RDB Board (DT)
> > [ 0.807811] Call trace:
> > [ 0.807812] dump_backtrace+0x0/0x1c0
> > [ 0.807813] show_stack+0x18/0x28
> > [ 0.807814] dump_stack+0xd8/0x134
> > [ 0.807814] panic+0x180/0x398
> > [ 0.807815] add_taint+0x0/0xb0
> > [ 0.807816] arm64_serror_panic+0x78/0x88
> > [ 0.807817] do_serror+0x68/0x180
> > [ 0.807818] el1_error+0x84/0x100
> > [ 0.807818] pci_generic_config_read+0x3c/0xe0
> > [ 0.807819] dw_pcie_rd_other_conf+0x78/0x110
> > [ 0.807820] pci_bus_read_config_dword+0x88/0xe8
> > [ 0.807821] pci_bus_generic_read_dev_vendor_id+0x30/0x1b0
> > [ 0.807822] pci_bus_read_dev_vendor_id+0x4c/0x78
> > [ 0.807823] pci_scan_single_device+0x80/0x100
> > [ 0.807824] pci_scan_slot+0x38/0x130
> > [ 0.807825] pci_scan_child_bus_extend+0x54/0x2a0
> > [ 0.807826] pci_scan_child_bus+0x14/0x20
> > [ 0.807827] pci_scan_bridge_extend+0x230/0x570
> > [ 0.807828] pci_scan_child_bus_extend+0x134/0x2a0
> > [ 0.807829] pci_scan_root_bus_bridge+0x64/0xf0
> > [ 0.807829] pci_host_probe+0x18/0xc8
> > [ 0.807830] dw_pcie_host_init+0x220/0x378
> > [ 0.807831] ls_pcie_probe+0x104/0x140
> > [ 0.807832] platform_drv_probe+0x54/0xa8
> > [ 0.807833] really_probe+0x118/0x3e0
> > [ 0.807834] driver_probe_device+0x5c/0xc0
> > [ 0.807835] device_driver_attach+0x74/0x80
> > [ 0.807835] __driver_attach+0x8c/0xd8
> > [ 0.807836] bus_for_each_dev+0x7c/0xd8
> > [ 0.807837] driver_attach+0x24/0x30
> > [ 0.807838] bus_add_driver+0x154/0x200
> > [ 0.807839] driver_register+0x64/0x120
> > [ 0.807839] __platform_driver_probe+0x7c/0x148
> > [ 0.807840] ls_pcie_driver_init+0x24/0x30
> > [ 0.807841] do_one_initcall+0x60/0x1d8
> > [ 0.807842] kernel_init_freeable+0x1f4/0x24c
> > [ 0.807843] kernel_init+0x14/0x118
> > [ 0.807843] ret_from_fork+0x10/0x34
> > [ 0.807854] SMP: stopping secondary CPUs
> > [ 0.807855] Kernel Offset: 0x394c64080000 from 0xffff800010000000
> > [ 0.807856] PHYS_OFFSET: 0xffff8bfd40000000
> > [ 0.807856] CPU features: 0x0240022,21806000
> > [ 0.807857] Memory Limit: none
> >
> > Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
> > Signed-off-by: Hou Zhiqiang <[email protected]>
> > ---
> > drivers/pci/controller/dwc/pcie-designware-host.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index c01c9d2fb3f9..e82b518430c5 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -442,6 +442,9 @@ static void __iomem *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> > struct pcie_port *pp = bus->sysdata;
> > struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >
> > + if (!dw_pcie_link_up(pci))
> > + return NULL;
> > +
> > busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> > PCIE_ATU_FUNC(PCI_FUNC(devfn));
> >
> > --
> > 2.17.1
> >

2020-10-20 17:14:26

[permalink] [raw]

Subject: RE: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

Hi Bjorn, Lorenzo and Kishon,

> -----Original Message-----
> From: Kishon Vijay Abraham I <[email protected]>
> Sent: 2020??10??19?? 13:41
> To: Z.q. Hou <[email protected]>; Bjorn Helgaas <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> dw_child_pcie_ops
>
> Hi Hou,
>
> On 19/10/20 10:54 am, Z.q. Hou wrote:
> > Hello Bjorn,
> >
> > Thanks a lot for your comments!
> >
> >> -----Original Message-----
> >> From: Bjorn Helgaas <[email protected]>
> >> Sent: 2020??10??16?? 6:48
> >> To: Z.q. Hou <[email protected]>
> >> Cc: [email protected]; [email protected];
> >> [email protected]; [email protected]; [email protected];
> >> [email protected]
> >> Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of
> >> dw_child_pcie_ops
> >>
> >> On Wed, Sep 16, 2020 at 01:41:30PM +0800, Zhiqiang Hou wrote:
> >>> From: Hou Zhiqiang <[email protected]>
> >>>
> >>> On NXP Layerscape platforms, it results in SError in the enumeration
> >>> of the PCIe controller, which is not connecting with an Endpoint
> >>> device. And it doesn't make sense to enumerate the Endpoints when
> >>> the PCIe link is down. So this patch added the link up check to
> >>> avoid to fire configuration transactions on link down bus.
> >>
> >> Lorenzo already applied this, but a couple questions:
> >>
> >> You call out NXP Layerscape specifically, but doesn't this affect
> >> other DWC-based platforms, too? You later mentioned imx6, Kishon
> >> mentioned dra7xx, Michael mentioned ls1028a, Naresh mentioned ls2088
> >> (probably both the same as your "NXP Layerscape").
> >
> > For NXP Layerscape platforms (the ls1028a and ls2088a are also NXP
> Layerscape platform), as the error response to AXI/AHB was enabled, it will
> get UR error and trigger SError on AXI bus when it accesses a non-existent
> BDF on a link down bus. I'm not clear about how it happens on dra7xxx and
> imx6, since they doesn't enable the error response to AXI/AHB.
>
> That's exactly the case with DRA7xx as the error response is enabled by
> default in the platform integration.

Got feedback from the imx6 owner that imx6 like the dra7xx has the error response enabled by default.
Now it's clear that the problem on all these platforms is the same.

Thanks,
Zhiqiang

>
> Thanks
> Kishon
>
> >
> >>
> >> The backtrace below contains a bunch of irrelevant info. The
> >> timestamps are pointless. The backtrace past
> >> pci_scan_single_device+0x80/0x100 or so really doesn't add anything
> either.
> >>
> >> It'd be nice to have a comment in the code because the code *looks*
> >> wrong and racy. Without a hint, everybody who sees it will have to
> >> dig through the history to see why we tolerate the race.
> >
> > Yes, agree, but seems the cause of the SError on dra7xx and imx6 is
> different from Layerscape platforms, we need to make it clear first.
> >
> > Thanks,
> > Zhiqiang
> >
> >>
> >>> [ 0.807773] SError Interrupt on CPU2, code 0xbf000002 -- SError
> >>> [ 0.807775] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
> >> 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> >>> [ 0.807776] Hardware name: LS1046A RDB Board (DT)
> >>> [ 0.807777] pstate: 20000085 (nzCv daIf -PAN -UAO BTYPE=--)
> >>> [ 0.807778] pc : pci_generic_config_read+0x3c/0xe0
> >>> [ 0.807778] lr : pci_generic_config_read+0x24/0xe0
> >>> [ 0.807779] sp : ffff80001003b7b0
> >>> [ 0.807780] x29: ffff80001003b7b0 x28: ffff80001003ba74
> >>> [ 0.807782] x27: ffff000971d96800 x26: ffff00096e77e0a8
> >>> [ 0.807784] x25: ffff80001003b874 x24: ffff80001003b924
> >>> [ 0.807786] x23: 0000000000000004 x22: 0000000000000000
> >>> [ 0.807788] x21: 0000000000000000 x20: ffff80001003b874
> >>> [ 0.807790] x19: 0000000000000004 x18: ffffffffffffffff
> >>> [ 0.807791] x17: 00000000000000c0 x16: fffffe0025981840
> >>> [ 0.807793] x15: ffffb94c75b69948 x14: 62203a383634203a
> >>> [ 0.807795] x13: 666e6f635f726568 x12: 202c31203d207265
> >>> [ 0.807797] x11: 626d756e3e2d7375 x10: 656877202c307830
> >>> [ 0.807799] x9 : 203d206e66766564 x8 : 0000000000000908
> >>> [ 0.807801] x7 : 0000000000000908 x6 : ffff800010900000
> >>> [ 0.807802] x5 : ffff00096e77e080 x4 : 0000000000000000
> >>> [ 0.807804] x3 : 0000000000000003 x2 : 84fa3440ff7e7000
> >>> [ 0.807806] x1 : 0000000000000000 x0 : ffff800010034000
> >>> [ 0.807808] Kernel panic - not syncing: Asynchronous SError
> Interrupt
> >>> [ 0.807809] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
> >> 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> >>> [ 0.807810] Hardware name: LS1046A RDB Board (DT)
> >>> [ 0.807811] Call trace:
> >>> [ 0.807812] dump_backtrace+0x0/0x1c0
> >>> [ 0.807813] show_stack+0x18/0x28
> >>> [ 0.807814] dump_stack+0xd8/0x134
> >>> [ 0.807814] panic+0x180/0x398
> >>> [ 0.807815] add_taint+0x0/0xb0
> >>> [ 0.807816] arm64_serror_panic+0x78/0x88
> >>> [ 0.807817] do_serror+0x68/0x180
> >>> [ 0.807818] el1_error+0x84/0x100
> >>> [ 0.807818] pci_generic_config_read+0x3c/0xe0
> >>> [ 0.807819] dw_pcie_rd_other_conf+0x78/0x110
> >>> [ 0.807820] pci_bus_read_config_dword+0x88/0xe8
> >>> [ 0.807821] pci_bus_generic_read_dev_vendor_id+0x30/0x1b0
> >>> [ 0.807822] pci_bus_read_dev_vendor_id+0x4c/0x78
> >>> [ 0.807823] pci_scan_single_device+0x80/0x100
> >>> [ 0.807824] pci_scan_slot+0x38/0x130
> >>> [ 0.807825] pci_scan_child_bus_extend+0x54/0x2a0
> >>> [ 0.807826] pci_scan_child_bus+0x14/0x20
> >>> [ 0.807827] pci_scan_bridge_extend+0x230/0x570
> >>> [ 0.807828] pci_scan_child_bus_extend+0x134/0x2a0
> >>> [ 0.807829] pci_scan_root_bus_bridge+0x64/0xf0
> >>> [ 0.807829] pci_host_probe+0x18/0xc8
> >>> [ 0.807830] dw_pcie_host_init+0x220/0x378
> >>> [ 0.807831] ls_pcie_probe+0x104/0x140
> >>> [ 0.807832] platform_drv_probe+0x54/0xa8
> >>> [ 0.807833] really_probe+0x118/0x3e0
> >>> [ 0.807834] driver_probe_device+0x5c/0xc0
> >>> [ 0.807835] device_driver_attach+0x74/0x80
> >>> [ 0.807835] __driver_attach+0x8c/0xd8
> >>> [ 0.807836] bus_for_each_dev+0x7c/0xd8
> >>> [ 0.807837] driver_attach+0x24/0x30
> >>> [ 0.807838] bus_add_driver+0x154/0x200
> >>> [ 0.807839] driver_register+0x64/0x120
> >>> [ 0.807839] __platform_driver_probe+0x7c/0x148
> >>> [ 0.807840] ls_pcie_driver_init+0x24/0x30
> >>> [ 0.807841] do_one_initcall+0x60/0x1d8
> >>> [ 0.807842] kernel_init_freeable+0x1f4/0x24c
> >>> [ 0.807843] kernel_init+0x14/0x118
> >>> [ 0.807843] ret_from_fork+0x10/0x34
> >>> [ 0.807854] SMP: stopping secondary CPUs
> >>> [ 0.807855] Kernel Offset: 0x394c64080000 from
> 0xffff800010000000
> >>> [ 0.807856] PHYS_OFFSET: 0xffff8bfd40000000
> >>> [ 0.807856] CPU features: 0x0240022,21806000
> >>> [ 0.807857] Memory Limit: none
> >>>
> >>> Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
> >>> Signed-off-by: Hou Zhiqiang <[email protected]>
> >>> ---
> >>> drivers/pci/controller/dwc/pcie-designware-host.c | 6 ++++++
> >>> 1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> >>> b/drivers/pci/controller/dwc/pcie-designware-host.c
> >>> index c01c9d2fb3f9..e82b518430c5 100644
> >>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> >>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> >>> @@ -442,6 +442,9 @@ static void __iomem
> >> *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> >>> struct pcie_port *pp = bus->sysdata;
> >>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> >>>
> >>> + if (!dw_pcie_link_up(pci))
> >>> + return NULL;
> >>> +
> >>> busdev = PCIE_ATU_BUS(bus->number) |
> >> PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> >>> PCIE_ATU_FUNC(PCI_FUNC(devfn));
> >>>
> >>> --
> >>> 2.17.1
> >>>

2020-10-20 21:57:14

[permalink] [raw]

Subject: Re: [PATCH] PCI: dwc: Added link up check in map_bus of dw_child_pcie_ops

On Tue, Oct 20, 2020 at 02:13:13AM +0000, Z.q. Hou wrote:

[...]

> > > For NXP Layerscape platforms (the ls1028a and ls2088a are also NXP
> > Layerscape platform), as the error response to AXI/AHB was enabled, it will
> > get UR error and trigger SError on AXI bus when it accesses a non-existent
> > BDF on a link down bus. I'm not clear about how it happens on dra7xxx and
> > imx6, since they doesn't enable the error response to AXI/AHB.
> >
> > That's exactly the case with DRA7xx as the error response is enabled by
> > default in the platform integration.
>
> Got feedback from the imx6 owner that imx6 like the dra7xx has the
> error response enabled by default. Now it's clear that the problem on
> all these platforms is the same.

Ok. Now the question is: on these platforms we trigger an SError because
the link is down. If the link is down there is no point in enumerating
the PCI hierarchy, is there ? If I am right the link-up check should be
moved at probe time and skip enumeration if the link is down, map_bus()
is not really the place where it should be - having it there is
misleading and it is racy code whatever we do.

We may still merge this code as a temporary workaround but the DWC
drivers should be reworked to skip enumeration if the link is down.

Please let me know if my reading is correct.

Thanks,
Lorenzo

> Thanks,
> Zhiqiang
>
> >
> > Thanks
> > Kishon
> >
> > >
> > >>
> > >> The backtrace below contains a bunch of irrelevant info. The
> > >> timestamps are pointless. The backtrace past
> > >> pci_scan_single_device+0x80/0x100 or so really doesn't add anything
> > either.
> > >>
> > >> It'd be nice to have a comment in the code because the code *looks*
> > >> wrong and racy. Without a hint, everybody who sees it will have to
> > >> dig through the history to see why we tolerate the race.
> > >
> > > Yes, agree, but seems the cause of the SError on dra7xx and imx6 is
> > different from Layerscape platforms, we need to make it clear first.
> > >
> > > Thanks,
> > > Zhiqiang
> > >
> > >>
> > >>> [ 0.807773] SError Interrupt on CPU2, code 0xbf000002 -- SError
> > >>> [ 0.807775] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
> > >> 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> > >>> [ 0.807776] Hardware name: LS1046A RDB Board (DT)
> > >>> [ 0.807777] pstate: 20000085 (nzCv daIf -PAN -UAO BTYPE=--)
> > >>> [ 0.807778] pc : pci_generic_config_read+0x3c/0xe0
> > >>> [ 0.807778] lr : pci_generic_config_read+0x24/0xe0
> > >>> [ 0.807779] sp : ffff80001003b7b0
> > >>> [ 0.807780] x29: ffff80001003b7b0 x28: ffff80001003ba74
> > >>> [ 0.807782] x27: ffff000971d96800 x26: ffff00096e77e0a8
> > >>> [ 0.807784] x25: ffff80001003b874 x24: ffff80001003b924
> > >>> [ 0.807786] x23: 0000000000000004 x22: 0000000000000000
> > >>> [ 0.807788] x21: 0000000000000000 x20: ffff80001003b874
> > >>> [ 0.807790] x19: 0000000000000004 x18: ffffffffffffffff
> > >>> [ 0.807791] x17: 00000000000000c0 x16: fffffe0025981840
> > >>> [ 0.807793] x15: ffffb94c75b69948 x14: 62203a383634203a
> > >>> [ 0.807795] x13: 666e6f635f726568 x12: 202c31203d207265
> > >>> [ 0.807797] x11: 626d756e3e2d7375 x10: 656877202c307830
> > >>> [ 0.807799] x9 : 203d206e66766564 x8 : 0000000000000908
> > >>> [ 0.807801] x7 : 0000000000000908 x6 : ffff800010900000
> > >>> [ 0.807802] x5 : ffff00096e77e080 x4 : 0000000000000000
> > >>> [ 0.807804] x3 : 0000000000000003 x2 : 84fa3440ff7e7000
> > >>> [ 0.807806] x1 : 0000000000000000 x0 : ffff800010034000
> > >>> [ 0.807808] Kernel panic - not syncing: Asynchronous SError
> > Interrupt
> > >>> [ 0.807809] CPU: 2 PID: 1 Comm: swapper/0 Not tainted
> > >> 5.9.0-rc5-next-20200914-00001-gf965d3ec86fa #67
> > >>> [ 0.807810] Hardware name: LS1046A RDB Board (DT)
> > >>> [ 0.807811] Call trace:
> > >>> [ 0.807812] dump_backtrace+0x0/0x1c0
> > >>> [ 0.807813] show_stack+0x18/0x28
> > >>> [ 0.807814] dump_stack+0xd8/0x134
> > >>> [ 0.807814] panic+0x180/0x398
> > >>> [ 0.807815] add_taint+0x0/0xb0
> > >>> [ 0.807816] arm64_serror_panic+0x78/0x88
> > >>> [ 0.807817] do_serror+0x68/0x180
> > >>> [ 0.807818] el1_error+0x84/0x100
> > >>> [ 0.807818] pci_generic_config_read+0x3c/0xe0
> > >>> [ 0.807819] dw_pcie_rd_other_conf+0x78/0x110
> > >>> [ 0.807820] pci_bus_read_config_dword+0x88/0xe8
> > >>> [ 0.807821] pci_bus_generic_read_dev_vendor_id+0x30/0x1b0
> > >>> [ 0.807822] pci_bus_read_dev_vendor_id+0x4c/0x78
> > >>> [ 0.807823] pci_scan_single_device+0x80/0x100
> > >>> [ 0.807824] pci_scan_slot+0x38/0x130
> > >>> [ 0.807825] pci_scan_child_bus_extend+0x54/0x2a0
> > >>> [ 0.807826] pci_scan_child_bus+0x14/0x20
> > >>> [ 0.807827] pci_scan_bridge_extend+0x230/0x570
> > >>> [ 0.807828] pci_scan_child_bus_extend+0x134/0x2a0
> > >>> [ 0.807829] pci_scan_root_bus_bridge+0x64/0xf0
> > >>> [ 0.807829] pci_host_probe+0x18/0xc8
> > >>> [ 0.807830] dw_pcie_host_init+0x220/0x378
> > >>> [ 0.807831] ls_pcie_probe+0x104/0x140
> > >>> [ 0.807832] platform_drv_probe+0x54/0xa8
> > >>> [ 0.807833] really_probe+0x118/0x3e0
> > >>> [ 0.807834] driver_probe_device+0x5c/0xc0
> > >>> [ 0.807835] device_driver_attach+0x74/0x80
> > >>> [ 0.807835] __driver_attach+0x8c/0xd8
> > >>> [ 0.807836] bus_for_each_dev+0x7c/0xd8
> > >>> [ 0.807837] driver_attach+0x24/0x30
> > >>> [ 0.807838] bus_add_driver+0x154/0x200
> > >>> [ 0.807839] driver_register+0x64/0x120
> > >>> [ 0.807839] __platform_driver_probe+0x7c/0x148
> > >>> [ 0.807840] ls_pcie_driver_init+0x24/0x30
> > >>> [ 0.807841] do_one_initcall+0x60/0x1d8
> > >>> [ 0.807842] kernel_init_freeable+0x1f4/0x24c
> > >>> [ 0.807843] kernel_init+0x14/0x118
> > >>> [ 0.807843] ret_from_fork+0x10/0x34
> > >>> [ 0.807854] SMP: stopping secondary CPUs
> > >>> [ 0.807855] Kernel Offset: 0x394c64080000 from
> > 0xffff800010000000
> > >>> [ 0.807856] PHYS_OFFSET: 0xffff8bfd40000000
> > >>> [ 0.807856] CPU features: 0x0240022,21806000
> > >>> [ 0.807857] Memory Limit: none
> > >>>
> > >>> Fixes: c2b0c098fbd1 ("PCI: dwc: Use generic config accessors")
> > >>> Signed-off-by: Hou Zhiqiang <[email protected]>
> > >>> ---
> > >>> drivers/pci/controller/dwc/pcie-designware-host.c | 6 ++++++
> > >>> 1 file changed, 6 insertions(+)
> > >>>
> > >>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > >>> b/drivers/pci/controller/dwc/pcie-designware-host.c
> > >>> index c01c9d2fb3f9..e82b518430c5 100644
> > >>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > >>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > >>> @@ -442,6 +442,9 @@ static void __iomem
> > >> *dw_pcie_other_conf_map_bus(struct pci_bus *bus,
> > >>> struct pcie_port *pp = bus->sysdata;
> > >>> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > >>>
> > >>> + if (!dw_pcie_link_up(pci))
> > >>> + return NULL;
> > >>> +
> > >>> busdev = PCIE_ATU_BUS(bus->number) |
> > >> PCIE_ATU_DEV(PCI_SLOT(devfn)) |
> > >>> PCIE_ATU_FUNC(PCI_FUNC(devfn));
> > >>>
> > >>> --
> > >>> 2.17.1
> > >>>

2020-10-21 12:53:26