| Message ID | 5-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 |
On Mon, Jun 07, 2021 at 09:55:47PM -0300, Jason Gunthorpe wrote: > This is intended as a replacement API for device_bind_driver(). It has at > least the following benefits: > > - Internal locking. Few of the users of device_bind_driver() follow the > locking rules > > - Calls device driver probe() internally. Notably this means that devm > support for probe works correctly as probe() error will call > devres_release_all() > > - struct device_driver -> dev_groups is supported > > - Simplified calling convention, no need to manually call probe(). Btw, it would be nice to convert at least one existing user of device_bind_driver to show this. Also maybe Cc all maintainers of subsystems using device_bind_driver so that they get a headsup and maybe quickly move over? > @@ -1077,6 +1079,7 @@ int device_driver_attach(struct device_driver *drv, struct device *dev) > return -EAGAIN; > return ret; > } > +EXPORT_SYMBOL_GPL(device_driver_attach); Ok, this means my earlier suggestions of a locked driver_probe_device helper needs a different name as we really don't want to expose flags and always return -EAGAIN here. So maybe rename driver_probe_device to __driver_probe_device, have a driver_probe_device around it that does the locking and keep device_driver_attach for the newly exported API. The kerneldoc for device_driver_attach is pretty sparse, it might want a little brushup as well.
On Tue, Jun 08, 2021 at 07:19:33AM +0100, Christoph Hellwig wrote: > On Mon, Jun 07, 2021 at 09:55:47PM -0300, Jason Gunthorpe wrote: > > This is intended as a replacement API for device_bind_driver(). It has at > > least the following benefits: > > > > - Internal locking. Few of the users of device_bind_driver() follow the > > locking rules > > > > - Calls device driver probe() internally. Notably this means that devm > > support for probe works correctly as probe() error will call > > devres_release_all() > > > > - struct device_driver -> dev_groups is supported > > > > - Simplified calling convention, no need to manually call probe(). > > Btw, it would be nice to convert at least one existing user of > device_bind_driver to show this. Also maybe Cc all maintainers of > subsystems using device_bind_driver so that they get a headsup and > maybe quickly move over? Sure. I would do the MDIO phy, though I cann't test that code, I'm at least familiar with it.. > > @@ -1077,6 +1079,7 @@ int device_driver_attach(struct device_driver *drv, struct device *dev) > > return -EAGAIN; > > return ret; > > } > > +EXPORT_SYMBOL_GPL(device_driver_attach); > > Ok, this means my earlier suggestions of a locked driver_probe_device > helper needs a different name as we really don't want to expose flags > and always return -EAGAIN here. So maybe rename driver_probe_device > to __driver_probe_device, have a driver_probe_device around it that > does the locking and keep device_driver_attach for the newly exported > API. I put the squashing to -EAGAIN here specifically because of this export. EPROBE_DEFER should not be returned to userspace and if I leak it out to drivers at this point then one of them will eventually do it wrong, as we saw already in sysfs bind_store Jason
diff --git a/drivers/base/base.h b/drivers/base/base.h index e5f9b7e656c34b..404db83ee5ecb5 100644 --- a/drivers/base/base.h +++ b/drivers/base/base.h @@ -152,7 +152,6 @@ extern int driver_add_groups(struct device_driver *drv, const struct attribute_group **groups); extern void driver_remove_groups(struct device_driver *drv, const struct attribute_group **groups); -int device_driver_attach(struct device_driver *drv, struct device *dev); void device_driver_detach(struct device *dev); extern char *make_class_name(const char *name, struct kobject *kobj); diff --git a/drivers/base/dd.c b/drivers/base/dd.c index edda7aad43a3f7..9a5792527326b7 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -471,6 +471,8 @@ static void driver_sysfs_remove(struct device *dev) * (It is ok to call with no other effort from a driver's probe() method.) * * This function must be called with the device lock held. + * + * Callers should prefer to use device_driver_attach() instead. */ int device_bind_driver(struct device *dev) { @@ -1077,6 +1079,7 @@ int device_driver_attach(struct device_driver *drv, struct device *dev) return -EAGAIN; return ret; } +EXPORT_SYMBOL_GPL(device_driver_attach); static void __driver_attach_async_helper(void *_dev, async_cookie_t cookie) { diff --git a/include/linux/device.h b/include/linux/device.h index 38a2071cf77685..d03544f04aa9ee 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -847,6 +847,8 @@ static inline void *dev_get_platdata(const struct device *dev) * Manual binding of a device to driver. See drivers/base/bus.c * for information on use. */ +int __must_check device_driver_attach(struct device_driver *drv, + struct device *dev); int __must_check device_bind_driver(struct device *dev); void device_release_driver(struct device *dev); int __must_check device_attach(struct device *dev);
This is intended as a replacement API for device_bind_driver(). It has at least the following benefits: - Internal locking. Few of the users of device_bind_driver() follow the locking rules - Calls device driver probe() internally. Notably this means that devm support for probe works correctly as probe() error will call devres_release_all() - struct device_driver -> dev_groups is supported - Simplified calling convention, no need to manually call probe(). The general usage is for situations that already know what driver to bind and need to ensure the bind is synchronized with other logic. Call device_driver_attach() after device_add(). If probe() returns a failure then this will be preserved up through to the error return of device_driver_attach(). Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/base/base.h | 1 - drivers/base/dd.c | 3 +++ include/linux/device.h | 2 ++ 3 files changed, 5 insertions(+), 1 deletion(-)