Re: [BISECTED] Regression: Solidrun Clearfog Base won't boot since "PCI: mvebu: Only remap I/O space if configured"

From: Thomas Petazzoni
Date: Mon Sep 24 2018 - 09:10:45 EST


Hello,

On Mon, 24 Sep 2018 13:46:29 +0100, Lorenzo Pieralisi wrote:

> What I think you can do short term, given that AFAICS MVEBU is not
> removable, instead of using pci_host_probe() you move part of its code
> into the driver and make sure that you remap IO as last operation before
> probe completion (ie after scanning the host bridge) so that you do not
> need to unmap it on failure; write a commit log summarising/linking this
> thread please and when v4.20 lands we will give this a more thorough
> look as Russell requested.
>
> How does that sound ?

The only thing that can fail in pci_host_probe() is:

ret = pci_scan_root_bus_bridge(bridge);
if (ret < 0) {
dev_err(bridge->dev.parent, "Scanning root bridge
failed"); return ret;
}

In the pci-mvebu driver prior to the conversion to pci_host_probe(),
the code flow at the end of ->probe() was:

mvebu_pcie_enable()
pci_common_init_dev()
pcibios_init_hw()

and pcibios_init_hw() calls pci_scan_root_bus_bridge(), without doing
much about the return value other than issuing a warning:

ret = pci_scan_root_bus_bridge(bridge);
}

if (WARN(ret < 0, "PCI: unable to scan bus!")) {
pci_free_host_bridge(bridge);
break;
}

I.e, even before the conversion to pci_host_probe(), in case of
failure in pci_scan_root_bus_bridge(), we would have the I/O mapping in
place, but the PCI controller not registered.

We could keep the same (not great) behavior by doing:

diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
index 50eb0729385b..487492f0c5f7 100644
--- a/drivers/pci/controller/pci-mvebu.c
+++ b/drivers/pci/controller/pci-mvebu.c
@@ -1179,9 +1179,6 @@ static int mvebu_pcie_parse_request_resources(struct mvebu_pcie *pcie)
resource_size(&pcie->io) - 1);
pcie->realio.name = "PCI I/O";

- for (i = 0; i < resource_size(&pcie->realio); i += SZ_64K)
- pci_ioremap_io(i, pcie->io.start + i);
-
pci_add_resource(&pcie->resources, &pcie->realio);
}

@@ -1197,7 +1194,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
struct device_node *child;
int num, i, ret;

- bridge = devm_pci_alloc_host_bridge(dev, sizeof(struct mvebu_pcie));
+ bridge = pci_alloc_host_bridge(sizeof(struct mvebu_pcie));
if (!bridge)
return -ENOMEM;

@@ -1212,8 +1209,10 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
num = of_get_available_child_count(np);

pcie->ports = devm_kcalloc(dev, num, sizeof(*pcie->ports), GFP_KERNEL);
- if (!pcie->ports)
- return -ENOMEM;
+ if (!pcie->ports) {
+ ret = -ENOMEM;
+ goto free_host_bridge;
+ }

i = 0;
for_each_available_child_of_node(np, child) {
@@ -1222,7 +1221,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
ret = mvebu_pcie_parse_port(pcie, port, child);
if (ret < 0) {
of_node_put(child);
- return ret;
+ goto free_host_bridge;
} else if (ret == 0) {
continue;
}
@@ -1268,7 +1267,21 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
bridge->align_resource = mvebu_pcie_align_resource;
bridge->msi = pcie->msi;

- return pci_host_probe(bridge);
+ if (resource_size(&pcie->io) != 0) {
+ for (i = 0; i < resource_size(&pcie->realio); i += SZ_64K)
+ pci_ioremap_io(i, pcie->io.start + i);
+ }
+
+ ret = pci_host_probe(bridge);
+ if (ret)
+ pci_free_host_bridge(bridge);
+
+ /* Yes, when pci_host_probe() returns a failure, we don't care */
+ return 0;
+
+free_host_bridge:
+ pci_free_host_bridge(bridge);
+ return ret;
}

static const struct of_device_id mvebu_pcie_of_match_table[] = {

I.e, we simply ignore the failure of pci_host_probe().

To be honest, I really prefer the option of introducing pci_unmap_io().

Best regards,

Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com