| Message ID | 20170426111809.19922-9-lorenzo.pieralisi@arm.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > The introduction of pci_scan_root_bus_bridge() provides a PCI core > API to scan a PCI root bus backed by an already initialized > struct pci_host_bridge object, which simplifies the bus scan > interface and makes the PCI scan root bus interface easier to > generalize as members are added to the struct pci_host_bridge(). > > Convert PCI designware host code to pci_scan_root_bus_bridge() to > improve the PCI root bus scanning interface. > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Jingoo Han <jingoohan1@gmail.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Joao Pinto <Joao.Pinto@synopsys.com> > --- > drivers/pci/dwc/pcie-designware-host.c | 36 +++++++++++++++++++++------------- > 1 file changed, 22 insertions(+), 14 deletions(-) > > diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c > index 5ba3349..e43c21a 100644 > --- a/drivers/pci/dwc/pcie-designware-host.c > +++ b/drivers/pci/dwc/pcie-designware-host.c > @@ -294,16 +294,21 @@ int dw_pcie_host_init(struct pcie_port *pp) > dev_err(dev, "missing *config* reg space\n"); > } > > - ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &pp->io_base); > + bridge = pci_alloc_host_bridge(0); > + if (!bridge) > + return -ENOMEM; > + I think here we warn to have the pci_alloc_host_bridge() called in the individual drivers, to have them allocate the dw_pcie structure as part of the host bridge allocation, before calling hisi_add_pcie_port(). Arnd
On Fri, Apr 28, 2017 at 03:13:04PM +0200, Arnd Bergmann wrote: > On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > The introduction of pci_scan_root_bus_bridge() provides a PCI core > > API to scan a PCI root bus backed by an already initialized > > struct pci_host_bridge object, which simplifies the bus scan > > interface and makes the PCI scan root bus interface easier to > > generalize as members are added to the struct pci_host_bridge(). > > > > Convert PCI designware host code to pci_scan_root_bus_bridge() to > > improve the PCI root bus scanning interface. > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Cc: Jingoo Han <jingoohan1@gmail.com> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: Joao Pinto <Joao.Pinto@synopsys.com> > > --- > > drivers/pci/dwc/pcie-designware-host.c | 36 +++++++++++++++++++++------------- > > 1 file changed, 22 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c > > index 5ba3349..e43c21a 100644 > > --- a/drivers/pci/dwc/pcie-designware-host.c > > +++ b/drivers/pci/dwc/pcie-designware-host.c > > @@ -294,16 +294,21 @@ int dw_pcie_host_init(struct pcie_port *pp) > > dev_err(dev, "missing *config* reg space\n"); > > } > > > > - ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &pp->io_base); > > + bridge = pci_alloc_host_bridge(0); > > + if (!bridge) > > + return -ENOMEM; > > + > > I think here we warn to have the pci_alloc_host_bridge() called in the s/warn/want, correct ? > individual drivers, to have them allocate the dw_pcie structure as > part of the host bridge allocation, before calling > hisi_add_pcie_port(). I see what you mean I will see how I can do it with this series or another one, it won't certainly make this DW stuff any shinier, it is really really hard to read. Thanks, Lorenzo
On Fri, Apr 28, 2017 at 03:13:04PM +0200, Arnd Bergmann wrote: > On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > The introduction of pci_scan_root_bus_bridge() provides a PCI core > > API to scan a PCI root bus backed by an already initialized > > struct pci_host_bridge object, which simplifies the bus scan > > interface and makes the PCI scan root bus interface easier to > > generalize as members are added to the struct pci_host_bridge(). > > > > Convert PCI designware host code to pci_scan_root_bus_bridge() to > > improve the PCI root bus scanning interface. > > > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > > Cc: Jingoo Han <jingoohan1@gmail.com> > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: Joao Pinto <Joao.Pinto@synopsys.com> > > --- > > drivers/pci/dwc/pcie-designware-host.c | 36 +++++++++++++++++++++------------- > > 1 file changed, 22 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c > > index 5ba3349..e43c21a 100644 > > --- a/drivers/pci/dwc/pcie-designware-host.c > > +++ b/drivers/pci/dwc/pcie-designware-host.c > > @@ -294,16 +294,21 @@ int dw_pcie_host_init(struct pcie_port *pp) > > dev_err(dev, "missing *config* reg space\n"); > > } > > > > - ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &pp->io_base); > > + bridge = pci_alloc_host_bridge(0); > > + if (!bridge) > > + return -ENOMEM; > > + > > I think here we warn to have the pci_alloc_host_bridge() called in the > individual > drivers, to have them allocate the dw_pcie structure as part of the host > bridge allocation, before calling hisi_add_pcie_port(). I am almost done refactoring this series and this is the last review to address. What you suggest above makes sense but: 1) it means patching ALL dw_pcie_host_init() callers and allocate the host bridge there, not sure it simplifies, certainly it complicates the error handling path (given that the host bridge memory is probably the only memory that is allocated with kzalloc instead of devm interface so it needs unwinding) 2) I noticed that allocating the host bridge in the caller complicates error handling, in particular code in the mainline (ie tegra and ftpci100) using pci_alloc_host_bridge() needs already patching since it may leak memory if PCI host bridge drivers probing fails. I can implement (1) and (2) but I wanted to ask first to understand if that's the direction we want to take. Lorenzo
On Fri, Jun 2, 2017 at 1:49 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Fri, Apr 28, 2017 at 03:13:04PM +0200, Arnd Bergmann wrote: >> On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi >> <lorenzo.pieralisi@arm.com> wrote: >> > The introduction of pci_scan_root_bus_bridge() provides a PCI core >> > API to scan a PCI root bus backed by an already initialized >> > struct pci_host_bridge object, which simplifies the bus scan >> > interface and makes the PCI scan root bus interface easier to >> > generalize as members are added to the struct pci_host_bridge(). >> > >> > Convert PCI designware host code to pci_scan_root_bus_bridge() to >> > improve the PCI root bus scanning interface. >> > >> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> > Cc: Jingoo Han <jingoohan1@gmail.com> >> > Cc: Bjorn Helgaas <bhelgaas@google.com> >> > Cc: Joao Pinto <Joao.Pinto@synopsys.com> >> > --- >> > drivers/pci/dwc/pcie-designware-host.c | 36 +++++++++++++++++++++------------- >> > 1 file changed, 22 insertions(+), 14 deletions(-) >> > >> > diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c >> > index 5ba3349..e43c21a 100644 >> > --- a/drivers/pci/dwc/pcie-designware-host.c >> > +++ b/drivers/pci/dwc/pcie-designware-host.c >> > @@ -294,16 +294,21 @@ int dw_pcie_host_init(struct pcie_port *pp) >> > dev_err(dev, "missing *config* reg space\n"); >> > } >> > >> > - ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &pp->io_base); >> > + bridge = pci_alloc_host_bridge(0); >> > + if (!bridge) >> > + return -ENOMEM; >> > + >> >> I think here we warn to have the pci_alloc_host_bridge() called in the >> individual >> drivers, to have them allocate the dw_pcie structure as part of the host >> bridge allocation, before calling hisi_add_pcie_port(). > > I am almost done refactoring this series and this is the last review to > address. What you suggest above makes sense but: > > 1) it means patching ALL dw_pcie_host_init() callers and allocate the > host bridge there, not sure it simplifies, certainly it complicates > the error handling path (given that the host bridge memory is > probably the only memory that is allocated with kzalloc instead of > devm interface so it needs unwinding) > > 2) I noticed that allocating the host bridge in the caller complicates > error handling, in particular code in the mainline (ie tegra and > ftpci100) using pci_alloc_host_bridge() needs already patching since it > may leak memory if PCI host bridge drivers probing fails. > > I can implement (1) and (2) but I wanted to ask first to understand if > that's the direction we want to take. Good question. Maybe we should add a devm_pci_alloc_host_bridge() instead? Would that solve these problems without creating bigger ones? Arnd
On Fri, Jun 02, 2017 at 03:12:48PM +0200, Arnd Bergmann wrote: > On Fri, Jun 2, 2017 at 1:49 PM, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > On Fri, Apr 28, 2017 at 03:13:04PM +0200, Arnd Bergmann wrote: > >> On Wed, Apr 26, 2017 at 1:17 PM, Lorenzo Pieralisi > >> <lorenzo.pieralisi@arm.com> wrote: > >> > The introduction of pci_scan_root_bus_bridge() provides a PCI core > >> > API to scan a PCI root bus backed by an already initialized > >> > struct pci_host_bridge object, which simplifies the bus scan > >> > interface and makes the PCI scan root bus interface easier to > >> > generalize as members are added to the struct pci_host_bridge(). > >> > > >> > Convert PCI designware host code to pci_scan_root_bus_bridge() to > >> > improve the PCI root bus scanning interface. > >> > > >> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > >> > Cc: Jingoo Han <jingoohan1@gmail.com> > >> > Cc: Bjorn Helgaas <bhelgaas@google.com> > >> > Cc: Joao Pinto <Joao.Pinto@synopsys.com> > >> > --- > >> > drivers/pci/dwc/pcie-designware-host.c | 36 +++++++++++++++++++++------------- > >> > 1 file changed, 22 insertions(+), 14 deletions(-) > >> > > >> > diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c > >> > index 5ba3349..e43c21a 100644 > >> > --- a/drivers/pci/dwc/pcie-designware-host.c > >> > +++ b/drivers/pci/dwc/pcie-designware-host.c > >> > @@ -294,16 +294,21 @@ int dw_pcie_host_init(struct pcie_port *pp) > >> > dev_err(dev, "missing *config* reg space\n"); > >> > } > >> > > >> > - ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &pp->io_base); > >> > + bridge = pci_alloc_host_bridge(0); > >> > + if (!bridge) > >> > + return -ENOMEM; > >> > + > >> > >> I think here we warn to have the pci_alloc_host_bridge() called in the > >> individual > >> drivers, to have them allocate the dw_pcie structure as part of the host > >> bridge allocation, before calling hisi_add_pcie_port(). > > > > I am almost done refactoring this series and this is the last review to > > address. What you suggest above makes sense but: > > > > 1) it means patching ALL dw_pcie_host_init() callers and allocate the > > host bridge there, not sure it simplifies, certainly it complicates > > the error handling path (given that the host bridge memory is > > probably the only memory that is allocated with kzalloc instead of > > devm interface so it needs unwinding) > > > > 2) I noticed that allocating the host bridge in the caller complicates > > error handling, in particular code in the mainline (ie tegra and > > ftpci100) using pci_alloc_host_bridge() needs already patching since it > > may leak memory if PCI host bridge drivers probing fails. > > > > I can implement (1) and (2) but I wanted to ask first to understand if > > that's the direction we want to take. > > Good question. Maybe we should add a devm_pci_alloc_host_bridge() > instead? Would that solve these problems without creating bigger ones? It would be yet another PCI core call (ie unfortunately we still need pci_alloc_host_bridge() for bridges with no parent device eg bios32) but I think that's the only option we have if we do not want to clutter the host bridges error paths with host bridge free calls. Implementing it should be relatively simple by reshuffling functions. Thoughts ? Lorenzo
On Fri, Jun 2, 2017 at 3:56 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Fri, Jun 02, 2017 at 03:12:48PM +0200, Arnd Bergmann wrote: >> > I am almost done refactoring this series and this is the last review to >> > address. What you suggest above makes sense but: >> > >> > 1) it means patching ALL dw_pcie_host_init() callers and allocate the >> > host bridge there, not sure it simplifies, certainly it complicates >> > the error handling path (given that the host bridge memory is >> > probably the only memory that is allocated with kzalloc instead of >> > devm interface so it needs unwinding) >> > >> > 2) I noticed that allocating the host bridge in the caller complicates >> > error handling, in particular code in the mainline (ie tegra and >> > ftpci100) using pci_alloc_host_bridge() needs already patching since it >> > may leak memory if PCI host bridge drivers probing fails. >> > >> > I can implement (1) and (2) but I wanted to ask first to understand if >> > that's the direction we want to take. >> >> Good question. Maybe we should add a devm_pci_alloc_host_bridge() >> instead? Would that solve these problems without creating bigger ones? > > It would be yet another PCI core call (ie unfortunately we still need > pci_alloc_host_bridge() for bridges with no parent device eg bios32) but > I think that's the only option we have if we do not want to clutter the > host bridges error paths with host bridge free calls. > > Implementing it should be relatively simple by reshuffling functions. > > Thoughts ? I think adding this one would be fairly harmless because it has a clear purpose and can easily be mixed with the rest of the API. In one of the earlier drafts, we discussed just letting the user call an initialization function to set the members and otherwise using kmalloc() themselves. We decided on this one to save an extra function call in each driver, but in retrospect, it would have saved us the discussion now. If we end up adding a devm_pci_alloc_host_bridge (or maybe pcim_alloc_host_bridge), we also need to decide what part of the cleanup would happen during the unwinding, besides the kfree(). Arnd
diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c index 5ba3349..e43c21a 100644 --- a/drivers/pci/dwc/pcie-designware-host.c +++ b/drivers/pci/dwc/pcie-designware-host.c @@ -279,9 +279,9 @@ int dw_pcie_host_init(struct pcie_port *pp) struct device_node *np = dev->of_node; struct platform_device *pdev = to_platform_device(dev); struct pci_bus *bus, *child; + struct pci_host_bridge *bridge; struct resource *cfg_res; int i, ret; - LIST_HEAD(res); struct resource_entry *win, *tmp; cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config"); @@ -294,16 +294,21 @@ int dw_pcie_host_init(struct pcie_port *pp) dev_err(dev, "missing *config* reg space\n"); } - ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &pp->io_base); + bridge = pci_alloc_host_bridge(0); + if (!bridge) + return -ENOMEM; + + ret = of_pci_get_host_bridge_resources(np, 0, 0xff, + &bridge->windows, &pp->io_base); if (ret) return ret; - ret = devm_request_pci_bus_resources(dev, &res); + ret = devm_request_pci_bus_resources(dev, &bridge->windows); if (ret) goto error; /* Get the I/O and memory ranges from DT */ - resource_list_for_each_entry_safe(win, tmp, &res) { + resource_list_for_each_entry_safe(win, tmp, &bridge->windows) { switch (resource_type(win->res)) { case IORESOURCE_IO: ret = pci_remap_iospace(win->res, pp->io_base); @@ -397,19 +402,22 @@ int dw_pcie_host_init(struct pcie_port *pp) pp->ops->host_init(pp); pp->root_bus_nr = pp->busn->start; + + bridge->dev.parent = dev; + bridge->sysdata = pp; + bridge->busnr = pp->root_bus_nr; + bridge->ops = &dw_pcie_ops; if (IS_ENABLED(CONFIG_PCI_MSI)) { - bus = pci_scan_root_bus_msi(dev, pp->root_bus_nr, - &dw_pcie_ops, pp, &res, - &dw_pcie_msi_chip); + bridge->msi = &dw_pcie_msi_chip; dw_pcie_msi_chip.dev = dev; - } else - bus = pci_scan_root_bus(dev, pp->root_bus_nr, &dw_pcie_ops, - pp, &res); - if (!bus) { - ret = -ENOMEM; - goto error; } + ret = pci_scan_root_bus_bridge(bridge); + if (ret) + goto error; + + bus = bridge->bus; + if (pp->ops->scan_bus) pp->ops->scan_bus(pp); @@ -428,7 +436,7 @@ int dw_pcie_host_init(struct pcie_port *pp) return 0; error: - pci_free_resource_list(&res); + pci_free_host_bridge(bridge); return ret; }
The introduction of pci_scan_root_bus_bridge() provides a PCI core API to scan a PCI root bus backed by an already initialized struct pci_host_bridge object, which simplifies the bus scan interface and makes the PCI scan root bus interface easier to generalize as members are added to the struct pci_host_bridge(). Convert PCI designware host code to pci_scan_root_bus_bridge() to improve the PCI root bus scanning interface. Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: Jingoo Han <jingoohan1@gmail.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Joao Pinto <Joao.Pinto@synopsys.com> --- drivers/pci/dwc/pcie-designware-host.c | 36 +++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 14 deletions(-)