diff mbox series

[RFC,07/15] PCI/ACPI: clean up acpi_pci_root_create()

Message ID 20180817102645.3839621-8-arnd@arndb.de (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series PCI: turn some __weak functions into callbacks | expand

Commit Message

Arnd Bergmann Aug. 17, 2018, 10:26 a.m. UTC
The acpi_pci_create_root_bus() can be fully integrated into
acpi_pci_root_create(), improving a few things:

* We can call pci_scan_root_bus_bridge(), which registers and
  scans the bridge in one step.
* After a failure in pci_register_host_bridge(), we correctly
  clean up the resources.
* The bridge settings (release function, flags, operations etc)
  can get set up before registering the bridge.
* Further cleanup would be possible, removing duplication between
  pci_host_bridge and some ACPI structures.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/acpi/pci_root.c | 68 +++++++++++++++--------------------------
 1 file changed, 24 insertions(+), 44 deletions(-)

Comments

Rafael J. Wysocki Aug. 20, 2018, 8:23 a.m. UTC | #1
On Fri, Aug 17, 2018 at 12:33 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> The acpi_pci_create_root_bus() can be fully integrated into
> acpi_pci_root_create(), improving a few things:
>
> * We can call pci_scan_root_bus_bridge(), which registers and
>   scans the bridge in one step.
> * After a failure in pci_register_host_bridge(), we correctly
>   clean up the resources.
> * The bridge settings (release function, flags, operations etc)
>   can get set up before registering the bridge.
> * Further cleanup would be possible, removing duplication between
>   pci_host_bridge and some ACPI structures.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/acpi/pci_root.c | 68 +++++++++++++++--------------------------
>  1 file changed, 24 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 85dbcf47015b..5f73de3b67c8 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -873,34 +873,6 @@ static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
>         __acpi_pci_root_release_info(bridge->release_data);
>  }
>
> -static struct pci_bus *acpi_pci_create_root_bus(struct device *parent, int bus,
> -               struct pci_ops *ops, void *sysdata, struct list_head *resources)
> -{
> -       int error;
> -       struct pci_host_bridge *bridge;
> -
> -       bridge = pci_alloc_host_bridge(0);
> -       if (!bridge)
> -               return NULL;
> -
> -       bridge->dev.parent = parent;
> -
> -       list_splice_init(resources, &bridge->windows);
> -       bridge->sysdata = sysdata;
> -       bridge->busnr = bus;
> -       bridge->ops = ops;
> -
> -       error = pci_register_host_bridge(bridge);
> -       if (error < 0)
> -               goto err_out;
> -
> -       return bridge->bus;
> -
> -err_out:
> -       kfree(bridge);
> -       return NULL;
> -}
> -
>  struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>                                      struct acpi_pci_root_ops *ops,
>                                      struct acpi_pci_root_info *info,
> @@ -909,8 +881,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>         int ret, busnum = root->secondary.start;
>         struct acpi_device *device = root->device;
>         int node = acpi_get_node(device->handle);
> -       struct pci_bus *bus;
> -       struct pci_host_bridge *host_bridge;
> +       struct pci_host_bridge *bridge;

Why "bridge" and not "host" or even something to stand for "root complex"?

Or maybe it can still be "host_bridge"?

>
>         info->root = root;
>         info->bridge = device;
> @@ -930,30 +901,39 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
>
>         pci_acpi_root_add_resources(info);
>         pci_add_resource(&info->resources, &root->secondary);
> -       bus = acpi_pci_create_root_bus(NULL, busnum, ops->pci_ops,
> -                                 sysdata, &info->resources);
> -       if (!bus)
> +
> +       bridge = pci_alloc_host_bridge(0);
> +       if (!bridge)
>                 goto out_release_info;
>
> -       host_bridge = to_pci_host_bridge(bus->bridge);
> +       list_splice_init(&info->resources, &bridge->windows);
> +       bridge->sysdata = sysdata;
> +       bridge->busnr = busnum;
> +       bridge->ops = ops->pci_ops;
> +       pci_set_host_bridge_release(bridge, acpi_pci_root_release_info,
> +                                   info);
> +
>         if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
> -               host_bridge->native_pcie_hotplug = 0;
> +               bridge->native_pcie_hotplug = 0;
>         if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
> -               host_bridge->native_shpc_hotplug = 0;
> +               bridge->native_shpc_hotplug = 0;
>         if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
> -               host_bridge->native_aer = 0;
> +               bridge->native_aer = 0;
>         if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
> -               host_bridge->native_pme = 0;
> +               bridge->native_pme = 0;
>         if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
> -               host_bridge->native_ltr = 0;
> +               bridge->native_ltr = 0;
> +
> +       ret = pci_scan_root_bus_bridge(bridge);
> +       if (ret < 0)
> +               goto out_release_bridge;
>
> -       pci_scan_child_bus(bus);
> -       pci_set_host_bridge_release(host_bridge, acpi_pci_root_release_info,
> -                                   info);
>         if (node != NUMA_NO_NODE)
> -               dev_printk(KERN_DEBUG, &bus->dev, "on NUMA node %d\n", node);
> -       return bus;
> +               dev_printk(KERN_DEBUG, &bridge->bus->dev, "on NUMA node %d\n", node);
> +       return bridge->bus;
>
> +out_release_bridge:
> +       pci_free_host_bridge(bridge);
>  out_release_info:
>         __acpi_pci_root_release_info(info);
>         return NULL;
> --
> 2.18.0
>
Arnd Bergmann Aug. 20, 2018, 11:19 a.m. UTC | #2
On Mon, Aug 20, 2018 at 10:23 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Aug 17, 2018 at 12:33 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > @@ -909,8 +881,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> >         int ret, busnum = root->secondary.start;
> >         struct acpi_device *device = root->device;
> >         int node = acpi_get_node(device->handle);
> > -       struct pci_bus *bus;
> > -       struct pci_host_bridge *host_bridge;
> > +       struct pci_host_bridge *bridge;
>
> Why "bridge" and not "host" or even something to stand for "root complex"?
>
> Or maybe it can still be "host_bridge"?

I did this for consistency with the naming in drivers/pci/probe.c,
which always declares the local variable as 'struct pci_host_bridge *bridge'.
It's easy to change here if you feel strongly about it (I don't).

        Arnd
Rafael J. Wysocki Aug. 20, 2018, 11:24 a.m. UTC | #3
On Mon, Aug 20, 2018 at 1:20 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Aug 20, 2018 at 10:23 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Fri, Aug 17, 2018 at 12:33 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > @@ -909,8 +881,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> > >         int ret, busnum = root->secondary.start;
> > >         struct acpi_device *device = root->device;
> > >         int node = acpi_get_node(device->handle);
> > > -       struct pci_bus *bus;
> > > -       struct pci_host_bridge *host_bridge;
> > > +       struct pci_host_bridge *bridge;
> >
> > Why "bridge" and not "host" or even something to stand for "root complex"?
> >
> > Or maybe it can still be "host_bridge"?
>
> I did this for consistency with the naming in drivers/pci/probe.c,
> which always declares the local variable as 'struct pci_host_bridge *bridge'.
> It's easy to change here if you feel strongly about it (I don't).

I would leave host_bridge here.  It would make the patch smaller too I think.
Arnd Bergmann Aug. 20, 2018, 11:36 a.m. UTC | #4
On Mon, Aug 20, 2018 at 1:24 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Aug 20, 2018 at 1:20 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Mon, Aug 20, 2018 at 10:23 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > On Fri, Aug 17, 2018 at 12:33 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > > @@ -909,8 +881,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
> > > >         int ret, busnum = root->secondary.start;
> > > >         struct acpi_device *device = root->device;
> > > >         int node = acpi_get_node(device->handle);
> > > > -       struct pci_bus *bus;
> > > > -       struct pci_host_bridge *host_bridge;
> > > > +       struct pci_host_bridge *bridge;
> > >
> > > Why "bridge" and not "host" or even something to stand for "root complex"?
> > >
> > > Or maybe it can still be "host_bridge"?
> >
> > I did this for consistency with the naming in drivers/pci/probe.c,
> > which always declares the local variable as 'struct pci_host_bridge *bridge'.
> > It's easy to change here if you feel strongly about it (I don't).
>
> I would leave host_bridge here.  It would make the patch smaller too I think.

Ok, I've changed my local copy as you suggested now.

      Arnd
diff mbox series

Patch

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 85dbcf47015b..5f73de3b67c8 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -873,34 +873,6 @@  static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
 	__acpi_pci_root_release_info(bridge->release_data);
 }
 
-static struct pci_bus *acpi_pci_create_root_bus(struct device *parent, int bus,
-		struct pci_ops *ops, void *sysdata, struct list_head *resources)
-{
-	int error;
-	struct pci_host_bridge *bridge;
-
-	bridge = pci_alloc_host_bridge(0);
-	if (!bridge)
-		return NULL;
-
-	bridge->dev.parent = parent;
-
-	list_splice_init(resources, &bridge->windows);
-	bridge->sysdata = sysdata;
-	bridge->busnr = bus;
-	bridge->ops = ops;
-
-	error = pci_register_host_bridge(bridge);
-	if (error < 0)
-		goto err_out;
-
-	return bridge->bus;
-
-err_out:
-	kfree(bridge);
-	return NULL;
-}
-
 struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 				     struct acpi_pci_root_ops *ops,
 				     struct acpi_pci_root_info *info,
@@ -909,8 +881,7 @@  struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 	int ret, busnum = root->secondary.start;
 	struct acpi_device *device = root->device;
 	int node = acpi_get_node(device->handle);
-	struct pci_bus *bus;
-	struct pci_host_bridge *host_bridge;
+	struct pci_host_bridge *bridge;
 
 	info->root = root;
 	info->bridge = device;
@@ -930,30 +901,39 @@  struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root,
 
 	pci_acpi_root_add_resources(info);
 	pci_add_resource(&info->resources, &root->secondary);
-	bus = acpi_pci_create_root_bus(NULL, busnum, ops->pci_ops,
-				  sysdata, &info->resources);
-	if (!bus)
+
+	bridge = pci_alloc_host_bridge(0);
+	if (!bridge)
 		goto out_release_info;
 
-	host_bridge = to_pci_host_bridge(bus->bridge);
+	list_splice_init(&info->resources, &bridge->windows);
+	bridge->sysdata = sysdata;
+	bridge->busnr = busnum;
+	bridge->ops = ops->pci_ops;
+	pci_set_host_bridge_release(bridge, acpi_pci_root_release_info,
+				    info);
+
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_NATIVE_HP_CONTROL))
-		host_bridge->native_pcie_hotplug = 0;
+		bridge->native_pcie_hotplug = 0;
 	if (!(root->osc_control_set & OSC_PCI_SHPC_NATIVE_HP_CONTROL))
-		host_bridge->native_shpc_hotplug = 0;
+		bridge->native_shpc_hotplug = 0;
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_AER_CONTROL))
-		host_bridge->native_aer = 0;
+		bridge->native_aer = 0;
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_PME_CONTROL))
-		host_bridge->native_pme = 0;
+		bridge->native_pme = 0;
 	if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
-		host_bridge->native_ltr = 0;
+		bridge->native_ltr = 0;
+
+	ret = pci_scan_root_bus_bridge(bridge);
+	if (ret < 0)
+		goto out_release_bridge;
 
-	pci_scan_child_bus(bus);
-	pci_set_host_bridge_release(host_bridge, acpi_pci_root_release_info,
-				    info);
 	if (node != NUMA_NO_NODE)
-		dev_printk(KERN_DEBUG, &bus->dev, "on NUMA node %d\n", node);
-	return bus;
+		dev_printk(KERN_DEBUG, &bridge->bus->dev, "on NUMA node %d\n", node);
+	return bridge->bus;
 
+out_release_bridge:
+	pci_free_host_bridge(bridge);
 out_release_info:
 	__acpi_pci_root_release_info(info);
 	return NULL;