| Message ID | 20180817102645.3839621-9-arnd@arndb.de (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | PCI: turn some __weak functions into callbacks | expand |
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 >
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
On Mon, Aug 20, 2018 at 1:17 PM Arnd Bergmann <arnd@arndb.de> wrote: > > 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. I see, sorry for the noise.
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)
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(-)