linux-kernel - *_pcie_establish

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 1 Aug 2019 16:25:29 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Jingoo Han <jingoohan1@...il.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>
Cc:     Fawad Lateef <fawadlateef@...il.com>,
        Yue Wang <yue.wang@...ogic.com>,
        Kevin Hilman <khilman@...libre.com>,
        Andy Gross <agross@...nel.org>,
        Stanimir Varbanov <svarbanov@...sol.com>,
        Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: *_pcie_establish_link() usage

Hi,

I got the following dmesg log from Fawad [1]:

  imx6q-pcie 1ffc000.pcie: host bridge /soc/pcie@...c000 ranges:
  imx6q-pcie 1ffc000.pcie:    IO 0x01f80000..0x01f8ffff -> 0x00000000
  imx6q-pcie 1ffc000.pcie:   MEM 0x01000000..0x01efffff -> 0x01000000
  imx6q-pcie 1ffc000.pcie: Link up
  imx6q-pcie 1ffc000.pcie: Link: Gen2 disabled
  imx6q-pcie 1ffc000.pcie: Link up, Gen1
  imx6q-pcie 1ffc000.pcie: PCI host bridge to bus 0000:00
  pci 0000:00:00.0: [16c3:abcd] type 01 class 0x060400
  pci 0000:00:00.0: PCI bridge to [bus 01-ff]

This is unrelated to the problem Fawad is working on, but the above
looks wrong to me because it associates the "Link up" and link speed
info with the host bridge (imx6q-pcie 1ffc000.pcie), not the Root Port
(pci 0000:00:00.0).

I see that *_pcie_establish_link() is generally called from
*_pcie_host_init(), typically via the struct
dw_pcie_host_ops.host_init pointer, e.g.,

  dra7xx_pcie_probe
    dra7xx_add_pcie_port(dra7xx)
      struct dw_pcie *pci = dra7xx->pci
      struct pcie_port *pp = &pci->pp
      dw_pcie_host_init(struct pcie_port *pp)
	bridge = devm_pci_alloc_host_bridge
	devm_of_pci_get_host_bridge_resources
	pp->ops->host_init(pp)
	  dra7xx_pcie_host_init               # .host_init
	    dra7xx_pcie_establish_link        # <--- bring up link
	    dw_pcie_wait_for_link
	pci_scan_root_bus_bridge(bridge)      # <--- enumerate
	pp->root_bus = bridge->bus
	pci_bus_add_devices(pp->root_bus)

AFAICT, the *_pcie_establish_link() functions all operate on a single
PCIe link, i.e., they are bringing up the link going downstream from a
single Root Port.

It looks like this only allows a single Root Port per struct dw_pcie
device.  Is that true?  *Should* that be true?

It looks like we bring up the link before enumerating.  In some cases,
(meson_pcie_host_init(), qcom_pcie_host_init(),
uniphier_pcie_host_init()) if the link doesn't come up, we return
failure, which means dw_pcie_host_init() will not enumerate devices at
all.

That seems wrong -- can't we have Root Complex Integrated Endpoints
and even other Root Ports on that root bus?  Those should be
accessible and possibly useful even if we can't bring up a link on
*one* Root Port.

I would *expect* that we would enumerate all the devices on the root
bus.  Then if we find one or more Root Ports, we might try to bring up
the link for each one, and if successful, enumerate the downstream
devices.

I'm confused.  Is there some restriction that means there can only be
a single Root Port in this design, and no RCiEPs?  Even if there is,
can we change the code so it enumerates the root bus first and brings
up links as necessary so it matches the generic PCIe topology better?

Bjorn

[1] https://lore.kernel.org/linux-pci/CAGgoGu7rot61LSgu2syOMq9Onx26_u3PEtS7pf_QNQRxOaifhg@mail.gmail.com/