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

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Dave Airlie <airlied@gmail.com>,
	Tiago Vignatti <tiago.vignatti@nokia.com>,
	Jesse Barnes <jbarnes@virtuousgeek.org>,
	Dave Airlie <airlied@redhat.com>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] vga: implements VGA arbitration on Linux
Date: Fri, 17 Jul 2009 10:20:10 +1000	[thread overview]
Message-ID: <1247790010.27937.71.camel@pasglop> (raw)
In-Reply-To: <20090716094855.455c3f77@lxorguk.ukuu.org.uk>

On Thu, 2009-07-16 at 09:48 +0100, Alan Cox wrote:
> 
> > >> +             pdev = vga_default_device();
> > >
> > > What if the BIOS provided device was hot unplugged ?
> > 
> > we just use the pdev as a cookie, if it was hot unplugged we'll
> > have gotten a callback to remove it from the VGA device list
> > and the lookup which happens 5 lines later inside the spinlock
> > will fail.
> 
> What if I inserted a new device - the allocator might give me a new
> device with the same pointer if its reusing the same slab entry for
> that
> size. Unlikely but given a zillion boxes this starts to occur 8(

The original proof-of concept draft code I wrote (and this is -very-
close to it :-) didn't quite handle hotplug of the default device. That
does need to be sorted.

But then, I have a hard time finding any useful locking to use vs. PCI
hotplug, so it's a non trivial matter.

> Not commented on - but a serious question would be "do we actually
> care enough or are there really devices with just I/O and just vga
> memory access used ?"

I honestly cannot remember why I split the 4 resource types that way
back then. I have the nagging feeling that I had a good reason to do so
but it totally eludes me today :-)

The one main thing I wanted was to keep legacy vs. standard resources so
that the all the portions of a driver (DDX, DRM, etc...) that use
standard resources can continue to lock/unlock just these, without
having to know whether one of the elements disable legacy decoding (or
is trying to request it again).

But maybe somebody with a fresh new look on all this will rightfully say
"fuck it, that's too complicated" and turn the whole thing into a single
token to pass around.

> Your cookie validity is suspect I fear. 

Yes, it is sadly.

> Also holding the device reference means you stop that exact set of
> resources being reissued too early and
> you (or clients) scribbling on them through unfortunate timing. So I
> think you actually do need to grab references properly, Doesn't make
> the
> code much more complex that I can see.
> 
Well, we are in the device addition/removal path, so the list of vga
devices doesn't need to hold a reference here. However, we probably need
to make sure we take one when we peek something from that list.

Cheers,
Ben.