On Wed, Dec 13, 2017 at 06:53:53PM -0500, Jim Quinlan wrote:

On Tue, Dec 12, 2017 at 5:16 PM, Bjorn Helgaas [off-list ref] wrote:

quoted

On Tue, Nov 14, 2017 at 05:12:07PM -0500, Jim Quinlan wrote:

quoted

This commit adds the basic Broadcom STB PCIe controller.  Missing is
the ability to process MSI and also handle dma-ranges for inbound
memory accesses.  These two functionalities are added in subsequent
commits.

The PCIe block contains an MDIO interface.  This is a local interface
only accessible by the PCIe controller.  It cannot be used or shared
by any other HW.  As such, the small amount of code for this
controller is included in this driver as there is little upside to put
it elsewhere.

...

quoted

quoted

+static bool brcm_pcie_valid_device(struct brcm_pcie *pcie, struct pci_bus *bus,
+                                int dev)
+{
+     if (pci_is_root_bus(bus)) {
+             if (dev > 0)
+                     return false;
+     } else {
+             /* If there is no link, then there is no device */
+             if (!brcm_pcie_link_up(pcie))
+                     return false;

This is racy, since the link can go down after you check but before
you do the config access.  I assume your hardware can deal with a
config access that targets a link that is down?

Yes, that can happen but there is really nothing that can be done if
the link goes down in that vulnerability window.  What do you suggest
doing?

Most hardware drops writes and returns ~0 on reads if the link is
down.  I assume your hardware does something similar, and that should
be enough.  You shouldn't have to check whether the link is up.

The hardware might report errors, e.g., via AER, if the link is down.
And we might not not handle those nicely.  If we have issues there, we
should find out and fix them.

I see that dwc, altera, rockchip, and xilinx all do check for link up
there, too.  I can't remember if they had a legitimate reason, or if I
just missed it.

quoted

quoted

+static void brcm_pcie_remove_controller(struct brcm_pcie *pcie)
+{
+     struct list_head *pos, *q;
+     struct brcm_pcie *tmp;
+
+     mutex_lock(&brcm_pcie_lock);
+     list_for_each_safe(pos, q, &brcm_pcie) {
+             tmp = list_entry(pos, struct brcm_pcie, list);
+             if (tmp == pcie) {
+                     list_del(pos);
+                     if (list_empty(&brcm_pcie))
+                             num_memc = 0;
+                     break;
+             }
+     }
+     mutex_unlock(&brcm_pcie_lock);

I'm missing something.  I don't see that num_memc is ever set to
anything *other* than zero.

The num_memc is set and used in the dma commit.  I will remove its
declaration from this commit.

Thanks, that will make the patches much easier to read.

quoted

quoted

+     pcie->id = of_get_pci_domain_nr(dn);

Why do you call of_get_pci_domain_nr() directly?  No other driver
does.

We use the domain as the controller number (id).  We use the id to
identify and fix a HW bug that only affects the 2nd controller; see
the clause
" } else if (of_machine_is_compatible("brcm,bcm7278a0")) {".

pci_register_host_bridge() already sets bus->domain_nr for every host
bridge.  That path calls of_get_pci_domain_nr() eventually.   But I
guess that's too late for your use case, because you have this:

  brcm_pcie_probe
    brcm_pcie_setup
      brcm_pcie_bridge_sw_init_set
        if (of_machine_is_compatible("brcm,bcm7278a0")) {
          offset = pcie->id ? ...                    <--- use
    pci_scan_root_bus_bridge
      pci_register_host_bridge
        bus->domain_nr = pci_bus_find_domain_nr      <--- available

I guess you haven't added a binding for brcm,bcm7278a0 yet?

I'm not really sure that using the linux,pci-domain DT property is the
best way to distinguish the two controllers.  Maybe Rob has an
opinion?

quoted

quoted

+     if (pcie->id < 0)
+             return pcie->id;

Bjorn