Messages in this thread | | | Date | Thu, 14 Jul 2005 19:10:14 +0200 | From | Francois Romieu <> | Subject | Re: [RFC][PATCH] split PCI probing code [1/9] |
| |
Adam Belay <abelay@novell.com> : [...]
Some nits + a suspect error branch. It seems nice otherwise.
> --- a/drivers/pci/bus/bus.c 1969-12-31 19:00:00.000000000 -0500 > +++ b/drivers/pci/bus/bus.c 2005-07-10 22:32:53.000000000 -0400 [...] > +struct pci_bus * pci_alloc_bus(void) > +{ > + struct pci_bus *b; > + > + b = kmalloc(sizeof(*b), GFP_KERNEL); > + if (b) { > + memset(b, 0, sizeof(*b));
mm/slab.c provides kcalloc.
[...] > --- a/drivers/pci/bus/config.c 1969-12-31 19:00:00.000000000 -0500 > +++ b/drivers/pci/bus/config.c 2005-07-12 00:52:35.147664368 -0400 [...] > +static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom) > +{ > + unsigned int pos, reg, next; > + u32 l, sz; > + struct resource *res; > + > + for(pos=0; pos<howmany; pos = next) {
for (pos = 0; pos < howmany; pos = next) {
[...] > +static struct pci_dev * __devinit > +pci_scan_device(struct pci_bus *bus, int devfn) > +{ [...] > + dev = kmalloc(sizeof(struct pci_dev), GFP_KERNEL); > + if (!dev) > + return NULL; > + > + memset(dev, 0, sizeof(struct pci_dev));
kcalloc
[...] > + /* Assume 32-bit PCI; let 64-bit PCI cards (which are far rarer) > + set this higher, assuming the system even supports it. */ > + dev->dma_mask = 0xffffffff;
DMA_32BIT_MASK
> + if (pci_setup_device(dev) < 0) { > + kfree(dev); > + return NULL; > + } > + device_initialize(&dev->dev); > + dev->dev.release = pci_release_dev; > + pci_dev_get(dev); > + > + pci_name_device(dev); > + > + dev->dev.dma_mask = &dev->dma_mask; > + dev->dev.coherent_dma_mask = 0xffffffffull;
DMA_32BIT_MASK
[...] > +struct pci_dev * __devinit > +pci_scan_single_device(struct pci_bus *bus, int devfn) > +{ > + struct pci_dev *dev; > + > + dev = pci_scan_device(bus, devfn); > + pci_scan_msi_device(dev); > + > + if (!dev) > + return NULL;
Why not do the test immediately ?
[...] > --- a/drivers/pci/bus/probe.c 1969-12-31 19:00:00.000000000 -0500 > +++ b/drivers/pci/bus/probe.c 2005-07-12 00:55:50.580953992 -0400 [...] > +int __devinit pci_scan_bridge(struct pci_bus *bus, struct pci_dev * dev, int max, int pass) [...] > + > + /* Prevent assigning a bus number that already exists. > + * This can happen when a bridge is hot-plugged */ > + if (pci_find_bus(pci_domain_nr(bus), max+1))
if (pci_find_bus(pci_domain_nr(bus), max + 1))
[...] > + /* > + * For CardBus bridges, we leave 4 bus numbers > + * as cards with a PCI-to-PCI bridge can be > + * inserted later. > + */ > + for (i=0; i<CARDBUS_RESERVE_BUSNR; i++)
for (i = 0; i < CARDBUS_RESERVE_BUSNR; i++)
[...] > +int __devinit pci_scan_slot(struct pci_bus *bus, int devfn) > +{ > + int func, nr = 0; > + int scan_all_fns; > + > + scan_all_fns = pcibios_scan_all_fns(bus, devfn); > + > + for (func = 0; func < 8; func++, devfn++) { > + struct pci_dev *dev; > + > + dev = pci_scan_single_device(bus, devfn); > + if (dev) { > + nr++; > + > + /* > + * If this is a single function device, > + * don't scan past the first function. > + */ > + if (!dev->multifunction) { > + if (func > 0) { > + dev->multifunction = 1; > + } else { > + break; > + }
if (func == 0) break; dev->multifunction = 1;
[...] > +unsigned int __devinit pci_scan_child_bus(struct pci_bus *bus) > +{ [...] > + pcibios_fixup_bus(bus); > + for (pass=0; pass < 2; pass++)
for (pass = 0; pass < 2; pass++)
[...] > +struct pci_bus * __devinit pci_scan_bus_parented(struct device *parent, int bus, struct pci_ops *ops, void *sysdata) > +{ > + int error; > + struct pci_bus *b; > + struct device *dev; > + > + b = pci_alloc_bus(); > + if (!b) > + return NULL; > + > + dev = kmalloc(sizeof(*dev), GFP_KERNEL); > + if (!dev){ > + kfree(b); > + return NULL; > + }
The code below uses goto. Why not here ?
> + > + b->sysdata = sysdata; > + b->ops = ops; > + > + if (pci_find_bus(pci_domain_nr(b), bus)) { > + /* If we already got to this bus through a different bridge, ignore it */ > + pr_debug("PCI: Bus %04x:%02x already known\n", pci_domain_nr(b), bus); > + goto err_out; > + } > + spin_lock(&pci_bus_lock); > + list_add_tail(&b->node, &pci_root_buses); > + spin_unlock(&pci_bus_lock); > + > + memset(dev, 0, sizeof(*dev));
kcalloc
> + dev->parent = parent; > + dev->release = pci_release_bus_bridge_dev; > + sprintf(dev->bus_id, "pci%04x:%02x", pci_domain_nr(b), bus); > + error = device_register(dev); > + if (error) > + goto dev_reg_err;
If you make this goto and the others 'err_WHAT_SHOULD_BE_DONE_some_number', one can verify the error branch without going forth and back.
> + b->bridge = get_device(dev); > + > + b->class_dev.class = &pcibus_class; > + sprintf(b->class_dev.class_id, "%04x:%02x", pci_domain_nr(b), bus); > + error = class_device_register(&b->class_dev); > + if (error) > + goto class_dev_reg_err;
Should not "get_device" above be balanced ?
> + error = class_device_create_file(&b->class_dev, &class_device_attr_cpuaffinity); > + if (error) > + goto class_dev_create_file_err; > + > + /* Create legacy_io and legacy_mem files for this bus */ > + pci_create_legacy_files(b); > + > + error = sysfs_create_link(&b->class_dev.kobj, &b->bridge->kobj, "bridge"); > + if (error) > + goto sys_create_link_err;
Add "pci_remove_legacy_files" on the error branch ?
> + > + b->number = b->secondary = bus; > + b->resource[0] = &ioport_resource; > + b->resource[1] = &iomem_resource; > + > + b->subordinate = pci_scan_child_bus(b); > + > + return b; > + > +sys_create_link_err: > + class_device_remove_file(&b->class_dev, &class_device_attr_cpuaffinity); > +class_dev_create_file_err: > + class_device_unregister(&b->class_dev); > +class_dev_reg_err: > + device_unregister(dev); > +dev_reg_err: > + spin_lock(&pci_bus_lock); > + list_del(&b->node); > + spin_unlock(&pci_bus_lock); > +err_out: > + kfree(dev); > + kfree(b); > + return NULL; > +} > +
-- Ueimor - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |