Benjamin Herrenschmidt <[EMAIL PROTECTED]> ha scritto:
> Ok, so here is a first, totally untested draft for the kernel side
> of the VGA arbiter.
Hi Ben,
I've a few comments:
> Index: linux-work/drivers/pci/vga.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-work/drivers/pci/vga.c 2005-03-08 18:04:57.000000000 +1100
[...]
> +#ifndef __ARCH_HAS_VGA_DISABLE_RESOURCES
> +static inline void vga_disable_resources(struct pci_dev *pdev,
> + unsigned int rsrc,
> + unsigned int change_bridge)
> +{
> + struct pci_bus *bus;
> + struct pci_dev *bridge;
> + u16 cmd;
> +
> + pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> + if (rsrc & VGA_RSRC_IO)
> + cmd &= ~PCI_COMMAND_IO;
> + if (rsrc & VGA_RSRC_MEM)
> + cmd &= ~PCI_COMMAND_MEMORY;
> + pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +
> + if (!change_bridge)
> + return;
> +
> + 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:
if (cmd & PCI_BRIDGE_CTL_VGA) {
cmd &= ~PCI_BRIDGE_CTL_VGA;
pci_write_config_word(bridge,
PCI_BRIDGE_CONTROL, cmd);
}
> + cmd &= ~PCI_BRIDGE_CTL_VGA;
> + pci_write_config_word(bridge, PCI_BRIDGE_CONTROL,
> cmd);
> + }
> + bus = bus->parent;
> + }
> +}
> +#endif
> +
> +#ifndef __ARCH_HAS_VGA_ENABLE_RESOURCES
> +static inline void vga_enable_resources(struct pci_dev *pdev,
> + unsigned int rsrc)
> +{
> + struct pci_bus *bus;
> + struct pci_dev *bridge;
> + u16 cmd;
> +
> + pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> + if (rsrc & VGA_RSRC_IO)
> + cmd |= PCI_COMMAND_IO;
> + if (rsrc & VGA_RSRC_MEM)
> + cmd |= PCI_COMMAND_MEMORY;
> + pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +
> + 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;
Same here.
> + cmd |= PCI_BRIDGE_CTL_VGA;
> + pci_write_config_word(bridge, PCI_BRIDGE_CONTROL,
> cmd);
> + }
> + bus = bus->parent;
> + }
> +}
> +#endif
> +/*
> + * Currently, we assume that the "initial" setup of the system is
> + * sane, that is we don't come up with conflicting devices, which
> + * would be annoying. We could double check and be better at
> + * deciding who is the default here, but we don't.
> + */
> +void vga_arbiter_add_pci_device(struct pci_dev *pdev)
> +{
> + struct vga_device *vgadev;
> + unsigned long flags;
> + struct pci_bus *bus;
> + struct pci_dev *bridge;
> + u16 cmd;
> +
> + /* 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.
> + memset(vgadev, 0, sizeof(*vgadev));
> +
> + /* Take lock & check for duplicates */
> + spin_lock_irqsave(&vga_lock, flags);
> + if (vgadev_find(pdev) != NULL) {
> + WARN_ON(1);
> + goto fail;
> + }
> + vgadev->pdev = pdev;
> +
> + /* By default, assume we decode everything */
> + vgadev->decodes = VGA_RSRC_IO | VGA_RSRC_MEM;
> +
> + /* Mark that we "own" resources based on our enables, we will
> + * clear that below if the bridge isn't forwarding
> + */
> + pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> + if (cmd & PCI_COMMAND_IO)
> + vgadev->owns |= VGA_RSRC_IO;
> + if (cmd & PCI_COMMAND_MEMORY)
> + vgadev->owns |= VGA_RSRC_MEM;
> +
> + /* Check if VGA cycles can get down to us */
> + bus = pdev->bus;
> + while (bus) {
> + bridge = bus->self;
> + if (bridge) {
> + u16 l;
> + pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &l);
> + if (!(l & PCI_BRIDGE_CTL_VGA)) {
> + vgadev->owns = 0;
> + break;
> + }
> + }
> + bus = bus->parent;
> + }
> +
> + /* Deal with VGA default device. Use first enabled one
> + * by default if arch doesn't have it's own hook
> + */
> +#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
> + if (vga_default == NULL && (vgadev->owns & VGA_RSRC_MEM) &&
> + (vgadev->owns & VGA_RSRC_IO))
> + vga_default = pdev;
> +
> +#endif
> +
> + /* Add to the list */
> + list_add(&vgadev->list, &vga_list);
> + spin_unlock_irqrestore(&vga_lock, flags);
Missing return?
> + 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.
> +}
Luca
--
Home: http://kronoz.cjb.net
"La mia teoria scientifica preferita e` quella secondo la quale gli
anelli di Saturno sarebbero interamente composti dai bagagli andati
persi nei viaggi aerei." -- Mark Russel
-
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/
