lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20090811111457.6411623e@iki.fi>
Date:	Tue, 11 Aug 2009 11:14:57 +0300
From:	Pekka Paalanen <pq@....fi>
To:	Dave Airlie <airlied@...il.com>
Cc:	jbarnes@...tuousgeek.org, Dave Airlie <airlied@...hat.com>,
	linux-kernel@...r.kernel.org, dri-devel@...ts.sf.net
Subject: Re: [PATCH] vga: implements VGA arbitration on Linux

Hi,

some minor comments below.

On Tue, 11 Aug 2009 15:52:06 +1000
Dave Airlie <airlied@...il.com> wrote:

> From: Tiago Vignatti <tiago.vignatti@...ia.com>
> 
> Background:
> Graphic devices are accessed through ranges in I/O or memory space. While most
> modern devices allow relocation of such ranges, some "Legacy" VGA devices
> implemented on PCI will typically have the same "hard-decoded" addresses as
> they did on ISA. For more details see "PCI Bus Binding to IEEE Std 1275-1994
> Standard for Boot (Initialization Configuration) Firmware Revision 2.1"
> Section 7, Legacy Devices.
> 
> The Resource Access Control (RAC) module inside the X server currently does
> the task of arbitration when more than one legacy device co-exists on the same
> machine. But the problem happens when these devices are trying to be accessed
> by different userspace clients (e.g. two server in parallel). Their address
> assignments conflict. Therefore an arbitration scheme _outside_ of the X
> server is needed to control the sharing of these resources. This document
> introduces the operation of the VGA arbiter implemented for Linux kernel.
> 
> Signed-off-by: Tiago Vignatti <tiago.vignatti@...ia.com>
> Signed-off-by: Dave Airlie <airlied@...hat.com>
> ---
>  drivers/gpu/Makefile     |    2 +-
>  drivers/gpu/vga/Kconfig  |   10 +
>  drivers/gpu/vga/Makefile |    1 +
>  drivers/gpu/vga/vgaarb.c | 1206 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.c        |   44 ++
>  drivers/video/Kconfig    |    2 +
>  include/linux/pci.h      |    2 +
>  include/linux/vgaarb.h   |  195 ++++++++
>  8 files changed, 1461 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/gpu/vga/Kconfig
>  create mode 100644 drivers/gpu/vga/Makefile
>  create mode 100644 drivers/gpu/vga/vgaarb.c
>  create mode 100644 include/linux/vgaarb.h
> 
> diff --git a/drivers/gpu/Makefile b/drivers/gpu/Makefile
> index de566cf..30879df 100644
> --- a/drivers/gpu/Makefile
> +++ b/drivers/gpu/Makefile
> @@ -1 +1 @@
> -obj-y			+= drm/
> +obj-y			+= drm/ vga/
> diff --git a/drivers/gpu/vga/Kconfig b/drivers/gpu/vga/Kconfig
> new file mode 100644
> index 0000000..790e675
> --- /dev/null
> +++ b/drivers/gpu/vga/Kconfig
> @@ -0,0 +1,10 @@
> +config VGA_ARB
> +	bool "VGA Arbitration" if EMBEDDED
> +	default y
> +	depends on PCI
> +	help
> +	  Some "legacy" VGA devices implemented on PCI typically have the same
> +	  hard-decoded addresses as they did on ISA. When multiple PCI devices
> +	  are accessed at same time they need some kind of coordination. Please
> +	  see Documentation/vgaarbiter.txt for more details. Select this to
> +	  enable VGA arbiter.

The file Documentation/vgaarbiter.txt does not exist.

> diff --git a/drivers/gpu/vga/Makefile b/drivers/gpu/vga/Makefile
> new file mode 100644
> index 0000000..7cc8c1e
> --- /dev/null
> +++ b/drivers/gpu/vga/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_VGA_ARB)  += vgaarb.o
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> new file mode 100644
> index 0000000..199138f
> --- /dev/null
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -0,0 +1,1206 @@
> +/*
> + * vgaarb.c
> + *
> + * (C) Copyright 2005 Benjamin Herrenschmidt <benh@...nel.crashing.org>
> + * (C) Copyright 2007 Paulo R. Zanoni <przanoni@...il.com>
> + * (C) Copyright 2007, 2009 Tiago Vignatti <vignatti@...edesktop.org>
> + *
> + * Implements the VGA arbitration. For details refer to
> + * Documentation/vgaarbiter.txt

Another reference to vgaarbiter.txt.

> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/pci.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/list.h>
> +#include <linux/sched.h>
> +#include <linux/wait.h>
> +#include <linux/spinlock.h>
> +#include <linux/poll.h>
> +#include <linux/miscdevice.h>
> +
> +#include <linux/uaccess.h>
> +
> +#include <linux/vgaarb.h>
> +
> +static void vga_arbiter_notify_clients(void);
> +/*
> + * We keep a list of all vga devices in the system to speed
> + * up the various operations of the arbiter
> + */
> +struct vga_device {
> +	struct list_head list;
> +	struct pci_dev *pdev;
> +	unsigned int decodes;	/* what does it decodes */
> +	unsigned int owns;	/* what does it owns */
> +	unsigned int locks;	/* what does it locks */
> +	unsigned int io_lock_cnt;	/* legacy IO lock count */
> +	unsigned int mem_lock_cnt;	/* legacy MEM lock count */
> +	unsigned int io_norm_cnt;	/* normal IO count */
> +	unsigned int mem_norm_cnt;	/* normal MEM count */
> +
> +	/* allow IRQ enable/disable hook */
> +	void *cookie;
> +	void (*irq_set_state)(void *cookie, bool enable);
> +	unsigned int (*set_vga_decode)(void *cookie, bool decode);
> +};
> +
> +static LIST_HEAD(vga_list);
> +static int vga_count, vga_decode_count;
> +static bool vga_arbiter_used;
> +static DEFINE_SPINLOCK(vga_lock);
> +static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
> +
> +
> +static const char *vga_iostate_to_str(unsigned int iostate)
> +{
> +	/* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */
> +	iostate &= VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM;
> +	switch (iostate) {
> +	case VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM:
> +		return "io+mem";
> +	case VGA_RSRC_LEGACY_IO:
> +		return "io";
> +	case VGA_RSRC_LEGACY_MEM:
> +		return "mem";
> +	}
> +	return "none";
> +}
> +
> +static int vga_str_to_iostate(char *buf, int str_size, int *io_state)
> +{
> +	/* we could in theory hand out locks on IO and mem
> +	 * separately to userspace but it can cause deadlocks */
> +	if (strncmp(buf, "none", 4) == 0) {
> +		*io_state = VGA_RSRC_NONE;
> +		return 1;
> +	}
> +
> +	/* XXX We're not chekcing the str_size! */
> +	if (strncmp(buf, "io+mem", 6) == 0)
> +		goto both;
> +	else if (strncmp(buf, "io", 2) == 0)
> +		goto both;
> +	else if (strncmp(buf, "mem", 3) == 0)
> +		goto both;
> +	return 0;
> +both:
> +	*io_state = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM;
> +	return 1;
> +}
> +
> +#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
> +/* this is only used a cookie - it should not be dereferenced */
> +static struct pci_dev *vga_default;
> +#endif
> +
> +static void vga_arb_device_card_gone(struct pci_dev *pdev);
> +
> +/* Find somebody in our list */
> +static struct vga_device *vgadev_find(struct pci_dev *pdev)
> +{
> +	struct vga_device *vgadev;
> +
> +	list_for_each_entry(vgadev, &vga_list, list)
> +		if (pdev == vgadev->pdev)
> +			return vgadev;
> +	return NULL;
> +}
> +
> +/* Returns the default VGA device (vgacon's babe) */
> +#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
> +struct pci_dev *vga_default_device(void)
> +{
> +	return vga_default;
> +}
> +#endif
> +
> +static inline void vga_irq_set_state(struct vga_device *vgadev, bool state)
> +{
> +	if (vgadev->irq_set_state)
> +		vgadev->irq_set_state(vgadev->cookie, state);
> +}
> +
> +
> +/* If we don't ever use VGA arb we should avoid
> +   turning off anything anywhere due to old X servers getting
> +   confused about the boot device not being VGA */
> +static void vga_check_first_use(void)
> +{
> +	/* we should inform all GPUs in the system that
> +	 * VGA arb has occured and to try and disable resources
> +	 * if they can */
> +	if (!vga_arbiter_used) {
> +		vga_arbiter_used = true;
> +		vga_arbiter_notify_clients();
> +	}
> +}
> +
> +static struct vga_device *__vga_tryget(struct vga_device *vgadev,
> +				       unsigned int rsrc)
> +{
> +	unsigned int wants, legacy_wants, match;
> +	struct vga_device *conflict;
> +	unsigned int pci_bits;
> +	/* Account for "normal" resources to lock. If we decode the legacy,
> +	 * counterpart, we need to request it as well
> +	 */
> +	if ((rsrc & VGA_RSRC_NORMAL_IO) &&
> +	    (vgadev->decodes & VGA_RSRC_LEGACY_IO))
> +		rsrc |= VGA_RSRC_LEGACY_IO;
> +	if ((rsrc & VGA_RSRC_NORMAL_MEM) &&
> +	    (vgadev->decodes & VGA_RSRC_LEGACY_MEM))
> +		rsrc |= VGA_RSRC_LEGACY_MEM;
> +
> +	pr_devel("%s: %d\n", __func__, rsrc);
> +	pr_devel("%s: owns: %d\n", __func__, vgadev->owns);
> +
> +	/* Check what resources we need to acquire */
> +	wants = rsrc & ~vgadev->owns;
> +
> +	/* We already own everything, just mark locked & bye bye */
> +	if (wants == 0)
> +		goto lock_them;
> +
> +	/* We don't need to request a legacy resource, we just enable
> +	 * appropriate decoding and go
> +	 */

Does this comment mean "We don't need to request a legacy resource,
therefore ..." or "If a legacy resource is not required, ..."?
Well, it gets clear when I read and think about the code, but
the comment got me confused for a moment.

> +	legacy_wants = wants & VGA_RSRC_LEGACY_MASK;
> +	if (legacy_wants == 0)
> +		goto enable_them;
> +
> +	/* Ok, we don't, let's find out how we need to kick off */

We don't what?

> +	list_for_each_entry(conflict, &vga_list, list) {
> +		unsigned int lwants = legacy_wants;
> +		unsigned int change_bridge = 0;
> +
> +		/* Don't conflict with myself */
> +		if (vgadev == conflict)
> +			continue;
> +
> +		/* Check if the architecture allows a conflict between those
> +		 * 2 devices or if they are on separate domains
> +		 */
> +		if (!vga_conflicts(vgadev->pdev, conflict->pdev))
> +			continue;
> +
> +		/* We have a possible conflict. before we go further, we must
> +		 * check if we sit on the same bus as the conflicting device.
> +		 * if we don't, then we must tie both IO and MEM resources
> +		 * together since there is only a single bit controlling
> +		 * VGA forwarding on P2P bridges
> +		 */
> +		if (vgadev->pdev->bus != conflict->pdev->bus) {
> +			change_bridge = 1;
> +			lwants = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM;
> +		}
> +
> +		/* Check if the guy has a lock on the resource. If he does,
> +		 * return the conflicting entry
> +		 */
> +		if (conflict->locks & lwants)
> +			return conflict;
> +
> +		/* Ok, now check if he owns the resource we want. We don't need
> +		 * to check "decodes" since it should be impossible to own
> +		 * own legacy resources you don't decode unless I have a bug
> +		 * in this code...
> +		 */
> +		WARN_ON(conflict->owns & ~conflict->decodes);
> +		match = lwants & conflict->owns;
> +		if (!match)
> +			continue;
> +
> +		/* looks like he doesn't have a lock, we can steal
> +		 * them from him
> +		 */
> +		vga_irq_set_state(conflict, false);
> +
> +		pci_bits = 0;
> +		if (lwants & (VGA_RSRC_LEGACY_MEM|VGA_RSRC_NORMAL_MEM))
> +			pci_bits |= PCI_COMMAND_MEMORY;
> +		if (lwants & (VGA_RSRC_LEGACY_IO|VGA_RSRC_NORMAL_IO))
> +			pci_bits |= PCI_COMMAND_IO;
> +
> +		pci_set_vga_state(conflict->pdev, false, pci_bits,
> +				  change_bridge);
> +		conflict->owns &= ~lwants;
> +		/* If he also owned non-legacy, that is no longer the case */
> +		if (lwants & VGA_RSRC_LEGACY_MEM)
> +			conflict->owns &= ~VGA_RSRC_NORMAL_MEM;
> +		if (lwants & VGA_RSRC_LEGACY_IO)
> +			conflict->owns &= ~VGA_RSRC_NORMAL_IO;
> +	}
> +
> +enable_them:
> +	/* ok dude, we got it, everybody conflicting has been disabled, let's
> +	 * enable us. Make sure we don't mark a bit in "owns" that we don't
> +	 * also have in "decodes". We can lock resources we don't decode but
> +	 * not own them.
> +	 */
> +	pci_bits = 0;
> +	if (wants & (VGA_RSRC_LEGACY_MEM|VGA_RSRC_NORMAL_MEM))
> +		pci_bits |= PCI_COMMAND_MEMORY;
> +	if (wants & (VGA_RSRC_LEGACY_IO|VGA_RSRC_NORMAL_IO))
> +		pci_bits |= PCI_COMMAND_IO;
> +	pci_set_vga_state(vgadev->pdev, true, pci_bits, !!(wants & VGA_RSRC_LEGACY_MASK));
> +
> +	vga_irq_set_state(vgadev, true);
> +	vgadev->owns |= (wants & vgadev->decodes);
> +lock_them:
> +	vgadev->locks |= (rsrc & VGA_RSRC_LEGACY_MASK);
> +	if (rsrc & VGA_RSRC_LEGACY_IO)
> +		vgadev->io_lock_cnt++;
> +	if (rsrc & VGA_RSRC_LEGACY_MEM)
> +		vgadev->mem_lock_cnt++;
> +	if (rsrc & VGA_RSRC_NORMAL_IO)
> +		vgadev->io_norm_cnt++;
> +	if (rsrc & VGA_RSRC_NORMAL_MEM)
> +		vgadev->mem_norm_cnt++;
> +
> +	return NULL;
> +}
> +
> +static void __vga_put(struct vga_device *vgadev, unsigned int rsrc)
> +{
> +	unsigned int old_locks = vgadev->locks;
> +
> +	pr_devel("%s\n", __func__);
> +
> +	/* Update our counters, and account for equivalent legacy resources
> +	 * if we decode them
> +	 */
> +	if ((rsrc & VGA_RSRC_NORMAL_IO) && vgadev->io_norm_cnt > 0) {
> +		vgadev->io_norm_cnt--;
> +		if (vgadev->decodes & VGA_RSRC_LEGACY_IO)
> +			rsrc |= VGA_RSRC_LEGACY_IO;
> +	}
> +	if ((rsrc & VGA_RSRC_NORMAL_MEM) && vgadev->mem_norm_cnt > 0) {
> +		vgadev->mem_norm_cnt--;
> +		if (vgadev->decodes & VGA_RSRC_LEGACY_MEM)
> +			rsrc |= VGA_RSRC_LEGACY_MEM;
> +	}
> +	if ((rsrc & VGA_RSRC_LEGACY_IO) && vgadev->io_lock_cnt > 0)
> +		vgadev->io_lock_cnt--;
> +	if ((rsrc & VGA_RSRC_LEGACY_MEM) && vgadev->mem_lock_cnt > 0)
> +		vgadev->mem_lock_cnt--;
> +
> +	/* Just clear lock bits, we do lazy operations so we don't really
> +	 * have to bother about anything else at this point
> +	 */
> +	if (vgadev->io_lock_cnt == 0)
> +		vgadev->locks &= ~VGA_RSRC_LEGACY_IO;
> +	if (vgadev->mem_lock_cnt == 0)
> +		vgadev->locks &= ~VGA_RSRC_LEGACY_MEM;
> +
> +	/* Kick the wait queue in case somebody was waiting if we actually
> +	 * released something
> +	 */
> +	if (old_locks != vgadev->locks)
> +		wake_up_all(&vga_wait_queue);
> +}
> +
> +int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible)
> +{
> +	struct vga_device *vgadev, *conflict;
> +	unsigned long flags;
> +	wait_queue_t wait;
> +	int rc = 0;
> +
> +	vga_check_first_use();
> +	/* The one who calls us should check for this, but lets be sure... */
> +	if (pdev == NULL)
> +		pdev = vga_default_device();
> +	if (pdev == NULL)
> +		return 0;
> +
> +	for (;;) {
> +		spin_lock_irqsave(&vga_lock, flags);
> +		vgadev = vgadev_find(pdev);
> +		if (vgadev == NULL) {
> +			spin_unlock_irqrestore(&vga_lock, flags);
> +			rc = -ENODEV;
> +			break;
> +		}
> +		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
> +		 * 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);
> +	}
> +	return rc;
> +}
> +EXPORT_SYMBOL(vga_get);
> +
> +int vga_tryget(struct pci_dev *pdev, unsigned int rsrc)
> +{
> +	struct vga_device *vgadev;
> +	unsigned long flags;
> +	int rc = 0;
> +
> +	vga_check_first_use();
> +
> +	/* The one who calls us should check for this, but lets be sure... */
> +	if (pdev == NULL)
> +		pdev = vga_default_device();
> +	if (pdev == NULL)
> +		return 0;
> +	spin_lock_irqsave(&vga_lock, flags);
> +	vgadev = vgadev_find(pdev);
> +	if (vgadev == NULL) {
> +		rc = -ENODEV;
> +		goto bail;
> +	}
> +	if (__vga_tryget(vgadev, rsrc))
> +		rc = -EBUSY;
> +bail:
> +	spin_unlock_irqrestore(&vga_lock, flags);
> +	return rc;
> +}
> +EXPORT_SYMBOL(vga_tryget);
> +
> +void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> +{
> +	struct vga_device *vgadev;
> +	unsigned long flags;
> +
> +	/* The one who calls us should check for this, but lets be sure... */
> +	if (pdev == NULL)
> +		pdev = vga_default_device();
> +	if (pdev == NULL)
> +		return;
> +	spin_lock_irqsave(&vga_lock, flags);
> +	vgadev = vgadev_find(pdev);
> +	if (vgadev == NULL)
> +		goto bail;
> +	__vga_put(vgadev, rsrc);
> +bail:
> +	spin_unlock_irqrestore(&vga_lock, flags);
> +}
> +EXPORT_SYMBOL(vga_put);
> +
> +/*
> + * Currently, we assume that the "initial" setup of the system is
> + * not sane, that is we come up with conflicting devices and let
> + * the arbiter's client decides if devices decodes or not legacy
> + * things.
> + */
> +static bool 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 false;
> +
> +	/* Allocate structure */
> +	vgadev = kmalloc(sizeof(struct vga_device), GFP_KERNEL);
> +	if (vgadev == NULL) {
> +		pr_err("vgaarb: failed to allocate pci device\n");
> +		/* What to do on allocation failure ? For now, let's
> +		 * just do nothing, I'm not sure there is anything saner
> +		 * to be done
> +		 */
> +		return false;
> +	}
> +
> +	memset(vgadev, 0, sizeof(*vgadev));
> +
> +	/* Take lock & check for duplicates */
> +	spin_lock_irqsave(&vga_lock, flags);
> +	if (vgadev_find(pdev) != NULL) {
> +		BUG_ON(1);
> +		goto fail;
> +	}
> +	vgadev->pdev = pdev;
> +
> +	/* By default, assume we decode everything */
> +	vgadev->decodes = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM |
> +			  VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM;
> +
> +	/* by default mark it as decoding */
> +	vga_decode_count++;
> +	/* 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_LEGACY_IO;
> +	if (cmd & PCI_COMMAND_MEMORY)
> +		vgadev->owns |= VGA_RSRC_LEGACY_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_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK))
> +		vga_default = pci_dev_get(pdev);
> +#endif
> +
> +	/* Add to the list */
> +	list_add(&vgadev->list, &vga_list);
> +	vga_count++;
> +	pr_info("vgaarb: device added: PCI:%s,decodes=%s,owns=%s,locks=%s\n",
> +		pci_name(pdev),
> +		vga_iostate_to_str(vgadev->decodes),
> +		vga_iostate_to_str(vgadev->owns),
> +		vga_iostate_to_str(vgadev->locks));
> +
> +	spin_unlock_irqrestore(&vga_lock, flags);
> +	return true;
> +fail:
> +	spin_unlock_irqrestore(&vga_lock, flags);
> +	kfree(vgadev);
> +	return false;
> +}
> +
> +static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
> +{
> +	struct vga_device *vgadev;
> +	unsigned long flags;
> +	bool ret = true;
> +
> +	spin_lock_irqsave(&vga_lock, flags);
> +	vgadev = vgadev_find(pdev);
> +	if (vgadev == NULL) {
> +		ret = false;
> +		goto bail;
> +	}
> +
> +	if (vga_default == pdev) {
> +		pci_dev_put(vga_default);
> +		vga_default = NULL;
> +	}
> +
> +	if (vgadev->decodes & (VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM))
> +		vga_decode_count--;
> +
> +	/* Remove entry from list */
> +	list_del(&vgadev->list);
> +	vga_count--;
> +	/* Notify userland driver that the device is gone so it discards
> +	 * it's copies of the pci_dev pointer
> +	 */
> +	vga_arb_device_card_gone(pdev);
> +
> +	/* Wake up all possible waiters */
> +	wake_up_all(&vga_wait_queue);
> +bail:
> +	spin_unlock_irqrestore(&vga_lock, flags);
> +	kfree(vgadev);
> +	return ret;
> +}
> +
> +/* this is called with the lock */
> +static inline void vga_update_device_decodes(struct vga_device *vgadev,
> +					     int new_decodes)
> +{
> +	int old_decodes;
> +	struct vga_device *new_vgadev, *conflict;
> +
> +	old_decodes = vgadev->decodes;
> +	vgadev->decodes = new_decodes;
> +
> +	pr_info("vgaarb: device changed decodes: PCI:%s,olddecodes=%s,decodes=%s:owns=%s\n",
> +		pci_name(vgadev->pdev),
> +		vga_iostate_to_str(old_decodes),
> +		vga_iostate_to_str(vgadev->decodes),
> +		vga_iostate_to_str(vgadev->owns));
> +
> +
> +	/* if we own the decodes we should move them along to
> +	   another card */
> +	if ((vgadev->owns & old_decodes) && (vga_count > 1)) {
> +		/* set us to own nothing */
> +		vgadev->owns &= ~old_decodes;
> +		list_for_each_entry(new_vgadev, &vga_list, list) {
> +			if ((new_vgadev != vgadev) &&
> +			    (new_vgadev->decodes & VGA_RSRC_LEGACY_MASK)) {
> +				pr_info("vgaarb: transferring owner from PCI:%s to PCI:%s\n", pci_name(vgadev->pdev), pci_name(new_vgadev->pdev));
> +				conflict = __vga_tryget(new_vgadev, VGA_RSRC_LEGACY_MASK);
> +				if (!conflict)
> +					__vga_put(new_vgadev, VGA_RSRC_LEGACY_MASK);
> +				break;
> +			}
> +		}
> +	}
> +
> +	/* change decodes counter */
> +	if (old_decodes != new_decodes) {
> +		if (new_decodes & (VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM))
> +			vga_decode_count++;
> +		else
> +			vga_decode_count--;
> +	}
> +}
> +
> +void __vga_set_legacy_decoding(struct pci_dev *pdev, unsigned int decodes, bool userspace)
> +{
> +	struct vga_device *vgadev;
> +	unsigned long flags;
> +
> +	decodes &= VGA_RSRC_LEGACY_MASK;
> +
> +	spin_lock_irqsave(&vga_lock, flags);
> +	vgadev = vgadev_find(pdev);
> +	if (vgadev == NULL)
> +		goto bail;
> +
> +	/* don't let userspace futz with kernel driver decodes */
> +	if (userspace && vgadev->set_vga_decode)
> +		goto bail;
> +
> +	/* update the device decodes + counter */
> +	vga_update_device_decodes(vgadev, decodes);
> +
> +	/* XXX if somebody is going from "doesn't decode" to "decodes" state
> +	 * here, additional care must be taken as we may have pending owner
> +	 * ship of non-legacy region ...
> +	 */
> +bail:
> +	spin_unlock_irqrestore(&vga_lock, flags);
> +}
> +
> +void vga_set_legacy_decoding(struct pci_dev *pdev, unsigned int decodes)
> +{
> +	__vga_set_legacy_decoding(pdev, decodes, false);
> +}
> +EXPORT_SYMBOL(vga_set_legacy_decoding);
> +
> +/* return number of active VGA devices */

That comment is incorrect.

> +/* call with NULL to unregister */
> +int vga_client_register(struct pci_dev *pdev, void *cookie,
> +			void (*irq_set_state)(void *cookie, bool state),
> +			unsigned int (*set_vga_decode)(void *cookie, bool decode))
> +{
> +	int ret = -1;
> +	struct vga_device *vgadev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vga_lock, flags);
> +	vgadev = vgadev_find(pdev);
> +	if (!vgadev)
> +		goto bail;
> +
> +	vgadev->irq_set_state = irq_set_state;
> +	vgadev->set_vga_decode = set_vga_decode;
> +	vgadev->cookie = cookie;
> +	ret = 0;
> +
> +bail:
> +	spin_unlock_irqrestore(&vga_lock, flags);
> +	return ret;
> +
> +}
> +EXPORT_SYMBOL(vga_client_register);
> +
> +/*
> + * Char driver implementation
> + *
> + * Semantics is:
> + *
> + *  open       : open user instance of the arbitrer. by default, it's
> + *                attached to the default VGA device of the system.
> + *
> + *  close      : close user instance, release locks
> + *
> + *  read       : return a string indicating the status of the target.
> + *                an IO state string is of the form {io,mem,io+mem,none},
> + *                mc and ic are respectively mem and io lock counts (for
> + *                debugging/diagnostic only). "decodes" indicate what the
> + *                card currently decodes, "owns" indicates what is currently
> + *                enabled on it, and "locks" indicates what is locked by this
> + *                card. If the card is unplugged, we get "invalid" then for
> + *                card_ID and an -ENODEV error is returned for any command
> + *                until a new card is targeted
> + *
> + *   "<card_ID>,decodes=<io_state>,owns=<io_state>,locks=<io_state> (ic,mc)"
> + *
> + * write       : write a command to the arbiter. List of commands is:
> + *
> + *   target <card_ID>   : switch target to card <card_ID> (see below)
> + *   lock <io_state>    : acquires locks on target ("none" is invalid io_state)
> + *   trylock <io_state> : non-blocking acquire locks on target
> + *   unlock <io_state>  : release locks on target
> + *   unlock all         : release all locks on target held by this user
> + *   decodes <io_state> : set the legacy decoding attributes for the card
> + *
> + * poll         : event if something change on any card (not just the target)
> + *
> + * card_ID is of the form "PCI:domain:bus:dev.fn". It can be set to "default"
> + * to go back to the system default card (TODO: not implemented yet).
> + * Currently, only PCI is supported as a prefix, but the userland API may
> + * support other bus types in the future, even if the current kernel
> + * implementation doesn't.
> + *
> + * Note about locks:
> + *
> + * The driver keeps track of which user has what locks on which card. It
> + * supports stacking, like the kernel one. This complexifies the implementation
> + * a bit, but makes the arbiter more tolerant to userspace problems and able
> + * to properly cleanup in all cases when a process dies.
> + * Currently, a max of 16 cards simultaneously can have locks issued from
> + * userspace for a given user (file descriptor instance) of the arbiter.
> + *
> + * If the device is hot-unplugged, there is a hook inside the module to notify
> + * they being added/removed in the system and automatically added/removed in
> + * the arbiter.
> + */
> +
> +#define MAX_USER_CARDS         16
> +#define PCI_INVALID_CARD       ((struct pci_dev *)-1UL)
> +
> +/*
> + * Each user has an array of these, tracking which cards have locks
> + */
> +struct vga_arb_user_card {
> +	struct pci_dev *pdev;
> +	unsigned int mem_cnt;
> +	unsigned int io_cnt;
> +};
> +
> +struct vga_arb_private {
> +	struct list_head list;
> +	struct pci_dev *target;
> +	struct vga_arb_user_card cards[MAX_USER_CARDS];
> +	spinlock_t lock;
> +};
> +
> +static LIST_HEAD(vga_user_list);
> +static DEFINE_SPINLOCK(vga_user_lock);
> +
> +
> +/*
> + * This function gets a string in the format: "PCI:domain:bus:dev.fn" and
> + * returns the respective values. If the string is not in this format,
> + * it returns 0.
> + */
> +static int vga_pci_str_to_vars(char *buf, int count, unsigned int *domain,
> +			       unsigned int *bus, unsigned int *devfn)
> +{
> +	int n;
> +	unsigned int slot, func;
> +
> +
> +	n = sscanf(buf, "PCI:%x:%x:%x.%x", domain, bus, &slot, &func);
> +	if (n != 4)
> +		return 0;
> +
> +	*devfn = PCI_DEVFN(slot, func);
> +
> +	return 1;
> +}
> +
> +static ssize_t vga_arb_read(struct file *file, char __user * buf,
> +			    size_t count, loff_t *ppos)
> +{
> +	struct vga_arb_private *priv = file->private_data;
> +	struct vga_device *vgadev;
> +	struct pci_dev *pdev;
> +	unsigned long flags;
> +	size_t len;
> +	int rc;
> +	char *lbuf;
> +
> +	lbuf = kmalloc(1024, GFP_KERNEL);
> +	if (lbuf == NULL)
> +		return -ENOMEM;
> +
> +	/* Shields against vga_arb_device_card_gone (pci_dev going
> +	 * away), and allows access to vga list
> +	 */
> +	spin_lock_irqsave(&vga_lock, flags);
> +
> +	/* If we are targetting the default, use it */
> +	pdev = priv->target;
> +	if (pdev == NULL || pdev == PCI_INVALID_CARD) {
> +		spin_unlock_irqrestore(&vga_lock, flags);
> +		len = sprintf(lbuf, "invalid");
> +		goto done;
> +	}
> +
> +	/* Find card vgadev structure */
> +	vgadev = vgadev_find(pdev);
> +	if (vgadev == NULL) {
> +		/* Wow, it's not in the list, that shouldn't happen,
> +		 * let's fix us up and return invalid card
> +		 */
> +		if (pdev == priv->target)
> +			vga_arb_device_card_gone(pdev);
> +		spin_unlock_irqrestore(&vga_lock, flags);
> +		len = sprintf(lbuf, "invalid");
> +		goto done;
> +	}
> +
> +	/* Fill the buffer with infos */
> +	len = snprintf(lbuf, 1024,
> +		       "count:%d,PCI:%s,decodes=%s,owns=%s,locks=%s(%d:%d)\n",
> +		       vga_decode_count, pci_name(pdev),
> +		       vga_iostate_to_str(vgadev->decodes),
> +		       vga_iostate_to_str(vgadev->owns),
> +		       vga_iostate_to_str(vgadev->locks),
> +		       vgadev->io_lock_cnt, vgadev->mem_lock_cnt);
> +
> +	spin_unlock_irqrestore(&vga_lock, flags);
> +done:
> +
> +	/* Copy that to user */
> +	if (len > count)
> +		len = count;
> +	rc = copy_to_user(buf, lbuf, len);
> +	kfree(lbuf);
> +	if (rc)
> +		return -EFAULT;
> +	return len;
> +}
> +
> +/*
> + * TODO: To avoid parsing inside kernel and to improve the speed we may
> + * consider use ioctl here
> + */
> +static ssize_t vga_arb_write(struct file *file, const char __user * buf,
> +			     size_t count, loff_t *ppos)
> +{
> +	struct vga_arb_private *priv = file->private_data;
> +	struct vga_arb_user_card *uc = NULL;
> +	struct pci_dev *pdev;
> +
> +	unsigned int io_state;
> +
> +	char *kbuf, *curr_pos;
> +	size_t remaining = count;
> +
> +	int ret_val;
> +	int i;
> +
> +
> +	kbuf = kmalloc(count + 1, GFP_KERNEL);
> +	if (!kbuf)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(kbuf, buf, count)) {
> +		kfree(kbuf);
> +		return -EFAULT;
> +	}
> +	curr_pos = kbuf;
> +	kbuf[count] = '\0';	/* Just to make sure... */
> +
> +	if (strncmp(curr_pos, "lock ", 5) == 0) {
> +		curr_pos += 5;
> +		remaining -= 5;
> +
> +		pr_devel("client 0x%X called 'lock'\n", (int)priv);
> +
> +		if (!vga_str_to_iostate(curr_pos, remaining, &io_state)) {
> +			ret_val = -EPROTO;
> +			goto done;
> +		}
> +		if (io_state == VGA_RSRC_NONE) {
> +			ret_val = -EPROTO;
> +			goto done;
> +		}
> +
> +		pdev = priv->target;
> +		if (priv->target == NULL) {
> +			ret_val = -ENODEV;
> +			goto done;
> +		}
> +
> +		vga_get_uninterruptible(pdev, io_state);
> +
> +		/* Update the client's locks lists... */
> +		for (i = 0; i < MAX_USER_CARDS; i++) {
> +			if (priv->cards[i].pdev == pdev) {
> +				if (io_state & VGA_RSRC_LEGACY_IO)
> +					priv->cards[i].io_cnt++;
> +				if (io_state & VGA_RSRC_LEGACY_MEM)
> +					priv->cards[i].mem_cnt++;
> +				break;
> +			}
> +		}
> +
> +		ret_val = count;
> +		goto done;
> +	} else if (strncmp(curr_pos, "unlock ", 7) == 0) {
> +		curr_pos += 7;
> +		remaining -= 7;
> +
> +		pr_devel("client 0x%X called 'unlock'\n", (int)priv);
> +
> +		if (strncmp(curr_pos, "all", 3) == 0)
> +			io_state = VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM;
> +		else {
> +			if (!vga_str_to_iostate
> +			    (curr_pos, remaining, &io_state)) {
> +				ret_val = -EPROTO;
> +				goto done;
> +			}
> +			/* TODO: Add this?
> +			   if (io_state == VGA_RSRC_NONE) {
> +			   ret_val = -EPROTO;
> +			   goto done;
> +			   }
> +			  */
> +		}
> +
> +		pdev = priv->target;
> +		if (priv->target == NULL) {
> +			ret_val = -ENODEV;
> +			goto done;
> +		}
> +		for (i = 0; i < MAX_USER_CARDS; i++) {
> +			if (priv->cards[i].pdev == pdev)
> +				uc = &priv->cards[i];
> +		}
> +
> +		if (!uc)
> +			return -EINVAL;
> +
> +		if (io_state & VGA_RSRC_LEGACY_IO && uc->io_cnt == 0)
> +			return -EINVAL;
> +
> +		if (io_state & VGA_RSRC_LEGACY_MEM && uc->mem_cnt == 0)
> +			return -EINVAL;
> +
> +		vga_put(pdev, io_state);
> +
> +		if (io_state & VGA_RSRC_LEGACY_IO)
> +			uc->io_cnt--;
> +		if (io_state & VGA_RSRC_LEGACY_MEM)
> +			uc->mem_cnt--;
> +
> +		ret_val = count;
> +		goto done;
> +	} else if (strncmp(curr_pos, "trylock ", 8) == 0) {
> +		curr_pos += 8;
> +		remaining -= 8;
> +
> +		pr_devel("client 0x%X called 'trylock'\n", (int)priv);
> +
> +		if (!vga_str_to_iostate(curr_pos, remaining, &io_state)) {
> +			ret_val = -EPROTO;
> +			goto done;
> +		}
> +		/* TODO: Add this?
> +		   if (io_state == VGA_RSRC_NONE) {
> +		   ret_val = -EPROTO;
> +		   goto done;
> +		   }
> +		 */
> +
> +		pdev = priv->target;
> +		if (priv->target == NULL) {
> +			ret_val = -ENODEV;
> +			goto done;
> +		}
> +
> +		if (vga_tryget(pdev, io_state)) {
> +			/* Update the client's locks lists... */
> +			for (i = 0; i < MAX_USER_CARDS; i++) {
> +				if (priv->cards[i].pdev == pdev) {
> +					if (io_state & VGA_RSRC_LEGACY_IO)
> +						priv->cards[i].io_cnt++;
> +					if (io_state & VGA_RSRC_LEGACY_MEM)
> +						priv->cards[i].mem_cnt++;
> +					break;
> +				}
> +			}
> +			ret_val = count;
> +			goto done;
> +		} else {
> +			ret_val = -EBUSY;
> +			goto done;
> +		}
> +
> +	} else if (strncmp(curr_pos, "target ", 7) == 0) {
> +		unsigned int domain, bus, devfn;
> +		struct vga_device *vgadev;
> +
> +		curr_pos += 7;
> +		remaining -= 7;
> +		pr_devel("client 0x%X called 'target'\n", (int)priv);
> +		/* if target is default */
> +		if (!strncmp(buf, "default", 7))
> +			pdev = pci_dev_get(vga_default_device());
> +		else {
> +			if (!vga_pci_str_to_vars(curr_pos, remaining,
> +						 &domain, &bus, &devfn)) {
> +				ret_val = -EPROTO;
> +				goto done;
> +			}
> +
> +			pdev = pci_get_bus_and_slot(bus, devfn);
> +			if (!pdev) {
> +				pr_info("vgaarb: invalid PCI address!\n");
> +				ret_val = -ENODEV;
> +				goto done;
> +			}
> +		}
> +
> +		vgadev = vgadev_find(pdev);
> +		if (vgadev == NULL) {
> +			pr_info("vgaarb: this pci device is not a vga device\n");
> +			pci_dev_put(pdev);
> +			ret_val = -ENODEV;
> +			goto done;
> +		}
> +
> +		priv->target = pdev;
> +		for (i = 0; i < MAX_USER_CARDS; i++) {
> +			if (priv->cards[i].pdev == pdev)
> +				break;
> +			if (priv->cards[i].pdev == NULL) {
> +				priv->cards[i].pdev = pdev;
> +				priv->cards[i].io_cnt = 0;
> +				priv->cards[i].mem_cnt = 0;
> +				break;
> +			}
> +		}
> +		if (i == MAX_USER_CARDS) {
> +			pr_err("vgaarb: maximum user cards number reached!\n");
> +			pci_dev_put(pdev);
> +			/* XXX: which value to return? */
> +			ret_val =  -ENOMEM;
> +			goto done;
> +		}
> +
> +		ret_val = count;
> +		pci_dev_put(pdev);
> +		goto done;
> +
> +
> +	} else if (strncmp(curr_pos, "decodes ", 8) == 0) {
> +		curr_pos += 8;
> +		remaining -= 8;
> +		pr_devel("vgaarb: client 0x%X called 'decodes'\n", (int)priv);
> +
> +		if (!vga_str_to_iostate(curr_pos, remaining, &io_state)) {
> +			ret_val = -EPROTO;
> +			goto done;
> +		}
> +		pdev = priv->target;
> +		if (priv->target == NULL) {
> +			ret_val = -ENODEV;
> +			goto done;
> +		}
> +
> +		__vga_set_legacy_decoding(pdev, io_state, true);
> +		ret_val = count;
> +		goto done;
> +	}
> +	/* If we got here, the message written is not part of the protocol! */
> +	kfree(kbuf);
> +	return -EPROTO;
> +
> +done:
> +	kfree(kbuf);
> +	return ret_val;
> +}

How about splitting those else-if case implementations into separate
functions?

> +
> +static unsigned int vga_arb_fpoll(struct file *file, poll_table * wait)
> +{
> +	struct vga_arb_private *priv = file->private_data;
> +
> +	pr_devel("%s\n", __func__);
> +
> +	if (priv == NULL)
> +		return -ENODEV;
> +	poll_wait(file, &vga_wait_queue, wait);
> +	return POLLIN;
> +}
> +
> +static int vga_arb_open(struct inode *inode, struct file *file)
> +{
> +	struct vga_arb_private *priv;
> +	unsigned long flags;
> +
> +	pr_devel("%s\n", __func__);
> +
> +	priv = kmalloc(sizeof(struct vga_arb_private), GFP_KERNEL);
> +	if (priv == NULL)
> +		return -ENOMEM;
> +	memset(priv, 0, sizeof(*priv));
> +	spin_lock_init(&priv->lock);
> +	file->private_data = priv;
> +
> +	spin_lock_irqsave(&vga_user_lock, flags);
> +	list_add(&priv->list, &vga_user_list);
> +	spin_unlock_irqrestore(&vga_user_lock, flags);
> +
> +	/* Set the client' lists of locks */
> +	priv->target = vga_default_device(); /* Maybe this is still null! */
> +	priv->cards[0].pdev = priv->target;
> +	priv->cards[0].io_cnt = 0;
> +	priv->cards[0].mem_cnt = 0;
> +
> +
> +	return 0;
> +}
> +
> +static int vga_arb_release(struct inode *inode, struct file *file)
> +{
> +	struct vga_arb_private *priv = file->private_data;
> +	struct vga_arb_user_card *uc;
> +	unsigned long flags;
> +	int i;
> +
> +	pr_devel("%s\n", __func__);
> +
> +	if (priv == NULL)
> +		return -ENODEV;
> +
> +	spin_lock_irqsave(&vga_user_lock, flags);
> +	list_del(&priv->list);
> +	for (i = 0; i < MAX_USER_CARDS; i++) {
> +		uc = &priv->cards[i];
> +		if (uc->pdev == NULL)
> +			continue;
> +		pr_devel("uc->io_cnt == %d, uc->mem_cnt == %d\n",
> +			 uc->io_cnt, uc->mem_cnt);
> +		while (uc->io_cnt--)
> +			vga_put(uc->pdev, VGA_RSRC_LEGACY_IO);
> +		while (uc->mem_cnt--)
> +			vga_put(uc->pdev, VGA_RSRC_LEGACY_MEM);
> +	}
> +	spin_unlock_irqrestore(&vga_user_lock, flags);
> +
> +	kfree(priv);
> +
> +	return 0;
> +}
> +
> +static void vga_arb_device_card_gone(struct pci_dev *pdev)
> +{
> +}
> +
> +/*
> + * callback any registered clients to let them know we have a
> + * change in VGA cards
> + */
> +static void vga_arbiter_notify_clients(void)
> +{
> +	struct vga_device *vgadev;
> +	unsigned long flags;
> +	uint32_t new_decodes;
> +	bool new_state;
> +
> +	if (!vga_arbiter_used)
> +		return;
> +
> +	spin_lock_irqsave(&vga_lock, flags);
> +	list_for_each_entry(vgadev, &vga_list, list) {
> +		if (vga_count > 1)
> +			new_state = false;
> +		else
> +			new_state = true;
> +		if (vgadev->set_vga_decode) {
> +			new_decodes = vgadev->set_vga_decode(vgadev->cookie, new_state);
> +			vga_update_device_decodes(vgadev, new_decodes);
> +		}
> +	}
> +	spin_unlock_irqrestore(&vga_lock, flags);
> +}
> +
> +static int pci_notify(struct notifier_block *nb, unsigned long action,
> +		      void *data)
> +{
> +	struct device *dev = data;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	bool notify = false;
> +
> +	pr_devel("%s\n", __func__);
> +
> +	/* For now we're only intereted in devices added and removed. I didn't
> +	 * test this thing here, so someone needs to double check for the
> +	 * cases of hotplugable vga cards. */
> +	if (action == BUS_NOTIFY_ADD_DEVICE)
> +		notify = vga_arbiter_add_pci_device(pdev);
> +	else if (action == BUS_NOTIFY_DEL_DEVICE)
> +		notify = vga_arbiter_del_pci_device(pdev);
> +
> +	if (notify)
> +		vga_arbiter_notify_clients();
> +	return 0;
> +}
> +
> +static struct notifier_block pci_notifier = {
> +	.notifier_call = pci_notify,
> +};
> +
> +static const struct file_operations vga_arb_device_fops = {
> +	.read = vga_arb_read,
> +	.write = vga_arb_write,
> +	.poll = vga_arb_fpoll,
> +	.open = vga_arb_open,
> +	.release = vga_arb_release,
> +};
> +
> +static struct miscdevice vga_arb_device = {
> +	MISC_DYNAMIC_MINOR, "vga_arbiter", &vga_arb_device_fops
> +};
> +
> +static int __init vga_arb_device_init(void)
> +{
> +	int rc;
> +	struct pci_dev *pdev;
> +
> +	rc = misc_register(&vga_arb_device);
> +	if (rc < 0)
> +		pr_err("vgaarb: error %d registering device\n", rc);
> +
> +	bus_register_notifier(&pci_bus_type, &pci_notifier);
> +
> +	/* We add all pci devices satisfying vga class in the arbiter by
> +	 * default */
> +	pdev = NULL;
> +	while ((pdev =
> +		pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> +			       PCI_ANY_ID, pdev)) != NULL)
> +		vga_arbiter_add_pci_device(pdev);
> +
> +	pr_info("vgaarb: loaded\n");
> +	return rc;
> +}
> +subsys_initcall(vga_arb_device_init);
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index dbd0f94..d837606 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2502,6 +2502,50 @@ int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type *type)
>  	return 0;
>  }
>  
> +/**
> + * pci_set_vga_state - set VGA decode state on device and parents if requested
> + * @dev the PCI device
> + * @decode - true = enable decoding, false = disable decoding
> + * @command_bits PCI_COMMAND_IO and/or PCI_COMMAND_MEMORY
> + * @change_bridge - traverse ancestors and change bridges
> + */
> +int pci_set_vga_state(struct pci_dev *dev, bool decode,
> +		      unsigned int command_bits, bool change_bridge)
> +{
> +	struct pci_bus *bus;
> +	struct pci_dev *bridge;
> +	u16 cmd;
> +
> +	WARN_ON(command_bits & ~(PCI_COMMAND_IO|PCI_COMMAND_MEMORY));
> +
> +	pci_read_config_word(dev, PCI_COMMAND, &cmd);
> +	if (decode == true)
> +		cmd |= command_bits;
> +	else
> +		cmd &= ~command_bits;
> +	pci_write_config_word(dev, PCI_COMMAND, cmd);
> +
> +	if (change_bridge == false)
> +		return 0;
> +
> +	bus = dev->bus;
> +	while (bus) {
> +		bridge = bus->self;
> +		if (bridge) {
> +			pci_read_config_word(bridge, PCI_BRIDGE_CONTROL,
> +					     &cmd);
> +			if (decode == true)
> +				cmd |= PCI_BRIDGE_CTL_VGA;
> +			else
> +				cmd &= ~PCI_BRIDGE_CTL_VGA;
> +			pci_write_config_word(bridge, PCI_BRIDGE_CONTROL,
> +					      cmd);
> +		}
> +		bus = bus->parent;
> +	}
> +	return 0;
> +}
> +
>  #define RESOURCE_ALIGNMENT_PARAM_SIZE COMMAND_LINE_SIZE
>  static char resource_alignment_param[RESOURCE_ALIGNMENT_PARAM_SIZE] = {0};
>  spinlock_t resource_alignment_lock = SPIN_LOCK_UNLOCKED;
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 3b54b39..a0d9ee1 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -7,6 +7,8 @@ menu "Graphics support"
>  
>  source "drivers/char/agp/Kconfig"
>  
> +source "drivers/gpu/vga/Kconfig"
> +
>  source "drivers/gpu/drm/Kconfig"
>  
>  config VGASTATE
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 115fb7b..7ba6eba 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -805,6 +805,8 @@ int pci_cfg_space_size_ext(struct pci_dev *dev);
>  int pci_cfg_space_size(struct pci_dev *dev);
>  unsigned char pci_bus_max_busnr(struct pci_bus *bus);
>  
> +int pci_set_vga_state(struct pci_dev *pdev, bool decode,
> +		      unsigned int command_bits, bool change_bridge);
>  /* kmem_cache style wrapper around pci_alloc_consistent() */
>  
>  #include <linux/dmapool.h>
> diff --git a/include/linux/vgaarb.h b/include/linux/vgaarb.h
> new file mode 100644
> index 0000000..68229ce
> --- /dev/null
> +++ b/include/linux/vgaarb.h
> @@ -0,0 +1,195 @@
> +/*
> + * vgaarb.c
> + *
> + * (C) Copyright 2005 Benjamin Herrenschmidt <benh@...nel.crashing.org>
> + * (C) Copyright 2007 Paulo R. Zanoni <przanoni@...il.com>
> + * (C) Copyright 2007, 2009 Tiago Vignatti <vignatti@...edesktop.org>
> + */
> +
> +#ifndef LINUX_VGA_H
> +
> +#include <asm/vga.h>
> +
> +/* Legacy VGA regions */
> +#define VGA_RSRC_NONE	       0x00
> +#define VGA_RSRC_LEGACY_IO     0x01
> +#define VGA_RSRC_LEGACY_MEM    0x02
> +#define VGA_RSRC_LEGACY_MASK   (VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM)
> +/* Non-legacy access */
> +#define VGA_RSRC_NORMAL_IO     0x04
> +#define VGA_RSRC_NORMAL_MEM    0x08
> +
> +/* Passing that instead of a pci_dev to use the system "default"
> + * device, that is the one used by vgacon. Archs will probably
> + * have to provide their own vga_default_device();
> + */
> +#define VGA_DEFAULT_DEVICE     (NULL)
> +
> +/* For use by clients */
> +
> +/**
> + *     vga_set_legacy_decoding
> + *
> + *     @pdev: pci device of the VGA card
> + *     @decodes: bit mask of what legacy regions the card decodes
> + *
> + *     Indicates to the arbiter if the card decodes legacy VGA IOs,
> + *     legacy VGA Memory, both, or none. All cards default to both,
> + *     the card driver (fbdev for example) should tell the arbiter
> + *     if it has disabled legacy decoding, so the card can be left
> + *     out of the arbitration process (and can be safe to take
> + *     interrupts at any time.
> + */
> +extern void vga_set_legacy_decoding(struct pci_dev *pdev,
> +									unsigned int decodes);
> +
> +/**
> + *     vga_get         - acquire & locks VGA resources
> + *
> + *     pdev: pci device of the VGA card or NULL for the system default
> + *     rsrc: bit mask of resources to acquire and lock
> + *     interruptible: blocking should be interruptible by signals ?

I don't know the kerneldoc format, but looks like argument names
should be prefixed with @.

> + *
> + *     This function acquires VGA resources for the given
> + *     card and mark those resources locked. If the resource requested
> + *     are "normal" (and not legacy) resources, the arbiter will first check
> + *     wether the card is doing legacy decoding for that type of resource. If
> + *     yes, the lock is "converted" into a legacy resource lock.
> + *     The arbiter will first look for all VGA cards that might conflict
> + *     and disable their IOs and/or Memory access, inlcuding VGA forwarding
> + *     on P2P bridges if necessary, so that the requested resources can
> + *     be used. Then, the card is marked as locking these resources and
> + *     the IO and/or Memory accesse are enabled on the card (including
> + *     VGA forwarding on parent P2P bridges if any).
> + *     This function will block if some conflicting card is already locking
> + *     one of the required resources (or any resource on a different bus
> + *     segment, since P2P bridges don't differenciate VGA memory and IO
> + *     afaik). You can indicate wether this blocking should be interruptible
> + *     by a signal (for userland interface) or not.
> + *     Must not be called at interrupt time or in atomic context.
> + *     If the card already owns the resources, the function succeeds.
> + *     Nested calls are supported (a per-resource counter is maintained)
> + */
> +
> +extern int vga_get(struct pci_dev *pdev, unsigned int rsrc,
> +											int interruptible);
> +
> +/**
> + *     vga_get_interruptible
> + *
> + *     Shortcut to vga_get
> + */
> +
> +static inline int vga_get_interruptible(struct pci_dev *pdev,
> +										unsigned int rsrc)
> +{
> +       return vga_get(pdev, rsrc, 1);
> +}
> +
> +/**
> + *     vga_get_interruptible

Should be vga_get_uninterruptible.

> + *
> + *     Shortcut to vga_get
> + */
> +
> +static inline int vga_get_uninterruptible(struct pci_dev *pdev,
> +											unsigned int rsrc)
> +{
> +       return vga_get(pdev, rsrc, 0);
> +}
> +
> +/**
> + *     vga_tryget      - try to acquire & lock legacy VGA resources
> + *
> + *     @pdev: pci devivce of VGA card or NULL for system default
> + *     @rsrc: bit mask of resources to acquire and lock
> + *
> + *     This function performs the same operation as vga_get(), but
> + *     will return an error (-EBUSY) instead of blocking if the resources
> + *     are already locked by another card. It can be called in any context
> + */
> +
> +extern int vga_tryget(struct pci_dev *pdev, unsigned int rsrc);
> +
> +/**
> + *     vga_put         - release lock on legacy VGA resources
> + *
> + *     @pdev: pci device of VGA card or NULL for system default
> + *     @rsrc: but mask of resource to release
> + *
> + *     This function releases resources previously locked by vga_get()
> + *     or vga_tryget(). The resources aren't disabled right away, so
> + *     that a subsequence vga_get() on the same card will succeed
> + *     immediately. Resources have a counter, so locks are only
> + *     released if the counter reaches 0.
> + */
> +
> +extern void vga_put(struct pci_dev *pdev, unsigned int rsrc);
> +
> +
> +/**
> + *     vga_default_device
> + *
> + *     This can be defined by the platform. The default implementation
> + *     is rather dumb and will probably only work properly on single
> + *     vga card setups and/or x86 platforms.
> + *
> + *     If your VGA default device is not PCI, you'll have to return
> + *     NULL here. In this case, I assume it will not conflict with
> + *     any PCI card. If this is not true, I'll have to define two archs
> + *     hooks for enabling/disabling the VGA default device if that is
> + *     possible. This may be a problem with real _ISA_ VGA cards, in
> + *     addition to a PCI one. I don't know at this point how to deal
> + *     with that card. Can theirs IOs be disabled at all ? If not, then
> + *     I suppose it's a matter of having the proper arch hook telling
> + *     us about it, so we basically never allow anybody to succeed a
> + *     vga_get()...
> + */
> +
> +#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
> +extern struct pci_dev *vga_default_device(void);
> +#endif
> +
> +/**
> + *     vga_conflicts
> + *
> + *     Architectures should define this if they have several
> + *     independant PCI domains that can afford concurrent VGA
> + *     decoding
> + */
> +
> +#ifndef __ARCH_HAS_VGA_CONFLICT
> +static inline int vga_conflicts(struct pci_dev *p1, struct pci_dev *p2)
> +{
> +       return 1;
> +}
> +#endif
> +
> +/*
> + * Register a client with the VGA arbitration logic
> + * return value: number of VGA devices in system.

The return value is documented incorrectly here.

> + *
> + * Clients have two callback mechanisms they can use.
> + * irq enable/disable callback -
> + *    If a client can't disable its GPUs VGA resources, then we
> + *    need to be able to ask it to turn off its irqs when we
> + *    turn off its mem and io decoding.
> + * set_vga_decode
> + *    If a client can disable its GPU VGA resource, it will
> + *    get a callback from this to set the encode/decode state
> + *
> + * Clients with disable abilities should check the return value
> + * of this function and if the VGA device count is > 1, should
> + * disable VGA decoding resources.
> + *
> + * Rationale: we cannot disable VGA decode resources unconditionally
> + * some single GPU laptops seem to require ACPI or BIOS access to the
> + * VGA registers to control things like backlights etc.
> + * Hopefully newer multi-GPU laptops do something saner, and desktops
> + * won't have any special ACPI for this.
> + */
> +int vga_client_register(struct pci_dev *pdev, void *cookie,
> +			void (*irq_set_state)(void *cookie, bool state),
> +			unsigned int (*set_vga_decode)(void *cookie, bool state));
> +
> +#endif /* LINUX_VGA_H */
> -- 
> 1.6.0.6

-- 
Pekka Paalanen
http://www.iki.fi/pq/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ