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/0000:03:00.0/driver_override
echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
echo 0000: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/0000:03:00.0/preferred_driver
echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
echo 0000: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 <[email protected]>
---
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(-)
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 25f0bc6..f780eb8 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -216,6 +216,13 @@ const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
return NULL;
}
+static const struct pci_device_id pci_device_id_any = {
+ .vendor = PCI_ANY_ID,
+ .device = PCI_ANY_ID,
+ .subvendor = PCI_ANY_ID,
+ .subdevice = PCI_ANY_ID,
+};
+
/**
* pci_match_device - Tell if a PCI device structure has a matching PCI device id structure
* @drv: the PCI driver to match against
@@ -229,18 +236,30 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv,
struct pci_dev *dev)
{
struct pci_dynid *dynid;
+ const struct pci_device_id *found_id = NULL;
+
+ /* When driver_override is set, only bind to the matching driver */
+ if (dev->driver_override && strcmp(dev->driver_override, drv->name))
+ return NULL;
/* Look at the dynamic ids first, before the static ones */
spin_lock(&drv->dynids.lock);
list_for_each_entry(dynid, &drv->dynids.list, node) {
if (pci_match_one_device(&dynid->id, dev)) {
- spin_unlock(&drv->dynids.lock);
- return &dynid->id;
+ found_id = &dynid->id;
+ break;
}
}
spin_unlock(&drv->dynids.lock);
- return pci_match_id(drv->id_table, dev);
+ if (!found_id)
+ found_id = pci_match_id(drv->id_table, dev);
+
+ /* driver_override will always match, send a dummy id */
+ if (!found_id && dev->driver_override)
+ found_id = &pci_device_id_any;
+
+ return found_id;
}
struct drv_dev_and_id {
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 276ef9c..70cb39d 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -510,6 +510,45 @@ static struct device_attribute sriov_numvfs_attr =
sriov_numvfs_show, sriov_numvfs_store);
#endif /* CONFIG_PCI_IOV */
+static ssize_t driver_override_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct pci_dev *pdev = to_pci_dev(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 pci_dev *pdev = to_pci_dev(dev);
+
+ return sprintf(buf, "%s\n", pdev->driver_override);
+}
+static DEVICE_ATTR_RW(driver_override);
+
static struct attribute *pci_dev_attrs[] = {
&dev_attr_resource.attr,
&dev_attr_vendor.attr,
@@ -532,6 +571,7 @@ static struct attribute *pci_dev_attrs[] = {
#if defined(CONFIG_PM_RUNTIME) && defined(CONFIG_ACPI)
&dev_attr_d3cold_allowed.attr,
#endif
+ &dev_attr_driver_override.attr,
NULL,
};
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 33aa2ca..6b666af 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -364,6 +364,7 @@ struct pci_dev {
#endif
phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */
size_t romlen; /* Length of ROM if it's not from the BAR */
+ char *driver_override; /* Driver name to force a match */
};
static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
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/0000:03:00.0/driver_override
> echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
> echo 0000: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/0000:03:00.0/preferred_driver
> echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
> echo 0000: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 <[email protected]>
> ---
>
> 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
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/0000:03:00.0/driver_override
> > echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
> > echo 0000: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/0000:03:00.0/preferred_driver
> > echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
> > echo 0000: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 <[email protected]>
> > ---
> >
> > 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 <[email protected]>
+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 allows devices to opt-out of driver binding using
+ a driver_override name such as "none".
Thanks,
Alex
On Tue, 01 Apr 2014 10:28:54 -0600
Alex Williamson <[email protected]> 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/0000:03:00.0/driver_override
> echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
> echo 0000: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/0000:03:00.0/preferred_driver
> echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
> echo 0000: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 <[email protected]>
> ---
>
> 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)) {
+ 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);
@@ -750,6 +792,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;
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 16f6654..7ffe809 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -28,6 +28,7 @@ struct platform_device {
struct resource *resource;
const struct platform_device_id *id_entry;
+ char *driver_override; /* Driver name to force a match */
/* MFD cell pointer */
struct mfd_cell *mfd_cell;
On Tue, Apr 01, 2014 at 06:52:12PM -0500, Kim Phillips wrote:
> On Tue, 01 Apr 2014 10:28:54 -0600
> Alex Williamson <[email protected]> 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/0000:03:00.0/driver_override
> > echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
> > echo 0000: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/0000:03:00.0/preferred_driver
> > echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
> > echo 0000: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 <[email protected]>
> > ---
> >
> > 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
modify/add/remove a sysfs file, it needs to have documentation for it.
> 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.
greg k-h
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/0000:03:00.0/driver_override
> > > echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
> > > echo 0000: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/0000:03:00.0/preferred_driver
> > > echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
> > > echo 0000: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 <[email protected]>
> > > ---
> > >
> > > 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 <[email protected]>
> +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
Would it make sense to suggest how this is done (e.g. this must be done
by writing into ....)?
> + is currently loaded in the kernel, no match will be found.
"no match will be found" when trying to bind any driver to the device,
or? Can this be clarified?
> + This also allows devices to opt-out of driver binding using
> + a driver_override name such as "none".
>
Otherwise it's very well written.
-Christoffer
On Tue, 1 Apr 2014 17:23:24 -0700
Greg KH <[email protected]> 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 <[email protected]> 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/0000:03:00.0/driver_override
> > > echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
> > > echo 0000: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/0000:03:00.0/preferred_driver
> > > echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
> > > echo 0000: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 <[email protected]>
> > > ---
> > >
> > > 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
On Wed, 2014-04-02 at 17:06 -0500, Kim Phillips wrote:
> On Tue, 1 Apr 2014 17:23:24 -0700
> Greg KH <[email protected]> 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 <[email protected]> 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/0000:03:00.0/driver_override
> > > > echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
> > > > echo 0000: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/0000:03:00.0/preferred_driver
> > > > echo 0000:03:00.0 > /sys/bus/pci/devices/0000:03:00.0/driver/unbind
> > > > echo 0000: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 <[email protected]>
> > > > ---
> > > >
> > > > 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.
I'm not finding any common new_id code either. Only PCI and USB
implement both new_id and remove_id and they don't share code or ABI
documentation. I also don't see much advantage to trying to push this
into struct device, we could save a few lines of show/store code, but
the functionality needs to be implemented by the bus driver. The base
code can't override bus drivers like PCI that do another match lookup to
retrieve the ID table entry to pass to the driver. At best we could
have common show/store keyed from a bool on struct bus_type. Thanks,
Alex
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 <[email protected]>
---
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 0000000..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 <[email protected]>
+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;
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 16f6654..153d303 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -28,6 +28,7 @@ struct platform_device {
struct resource *resource;
const struct platform_device_id *id_entry;
+ char *driver_override; /* Driver name to force a match */
/* MFD cell pointer */
struct mfd_cell *mfd_cell;
--
1.9.1
On 04/04/2014 06:35 PM, Kim Phillips wrote:
> 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 <[email protected]>
> ---
> 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 0000000..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 <[email protected]>
> +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';
> +
Seems to me that something like
cp = strchr(driver_override, '\n');
if (cp)
*cp = '\0';
would be much simpler.
Guenter