On Mon, Jun 11, 2012 at 11:42:35AM +0300, Jani Nikula wrote:
> On Fri, 08 Jun 2012, Daniel Vetter <daniel.vetter at 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 at 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 at ffwll.ch
Mobile: +41 (0)79 365 57 48
