[PATCH] vgaarb: call pci_disable|enable_device on decoding vga devices

[PATCH] vgaarb: call pci_disable|enable_device on decoding vga devices

[Daniel Vetter]

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);
+	}
+
 	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

[PATCH] vgaarb: call pci_disable|enable_device on decoding vga devices

[Daniel Vetter]

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".

v2: ret needs to be signed, otherwise ERR_PTR will just store a large
positive value on 64bit machines. Noticed by Jani Nikula.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/vga/vgaarb.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 3df8fc0..c0b1271 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;
@@ -172,6 +176,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;
+	int ret;
 	u32 flags = 0;
 
 	/* Account for "normal" resources to lock. If we decode the legacy,
@@ -264,6 +269,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 +299,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);
+	}
+
 	pci_set_vga_state(vgadev->pdev, true, pci_bits, flags);
 
 	if (!vgadev->bridge_has_one_vga) {
@@ -376,6 +391,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 +530,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 +600,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

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

[Jani Nikula]

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

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

[Daniel Vetter]

On Mon, Jun 11, 2012 at 11:42:35AM +0300, Jani Nikula wrote:
> 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.)

No coffee needed on your side, ret should clearly be signed. I'll fix this
asap, thanks for catching this.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

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

[Daniel Vetter]

Hi Dave,

This one plus the pci_disable_device and related error path cleanups are
part of the series to move the agp setup into drivers.

I've implemented your suggestion to work around the midlayer, so drm/i915
doesn't block for this at all. But I still think this is the right way to
solve this. Shall I resend all these patches so you have them in context?

Yours, Daniel

On Fri, Jun 08, 2012 at 06:20:39PM +0200, Daniel Vetter 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);
> +	}
> +
>  	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
> 

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48