Re: RFC: (re-)binding the VFIO platform driver to a platform device

[Scott Wood <scottwood@freescale.com>]

On Wed, 2013-10-02 at 11:43 -0700, Christoffer Dall wrote:
> On Wed, Oct 02, 2013 at 01:32:38PM -0500, Scott Wood wrote:
> > On Wed, 2013-10-02 at 13:25 -0500, Yoder Stuart-B08248 wrote:
> > > 
> > > > -----Original Message-----
> > > > From: Christoffer Dall [mailto:christoffer.dall@linaro.org]
> > > > Sent: Wednesday, October 02, 2013 10:14 AM
> > > > To: Alex Williamson
> > > > Cc: Kim Phillips; gregkh@linuxfoundation.org; linux-
> > > > kernel@vger.kernel.org; a.motakis@virtualopensystems.com; agraf@suse.de;
> > > > Yoder Stuart-B08248; Wood Scott-B07421; Sethi Varun-B16395; Bhushan
> > > > Bharat-R65777; peter.maydell@linaro.org; santosh.shukla@linaro.org;
> > > > kvm@vger.kernel.org
> > > > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform
> > > > device
> > > > 
> > > > Wouldn't a sysfs file to add compatibility strings to the vfio-platform
> > > > driver make driver_match_device return true and make everyone happy?
> > > 
> > > I had a similar thought.  Why can't we do something like:
> > > 
> > >   echo "fsl,i2c" > /sys/bus/platform/drivers/vfio-platform/new_compatible
> > >   echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
> > > 
> > > The first steps tell vfio-platform to register itself to handle
> > > "fsl,i2c" compatible devices.  The second step does the bind.
> > 
> > Needing to specify the compatible is hacky (we already know what device
> > we want to bind; why do we need to scrounge up more information than
> > that, and add a new sysfs interface for extending compatible matches,
> > and a more flexible data structure to back that up?), and is racy on
> > buses that can hotplug (which driver gets the new device?).
> 
> Why hacky?  It seems quite reasonable to me that the user has to tell a
> subsystem that from a certain point it should be capable of handling
> some device.

But the reason that driver can handle the device has nothing to do with
the compatible -- it can handle any device on the bus (except for
limitations checked for in the probe function), but it's a policy
decision whether we want it to handle any particular device (as opposed
to a particular type of device).

Now, if we end up requiring a device-aware kernel stub for VFIO platform
devices (as was discussed for handling reset and such), we won't want a
wildcard match, but we'd still want to say that devices only get bound
to vfio upon explicit request.  We also would not want userspace adding
to vfio's compatible list in that case.  So perhaps a flag for "only
bind on explicit request" should be separate from the ability to supply
a wildcard match.  VFIO PCI could still use the wildcard match.

> As for the data structure, isn't this a simple linked list?

It could be, but currently it's an array, which is what all the drivers
are expecting to provide.  Adding a second parallel mechanism (like PCI
dynid) seems excessive compared to adding a wildcard match (on PCI such
a mechanism happened to already exist, which made it easy to use for
this, even if it wasn't necessarily the best approach).  And then what
happens on non-device-tree platform devices?

> The problem with the race seems to be a common problem that hasn't even
> been solved for PCI yet, so I'm wondering if this is not an orthogonal
> issue with a separate solution, such as a priority or something like
> that.
>
> Yes, once you've added the new_compatible to the vfio-platform driver,
> it's up for grabs from both the new and the old driver, but that could
> be solved by always making sure that the vfio-platform driver is checked
> first.

...which is the opposite of what you want if a different device of the
same type is plugged in after you add the device type ID to vfio.  A
"bind only by request" flag would avoid that problem.

As for the possibility that the normal driver would claim it (maybe due
to the bus being rescanned after a hotplug event?), that could be
addressed with a mechanism to atomically unbind-and-rebind, or (perhaps
easier) by making it so that once a device has been explicitly unbound,
it can only be bound again by explicit request (which would also allow a
user to say "I don't want to use this device at all" without it getting
rebound later).

Better still would be an independent "don't bind by default" flag that
the user can set in sysfs (this is different from the driver's "don't
bind by default" flag that is set by the driver), so that the action is
reversible, and so a user can set this policy regardless of whether a
driver has been loaded yet.

> > What's wrong with a non-vfio-specific flag that a driver can set, that
> > indicates that the driver is willing to try to bind to any device on the
> > bus if explicitly requested via the existing sysfs bind mechanism?
> > 
> It sounds more hackish to me to invent some 'generic' flag to solve a
> very specific case. 

new_compatible would be to solve an even more specific case, but both
new_compatible and a don't-bind-by-default flag could have applications
elsewhere.

> What you're suggesting would let users specify that
> a serial driver should handle a NIC hardware, no?  That sounds much much
> worse to me.

The flag (and wildcard match, if applicable) would be set by the driver,
not by the user.  Whereas PCI's "new_id" and the "new_compatible" being
suggested in this thread would allow exactly that, assuming the driver
doesn't reject the device in the probe function.

-Scott