Re: [PATCH 1/2] vga: implements VGA arbitration on Linux

[Alan Cox]

> +#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;
> +
> +#ifdef DEBUG
> +	printk(KERN_DEBUG "%s\n", __func__);
> +#endif
> +	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +	if (rsrc & (VGA_RSRC_LEGACY_IO | VGA_RSRC_NORMAL_IO))
> +		cmd |= PCI_COMMAND_IO;
> +	if (rsrc & (VGA_RSRC_LEGACY_MEM | VGA_RSRC_NORMAL_MEM))
> +		cmd |= PCI_COMMAND_MEMORY;
> +	pci_write_config_word(pdev, PCI_COMMAND, cmd);

Locking question - what locks this lot against hotplug also touching
bridge settings ?

> +
> +	if (!(rsrc & VGA_RSRC_LEGACY_MASK))
> +		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)) {
> +				cmd |= PCI_BRIDGE_CTL_VGA;
> +				pci_write_config_word(bridge,
> +						      PCI_BRIDGE_CONTROL,
> +						      cmd);
> +			}
> +		}
> +		bus = bus->parent;
> +	}
> +}
> +#endif


> +	/* The one who calls us should check for this, but lets be sure... */
> +	if (pdev == NULL)
> +		pdev = vga_default_device();

What if the BIOS provided device was hot unplugged ?

> +		conflict = __vga_tryget(vgadev, rsrc);
> +		spin_unlock_irqrestore(&vga_lock, flags);
> +		if (conflict == NULL)
> +			break;
> +
> +
> +		/* We have a conflict, we wait until somebody kicks the
> +		 * work queue. Currently we have one work queue that we

If two drivers own half the resources and both are waiting for the rest
what handles the deadlock

> +		 * kick each time some resources are released, but it would
> +		 * be fairly easy to have a per device one so that we only
> +		 * need to attach to the conflicting device
> +		 */
> +		init_waitqueue_entry(&wait, current);
> +		add_wait_queue(&vga_wait_queue, &wait);
> +		set_current_state(interruptible ?
> +				  TASK_INTERRUPTIBLE :
> +				  TASK_UNINTERRUPTIBLE);
> +		if (signal_pending(current)) {
> +			rc = -EINTR;
> +			break;
> +		}
> +		schedule();
> +		remove_wait_queue(&vga_wait_queue, &wait);
> +		set_current_state(TASK_RUNNING);

Seems a very long winded way to write

	wait_event_interruptible(...)


> +	/* Allocate structure */
> +	vgadev = kmalloc(sizeof(struct vga_device), GFP_KERNEL);
> +	if (vgadev == NULL) {
> +		/* What to do on allocation failure ? For now, let's
> +		 * just do nothing, I'm not sure there is anything saner
> +		 * to be done
> +		 */

If this is an "oh dear" moment then at least printk something

>
> +	/* Set the client' lists of locks */
> +	priv->target = vga_default_device(); /* Maybe this is still null! */

PCI device refcounting ?

--
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/