RE: [RFC PATCH] PCI: Introduce new device binding path using pci_dev.driver_override

2014-04-10 Thread Stuart Yoder
 Another advantage to this approach is that we can specify a driver
 override to force a specific binding or prevent any binding.  For
 instance when an IOMMU group is exposed to userspace through VFIO
 we require that all devices within that group are owned by VFIO.
 However, devices can be hot-added into an IOMMU group, in which case
 we want to prevent the device from binding to any driver (preferred
 driver = none) or perhaps have it automatically bind to vfio-pci.
 With driver_override it's a simple matter for this field to be set
 internally when the device is first discovered to prevent driver
 matches.

Alex, I think the patch looks good, but did have a question about
the hot-adding device scenario above.   How do you get the field to
be set internally when the device is first discovered?   What
hook is there to do that prior to the driver match processing?

Thanks,
Stuart
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [RFC PATCH] PCI: Introduce new device binding path using pci_dev.driver_override

2014-04-10 Thread Alex Williamson
On Thu, 2014-04-10 at 20:52 +, Stuart Yoder wrote:
  Another advantage to this approach is that we can specify a driver
  override to force a specific binding or prevent any binding.  For
  instance when an IOMMU group is exposed to userspace through VFIO
  we require that all devices within that group are owned by VFIO.
  However, devices can be hot-added into an IOMMU group, in which case
  we want to prevent the device from binding to any driver (preferred
  driver = none) or perhaps have it automatically bind to vfio-pci.
  With driver_override it's a simple matter for this field to be set
  internally when the device is first discovered to prevent driver
  matches.
 
 Alex, I think the patch looks good, but did have a question about
 the hot-adding device scenario above.   How do you get the field to
 be set internally when the device is first discovered?   What
 hook is there to do that prior to the driver match processing?

Devices already need to be added to iommu groups prior to driver probe
which happens either by the iommu driver calling bus_register_notifier()
and looking for BUS_NOTIFY_ADD_DEVICE or they can register an add_device
callback in their iommu_ops and let the iommu subsystem register the
notifier.  When a device is then added to an iommu group, there is yet
another notifier.  vfio calls iommu_group_register_notifier() and gets
told via IOMMU_GROUP_NOTIFY_ADD_DEVICE when a device is added to a
group.  We currently have a WARN_ON when a device is added to a live
group - vfio_group_nb_add_dev().  I expect we'd just replace that with
detecting the bus_type of the device and allocating a string with the
proper vfio driver for the bus.  Thanks,

Alex

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [RFC PATCH] PCI: Introduce new device binding path using pci_dev.driver_override

2014-04-10 Thread Alex Williamson
On Thu, 2014-04-10 at 15:15 -0600, Alex Williamson wrote:
 On Thu, 2014-04-10 at 20:52 +, Stuart Yoder wrote:
   Another advantage to this approach is that we can specify a driver
   override to force a specific binding or prevent any binding.  For
   instance when an IOMMU group is exposed to userspace through VFIO
   we require that all devices within that group are owned by VFIO.
   However, devices can be hot-added into an IOMMU group, in which case
   we want to prevent the device from binding to any driver (preferred
   driver = none) or perhaps have it automatically bind to vfio-pci.
   With driver_override it's a simple matter for this field to be set
   internally when the device is first discovered to prevent driver
   matches.
  
  Alex, I think the patch looks good, but did have a question about
  the hot-adding device scenario above.   How do you get the field to
  be set internally when the device is first discovered?   What
  hook is there to do that prior to the driver match processing?
 
 Devices already need to be added to iommu groups prior to driver probe
 which happens either by the iommu driver calling bus_register_notifier()
 and looking for BUS_NOTIFY_ADD_DEVICE or they can register an add_device
 callback in their iommu_ops and let the iommu subsystem register the
 notifier.  When a device is then added to an iommu group, there is yet
 another notifier.  vfio calls iommu_group_register_notifier() and gets
 told via IOMMU_GROUP_NOTIFY_ADD_DEVICE when a device is added to a
 group.  We currently have a WARN_ON when a device is added to a live
 group - vfio_group_nb_add_dev().  I expect we'd just replace that with
 detecting the bus_type of the device and allocating a string with the
 proper vfio driver for the bus.  Thanks,

For instance, the below should work for PCI devices.  I imagine we'd
also want to print out some useful log message for this as well.
Thanks,

Alex

--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -486,7 +486,14 @@ static int vfio_group_nb_add_dev(struct vfio_group *group, 
if (!atomic_read(group-container_users))
return 0;
 
-   /* TODO Prevent device auto probing */
+   /* Only let PCI devices added to a live group bind to vfio-pci */
+   if (dev_is_pci(dev)) {
+   struct pci_dev *pdev = to_pci_dev(dev);
+   pdev-driver_override = kstrdup(vfio-pci, GFP_KERNEL);
+   if (pdev-driver_override)
+   return 0;
+   }
+
WARN(Device %s added to live group %d!\n, dev_name(dev),
 iommu_group_id(group-iommu_group));
 


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [RFC PATCH] PCI: Introduce new device binding path using pci_dev.driver_override

2014-04-05 Thread Kim Phillips
Needed by platform device drivers, such as the vfio-platform driver [1],
in order to bypass the existing OF, ACPI, id_table and name string matches,
and successfully be able to be bound to any device, like so:

echo vfio-platform  /sys/bus/platform/devices/fff51000.ethernet/driver_override
echo fff51000.ethernet  
/sys/bus/platform/devices/fff51000.ethernet/driver/unbind
echo fff51000.ethernet  /sys/bus/platform/drivers_probe

This mimics PCI: Introduce new device binding path using
pci_dev.driver_override [2], which is an interface enhancement
for more deterministic PCI device binding, e.g., when in the
presence of hotplug.

[1] http://lkml.iu.edu/hypermail/linux/kernel/1402.1/00177.html
[2] http://thread.gmane.org/gmane.linux.kernel.iommu/4605

Signed-off-by: Kim Phillips kim.phill...@freescale.com
---
if this looks ok, should it be included in the next version of the
vfio-platform submission series, like last time ([1] above)?

 Documentation/ABI/testing/sysfs-bus-platform | 17 ++
 drivers/base/platform.c  | 46 
 include/linux/platform_device.h  |  1 +
 3 files changed, 64 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-platform

diff --git a/Documentation/ABI/testing/sysfs-bus-platform 
b/Documentation/ABI/testing/sysfs-bus-platform
new file mode 100644
index 000..6b14a6a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-platform
@@ -0,0 +1,17 @@
+What:  /sys/bus/platform/devices/.../driver_override
+Date:  April 2014
+Contact:   Kim Phillips kim.phill...@freescale.com
+Description:
+   This file allows the driver for a device to be specified
+   which will override standard OF, ACPI, ID table, and name
+   matching.  When specified, only a driver with a name matching
+   the value written to driver_override will have an opportunity
+   to bind to the device.  The override may be cleared by
+   writing an empty string (ex. echo  driver_override), returning
+   the device to standard matching rules binding.  Writing to
+   driver_override does not automatically unbind the device from
+   its current driver or make any attempt to automatically load
+   the specified driver name.  If no driver with a matching name
+   is currently loaded in the kernel, no match will be found.
+   This also allows devices to opt-out of driver binding using
+   a driver_override name such as none.
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index e714709..ded1db1 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -22,6 +22,7 @@
 #include linux/pm_runtime.h
 #include linux/idr.h
 #include linux/acpi.h
+#include linux/limits.h
 
 #include base.h
 #include power/power.h
@@ -690,8 +691,49 @@ static ssize_t modalias_show(struct device *dev, struct 
device_attribute *a,
 }
 static DEVICE_ATTR_RO(modalias);
 
+static ssize_t driver_override_store(struct device *dev,
+struct device_attribute *attr,
+const char *buf, size_t count)
+{
+   struct platform_device *pdev = to_platform_device(dev);
+   char *driver_override, *old = pdev-driver_override;
+
+   if (count  PATH_MAX)
+   return -EINVAL;
+
+   driver_override = kstrndup(buf, count, GFP_KERNEL);
+   if (!driver_override)
+   return -ENOMEM;
+
+   while (strlen(driver_override) 
+  driver_override[strlen(driver_override) - 1] == '\n')
+   driver_override[strlen(driver_override) - 1] = '\0';
+
+   if (strlen(driver_override)) {
+   pdev-driver_override = driver_override;
+   } else {
+   kfree(driver_override);
+   pdev-driver_override = NULL;
+   }
+
+   kfree(old);
+
+   return count;
+}
+
+static ssize_t driver_override_show(struct device *dev,
+   struct device_attribute *attr, char *buf)
+{
+   struct platform_device *pdev = to_platform_device(dev);
+
+   return sprintf(buf, %s\n, pdev-driver_override);
+}
+static DEVICE_ATTR_RW(driver_override);
+
+
 static struct attribute *platform_dev_attrs[] = {
dev_attr_modalias.attr,
+   dev_attr_driver_override.attr,
NULL,
 };
 ATTRIBUTE_GROUPS(platform_dev);
@@ -747,6 +789,10 @@ static int platform_match(struct device *dev, struct 
device_driver *drv)
struct platform_device *pdev = to_platform_device(dev);
struct platform_driver *pdrv = to_platform_driver(drv);
 
+   /* When driver_override is set, only bind to the matching driver */
+   if (pdev-driver_override)
+   return !strcmp(pdev-driver_override, drv-name);
+
/* Attempt an OF style match first */
if (of_driver_match_device(dev, drv))
return 1;

Re: [RFC PATCH] PCI: Introduce new device binding path using pci_dev.driver_override

2014-04-02 Thread Kim Phillips
On Tue, 1 Apr 2014 17:23:24 -0700
Greg KH gre...@linuxfoundation.org wrote:

 On Tue, Apr 01, 2014 at 06:52:12PM -0500, Kim Phillips wrote:
  On Tue, 01 Apr 2014 10:28:54 -0600
  Alex Williamson alex.william...@redhat.com wrote:
  
   The driver_override field allows us to specify the driver for a device
   rather than relying on the driver to provide a positive match of the
   device.  This shortcuts the existing process of looking up the vendor
   and device ID, adding them to the driver new_id, binding the device,
   then removing the ID, but it also provides a couple advantages.
   
   First, the above process allows the driver to bind to any device
   matching the new_id for the window where it's enabled.  This is often
   not desired, such as the case of trying to bind a single device to a
   meta driver like pci-stub or vfio-pci.  Using driver_override we can
   do this deterministically using:
   
   echo pci-stub  /sys/bus/pci/devices/:03:00.0/driver_override
   echo :03:00.0  /sys/bus/pci/devices/:03:00.0/driver/unbind
   echo :03:00.0  /sys/bus/pci/drivers_probe
   
   Previously we could not invoke drivers_probe after adding a device
   to new_id for a driver as we get non-deterministic behavior whether
   the driver we intend or the standard driver will claim the device.
   Now it becomes a deterministic process, only the driver matching
   driver_override will probe the device.
   
   To return the device to the standard driver, we simply clear the
   driver_override and reprobe the device, ex:
   
   echo  /sys/bus/pci/devices/:03:00.0/preferred_driver
   echo :03:00.0  /sys/bus/pci/devices/:03:00.0/driver/unbind
   echo :03:00.0  /sys/bus/pci/drivers_probe
   
   Another advantage to this approach is that we can specify a driver
   override to force a specific binding or prevent any binding.  For
   instance when an IOMMU group is exposed to userspace through VFIO
   we require that all devices within that group are owned by VFIO.
   However, devices can be hot-added into an IOMMU group, in which case
   we want to prevent the device from binding to any driver (preferred
   driver = none) or perhaps have it automatically bind to vfio-pci.
   With driver_override it's a simple matter for this field to be set
   internally when the device is first discovered to prevent driver
   matches.
   
   Signed-off-by: Alex Williamson alex.william...@redhat.com
   ---
   
   Apologies for the exceptionally long cc list, this is a follow-up to
   Stuart's Subject: mechanism to allow a driver to bind to any device
   thread.  This is effectively a v2 of the proof-of-concept patch I
   posted in that thread.  This version changes to use a dummy id struct
   to return on an override match, which removes the collateral damage
   and greatly simplifies the patch.  This feels fairly well baked for
   PCI and I would expect that platform drivers could do a similar
   implementation.  From there perhaps we can discuss whether there's
   any advantage to placing driver_override on struct device.  The logic
   for incorporating it into the match still needs to happen per bus
   driver, so it might only contribute to consistency of the show/store
   sysfs attributes to move it up to struct device.  Please comment.
  
  Sounds like Greg likes this approach more than {drv,dev}_sysfs_only.
 
 I have made no such judgement, I only pointed out that if you

ok.  If no-one chimes in in favour of one or the other, driver_override
works for platform devices.

 modify/add/remove a sysfs file, it needs to have documentation for it.

ok, so the platform device implementation should add a new
Documentation/ABI/testing/sysfs-bus-platform...

  The diff below is the result of duplicating and converting this patch
  for platform devices, and, indeed, binding a device to the
  vfio-platform driver succeeds with:
  
  echo vfio-platform  
  /sys/bus/platform/devices/fff51000.ethernet/driver_override
  echo fff51000.ethernet  
  /sys/bus/platform/devices/fff51000.ethernet/driver/unbind
  echo fff51000.ethernet  /sys/bus/platform/drivers_probe
  
  However, it's almost pure duplication modulo the bus match code.  The
  only other place I can see where to put the common bus check is
  drivers/base/base.h:driver_match_device(), which I'm guessing is
  off-limits?  So should we leave this as per-bus code, and somehow
  refactor driver_override_{show,store}?
 
 If you can provide a way for this to be done in a bus-independant way,
 like we did for new_id and the like, I'd be open to reviewing it.

I may be blind, but I don't see any new_id-related code shared
between drivers/pci/pci-driver.c and, e.g., drivers/usb/serial/bus.c,
nor do I see anything new_id related in drivers/base/.

So if we are to follow the current model, the PCI and platform device
implementations should be maintained separately.

Kim
___
iommu mailing list
iommu@lists.linux-foundation.org

Re: [RFC PATCH] PCI: Introduce new device binding path using pci_dev.driver_override

2014-04-02 Thread Alex Williamson
On Wed, 2014-04-02 at 17:06 -0500, Kim Phillips wrote:
 On Tue, 1 Apr 2014 17:23:24 -0700
 Greg KH gre...@linuxfoundation.org wrote:
 
  On Tue, Apr 01, 2014 at 06:52:12PM -0500, Kim Phillips wrote:
   On Tue, 01 Apr 2014 10:28:54 -0600
   Alex Williamson alex.william...@redhat.com wrote:
   
The driver_override field allows us to specify the driver for a device
rather than relying on the driver to provide a positive match of the
device.  This shortcuts the existing process of looking up the vendor
and device ID, adding them to the driver new_id, binding the device,
then removing the ID, but it also provides a couple advantages.

First, the above process allows the driver to bind to any device
matching the new_id for the window where it's enabled.  This is often
not desired, such as the case of trying to bind a single device to a
meta driver like pci-stub or vfio-pci.  Using driver_override we can
do this deterministically using:

echo pci-stub  /sys/bus/pci/devices/:03:00.0/driver_override
echo :03:00.0  /sys/bus/pci/devices/:03:00.0/driver/unbind
echo :03:00.0  /sys/bus/pci/drivers_probe

Previously we could not invoke drivers_probe after adding a device
to new_id for a driver as we get non-deterministic behavior whether
the driver we intend or the standard driver will claim the device.
Now it becomes a deterministic process, only the driver matching
driver_override will probe the device.

To return the device to the standard driver, we simply clear the
driver_override and reprobe the device, ex:

echo  /sys/bus/pci/devices/:03:00.0/preferred_driver
echo :03:00.0  /sys/bus/pci/devices/:03:00.0/driver/unbind
echo :03:00.0  /sys/bus/pci/drivers_probe

Another advantage to this approach is that we can specify a driver
override to force a specific binding or prevent any binding.  For
instance when an IOMMU group is exposed to userspace through VFIO
we require that all devices within that group are owned by VFIO.
However, devices can be hot-added into an IOMMU group, in which case
we want to prevent the device from binding to any driver (preferred
driver = none) or perhaps have it automatically bind to vfio-pci.
With driver_override it's a simple matter for this field to be set
internally when the device is first discovered to prevent driver
matches.

Signed-off-by: Alex Williamson alex.william...@redhat.com
---

Apologies for the exceptionally long cc list, this is a follow-up to
Stuart's Subject: mechanism to allow a driver to bind to any device
thread.  This is effectively a v2 of the proof-of-concept patch I
posted in that thread.  This version changes to use a dummy id struct
to return on an override match, which removes the collateral damage
and greatly simplifies the patch.  This feels fairly well baked for
PCI and I would expect that platform drivers could do a similar
implementation.  From there perhaps we can discuss whether there's
any advantage to placing driver_override on struct device.  The logic
for incorporating it into the match still needs to happen per bus
driver, so it might only contribute to consistency of the show/store
sysfs attributes to move it up to struct device.  Please comment.
   
   Sounds like Greg likes this approach more than {drv,dev}_sysfs_only.
  
  I have made no such judgement, I only pointed out that if you
 
 ok.  If no-one chimes in in favour of one or the other, driver_override
 works for platform devices.
 
  modify/add/remove a sysfs file, it needs to have documentation for it.
 
 ok, so the platform device implementation should add a new
 Documentation/ABI/testing/sysfs-bus-platform...
 
   The diff below is the result of duplicating and converting this patch
   for platform devices, and, indeed, binding a device to the
   vfio-platform driver succeeds with:
   
   echo vfio-platform  
   /sys/bus/platform/devices/fff51000.ethernet/driver_override
   echo fff51000.ethernet  
   /sys/bus/platform/devices/fff51000.ethernet/driver/unbind
   echo fff51000.ethernet  /sys/bus/platform/drivers_probe
   
   However, it's almost pure duplication modulo the bus match code.  The
   only other place I can see where to put the common bus check is
   drivers/base/base.h:driver_match_device(), which I'm guessing is
   off-limits?  So should we leave this as per-bus code, and somehow
   refactor driver_override_{show,store}?
  
  If you can provide a way for this to be done in a bus-independant way,
  like we did for new_id and the like, I'd be open to reviewing it.
 
 I may be blind, but I don't see any new_id-related code shared
 between drivers/pci/pci-driver.c and, e.g., drivers/usb/serial/bus.c,
 nor do I see anything new_id related in drivers/base/.
 
 So if we are to follow the current model, the PCI and platform device
 

Re: [RFC PATCH] PCI: Introduce new device binding path using pci_dev.driver_override

2014-04-01 Thread Greg KH
On Tue, Apr 01, 2014 at 10:28:54AM -0600, Alex Williamson wrote:
 The driver_override field allows us to specify the driver for a device
 rather than relying on the driver to provide a positive match of the
 device.  This shortcuts the existing process of looking up the vendor
 and device ID, adding them to the driver new_id, binding the device,
 then removing the ID, but it also provides a couple advantages.
 
 First, the above process allows the driver to bind to any device
 matching the new_id for the window where it's enabled.  This is often
 not desired, such as the case of trying to bind a single device to a
 meta driver like pci-stub or vfio-pci.  Using driver_override we can
 do this deterministically using:
 
 echo pci-stub  /sys/bus/pci/devices/:03:00.0/driver_override
 echo :03:00.0  /sys/bus/pci/devices/:03:00.0/driver/unbind
 echo :03:00.0  /sys/bus/pci/drivers_probe
 
 Previously we could not invoke drivers_probe after adding a device
 to new_id for a driver as we get non-deterministic behavior whether
 the driver we intend or the standard driver will claim the device.
 Now it becomes a deterministic process, only the driver matching
 driver_override will probe the device.
 
 To return the device to the standard driver, we simply clear the
 driver_override and reprobe the device, ex:
 
 echo  /sys/bus/pci/devices/:03:00.0/preferred_driver
 echo :03:00.0  /sys/bus/pci/devices/:03:00.0/driver/unbind
 echo :03:00.0  /sys/bus/pci/drivers_probe
 
 Another advantage to this approach is that we can specify a driver
 override to force a specific binding or prevent any binding.  For
 instance when an IOMMU group is exposed to userspace through VFIO
 we require that all devices within that group are owned by VFIO.
 However, devices can be hot-added into an IOMMU group, in which case
 we want to prevent the device from binding to any driver (preferred
 driver = none) or perhaps have it automatically bind to vfio-pci.
 With driver_override it's a simple matter for this field to be set
 internally when the device is first discovered to prevent driver
 matches.
 
 Signed-off-by: Alex Williamson alex.william...@redhat.com
 ---
 
 Apologies for the exceptionally long cc list, this is a follow-up to
 Stuart's Subject: mechanism to allow a driver to bind to any device
 thread.  This is effectively a v2 of the proof-of-concept patch I
 posted in that thread.  This version changes to use a dummy id struct
 to return on an override match, which removes the collateral damage
 and greatly simplifies the patch.  This feels fairly well baked for
 PCI and I would expect that platform drivers could do a similar
 implementation.  From there perhaps we can discuss whether there's
 any advantage to placing driver_override on struct device.  The logic
 for incorporating it into the match still needs to happen per bus
 driver, so it might only contribute to consistency of the show/store
 sysfs attributes to move it up to struct device.  Please comment.
 Thanks,
 
 Alex
 
  drivers/pci/pci-driver.c |   25 ++---
  drivers/pci/pci-sysfs.c  |   40 
  include/linux/pci.h  |1 +
  3 files changed, 63 insertions(+), 3 deletions(-)

No Documentation/ABI/ update to reflect the ABI you are creating?

thanks,

greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [RFC PATCH] PCI: Introduce new device binding path using pci_dev.driver_override

2014-04-01 Thread Alex Williamson
On Tue, 2014-04-01 at 09:47 -0700, Greg KH wrote:
 On Tue, Apr 01, 2014 at 10:28:54AM -0600, Alex Williamson wrote:
  The driver_override field allows us to specify the driver for a device
  rather than relying on the driver to provide a positive match of the
  device.  This shortcuts the existing process of looking up the vendor
  and device ID, adding them to the driver new_id, binding the device,
  then removing the ID, but it also provides a couple advantages.
  
  First, the above process allows the driver to bind to any device
  matching the new_id for the window where it's enabled.  This is often
  not desired, such as the case of trying to bind a single device to a
  meta driver like pci-stub or vfio-pci.  Using driver_override we can
  do this deterministically using:
  
  echo pci-stub  /sys/bus/pci/devices/:03:00.0/driver_override
  echo :03:00.0  /sys/bus/pci/devices/:03:00.0/driver/unbind
  echo :03:00.0  /sys/bus/pci/drivers_probe
  
  Previously we could not invoke drivers_probe after adding a device
  to new_id for a driver as we get non-deterministic behavior whether
  the driver we intend or the standard driver will claim the device.
  Now it becomes a deterministic process, only the driver matching
  driver_override will probe the device.
  
  To return the device to the standard driver, we simply clear the
  driver_override and reprobe the device, ex:
  
  echo  /sys/bus/pci/devices/:03:00.0/preferred_driver
  echo :03:00.0  /sys/bus/pci/devices/:03:00.0/driver/unbind
  echo :03:00.0  /sys/bus/pci/drivers_probe
  
  Another advantage to this approach is that we can specify a driver
  override to force a specific binding or prevent any binding.  For
  instance when an IOMMU group is exposed to userspace through VFIO
  we require that all devices within that group are owned by VFIO.
  However, devices can be hot-added into an IOMMU group, in which case
  we want to prevent the device from binding to any driver (preferred
  driver = none) or perhaps have it automatically bind to vfio-pci.
  With driver_override it's a simple matter for this field to be set
  internally when the device is first discovered to prevent driver
  matches.
  
  Signed-off-by: Alex Williamson alex.william...@redhat.com
  ---
  
  Apologies for the exceptionally long cc list, this is a follow-up to
  Stuart's Subject: mechanism to allow a driver to bind to any device
  thread.  This is effectively a v2 of the proof-of-concept patch I
  posted in that thread.  This version changes to use a dummy id struct
  to return on an override match, which removes the collateral damage
  and greatly simplifies the patch.  This feels fairly well baked for
  PCI and I would expect that platform drivers could do a similar
  implementation.  From there perhaps we can discuss whether there's
  any advantage to placing driver_override on struct device.  The logic
  for incorporating it into the match still needs to happen per bus
  driver, so it might only contribute to consistency of the show/store
  sysfs attributes to move it up to struct device.  Please comment.
  Thanks,
  
  Alex
  
   drivers/pci/pci-driver.c |   25 ++---
   drivers/pci/pci-sysfs.c  |   40 
   include/linux/pci.h  |1 +
   3 files changed, 63 insertions(+), 3 deletions(-)
 
 No Documentation/ABI/ update to reflect the ABI you are creating?

Oops, thanks for the reminder.  I'd propose this:

diff --git a/Documentation/ABI/testing/sysfs-bus-pci 
b/Documentation/ABI/testing/sysfs-bus-pci
index a3c5a66..55ca6e5 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci
+++ b/Documentation/ABI/testing/sysfs-bus-pci
@@ -250,3 +250,21 @@ Description:
valid.  For example, writing a 2 to this file when sriov_numvfs
is not 0 and not 2 already will return an error. Writing a 10
when the value of sriov_totalvfs is 8 will return an error.
+
+What:  /sys/bus/pci/devices/.../driver_override
+Date:  April 2014
+Contact:   Alex Williamson alex.william...@redhat.com
+Description:
+   This file allows the driver for a device to be specified
+   which will override standard static and dynamic ID matching.
+   When specified, only a driver with a name matching the value
+   written to driver_override will have an opportunity to bind
+   to the device.  The override may be cleared by writing an
+   empty string (ex. echo  driver_override), returning the
+   device to standard matching rules binding.  Writing to
+   driver_override does not automatically unbind the device from
+   its current driver or make any attempt to automatically load
+   the specified driver name.  If no driver with a matching name
+   is currently loaded in the kernel, no match will be found.
+   This also 

Re: [RFC PATCH] PCI: Introduce new device binding path using pci_dev.driver_override

2014-04-01 Thread Kim Phillips
On Tue, 01 Apr 2014 10:28:54 -0600
Alex Williamson alex.william...@redhat.com wrote:

 The driver_override field allows us to specify the driver for a device
 rather than relying on the driver to provide a positive match of the
 device.  This shortcuts the existing process of looking up the vendor
 and device ID, adding them to the driver new_id, binding the device,
 then removing the ID, but it also provides a couple advantages.
 
 First, the above process allows the driver to bind to any device
 matching the new_id for the window where it's enabled.  This is often
 not desired, such as the case of trying to bind a single device to a
 meta driver like pci-stub or vfio-pci.  Using driver_override we can
 do this deterministically using:
 
 echo pci-stub  /sys/bus/pci/devices/:03:00.0/driver_override
 echo :03:00.0  /sys/bus/pci/devices/:03:00.0/driver/unbind
 echo :03:00.0  /sys/bus/pci/drivers_probe
 
 Previously we could not invoke drivers_probe after adding a device
 to new_id for a driver as we get non-deterministic behavior whether
 the driver we intend or the standard driver will claim the device.
 Now it becomes a deterministic process, only the driver matching
 driver_override will probe the device.
 
 To return the device to the standard driver, we simply clear the
 driver_override and reprobe the device, ex:
 
 echo  /sys/bus/pci/devices/:03:00.0/preferred_driver
 echo :03:00.0  /sys/bus/pci/devices/:03:00.0/driver/unbind
 echo :03:00.0  /sys/bus/pci/drivers_probe
 
 Another advantage to this approach is that we can specify a driver
 override to force a specific binding or prevent any binding.  For
 instance when an IOMMU group is exposed to userspace through VFIO
 we require that all devices within that group are owned by VFIO.
 However, devices can be hot-added into an IOMMU group, in which case
 we want to prevent the device from binding to any driver (preferred
 driver = none) or perhaps have it automatically bind to vfio-pci.
 With driver_override it's a simple matter for this field to be set
 internally when the device is first discovered to prevent driver
 matches.
 
 Signed-off-by: Alex Williamson alex.william...@redhat.com
 ---
 
 Apologies for the exceptionally long cc list, this is a follow-up to
 Stuart's Subject: mechanism to allow a driver to bind to any device
 thread.  This is effectively a v2 of the proof-of-concept patch I
 posted in that thread.  This version changes to use a dummy id struct
 to return on an override match, which removes the collateral damage
 and greatly simplifies the patch.  This feels fairly well baked for
 PCI and I would expect that platform drivers could do a similar
 implementation.  From there perhaps we can discuss whether there's
 any advantage to placing driver_override on struct device.  The logic
 for incorporating it into the match still needs to happen per bus
 driver, so it might only contribute to consistency of the show/store
 sysfs attributes to move it up to struct device.  Please comment.

Sounds like Greg likes this approach more than {drv,dev}_sysfs_only.

The diff below is the result of duplicating and converting this patch
for platform devices, and, indeed, binding a device to the
vfio-platform driver succeeds with:

echo vfio-platform  /sys/bus/platform/devices/fff51000.ethernet/driver_override
echo fff51000.ethernet  
/sys/bus/platform/devices/fff51000.ethernet/driver/unbind
echo fff51000.ethernet  /sys/bus/platform/drivers_probe

However, it's almost pure duplication modulo the bus match code.  The
only other place I can see where to put the common bus check is
drivers/base/base.h:driver_match_device(), which I'm guessing is
off-limits?  So should we leave this as per-bus code, and somehow
refactor driver_override_{show,store}?

Kim

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index bc78848..621c5bd2 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -22,6 +22,7 @@
 #include linux/pm_runtime.h
 #include linux/idr.h
 #include linux/acpi.h
+#include linux/limits.h
 
 #include base.h
 #include power/power.h
@@ -693,8 +694,49 @@ static ssize_t modalias_show(struct device *dev, struct 
device_attribute *a,
 }
 static DEVICE_ATTR_RO(modalias);
 
+static ssize_t driver_override_store(struct device *dev,
+struct device_attribute *attr,
+const char *buf, size_t count)
+{
+   struct platform_device *pdev = to_platform_device(dev);
+   char *driver_override, *old = pdev-driver_override;
+
+   if (count  PATH_MAX)
+   return -EINVAL;
+
+   driver_override = kstrndup(buf, count, GFP_KERNEL);
+   if (!driver_override)
+   return -ENOMEM;
+
+   while (strlen(driver_override) 
+  driver_override[strlen(driver_override) - 1] == '\n')
+   driver_override[strlen(driver_override) - 1] = '\0';
+
+   if (strlen(driver_override)) {
+

Re: [RFC PATCH] PCI: Introduce new device binding path using pci_dev.driver_override

2014-04-01 Thread Christoffer Dall
On Tue, Apr 01, 2014 at 11:15:40AM -0600, Alex Williamson wrote:
 On Tue, 2014-04-01 at 09:47 -0700, Greg KH wrote:
  On Tue, Apr 01, 2014 at 10:28:54AM -0600, Alex Williamson wrote:
   The driver_override field allows us to specify the driver for a device
   rather than relying on the driver to provide a positive match of the
   device.  This shortcuts the existing process of looking up the vendor
   and device ID, adding them to the driver new_id, binding the device,
   then removing the ID, but it also provides a couple advantages.
   
   First, the above process allows the driver to bind to any device
   matching the new_id for the window where it's enabled.  This is often
   not desired, such as the case of trying to bind a single device to a
   meta driver like pci-stub or vfio-pci.  Using driver_override we can
   do this deterministically using:
   
   echo pci-stub  /sys/bus/pci/devices/:03:00.0/driver_override
   echo :03:00.0  /sys/bus/pci/devices/:03:00.0/driver/unbind
   echo :03:00.0  /sys/bus/pci/drivers_probe
   
   Previously we could not invoke drivers_probe after adding a device
   to new_id for a driver as we get non-deterministic behavior whether
   the driver we intend or the standard driver will claim the device.
   Now it becomes a deterministic process, only the driver matching
   driver_override will probe the device.
   
   To return the device to the standard driver, we simply clear the
   driver_override and reprobe the device, ex:
   
   echo  /sys/bus/pci/devices/:03:00.0/preferred_driver
   echo :03:00.0  /sys/bus/pci/devices/:03:00.0/driver/unbind
   echo :03:00.0  /sys/bus/pci/drivers_probe
   
   Another advantage to this approach is that we can specify a driver
   override to force a specific binding or prevent any binding.  For
   instance when an IOMMU group is exposed to userspace through VFIO
   we require that all devices within that group are owned by VFIO.
   However, devices can be hot-added into an IOMMU group, in which case
   we want to prevent the device from binding to any driver (preferred
   driver = none) or perhaps have it automatically bind to vfio-pci.
   With driver_override it's a simple matter for this field to be set
   internally when the device is first discovered to prevent driver
   matches.
   
   Signed-off-by: Alex Williamson alex.william...@redhat.com
   ---
   
   Apologies for the exceptionally long cc list, this is a follow-up to
   Stuart's Subject: mechanism to allow a driver to bind to any device
   thread.  This is effectively a v2 of the proof-of-concept patch I
   posted in that thread.  This version changes to use a dummy id struct
   to return on an override match, which removes the collateral damage
   and greatly simplifies the patch.  This feels fairly well baked for
   PCI and I would expect that platform drivers could do a similar
   implementation.  From there perhaps we can discuss whether there's
   any advantage to placing driver_override on struct device.  The logic
   for incorporating it into the match still needs to happen per bus
   driver, so it might only contribute to consistency of the show/store
   sysfs attributes to move it up to struct device.  Please comment.
   Thanks,
   
   Alex
   
drivers/pci/pci-driver.c |   25 ++---
drivers/pci/pci-sysfs.c  |   40 
include/linux/pci.h  |1 +
3 files changed, 63 insertions(+), 3 deletions(-)
  
  No Documentation/ABI/ update to reflect the ABI you are creating?
 
 Oops, thanks for the reminder.  I'd propose this:
 
 diff --git a/Documentation/ABI/testing/sysfs-bus-pci 
 b/Documentation/ABI/testing/sysfs-bus-pci
 index a3c5a66..55ca6e5 100644
 --- a/Documentation/ABI/testing/sysfs-bus-pci
 +++ b/Documentation/ABI/testing/sysfs-bus-pci
 @@ -250,3 +250,21 @@ Description:
   valid.  For example, writing a 2 to this file when sriov_numvfs
   is not 0 and not 2 already will return an error. Writing a 10
   when the value of sriov_totalvfs is 8 will return an error.
 +
 +What:/sys/bus/pci/devices/.../driver_override
 +Date:April 2014
 +Contact: Alex Williamson alex.william...@redhat.com
 +Description:
 + This file allows the driver for a device to be specified
 + which will override standard static and dynamic ID matching.
 + When specified, only a driver with a name matching the value
 + written to driver_override will have an opportunity to bind
 + to the device.  The override may be cleared by writing an
 + empty string (ex. echo  driver_override), returning the
 + device to standard matching rules binding.  Writing to
 + driver_override does not automatically unbind the device from
 + its current driver or make any attempt to automatically load
 + the specified driver