On Tue, 2005-03-08 at 22:29 +0100, Kronos wrote:
> > + bus = pdev->bus;
> > + while (bus) {
> > + bridge = bus->self;
> > + if (bridge) {
> > + pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
> > &cmd);
> > + if (!(cmd & PCI_BRIDGE_CTL_VGA))
> > + continue;
>
> This seems wrong: if the condition is true the loop will restart with
> the same bus device and will never stop. I think you should do:
Yup. I always try to avoid nesting if's tho, which is why I wrote it
that way :) But yes, it should be fixed.
> > +
> > + /* Only deal with VGA class devices */
> > + if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> > + return;
> > +
> > + /* Allocate structure */
> > + vgadev = kmalloc(sizeof(struct vga_device), GFP_KERNEL);
>
> Not checking return value of kmalloc, this is evil :P
> Also it may be worth to change return type in order to signal the error.
Yah, yah, I know :) will fix. I'm not sure there is point signaling the
error to the PCI layer. It won't do anything good with it.
> > +#endif
> > +
> > + /* Add to the list */
> > + list_add(&vgadev->list, &vga_list);
> > + spin_unlock_irqrestore(&vga_lock, flags);
>
> Missing return?
Yup.
> > + fail:
> > + spin_unlock_irqrestore(&vga_lock, flags);
> > + kfree(vgadev);
> > +}
> > +
> > +void vga_arbiter_del_pci_device(struct pci_dev *pdev)
> > +{
> > + struct vga_device *vgadev;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&vga_lock, flags);
> > + vgadev = vgadev_find(pdev);
> > + if (vgadev == NULL)
> > + goto bail;
> > + if (vgadev->locks)
> > + __vga_put(vgadev, vgadev->locks);
> > + list_del(&vgadev->list);
> > + bail:
> > + spin_unlock_irqrestore(&vga_lock, flags);
> > + if (vgadev)
> > + kfree(vgadev);
>
> kfree(NULL) is fine, no need to check for null pointer.
>
Hehe, yes, but I don't like it :)
Thanks. I spotted a few other issues (I was quite tired yesterday when I
finished this code). I'll do another pass on it today. One thing is: I
don't have x86 hardware, or at least, nothing where I can have 2 VGA
cards in (I may have access to an old laptop). So I'll need help &
testers at one point.
Ben.
-
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/
