Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag toallow binding via sysfs only

[Greg Kroah-Hartman]

On Thu, Dec 19, 2013 at 09:06:21PM +0000, Stuart Yoder wrote:
> 
> 
> > -----Original Message-----
> > From: Greg Kroah-Hartman [mailto:gregkh@xxxxxxxxxxxxxxxxxxx]
> > Sent: Thursday, December 19, 2013 2:34 PM
> > To: Wood Scott-B07421
> > Cc: Kim Phillips; linux-kernel@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx;
> > Bhushan Bharat-R65777; Yoder Stuart-B08248; christoffer.dall@xxxxxxxxxx;
> > alex.williamson@xxxxxxxxxx; a.motakis@xxxxxxxxxxxxxxxxxxxxxx;
> > agraf@xxxxxxx; Sethi Varun-B16395
> > Subject: Re: [REPOST][PATCH 1/2] driver core: Add new device_driver flag
> > to allow binding via sysfs only
> > 
> > On Thu, Dec 19, 2013 at 02:22:11PM -0600, Scott Wood wrote:
> > > On Wed, 2013-12-18 at 17:07 -0800, Greg Kroah-Hartman wrote:
> > > > On Tue, Dec 03, 2013 at 12:34:46PM +0000, Kim Phillips wrote:
> > > > > VFIO supports pass-through of devices to user space - for sake
> > > > > of illustration, say a PCI e1000 device:
> > > > >
> > > > > - the e1000 is first unbound from the PCI e1000 driver via sysfs
> > > > > - the vfio-pci driver is told via new_id that it now handles e1000
> > devices
> > > > > - the e1000 is explicitly bound to vfio-pci through sysfs
> > > > >
> > > > > However, now we have two drivers in the system that both handle
> > e1000
> > > > > devices.  A hotplug event could then occur and it is ambiguous as
> > to which
> > > > > driver will claim the device.  The desired semantics is that vfio-
> > pci is
> > > > > only bound to devices by explicit request in sysfs.  This patch
> > makes this
> > > > > possible by introducing a sysfs_bind_only flag in struct
> > device_driver.
> > > >
> > > > Why deal with this at all and not just deal with the "bind" sysfs
> > file
> > > > instead?  That way no driver core logic needs to be changed at all,
> > and
> > > > your userspace tools know _exactly_ which device is being bound to
> > the
> > > > new device.
> > > >
> > > > Don't mess with the "new_id" file for stuff like this, as you point
> > out,
> > > > it's "tricky"...
> > >
> > > As discussed before, "bind" does not bypass the ID checks, and thus it
> > > does not work without either "new_id" or a wildcard match.
> > 
> > Ah, forgot about that.
> > 
> > > Or are you proposing changing "bind" so that it does bypass the ID
> > > checks?  Or perhaps a new "force_bind" file that does?
> > 
> > No.  But you can use bind/unbind along with the existing new_id file to
> > get what you want today.
> 
> Yes, but that only works for PCI.

No, not only PCI.

> There is no such concept for platform drivers.

Then fix that.

Or make your device not be a platform device, odds are that's the better
solution in the end, right?

> > If you just happen to bind a device to a wrong
> > driver for a while, that's not really a problem, right?
> 
> It's annoying but not the end of the world.
> 
> > I don't like this patch as we are adding lots of special and odd logic
> > to the core, for use by almost no one, which ensures that it will never
> > get tested, and will probably get broken in some subtle way in the
> > future.
> 
> It certainly will be used by users of vfio-platform.
> 
> Here is the problem-- the new platform device "match_any_dev" mechanism
> in patch 2 of this series is not going to work without "sysfs_bind_only".
> A platform driver that just sets "match_any_dev" will grab any or all 
> platform devices during normal bus probing.

No it will not, it will fail in the probe function as it knows to not
grab the device, just like any driver for other busses that say it can
"handle all Intel PCI devices" and the like.

> > Heck, I can't even ensure that you got it right now, with this tiny
> > patch, how do you know it works given that there are no users of this
> > flag anywhere (hint, you never showed me any...)
> 
> It has been tested on both vfio-pci and vfio-platform in some limited
> experiments as far as I know, but that code is not ready for upstream
> yet.

Then I'll wait until you get something that actually is worth
upstreaming before worrying about this again.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/