Re: [2.6 PATCH] Altix update

From: Bartlomiej Zolnierkiewicz
Date: Wed Feb 18 2004 - 13:04:39 EST



[ just a random nitpicking :-) ]

> +/*
> + * sn_alloc_pci_sysdata() - This routine allocates a pci controller
> + * which is expected as the pci_dev and pci_bus sysdata by the Linux
> + * PCI infrastructure.
> + */
> +struct pci_controller *
> +sn_alloc_pci_sysdata(void)

this can be static

> + pci_sysdata = sn_alloc_pci_sysdata();
> + if (!pci_sysdata) {
> + printk(KERN_WARNING "sn_pci_fixup_bus(): Unable to "
> + "allocate memory for pci_sysdata\n");
> + return -ENOMEM;
> + }
> + widget_sysdata = kmalloc(sizeof(struct sn_widget_sysdata),
> + GFP_KERNEL);
> + if (!widget_sysdata) {
> + printk(KERN_WARNING "sn_pci_fixup_bus(): Unable to "
> + "allocate memory for widget_sysdata\n");
> + return -ENOMEM;
> + }

in case of -ENOMEM pci_sysdata is leaked

> + /* Allocate a controller structure */
> + pci_sysdata = sn_alloc_pci_sysdata();
> + if (!pci_sysdata) {
> + printk(KERN_WARNING "sn_pci_fixup_slot: Unable to "
> + "allocate memory for pci_sysdata\n");
> + return -ENOMEM;
> + }
> +
> + /* Set the device vertex */
> + device_sysdata = kmalloc(sizeof(struct sn_device_sysdata), GFP_KERNEL);
> + if (!device_sysdata) {
> + printk(KERN_WARNING "sn_pci_fixup_slot: Unable to "
> + "allocate memory for device_sysdata\n");
> + return -ENOMEM;
> + }

same pci_sysdata leak on error

> + /* Allocate the IORESOURCE_IO space first */
> + for (idx = 0; idx < PCI_ROM_RESOURCE; idx++) {
> + unsigned long start, end, addr;
> +
> + device_sysdata->pio_map[idx] = NULL;
> +
> + if (!(dev->resource[idx].flags & IORESOURCE_IO))
> + continue;
> +
> + start = dev->resource[idx].start;
> + end = dev->resource[idx].end;
> + size = end - start;
> + if (!size)
> + continue;
> +
> + addr = (unsigned long)pciio_pio_addr(vhdl, 0,
> + PCIIO_SPACE_WIN(idx), 0, size,
> + &device_sysdata->pio_map[idx], 0);
> +
> + if (!addr) {
> + dev->resource[idx].start = 0;
> + dev->resource[idx].end = 0;
> + printk("sn_pci_fixup(): pio map failure for "
> + "%s bar%d\n", dev->slot_name, idx);
> + } else {
> + addr |= __IA64_UNCACHED_OFFSET;
> + dev->resource[idx].start = addr;
> + dev->resource[idx].end = addr + size;
> + }
> +
> + if (dev->resource[idx].flags & IORESOURCE_IO)
> + cmd |= PCI_COMMAND_IO;
> + }

Can dev->resource[idx].flags be modified by pciio_pio_addr()?
If not the last if() is redundant.

> + /* Allocate the IORESOURCE_MEM space next */
> + for (idx = 0; idx < PCI_ROM_RESOURCE; idx++) {
> + unsigned long start, end, addr;
> +
> + if ((dev->resource[idx].flags & IORESOURCE_IO))
> + continue;
> +
> + start = dev->resource[idx].start;
> + end = dev->resource[idx].end;
> + size = end - start;
> + if (!size)
> + continue;
> +
> + addr = (unsigned long)pciio_pio_addr(vhdl, 0,
> + PCIIO_SPACE_WIN(idx), 0, size,
> + &device_sysdata->pio_map[idx], 0);
> +
> + if (!addr) {
> + dev->resource[idx].start = 0;
> + dev->resource[idx].end = 0;
> + printk("sn_pci_fixup(): pio map failure for "
> + "%s bar%d\n", dev->slot_name, idx);
> + } else {
> + addr |= __IA64_UNCACHED_OFFSET;
> + dev->resource[idx].start = addr;
> + dev->resource[idx].end = addr + size;
> + }
> +
> + if (dev->resource[idx].flags & IORESOURCE_MEM)
> + cmd |= PCI_COMMAND_MEMORY;
> + }

whitespaces damage and spaces vs tabs (there is more in the patch)

IORESOURCE_IO and IORESOURCE_MEM cases beg for common helper function,
static int sn_pci_set_resource(struct pci_dev *dev, unsigned long flag)?

> + /*
> + * Update the Command Word on the Card.
> + */
> + cmd |= PCI_COMMAND_MASTER; /* If the device doesn't support */
> + /* bit gets dropped .. no harm */
> + pci_write_config_word(dev, PCI_COMMAND, cmd);
> +
> + pci_read_config_byte(dev, PCI_INTERRUPT_PIN, (unsigned char *)&lines);
> + device_vertex = device_sysdata->vhdl;
> + pci_provider = device_sysdata->pci_provider;
> + device_sysdata->intr_handle = NULL;
> +
> + if (!lines)
> + return 0;
> +
> + irqpdaindr->curr = dev;
> +
> + intr_handle = (pci_provider->intr_alloc)(device_vertex, NULL, lines,
> device_vertex); + if (intr_handle == NULL) {
> + printk(KERN_WARNING "sn_pci_fixup: pcibr_intr_alloc() failed\n");
> + return -ENOMEM;
> + }

pci_sysdata and device_sysdata leak on error

Cheers,
--bart

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/