On Monday 14 March 2016 12:04:37 Bjorn Helgaas wrote:

On Mon, Mar 14, 2016 at 03:51:01PM +0000, Bharat Kumar Gogada wrote:

quoted

quoted

On Sun, Mar 06, 2016 at 10:02:14PM +0530, Bharat Kumar Gogada wrote:

quoted

Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.

quoted

+static bool nwl_pcie_valid_device(struct pci_bus *bus, unsigned int
+devfn) {
+	struct nwl_pcie *pcie = bus->sysdata;
+
+	/* Check link,before accessing downstream ports */
+	if (bus->number != pcie->root_busno) {
+		if (!nwl_pcie_link_up(pcie))
+			return false;
+	}

This seems racy.  What if we check, and the link is up, but the
link goes down before we actually complete the config access?

I'm suggesting that this check for the link being up might be
superfluous.

quoted

Without the above check and also if there is no EP then we are getting kernel stack as follows,

Hello! Now I found this old thread... And I would like to ask, have you
solved this issue somehow? Because very similar problem I observe with
pci-aardvark.c, just it cause Synchronous External Abort on CPU.

quoted

[    2.654105] PCI host bridge /amba/pcie@fd0e0000 ranges:
[    2.659268]   No bus range found for /amba/pcie@fd0e0000, using [bus 00-ff]
[    2.666195]   MEM 0xe1000000..0xefffffff -> 0xe1000000
[    2.671410] nwl-pcie fd0e0000.pcie: PCI host bridge to bus 0000:00
[    2.677436] pci_bus 0000:00: root bus resource [bus 00-ff]
[    2.682883] pci_bus 0000:00: root bus resource [mem 0xe1000000-0xefffffff]
[    2.690031] Unhandled fault: synchronous external abort (0x96000210) at 0xffffff8000200000
[    2.690036] nwl-pcie fd0e0000.pcie: Slave error
[    2.702582] Internal error: : 96000210 [#1] SMP
[    2.707078] Modules linked in:
[    2.710108] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 4.5.0-rc6+ #5
[    2.716332] Hardware name: ZynqMP (DT)
[    2.720659] task: ffffffc0798bed00 ti: ffffffc0798c0000 task.ti: ffffffc0798c0000
[    2.728102] PC is at pci_generic_config_read+0x38/0x9c
[    2.733202] LR is at pci_generic_config_read+0x1c/0x9c
.......
[    3.322701] [<ffffffc000498b1c>] pci_generic_config_read+0x38/0x9c
[    3.328842] [<ffffffc000498f54>] pci_bus_read_config_dword+0x80/0xb0
[    3.335156] [<ffffffc00049abd4>] pci_bus_read_dev_vendor_id+0x30/0x104
[    3.341643] [<ffffffc00049c5b0>] pci_scan_single_device+0x50/0xc4
[    3.347698] [<ffffffc00049c674>] pci_scan_slot+0x50/0xe8
[    3.352974] [<ffffffc00049d530>] pci_scan_child_bus+0x30/0xd8
[    3.358683] [<ffffffc00049d210>] pci_scan_bridge+0x1fc/0x4ec
[    3.364306] [<ffffffc00049d58c>] pci_scan_child_bus+0x8c/0xd8
[    3.370016] [<ffffffc0004b2d9c>] nwl_pcie_probe+0x6c4/0x8e0
.....

quoted

The hardware should do something reasonable with the config access if it
can't send it down the link.

When Link is down and H/W gets a ECAM access request for downstream
ports, hardware responds by DECERR (decode error) status on AXI
Interface.

DECERR isn't a PCIe concept, so I assume it's something specific to
Xilinx.  In the general case of a PCIe switch, a config access that
targets a device where the link is down should cause an Unsupported
Request completion (see PCIe spec r3.0, section 2.9.1, quoted below).
Possibly your Root Complex turns Unsupported Request completions into
DECERR.

This looks like same design as with pci aardvark hw. PCIe Unsupported
Request or Completion Abort is converted to AXI DECERR (or SLVERR)
which is then reported to CPU. In my case AXI DECERR/SLVERR cause
Synchronous External Abort on CPU.

  2.9 Link Status Dependencies
  2.9.1 Transaction Layer Behavior in DL_Down Status

  DL_Down status indicates that there is no connection with another
  component on the Link, or that the connection with the other
  component has been lost and is not recoverable by the Physical or
  Data Link Layers.

  For a Downstream Port, DL_Down status is handled by:
  
   for Non-Posted Requests, forming completions for any Requests
   submitted by the device core for Transmission, returning
   Unsupported Request Completion Status, then discarding the Requests

Linux expects reads with Unsupported Request completion status to
return all 1's data to the CPU as in section 2.3.2:

  2.3.2 Completion Handling Rules

  Read Data Values with UR Completion Status

  Some system configuration software depends on reading a data value
  of all 1’s when a Configuration Read Request is terminated as an
  Unsupported Request, particularly when probing to determine the
  existence of a device in the system.  A Root Complex intended for
  use with software that depends on a read-data value of all 1’s must
  synthesize this value when UR Completion Status is returned for a
  Configuration Read Request.

quoted

So without any EP and without this condition, Linux kernel cannot
determine above response from H/W. So the above condition is useful
only when no EP is connected.

Now even if the link is up initially, but the link goes down before
we actually complete the config access, then H/W responds by DECERR,
then Linux kernel might throw similar stack. (We haven't observed
this condition yet)

It'd be hard to hit this race unless you added delay in
nwl_pcie_map_bus() after nwl_pcie_valid_device(), then removed the
device during that delay.

quoted

It looks like we need a different type of hardware response to get
rid of this situation, but it's not easy way.  Have you come across
this/similar kind of problem anywhere else?  Can you suggest if
there is any other way to handle this.

I'm not a hardware designer, so I don't know what to suggest here.
The current design does seem like a robustness issue: surprise removal
of a device may cause this external abort in rare cases.

With pci aardvark I'm able to reproduce this issue with surprise removal
of device.

Bharat, have you somehow resolved this issue? Seems that this kind of HW
design is not rare.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel