| Message ID | 2-v1-324b2038f212+1041f1-vfio3a_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow mdev drivers to directly create the vfio_device | expand |
This looks good as-is, but a suggestions for incremental improvements below: > @@ -1032,10 +1035,10 @@ int device_driver_attach(struct device_driver *drv, struct device *dev) > __device_driver_lock(dev, dev->parent); > > /* > - * If device has been removed or someone has already successfully > - * bound a driver before us just skip the driver probe call. > + * If device someone has already successfully bound a driver before us > + * just skip the driver probe call. > */ > - if (!dev->p->dead && !dev->driver) > + if (!dev->driver) > ret = driver_probe_device(drv, dev); > > __device_driver_unlock(dev, dev->parent); It is kinda silly to keep the ->driver check here given that one caller completely ignores the return value, and the other turns a 0 return into -ENODEV anyway. So I think this check can go away, turning device_driver_attach into a trivial wrapper for driver_probe_device that adds locking, which might be useful to reflect in the naming. > @@ -1047,19 +1050,11 @@ static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie) > { > struct device *dev = _dev; > struct device_driver *drv; > - int ret = 0; > + int ret; > > __device_driver_lock(dev, dev->parent); > - > drv = dev->p->async_driver; > - > - /* > - * If device has been removed or someone has already successfully > - * bound a driver before us just skip the driver probe call. > - */ > - if (!dev->p->dead && !dev->driver) > - ret = driver_probe_device(drv, dev); > - > + ret = driver_probe_device(drv, dev); > __device_driver_unlock(dev, dev->parent); And that helper could be used here as well.
On Tue, Jun 08, 2021 at 06:59:10AM +0100, Christoph Hellwig wrote: > This looks good as-is, but a suggestions for incremental improvements > below: > > > @@ -1032,10 +1035,10 @@ int device_driver_attach(struct device_driver *drv, struct device *dev) > > __device_driver_lock(dev, dev->parent); > > > > /* > > - * If device has been removed or someone has already successfully > > - * bound a driver before us just skip the driver probe call. > > + * If device someone has already successfully bound a driver before us > > + * just skip the driver probe call. > > */ > > - if (!dev->p->dead && !dev->driver) > > + if (!dev->driver) > > ret = driver_probe_device(drv, dev); > > > > __device_driver_unlock(dev, dev->parent); > > It is kinda silly to keep the ->driver check here given that one caller > completely ignores the return value, and the other turns a 0 return into > -ENODEV anyway. Later patches revise this though, I think you noticed it > So I think this check can go away, turning device_driver_attach into > a trivial wrapper for driver_probe_device that adds locking, which > might be useful to reflect in the naming. I was scared to change this because "0 on already bound" is uAPI in sysfs at this point. And what you noticed in bind_store: dev = bus_find_device_by_name(bus, NULL, buf); if (dev && dev->driver == NULL && driver_match_device(drv, dev)) { err = device_driver_attach(drv, dev); Seems troublesome as the driver_lock isn't held for that dev->driver read. So I'm inclined to delete the above, keep the check in device_driver_attach and not change the uAPI? Or the APIs need to be more complicated to support locked and unlocked callers/etc Jason
diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 9d79a139290271..c1a92cff159873 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -733,8 +733,9 @@ EXPORT_SYMBOL_GPL(wait_for_device_probe); * @drv: driver to bind a device to * @dev: device to try to bind to the driver * - * This function returns -ENODEV if the device is not registered, - * 1 if the device is bound successfully and 0 otherwise. + * This function returns -ENODEV if the device is not registered, -EBUSY if it + * already has a driver, and 1 if the device is bound successfully and 0 + * otherwise. * * This function must be called with @dev lock held. When called for a * USB interface, @dev->parent lock must be held as well. @@ -745,8 +746,10 @@ static int driver_probe_device(struct device_driver *drv, struct device *dev) { int ret = 0; - if (!device_is_registered(dev)) + if (dev->p->dead || !device_is_registered(dev)) return -ENODEV; + if (dev->driver) + return -EBUSY; dev->can_match = true; pr_debug("bus: '%s': %s: matched device %s with driver %s\n", @@ -1032,10 +1035,10 @@ int device_driver_attach(struct device_driver *drv, struct device *dev) __device_driver_lock(dev, dev->parent); /* - * If device has been removed or someone has already successfully - * bound a driver before us just skip the driver probe call. + * If device someone has already successfully bound a driver before us + * just skip the driver probe call. */ - if (!dev->p->dead && !dev->driver) + if (!dev->driver) ret = driver_probe_device(drv, dev); __device_driver_unlock(dev, dev->parent); @@ -1047,19 +1050,11 @@ static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie) { struct device *dev = _dev; struct device_driver *drv; - int ret = 0; + int ret; __device_driver_lock(dev, dev->parent); - drv = dev->p->async_driver; - - /* - * If device has been removed or someone has already successfully - * bound a driver before us just skip the driver probe call. - */ - if (!dev->p->dead && !dev->driver) - ret = driver_probe_device(drv, dev); - + ret = driver_probe_device(drv, dev); __device_driver_unlock(dev, dev->parent); dev_dbg(dev, "driver %s async attach completed: %d\n", drv->name, ret);
Checking if the dev is dead or if the dev is already bound is a required precondition to invoking driver_probe_device(). All the call chains leading here duplicate these checks. Add it directly to driver_probe_device() so the precondition is clear and remove the checks from device_driver_attach() and __driver_attach_async_helper(). The other call chain going through __device_attach_driver() does have these same checks but they are inlined into logic higher up the call stack and can't be removed. Preserve the sysfs uAPI behavior for store of succeeding if any driver is already bound, but consistently fail if the device is unregistered or dead. Done in preparation for the next patches which will add additional callers to driver_probe_device(). Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/base/dd.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-)