Adam Belay <[EMAIL PROTECTED]> :
[...]
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
