Messages in this thread | | | Subject | Re: [PATCH] VGA arbitration: draft of kernel side | From | Benjamin Herrenschmidt <> | Date | Wed, 09 Mar 2005 09:46:20 +1100 |
| |
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 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/
| |