> -----Original Message----- > From: kvm-owner@xxxxxxxxxxxxxxx [mailto:kvm-owner@xxxxxxxxxxxxxxx] On Behalf Of > Kim Phillips > Sent: Thursday, October 10, 2013 8:36 AM > To: Wood Scott-B07421 > Cc: Yoder Stuart-B08248; Wood Scott-B07421; christoffer.dall@xxxxxxxxxx; > alex.williamson@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > a.motakis@xxxxxxxxxxxxxxxxxxxxxx; agraf@xxxxxxx; Sethi Varun-B16395; Bhushan > Bharat-R65777; peter.maydell@xxxxxxxxxx; santosh.shukla@xxxxxxxxxx; > kvm@xxxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform device > > On Wed, 9 Oct 2013 15:03:19 -0500 > Scott Wood <scottwood@xxxxxxxxxxxxx> wrote: > > > On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote: > > > > From: Wood Scott-B07421 > > > > Sent: Wednesday, October 09, 2013 2:22 PM > > > > > > > > On Wed, 2013-10-09 at 14:02 -0500, Yoder Stuart-B08248 wrote: > > > > > Have been thinking about this issue some more. As Scott > > > > > mentioned, > > thanks for bringing this up again. > > > > > There's already a "bool suppress_bind_attrs" to prevent sysfs > > > > bind/unbind. I suggested a similar flag to mean the oppsosite -- > > > > bind > > > > *only* through sysfs. Greg KH was skeptical and wanted to see a > > > > patch before any further discussion. > > > > > > Ah, think I understand now...yes that works as well, and would be > > > less intrustive. So are you writing a patch? :) > > > > I've been meaning to since the previous round of discussion, but I've > > been busy. Would someone else be able to test it in the context of > > using it for VFIO? > > yes - see below. > > > Otherwise, that looks about right, for the driver side (though > > driver_attach could error out earlier rather than testing it inside > > the loop). > > I've made the changes you suggested and tested the resulting diff below on an > arndale board. I successfully performed the following sequence of commands > after first changing the i2c@12C80000 node in the device tree to be exclusively > compatible with "vfio": > > === > # ls -l /sys/bus/platform/drivers/vfio-platform/ > total 0 > --w------- 1 root root 4096 Sep 24 19:17 bind > --w------- 1 root root 4096 Sep 24 19:13 uevent > --w------- 1 root root 4096 Sep 24 19:18 unbind # ls -l > /sys/bus/platform/drivers/s3c-i2c total 0 > lrwxrwxrwx 1 root root 0 Sep 24 19:11 12c60000.i2c -> > ../../../../devices/12c60000.i2c > lrwxrwxrwx 1 root root 0 Sep 24 19:11 12c90000.i2c -> > ../../../../devices/12c90000.i2c > lrwxrwxrwx 1 root root 0 Sep 24 19:20 12ce0000.i2c -> > ../../../../devices/12ce0000.i2c > --w------- 1 root root 4096 Sep 24 19:18 bind > --w------- 1 root root 4096 Sep 24 19:11 uevent > --w------- 1 root root 4096 Sep 24 19:17 unbind # ls -l > /sys/devices/12c80000.i2c/driver # this is the one with the 'vfio' compatible > ls: cannot access /sys/devices/12c80000.i2c/driver: No such file or directory # > ls -l /sys/devices/12ce0000.i2c/driver lrwxrwxrwx 1 root root 0 Sep 24 19:18 > /sys/devices/12ce0000.i2c/driver -> ../../bus/platform/drivers/s3c-i2c > # echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/unbind > # ls -l /sys/devices/12ce0000.i2c/driver > ls: cannot access /sys/devices/12ce0000.i2c/driver: No such file or directory # > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind > # ls -l /sys/devices/12ce0000.i2c/driver lrwxrwxrwx 1 root root 0 Sep 24 19:21 > /sys/devices/12ce0000.i2c/driver -> ../../bus/platform/drivers/vfio-platform > # echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/unbind > # ls -l /sys/devices/12ce0000.i2c/driver # echo 12ce0000.i2c > > /sys/bus/platform/drivers/s3c-i2c/bind > [ 722.137524] s3c-i2c 12ce0000.i2c: slave address 0x38 [ 722.141037] s3c-i2c > 12ce0000.i2c: bus frequency set to 65 KHz [ 722.150605] s3c-i2c 12ce0000.i2c: > i2c-8: S3C I2C adapter # ls -l /sys/devices/12ce0000.i2c/driver lrwxrwxrwx 1 > root root 0 Sep 24 19:21 /sys/devices/12ce0000.i2c/driver -> > ../../bus/platform/drivers/s3c-i2c > # > ==== > > so it's correctly not allowing 'vfio' driver to bind to a device tree compatible > it's declared, and it then can bind the i2c @ 12ce0000 device to the vfio- > platform driver, and unbind and bind it back to the i2c driver. > > For clarity's sake, before this diff, the command: > > echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind > > would error with: > > echo: write error: No such device > > > The other half of fixing the raciness is to ensure that the device > > doesn't get bound back to a non-VFIO driver (e.g. due to a module load > > or new_id). The solution I proposed for that was a similar > > explicit-bind-only flag for a device, that the user sets through sysfs > > prior to unbinding. This would also be useful in non-VFIO contexts to > > simply say "I don't want to use this device at all". > > I can take a look at doing this if you're still busy. > > Thanks, > > Kim > > diff --git a/drivers/base/bus.c b/drivers/base/bus.c index 73f6c29..da81442 > 100644 > --- a/drivers/base/bus.c > +++ b/drivers/base/bus.c > @@ -201,7 +201,8 @@ static ssize_t bind_store(struct device_driver *drv, const > char *buf, > int err = -ENODEV; > > dev = bus_find_device_by_name(bus, NULL, buf); > - if (dev && dev->driver == NULL && driver_match_device(drv, dev)) { > + if (dev && dev->driver == NULL && (drv->sysfs_bind_only || > + driver_match_device(drv, dev))) { Should not we check if (dev && dev->driver == NULL && (device->explicit_bind_only && drv->explicit_bind_only) || driver_match_device(drv, dev))) > if (dev->parent) /* Needed for USB */ > device_lock(dev->parent); > device_lock(dev); > diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 35fa368..6f85279 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -389,7 +389,7 @@ static int __device_attach(struct device_driver *drv, void > *data) { > struct device *dev = data; > > - if (!driver_match_device(drv, dev)) > + if (drv->sysfs_bind_only || !driver_match_device(drv, dev)) Likewise .. Thanks -Bharat > return 0; > > return driver_probe_device(drv, dev); > @@ -476,6 +476,9 @@ static int __driver_attach(struct device *dev, void *data) > */ > int driver_attach(struct device_driver *drv) { > + if (drv->sysfs_bind_only) > + return 0; > + > return bus_for_each_dev(drv->bus, NULL, drv, __driver_attach); } > EXPORT_SYMBOL_GPL(driver_attach); diff --git a/drivers/vfio/vfio_platform.c > b/drivers/vfio/vfio_platform.c index b92d7bb..ba578b2 100644 > --- a/drivers/vfio/vfio_platform.c > +++ b/drivers/vfio/vfio_platform.c > @@ -297,6 +297,7 @@ static struct platform_driver vfio_platform_driver = { > .name = "vfio-platform", > .owner = THIS_MODULE, > .of_match_table = vfio_platform_match, > + .sysfs_bind_only = true, > }, > }; > > diff --git a/include/linux/device.h b/include/linux/device.h index > 94638ef..a3ae81e 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -199,6 +199,7 @@ extern struct klist *bus_get_device_klist(struct bus_type > *bus); > * @owner: The module owner. > * @mod_name: Used for built-in modules. > * @suppress_bind_attrs: Disables bind/unbind via sysfs. > + * @sysfs_bind_only: Only allow bind/unbind via sysfs. > * @of_match_table: The open firmware table. > * @acpi_match_table: The ACPI match table. > * @probe: Called to query the existence of a specific device, > @@ -232,6 +233,7 @@ struct device_driver { > const char *mod_name; /* used for built-in modules */ > > bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ > + bool sysfs_bind_only; /* only allow bind/unbind via sysfs */ > > const struct of_device_id *of_match_table; > const struct acpi_device_id *acpi_match_table; > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a > message to majordomo@xxxxxxxxxxxxxxx More majordomo info at > http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
