diff mbox series

[RFC,08/15] x86: PCI: clean up pcibios_scan_root()

Message ID 20180817102645.3839621-9-arnd@arndb.de
State New
Headers show
Series None | expand

Commit Message

Arnd Bergmann Aug. 17, 2018, 10:26 a.m. UTC 
pcibios_scan_root() is now just a wrapper around pci_scan_root_bus(),
and merging the two into one makes it shorter and more readable.

We can also take advantage of pci_alloc_host_bridge() doing the
allocation of the sysdata for us, which helps if we ever want to
allow hot-unplugging the host bridge itself.

We might be able to simplify it further using pci_host_probe(),
but I wasn't sure about the resource registration there.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 arch/x86/pci/common.c | 53 ++++++++++++++-----------------------------
 1 file changed, 17 insertions(+), 36 deletions(-)

-- 
2.18.0

Comments

Rafael J. Wysocki Aug. 20, 2018, 8:31 a.m. UTC | #1 
On Fri, Aug 17, 2018 at 12:32 PM Arnd Bergmann <arnd@arndb.de> wrote:
>

> pcibios_scan_root() is now just a wrapper around pci_scan_root_bus(),

> and merging the two into one makes it shorter and more readable.

>

> We can also take advantage of pci_alloc_host_bridge() doing the

> allocation of the sysdata for us, which helps if we ever want to

> allow hot-unplugging the host bridge itself.

>

> We might be able to simplify it further using pci_host_probe(),

> but I wasn't sure about the resource registration there.

>

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

>  arch/x86/pci/common.c | 53 ++++++++++++++-----------------------------

>  1 file changed, 17 insertions(+), 36 deletions(-)

>

> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c

> index e740d9aa4024..920d0885434c 100644

> --- a/arch/x86/pci/common.c

> +++ b/arch/x86/pci/common.c

> @@ -453,54 +453,35 @@ void __init dmi_check_pciprobe(void)

>         dmi_check_system(pciprobe_dmi_table);

>  }

>

> -static struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,

> -               struct pci_ops *ops, void *sysdata, struct list_head *resources)

> +void pcibios_scan_root(int busnum)

>  {

> +       struct pci_sysdata *sd;

>         struct pci_host_bridge *bridge;

>         int error;

>

> -       bridge = pci_alloc_host_bridge(0);

> -       if (!bridge)

> -               return NULL;

> +       bridge = pci_alloc_host_bridge(sizeof(sd));

> +       if (!bridge) {

> +               printk(KERN_ERR "PCI: OOM, skipping PCI bus %02x\n", busnum);

> +               return;

> +       }

> +       sd = pci_host_bridge_priv(bridge);


This looks fishy, as bridge->private is not set at this point AFAICS,
unless one of the previous patches changes that.

>

> -       list_splice_init(resources, &bridge->windows);

> -       bridge->dev.parent = parent;

> -       bridge->sysdata = sysdata;

> -       bridge->busnr = bus;

> -       bridge->ops = ops;

> +       sd->node = x86_pci_root_bus_node(busnum);

> +       x86_pci_root_bus_resources(busnum, &bridge->windows);

> +       bridge->sysdata = sd;

> +       bridge->busnr = busnum;

> +       bridge->ops = &pci_root_ops;

>

> +       printk(KERN_DEBUG "PCI: Probing PCI hardware (bus %02x)\n", busnum);

>         error = pci_scan_root_bus_bridge(bridge);

>         if (error < 0)

>                 goto err_out;

>

> -       return bridge->bus;

> +       pci_bus_add_devices(bridge->bus);

> +       return;

>

>  err_out:

> -       kfree(bridge);

> -       return NULL;

> -}

> -

> -void pcibios_scan_root(int busnum)

> -{

> -       struct pci_bus *bus;

> -       struct pci_sysdata *sd;

> -       LIST_HEAD(resources);

> -

> -       sd = kzalloc(sizeof(*sd), GFP_KERNEL);

> -       if (!sd) {

> -               printk(KERN_ERR "PCI: OOM, skipping PCI bus %02x\n", busnum);

> -               return;

> -       }

> -       sd->node = x86_pci_root_bus_node(busnum);

> -       x86_pci_root_bus_resources(busnum, &resources);

> -       printk(KERN_DEBUG "PCI: Probing PCI hardware (bus %02x)\n", busnum);

> -       bus = pci_scan_root_bus(NULL, busnum, &pci_root_ops, sd, &resources);

> -       if (!bus) {

> -               pci_free_resource_list(&resources);

> -               kfree(sd);

> -               return;

> -       }

> -       pci_bus_add_devices(bus);

> +       pci_free_host_bridge(bridge);

>  }

>

>  void __init pcibios_set_cache_line_size(void)

> --

> 2.18.0

>
Arnd Bergmann Aug. 20, 2018, 11:16 a.m. UTC | #2 
On Mon, Aug 20, 2018 at 10:31 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Aug 17, 2018 at 12:32 PM Arnd Bergmann <arnd@arndb.de> wrote:


> > -static struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,

> > -               struct pci_ops *ops, void *sysdata, struct list_head *resources)

> > +void pcibios_scan_root(int busnum)

> >  {

> > +       struct pci_sysdata *sd;

> >         struct pci_host_bridge *bridge;

> >         int error;

> >

> > -       bridge = pci_alloc_host_bridge(0);

> > -       if (!bridge)

> > -               return NULL;

> > +       bridge = pci_alloc_host_bridge(sizeof(sd));

> > +       if (!bridge) {

> > +               printk(KERN_ERR "PCI: OOM, skipping PCI bus %02x\n", busnum);

> > +               return;

> > +       }

> > +       sd = pci_host_bridge_priv(bridge);

>

> This looks fishy, as bridge->private is not set at this point AFAICS,

> unless one of the previous patches changes that.


bridge->private what comes after the bridge structure, and it's allocated
by pci_alloc_host_bridge() passing the size of the structure we want
for this private area.

         Arnd
diff mbox series

Patch

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index e740d9aa4024..920d0885434c 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -453,54 +453,35 @@  void __init dmi_check_pciprobe(void)
 	dmi_check_system(pciprobe_dmi_table);
 }
 
-static struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
-		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+void pcibios_scan_root(int busnum)
 {
+	struct pci_sysdata *sd;
 	struct pci_host_bridge *bridge;
 	int error;
 
-	bridge = pci_alloc_host_bridge(0);
-	if (!bridge)
-		return NULL;
+	bridge = pci_alloc_host_bridge(sizeof(sd));
+	if (!bridge) {
+		printk(KERN_ERR "PCI: OOM, skipping PCI bus %02x\n", busnum);
+		return;
+	}
+	sd = pci_host_bridge_priv(bridge);
 
-	list_splice_init(resources, &bridge->windows);
-	bridge->dev.parent = parent;
-	bridge->sysdata = sysdata;
-	bridge->busnr = bus;
-	bridge->ops = ops;
+	sd->node = x86_pci_root_bus_node(busnum);
+	x86_pci_root_bus_resources(busnum, &bridge->windows);
+	bridge->sysdata = sd;
+	bridge->busnr = busnum;
+	bridge->ops = &pci_root_ops;
 
+	printk(KERN_DEBUG "PCI: Probing PCI hardware (bus %02x)\n", busnum);
 	error = pci_scan_root_bus_bridge(bridge);
 	if (error < 0)
 		goto err_out;
 
-	return bridge->bus;
+	pci_bus_add_devices(bridge->bus);
+	return;
 
 err_out:
-	kfree(bridge);
-	return NULL;
-}
-
-void pcibios_scan_root(int busnum)
-{
-	struct pci_bus *bus;
-	struct pci_sysdata *sd;
-	LIST_HEAD(resources);
-
-	sd = kzalloc(sizeof(*sd), GFP_KERNEL);
-	if (!sd) {
-		printk(KERN_ERR "PCI: OOM, skipping PCI bus %02x\n", busnum);
-		return;
-	}
-	sd->node = x86_pci_root_bus_node(busnum);
-	x86_pci_root_bus_resources(busnum, &resources);
-	printk(KERN_DEBUG "PCI: Probing PCI hardware (bus %02x)\n", busnum);
-	bus = pci_scan_root_bus(NULL, busnum, &pci_root_ops, sd, &resources);
-	if (!bus) {
-		pci_free_resource_list(&resources);
-		kfree(sd);
-		return;
-	}
-	pci_bus_add_devices(bus);
+	pci_free_host_bridge(bridge);
 }
 
 void __init pcibios_set_cache_line_size(void)