Re: [Intel-gfx] [PATCH] vgaarb: call pci_disable|enable_device on decoding vga devices

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 08 Jun 2012, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> There's the neat little problem that some systems die if vga decoding
> isn't decoded anywhere, because linux disabled that pci device.
>
> Atm we solve that problem by simple not calling pci_disable_device at
> driver unregister time for drm pci devices. Which isn't to great
> because it leaks a pci_enable_device reference on module reload and
> also is rather inconsistent, since the driver load codcode in
> drm/drm_pci.c _does_ call pci_disable_device on the load error path.
>
> To fix this issue, let the vga arbiter hold a pci_enable_device
> reference on its own when the vga device decodes anything. This leaves
> the issue still open when the vga arbiter isn't loaded/compiled in,
> but I guess since drm module unload is riddled with dragons it's ok to
> say "just don't do this".
>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/vga/vgaarb.c |   27 +++++++++++++++++++++++++--
>  1 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index 3df8fc0..82f19a1 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -49,7 +49,11 @@
>  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
> + * up the various operations of the arbiter. Note that vgaarb keeps a
> + * pci_enable_device reference for all vga devices with owns != 0. This is to
> + * avoid drm devices from killing the system when they call pci_disable_device
> + * on module unload (as they should), because some systems don't like it if no
> + * one decodes vga.
>   */
>  struct vga_device {
>  	struct list_head list;
> @@ -171,7 +175,7 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
>  {
>  	unsigned int wants, legacy_wants, match;
>  	struct vga_device *conflict;
> -	unsigned int pci_bits;
> +	unsigned int pci_bits, ret;
>  	u32 flags = 0;
>  
>  	/* Account for "normal" resources to lock. If we decode the legacy,
> @@ -264,6 +268,10 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
>  
>  		pci_set_vga_state(conflict->pdev, false, pci_bits, flags);
>  		conflict->owns &= ~lwants;
> +
> +		if (conflict->owns == 0)
> +			pci_disable_device(conflict->pdev);
> +
>  		/* If he also owned non-legacy, that is no longer the case */
>  		if (lwants & VGA_RSRC_LEGACY_MEM)
>  			conflict->owns &= ~VGA_RSRC_NORMAL_MEM;
> @@ -290,6 +298,12 @@ enable_them:
>  	if (!!(wants & VGA_RSRC_LEGACY_MASK))
>  		flags |= PCI_VGA_STATE_CHANGE_BRIDGE;
>  
> +	if (vgadev->owns == 0) {
> +		ret = pci_enable_device(vgadev->pdev);
> +		if (ret)
> +			return ERR_PTR(ret);

Hi Daniel, I think unsigned ret will result in a positive ERR_PTR, which
won't be caught by IS_ERR_VALUE(). (Either that, or I need more coffee
to get the promotion rules right. Better use signed int no matter what.)

BR,
Jani.


> +	}
> +
>  	pci_set_vga_state(vgadev->pdev, true, pci_bits, flags);
>  
>  	if (!vgadev->bridge_has_one_vga) {
> @@ -376,6 +390,8 @@ int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible)
>  		if (conflict == NULL)
>  			break;
>  
> +		if (IS_ERR(conflict))
> +			return PTR_ERR(conflict);
>  
>  		/* We have a conflict, we wait until somebody kicks the
>  		 * work queue. Currently we have one work queue that we
> @@ -513,6 +529,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
>  	unsigned long flags;
>  	struct pci_bus *bus;
>  	struct pci_dev *bridge;
> +	int ret;
>  	u16 cmd;
>  
>  	/* Only deal with VGA class devices */
> @@ -582,6 +599,12 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
>  
>  	vga_arbiter_check_bridge_sharing(vgadev);
>  
> +	if (vgadev->owns != 0) {
> +		ret = pci_enable_device(vgadev->pdev);
> +		if (ret)
> +			goto fail;
> +	}
> +
>  	/* Add to the list */
>  	list_add(&vgadev->list, &vga_list);
>  	vga_count++;
> -- 
> 1.7.7.6
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx