Re: [PATCH] VGA arbitration: draft of kernel side

[Kronos <kronos@kronoz.cjb.net>]

Benjamin Herrenschmidt <benh@kernel.crashing.org> 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