| Message ID | 9F6FE96B71CF29479FF1CDC8046E15036DDA62@039-SN1MPN1-002.039d.mgd.msft.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 09, 2013 at 07:02:25PM +0000, Yoder Stuart-B08248 wrote: > Have been thinking about this issue some more. As Scott mentioned, > 'wildcard' matching for a driver can be fairly done in the platform > bus driver. We could add a new flag to the platform driver struct: > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 4f8bef3..4d6cf14 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -727,6 +727,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); > > + /* the driver matches any device */ > + if (pdrv->match_any) > + return 1; > + > /* Attempt an OF style match first */ > if (of_driver_match_device(dev, drv)) > return 1; > > However, the more problematic issue is that a bus driver has no way to > differentiate from an explicit bind request via sysfs and a bind that > happened through bus probing. That was by design, nice to see I implemented it properly :) > I think something like the new flag in the snippet below would enable the platform > bus to support platform drivers that only bind on explicit request: > > diff --git a/drivers/base/bus.c b/drivers/base/bus.c > index 4c289ab..daf6d24 100644 > --- a/drivers/base/bus.c > +++ b/drivers/base/bus.c > @@ -201,7 +201,7 @@ 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 && driver_match_device(drv, dev, 1)) { Magic flags are the spawn of your favorite anti-$DIETY. I'm never going to accept that, sorry. If you really want to do something "special" for the platform bus, then do it only for the platform bus. But even then, you'll find me arguing that you really don't want to do it at all, sorry. I'm still yet to be convinced this is even an issue at all, but maybe that's just the jetlag talking... greg k-h -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-10-09 at 14:02 -0500, Yoder Stuart-B08248 wrote: > Have been thinking about this issue some more. As Scott mentioned, > 'wildcard' matching for a driver can be fairly done in the platform > bus driver. We could add a new flag to the platform driver struct: > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 4f8bef3..4d6cf14 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -727,6 +727,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); > > + /* the driver matches any device */ > + if (pdrv->match_any) > + return 1; > + > /* Attempt an OF style match first */ > if (of_driver_match_device(dev, drv)) > return 1; > > However, the more problematic issue is that a bus driver has no way to > differentiate from an explicit bind request via sysfs and a bind that > happened through bus probing. Again, I think the wildcard match should be orthogonal to "don't bind by default" as far as the mechanism goes. 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. > diff --git a/drivers/base/base.h b/drivers/base/base.h > index 2cbc677..7a15ef3 100644 > --- a/drivers/base/base.h > +++ b/drivers/base/base.h > @@ -114,9 +114,9 @@ extern void driver_detach(struct device_driver *drv); > extern int driver_probe_device(struct device_driver *drv, struct device *dev); > extern void driver_deferred_probe_del(struct device *dev); > static inline int driver_match_device(struct device_driver *drv, > - struct device *dev) > + struct device *dev, int explicit_bind) > { > - return drv->bus->match ? drv->bus->match(dev, drv) : 1; > + return drv->bus->match ? drv->bus->match(dev, drv, explicit_bind) : 1; > } > > Of, course the above change would need to be propagated to the different > bus drivers that implement the 'match' function. ...which would not be a problem with my approach, because you could handle it in the callers of driver_match_device(). -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogV29vZCBTY290dC1CMDc0 MjENCj4gU2VudDogV2VkbmVzZGF5LCBPY3RvYmVyIDA5LCAyMDEzIDI6MjIgUE0NCj4gVG86IFlv ZGVyIFN0dWFydC1CMDgyNDgNCj4gQ2M6IFdvb2QgU2NvdHQtQjA3NDIxOyBLaW0gUGhpbGxpcHM7 IENocmlzdG9mZmVyIERhbGw7IEFsZXggV2lsbGlhbXNvbjsNCj4gbGludXgta2VybmVsQHZnZXIu a2VybmVsLm9yZzsgYS5tb3Rha2lzQHZpcnR1YWxvcGVuc3lzdGVtcy5jb207DQo+IGFncmFmQHN1 c2UuZGU7IFNldGhpIFZhcnVuLUIxNjM5NTsgQmh1c2hhbiBCaGFyYXQtUjY1Nzc3Ow0KPiBwZXRl ci5tYXlkZWxsQGxpbmFyby5vcmc7IHNhbnRvc2guc2h1a2xhQGxpbmFyby5vcmc7IGt2bUB2Z2Vy Lmtlcm5lbC5vcmc7DQo+IGdyZWdraEBsaW51eGZvdW5kYXRpb24ub3JnDQo+IFN1YmplY3Q6IFJl OiBSRkM6IChyZS0pYmluZGluZyB0aGUgVkZJTyBwbGF0Zm9ybSBkcml2ZXIgdG8gYSBwbGF0Zm9y bQ0KPiBkZXZpY2UNCj4gDQo+IE9uIFdlZCwgMjAxMy0xMC0wOSBhdCAxNDowMiAtMDUwMCwgWW9k ZXIgU3R1YXJ0LUIwODI0OCB3cm90ZToNCj4gPiBIYXZlIGJlZW4gdGhpbmtpbmcgYWJvdXQgdGhp cyBpc3N1ZSBzb21lIG1vcmUuICBBcyBTY290dCBtZW50aW9uZWQsDQo+ID4gJ3dpbGRjYXJkJyBt YXRjaGluZyBmb3IgYSBkcml2ZXIgY2FuIGJlIGZhaXJseSBkb25lIGluIHRoZSBwbGF0Zm9ybQ0K PiA+IGJ1cyBkcml2ZXIuICBXZSBjb3VsZCBhZGQgYSBuZXcgZmxhZyB0byB0aGUgcGxhdGZvcm0g ZHJpdmVyIHN0cnVjdDoNCj4gPg0KPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2Jhc2UvcGxhdGZv cm0uYyBiL2RyaXZlcnMvYmFzZS9wbGF0Zm9ybS5jDQo+ID4gaW5kZXggNGY4YmVmMy4uNGQ2Y2Yx NCAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL2Jhc2UvcGxhdGZvcm0uYw0KPiA+ICsrKyBiL2Ry aXZlcnMvYmFzZS9wbGF0Zm9ybS5jDQo+ID4gQEAgLTcyNyw2ICs3MjcsMTAgQEAgc3RhdGljIGlu dCBwbGF0Zm9ybV9tYXRjaChzdHJ1Y3QgZGV2aWNlICpkZXYsDQo+IHN0cnVjdCBkZXZpY2VfZHJp dmVyICpkcnYpDQo+ID4gICAgICAgICBzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2ID0gdG9f cGxhdGZvcm1fZGV2aWNlKGRldik7DQo+ID4gICAgICAgICBzdHJ1Y3QgcGxhdGZvcm1fZHJpdmVy ICpwZHJ2ID0gdG9fcGxhdGZvcm1fZHJpdmVyKGRydik7DQo+ID4NCj4gPiArICAgICAgIC8qIHRo ZSBkcml2ZXIgbWF0Y2hlcyBhbnkgZGV2aWNlICovDQo+ID4gKyAgICAgICBpZiAocGRydi0+bWF0 Y2hfYW55KQ0KPiA+ICsgICAgICAgICAgICAgICByZXR1cm4gMTsNCj4gPiArDQo+ID4gICAgICAg ICAvKiBBdHRlbXB0IGFuIE9GIHN0eWxlIG1hdGNoIGZpcnN0ICovDQo+ID4gICAgICAgICBpZiAo b2ZfZHJpdmVyX21hdGNoX2RldmljZShkZXYsIGRydikpDQo+ID4gICAgICAgICAgICAgICAgIHJl dHVybiAxOw0KPiA+DQo+ID4gSG93ZXZlciwgdGhlIG1vcmUgcHJvYmxlbWF0aWMgaXNzdWUgaXMg dGhhdCBhIGJ1cyBkcml2ZXIgaGFzIG5vIHdheSB0bw0KPiA+IGRpZmZlcmVudGlhdGUgZnJvbSBh biBleHBsaWNpdCBiaW5kIHJlcXVlc3QgdmlhIHN5c2ZzIGFuZCBhIGJpbmQgdGhhdA0KPiA+IGhh cHBlbmVkIHRocm91Z2ggYnVzIHByb2JpbmcuDQo+IA0KPiBBZ2FpbiwgSSB0aGluayB0aGUgd2ls ZGNhcmQgbWF0Y2ggc2hvdWxkIGJlIG9ydGhvZ29uYWwgdG8gImRvbid0IGJpbmQgYnkNCj4gZGVm YXVsdCIgYXMgZmFyIGFzIHRoZSBtZWNoYW5pc20gZ29lcy4NCj4gDQo+IFRoZXJlJ3MgYWxyZWFk eSBhICJib29sIHN1cHByZXNzX2JpbmRfYXR0cnMiIHRvIHByZXZlbnQgc3lzZnMNCj4gYmluZC91 bmJpbmQuICBJIHN1Z2dlc3RlZCBhIHNpbWlsYXIgZmxhZyB0byBtZWFuIHRoZSBvcHBzb3NpdGUg LS0gYmluZA0KPiAqb25seSogdGhyb3VnaCBzeXNmcy4gIEdyZWcgS0ggd2FzIHNrZXB0aWNhbCBh bmQgd2FudGVkIHRvIHNlZSBhIHBhdGNoDQo+IGJlZm9yZSBhbnkgZnVydGhlciBkaXNjdXNzaW9u Lg0KDQpBaCwgdGhpbmsgSSB1bmRlcnN0YW5kIG5vdy4uLnllcyB0aGF0IHdvcmtzIGFzIHdlbGws IGFuZCB3b3VsZCBiZQ0KbGVzcyBpbnRydXN0aXZlLiAgIFNvIGFyZSB5b3Ugd3JpdGluZyBhIHBh dGNoPyA6KQ0KDQpJdCB3b3VsZCBiZSBzb21ldGhpbmcgbGlrZSB0aGlzLCByaWdodD8NCg0KZGlm ZiAtLWdpdCBhL2RyaXZlcnMvYmFzZS9kZC5jIGIvZHJpdmVycy9iYXNlL2RkLmMNCmluZGV4IDM1 ZmEzNjguLmM5YTYxZWEgMTAwNjQ0DQotLS0gYS9kcml2ZXJzL2Jhc2UvZGQuYw0KKysrIGIvZHJp dmVycy9iYXNlL2RkLmMNCkBAIC0zODksNyArMzg5LDcgQEAgc3RhdGljIGludCBfX2RldmljZV9h dHRhY2goc3RydWN0IGRldmljZV9kcml2ZXIgKmRydiwgdm9pZCAqZGF0YSkNCiB7DQogICAgICAg IHN0cnVjdCBkZXZpY2UgKmRldiA9IGRhdGE7DQoNCi0gICAgICAgaWYgKCFkcml2ZXJfbWF0Y2hf ZGV2aWNlKGRydiwgZGV2KSkNCisgICAgICAgaWYgKCFkcnYtPmV4cGxpY2l0X2JpbmRfb25seSAm JiAhZHJpdmVyX21hdGNoX2RldmljZShkcnYsIGRldikpDQogICAgICAgICAgICAgICAgcmV0dXJu IDA7DQoNCiAgICAgICAgcmV0dXJuIGRyaXZlcl9wcm9iZV9kZXZpY2UoZHJ2LCBkZXYpOw0KQEAg LTQ1MCw3ICs0NTAsNyBAQCBzdGF0aWMgaW50IF9fZHJpdmVyX2F0dGFjaChzdHJ1Y3QgZGV2aWNl ICpkZXYsIHZvaWQgKmRhdGEpDQogICAgICAgICAqIGlzIGFuIGVycm9yLg0KICAgICAgICAgKi8N Cg0KLSAgICAgICBpZiAoIWRyaXZlcl9tYXRjaF9kZXZpY2UoZHJ2LCBkZXYpKQ0KKyAgICAgICBp ZiAoIWRydi0+ZXhwbGljaXRfYmluZF9vbmx5ICYmICFkcml2ZXJfbWF0Y2hfZGV2aWNlKGRydiwg ZGV2KSkNCiAgICAgICAgICAgICAgICByZXR1cm4gMDsNCg0KICAgICAgICBpZiAoZGV2LT5wYXJl bnQpICAgICAgICAvKiBOZWVkZWQgZm9yIFVTQiAqLw0KZGlmZiAtLWdpdCBhL2luY2x1ZGUvbGlu dXgvZGV2aWNlLmggYi9pbmNsdWRlL2xpbnV4L2RldmljZS5oDQppbmRleCAyYTlkNmVkLi5mMmJl OTgwIDEwMDY0NA0KLS0tIGEvaW5jbHVkZS9saW51eC9kZXZpY2UuaA0KKysrIGIvaW5jbHVkZS9s aW51eC9kZXZpY2UuaA0KQEAgLTIzNiw2ICsyMzYsNyBAQCBzdHJ1Y3QgZGV2aWNlX2RyaXZlciB7 DQogICAgICAgIGNvbnN0IGNoYXIgICAgICAgICAgICAgICptb2RfbmFtZTsgICAgICAvKiB1c2Vk IGZvciBidWlsdC1pbiBtb2R1bGVzICovDQoNCiAgICAgICAgYm9vbCBzdXBwcmVzc19iaW5kX2F0 dHJzOyAgICAgICAvKiBkaXNhYmxlcyBiaW5kL3VuYmluZCB2aWEgc3lzZnMgKi8NCisgICAgICAg Ym9vbCBleHBsaWNpdF9iaW5kX29ubHk7ICAgICAgICAvKiBlbmFibGVzIGJpbmQvdW5iaW5kIHZp YSBzeXNmcyBvbmx5ICovDQoNCiAgICAgICAgY29uc3Qgc3RydWN0IG9mX2RldmljZV9pZCAgICAg ICAqb2ZfbWF0Y2hfdGFibGU7DQogICAgICAgIGNvbnN0IHN0cnVjdCBhY3BpX2RldmljZV9pZCAg ICAgKmFjcGlfbWF0Y2hfdGFibGU7DQoNCg0KU3R1YXJ0DQoNCg0K -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-10-09 at 12:16 -0700, gregkh@linuxfoundation.org wrote: > On Wed, Oct 09, 2013 at 07:02:25PM +0000, Yoder Stuart-B08248 wrote: > > Have been thinking about this issue some more. As Scott mentioned, > > 'wildcard' matching for a driver can be fairly done in the platform > > bus driver. We could add a new flag to the platform driver struct: > > > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > > index 4f8bef3..4d6cf14 100644 > > --- a/drivers/base/platform.c > > +++ b/drivers/base/platform.c > > @@ -727,6 +727,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); > > > > + /* the driver matches any device */ > > + if (pdrv->match_any) > > + return 1; > > + > > /* Attempt an OF style match first */ > > if (of_driver_match_device(dev, drv)) > > return 1; > > > > However, the more problematic issue is that a bus driver has no way to > > differentiate from an explicit bind request via sysfs and a bind that > > happened through bus probing. > > That was by design, nice to see I implemented it properly :) > > > I think something like the new flag in the snippet below would enable the platform > > bus to support platform drivers that only bind on explicit request: > > > > diff --git a/drivers/base/bus.c b/drivers/base/bus.c > > index 4c289ab..daf6d24 100644 > > --- a/drivers/base/bus.c > > +++ b/drivers/base/bus.c > > @@ -201,7 +201,7 @@ 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 && driver_match_device(drv, dev, 1)) { > > Magic flags are the spawn of your favorite anti-$DIETY. I'm never going > to accept that, sorry. > > If you really want to do something "special" for the platform bus, then > do it only for the platform bus. But even then, you'll find me arguing > that you really don't want to do it at all, sorry. It's not (or shouldn't be) special for the platform bus. The "don't bind by default" flag (note that I am *not* referring to the above code snippet) would be useful for VFIO PCI as well, as it would replace the hacky and racy usage of new_id. > I'm still yet to be convinced this is even an issue at all, but maybe > that's just the jetlag talking... If this is because you think we should use new_id, we just had a discussion in this thread about why that's no good. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote: > > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: Wednesday, October 09, 2013 2:22 PM > > To: Yoder Stuart-B08248 > > Cc: Wood Scott-B07421; Kim Phillips; Christoffer Dall; Alex Williamson; > > linux-kernel@vger.kernel.org; a.motakis@virtualopensystems.com; > > agraf@suse.de; Sethi Varun-B16395; Bhushan Bharat-R65777; > > peter.maydell@linaro.org; santosh.shukla@linaro.org; kvm@vger.kernel.org; > > gregkh@linuxfoundation.org > > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform > > device > > > > On Wed, 2013-10-09 at 14:02 -0500, Yoder Stuart-B08248 wrote: > > > Have been thinking about this issue some more. As Scott mentioned, > > > 'wildcard' matching for a driver can be fairly done in the platform > > > bus driver. We could add a new flag to the platform driver struct: > > > > > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > > > index 4f8bef3..4d6cf14 100644 > > > --- a/drivers/base/platform.c > > > +++ b/drivers/base/platform.c > > > @@ -727,6 +727,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); > > > > > > + /* the driver matches any device */ > > > + if (pdrv->match_any) > > > + return 1; > > > + > > > /* Attempt an OF style match first */ > > > if (of_driver_match_device(dev, drv)) > > > return 1; > > > > > > However, the more problematic issue is that a bus driver has no way to > > > differentiate from an explicit bind request via sysfs and a bind that > > > happened through bus probing. > > > > Again, I think the wildcard match should be orthogonal to "don't bind by > > default" as far as the mechanism goes. > > > > 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? > It would be something like this, right? > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 35fa368..c9a61ea 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->explicit_bind_only && !driver_match_device(drv, dev)) > return 0; if (drv->explicit_bind_only || !driver_match_device(drv, dev)) return 0; > return driver_probe_device(drv, dev); > @@ -450,7 +450,7 @@ static int __driver_attach(struct device *dev, void *data) > * is an error. > */ > > - if (!driver_match_device(drv, dev)) > + if (!drv->explicit_bind_only && !driver_match_device(drv, dev)) > return 0; Likewise -- or error out earlier in driver_attach(). Otherwise, that looks about right, for the driver side (though driver_attach could error out earlier rather than testing it inside the loop). 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". -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Thursday, October 10, 2013 1:33 AM > To: Yoder Stuart-B08248 > Cc: Wood Scott-B07421; Kim Phillips; Christoffer Dall; Alex Williamson; linux- > kernel@vger.kernel.org; a.motakis@virtualopensystems.com; agraf@suse.de; Sethi > Varun-B16395; Bhushan Bharat-R65777; peter.maydell@linaro.org; > santosh.shukla@linaro.org; kvm@vger.kernel.org; gregkh@linuxfoundation.org > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform device > > On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote: > > > > > -----Original Message----- > > > From: Wood Scott-B07421 > > > Sent: Wednesday, October 09, 2013 2:22 PM > > > To: Yoder Stuart-B08248 > > > Cc: Wood Scott-B07421; Kim Phillips; Christoffer Dall; Alex > > > Williamson; linux-kernel@vger.kernel.org; > > > a.motakis@virtualopensystems.com; agraf@suse.de; Sethi Varun-B16395; > > > Bhushan Bharat-R65777; peter.maydell@linaro.org; > > > santosh.shukla@linaro.org; kvm@vger.kernel.org; > > > gregkh@linuxfoundation.org > > > Subject: Re: RFC: (re-)binding the VFIO platform driver to a > > > platform device > > > > > > On Wed, 2013-10-09 at 14:02 -0500, Yoder Stuart-B08248 wrote: > > > > Have been thinking about this issue some more. As Scott > > > > mentioned, 'wildcard' matching for a driver can be fairly done in > > > > the platform bus driver. We could add a new flag to the platform driver > struct: > > > > > > > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > > > > index 4f8bef3..4d6cf14 100644 > > > > --- a/drivers/base/platform.c > > > > +++ b/drivers/base/platform.c > > > > @@ -727,6 +727,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); > > > > > > > > + /* the driver matches any device */ > > > > + if (pdrv->match_any) > > > > + return 1; > > > > + > > > > /* Attempt an OF style match first */ > > > > if (of_driver_match_device(dev, drv)) > > > > return 1; > > > > > > > > However, the more problematic issue is that a bus driver has no > > > > way to differentiate from an explicit bind request via sysfs and a > > > > bind that happened through bus probing. > > > > > > Again, I think the wildcard match should be orthogonal to "don't > > > bind by default" as far as the mechanism goes. > > > > > > 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? I wish I could have but I do not have vfio-platform stuff. > > > It would be something like this, right? > > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c index > > 35fa368..c9a61ea 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->explicit_bind_only && !driver_match_device(drv, > > + dev)) > > return 0; > > if (drv->explicit_bind_only || !driver_match_device(drv, dev)) > return 0; Scott, I am trying to understand what you are proposing here (example "DEVICE" can be handled by "DRIVER1" and "VFIO-PLATFORM-DRIVER"): - By default drv->explicit_bind_only will be clear in all drivers. - By default device->explicit_bind_only will also be clear for all devices. - On boot, matching devices will bound to the respective driver (DEVICE >==> DRIVER1). This will never bound with VFIO-PLATFORM-DRIVER. So far same as before. - Via Sysfs interface set drv->explicit_bind_only for VFIO-PLATFORM-DRIVER. - Then for the devices user want, set device->explicit_bind_only. - unbind DEVICE from DRIVER1 - bind DEVICE with VFIO-PLATFORM-DRIVER. This time it will be successful because (device->explicit_bind_only && drv->explicit_bind_only) is set. - Now when done, unbind the DEVICE from VFIO-PLATFORM-DRIVER. - Now user can re-bind the device with either DRIVER1 or VFIO-PLATFORM-DRIVER. - Now once drv->explicit_bind_only is set in VFIO-PLATFORM-DRIVER, and a new device comes (device - hotplug) then can gets bound to matching drive and not with VFIO-PLATFORM-DRIVER. This looks ok to me :) Thanks -Bharat > > > return driver_probe_device(drv, dev); @@ -450,7 +450,7 @@ > > static int __driver_attach(struct device *dev, void *data) > > * is an error. > > */ > > > > - if (!driver_match_device(drv, dev)) > > + if (!drv->explicit_bind_only && !driver_match_device(drv, > > + dev)) > > return 0; > > Likewise -- or error out earlier in driver_attach(). > > Otherwise, that looks about right, for the driver side (though driver_attach > could error out earlier rather than testing it inside the loop). > > 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". > > -Scott >
> I am trying to understand what you are proposing here (example "DEVICE" > can be handled by "DRIVER1" and "VFIO-PLATFORM-DRIVER"): > - By default drv->explicit_bind_only will be clear in all drivers. > - By default device->explicit_bind_only will also be clear for all > devices. > - On boot, matching devices will bound to the respective driver (DEVICE > >==> DRIVER1). > This will never bound with VFIO-PLATFORM-DRIVER. So far same as > before. > - Via Sysfs interface set drv->explicit_bind_only for VFIO-PLATFORM- > DRIVER. No. VFIO-PLATFORM-DRIVER is _always_ explicit_bind_only and thus will be statically set in the driver. See Kim's patch. > - Then for the devices user want, set device->explicit_bind_only. > - unbind DEVICE from DRIVER1 > - bind DEVICE with VFIO-PLATFORM-DRIVER. This time it will be successful > because (device->explicit_bind_only && drv->explicit_bind_only) is set. > - Now when done, unbind the DEVICE from VFIO-PLATFORM-DRIVER. > - Now user can re-bind the device with either DRIVER1 or VFIO-PLATFORM- > DRIVER. > - Now once drv->explicit_bind_only is set in VFIO-PLATFORM-DRIVER, and a > new device comes (device - hotplug) then can gets bound to matching drive > and not with VFIO-PLATFORM-DRIVER. Otherwise, it looks correct to me. Stuart
On Thu, 2013-10-10 at 02:45 -0500, Bhushan Bharat-R65777 wrote: > > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: Thursday, October 10, 2013 1:33 AM > > To: Yoder Stuart-B08248 > > Cc: Wood Scott-B07421; Kim Phillips; Christoffer Dall; Alex Williamson; linux- > > kernel@vger.kernel.org; a.motakis@virtualopensystems.com; agraf@suse.de; Sethi > > Varun-B16395; Bhushan Bharat-R65777; peter.maydell@linaro.org; > > santosh.shukla@linaro.org; kvm@vger.kernel.org; gregkh@linuxfoundation.org > > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform device > > > > On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote: > > > 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? > > I wish I could have but I do not have vfio-platform stuff. VFIO PCI without new_id would also be a useful test. -Scott -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Thursday, October 10, 2013 8:53 PM > To: Bhushan Bharat-R65777 > Cc: Wood Scott-B07421; Yoder Stuart-B08248; Kim Phillips; Christoffer Dall; Alex > Williamson; linux-kernel@vger.kernel.org; a.motakis@virtualopensystems.com; > agraf@suse.de; Sethi Varun-B16395; peter.maydell@linaro.org; > santosh.shukla@linaro.org; kvm@vger.kernel.org; gregkh@linuxfoundation.org > Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform device > > On Thu, 2013-10-10 at 02:45 -0500, Bhushan Bharat-R65777 wrote: > > > > > -----Original Message----- > > > From: Wood Scott-B07421 > > > Sent: Thursday, October 10, 2013 1:33 AM > > > To: Yoder Stuart-B08248 > > > Cc: Wood Scott-B07421; Kim Phillips; Christoffer Dall; Alex > > > Williamson; linux- kernel@vger.kernel.org; > > > a.motakis@virtualopensystems.com; agraf@suse.de; Sethi Varun-B16395; > > > Bhushan Bharat-R65777; peter.maydell@linaro.org; > > > santosh.shukla@linaro.org; kvm@vger.kernel.org; > > > gregkh@linuxfoundation.org > > > Subject: Re: RFC: (re-)binding the VFIO platform driver to a > > > platform device > > > > > > On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote: > > > > 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? > > > > I wish I could have but I do not have vfio-platform stuff. > > VFIO PCI without new_id would also be a useful test. I will do that :) -Bharat > > -Scott >
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 4f8bef3..4d6cf14 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -727,6 +727,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); + /* the driver matches any device */ + if (pdrv->match_any) + return 1; + /* Attempt an OF style match first */ if (of_driver_match_device(dev, drv)) return 1; However, the more problematic issue is that a bus driver has no way to differentiate from an explicit bind request via sysfs and a bind that happened through bus probing. I think something like the new flag in the snippet below would enable the platform bus to support platform drivers that only bind on explicit request: diff --git a/drivers/base/bus.c b/drivers/base/bus.c index 4c289ab..daf6d24 100644 --- a/drivers/base/bus.c +++ b/drivers/base/bus.c @@ -201,7 +201,7 @@ 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 && driver_match_device(drv, dev, 1)) { 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..bb53785 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 (!driver_match_device(drv, dev, 0)) return 0; return driver_probe_device(drv, dev); @@ -450,7 +450,7 @@ static int __driver_attach(struct device *dev, void *data) * is an error. */ - if (!driver_match_device(drv, dev)) + if (!driver_match_device(drv, dev, 0)) return 0; if (dev->parent) /* Needed for USB */ diff --git a/drivers/base/base.h b/drivers/base/base.h index 2cbc677..7a15ef3 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -114,9 +114,9 @@ extern void driver_detach(struct device_driver *drv); extern int driver_probe_device(struct device_driver *drv, struct device *dev); extern void driver_deferred_probe_del(struct device *dev); static inline int driver_match_device(struct device_driver *drv, - struct device *dev) + struct device *dev, int explicit_bind) { - return drv->bus->match ? drv->bus->match(dev, drv) : 1; + return drv->bus->match ? drv->bus->match(dev, drv, explicit_bind) : 1; } Of, course the above change would need to be propagated to the different