| Message ID | 3-v1-7dedf20b2b75+4f785-vfio2_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make vfio_mdev type safe | expand |
> static struct mdev_driver vfio_mdev_driver = { > + .driver = { > + .name = "vfio_mdev", > + .owner = THIS_MODULE, > + .mod_name = KBUILD_MODNAME, > + }, What is the mod_name initialization for? I've not really seen that in anywere else, and the only user seems to be module_add_driver for a rather odd case we shouldn't hit here. The rest looks good to me: Reviewed-by: Christoph Hellwig <hch@lst.de>
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Wednesday, March 24, 2021 1:55 AM > > This is only done once, we don't need to generate code to initialize a > structure stored in the ELF .data segment. Fill in the three required > .driver members directly instead of copying data into them during > mdev_register_driver(). > > Further the to_mdev_driver() function doesn't belong in a public header, > just inline it into the two places that need it. Finally, we can now > clearly see that 'drv' derived from dev->driver cannot be NULL, firstly > because the driver core forbids it, and secondly because NULL won't pass > through the container_of(). Remove the dead code. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com> > --- > Documentation/driver-api/vfio-mediated-device.rst | 5 +---- > drivers/vfio/mdev/mdev_driver.c | 15 +++++++-------- > drivers/vfio/mdev/vfio_mdev.c | 8 ++++++-- > include/linux/mdev.h | 6 +----- > 4 files changed, 15 insertions(+), 19 deletions(-) > > diff --git a/Documentation/driver-api/vfio-mediated-device.rst > b/Documentation/driver-api/vfio-mediated-device.rst > index c43c1dc3333373..1779b85f014e2f 100644 > --- a/Documentation/driver-api/vfio-mediated-device.rst > +++ b/Documentation/driver-api/vfio-mediated-device.rst > @@ -98,13 +98,11 @@ structure to represent a mediated device's driver:: > > /* > * struct mdev_driver [2] - Mediated device's driver > - * @name: driver name > * @probe: called when new device created > * @remove: called when device removed > * @driver: device driver structure > */ > struct mdev_driver { > - const char *name; > int (*probe) (struct mdev_device *dev); > void (*remove) (struct mdev_device *dev); > struct device_driver driver; > @@ -115,8 +113,7 @@ to register and unregister itself with the core driver: > > * Register:: > > - extern int mdev_register_driver(struct mdev_driver *drv, > - struct module *owner); > + extern int mdev_register_driver(struct mdev_driver *drv); > > * Unregister:: > > diff --git a/drivers/vfio/mdev/mdev_driver.c > b/drivers/vfio/mdev/mdev_driver.c > index 44c3ba7e56d923..041699571b7e55 100644 > --- a/drivers/vfio/mdev/mdev_driver.c > +++ b/drivers/vfio/mdev/mdev_driver.c > @@ -39,7 +39,8 @@ static void mdev_detach_iommu(struct mdev_device > *mdev) > > static int mdev_probe(struct device *dev) > { > - struct mdev_driver *drv = to_mdev_driver(dev->driver); > + struct mdev_driver *drv = > + container_of(dev->driver, struct mdev_driver, driver); > struct mdev_device *mdev = to_mdev_device(dev); > int ret; > > @@ -47,7 +48,7 @@ static int mdev_probe(struct device *dev) > if (ret) > return ret; > > - if (drv && drv->probe) { > + if (drv->probe) { > ret = drv->probe(mdev); > if (ret) > mdev_detach_iommu(mdev); > @@ -58,10 +59,11 @@ static int mdev_probe(struct device *dev) > > static int mdev_remove(struct device *dev) > { > - struct mdev_driver *drv = to_mdev_driver(dev->driver); > + struct mdev_driver *drv = > + container_of(dev->driver, struct mdev_driver, driver); > struct mdev_device *mdev = to_mdev_device(dev); > > - if (drv && drv->remove) > + if (drv->remove) > drv->remove(mdev); > > mdev_detach_iommu(mdev); > @@ -79,16 +81,13 @@ EXPORT_SYMBOL_GPL(mdev_bus_type); > /** > * mdev_register_driver - register a new MDEV driver > * @drv: the driver to register > - * @owner: module owner of driver to be registered > * > * Returns a negative value on error, otherwise 0. > **/ > -int mdev_register_driver(struct mdev_driver *drv, struct module *owner) > +int mdev_register_driver(struct mdev_driver *drv) > { > /* initialize common driver fields */ > - drv->driver.name = drv->name; > drv->driver.bus = &mdev_bus_type; > - drv->driver.owner = owner; > > /* register with core */ > return driver_register(&drv->driver); > diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c > index 91b7b8b9eb9cb8..cc9507ed85a181 100644 > --- a/drivers/vfio/mdev/vfio_mdev.c > +++ b/drivers/vfio/mdev/vfio_mdev.c > @@ -152,14 +152,18 @@ static void vfio_mdev_remove(struct mdev_device > *mdev) > } > > static struct mdev_driver vfio_mdev_driver = { > - .name = "vfio_mdev", > + .driver = { > + .name = "vfio_mdev", > + .owner = THIS_MODULE, > + .mod_name = KBUILD_MODNAME, > + }, > .probe = vfio_mdev_probe, > .remove = vfio_mdev_remove, > }; > > static int __init vfio_mdev_init(void) > { > - return mdev_register_driver(&vfio_mdev_driver, THIS_MODULE); > + return mdev_register_driver(&vfio_mdev_driver); > } > > static void __exit vfio_mdev_exit(void) > diff --git a/include/linux/mdev.h b/include/linux/mdev.h > index 52f7ea19dd0f56..cb771c712da0f4 100644 > --- a/include/linux/mdev.h > +++ b/include/linux/mdev.h > @@ -137,21 +137,17 @@ struct mdev_type_attribute > mdev_type_attr_##_name = \ > > /** > * struct mdev_driver - Mediated device driver > - * @name: driver name > * @probe: called when new device created > * @remove: called when device removed > * @driver: device driver structure > * > **/ > struct mdev_driver { > - const char *name; > int (*probe)(struct mdev_device *dev); > void (*remove)(struct mdev_device *dev); > struct device_driver driver; > }; > > -#define to_mdev_driver(drv) container_of(drv, struct mdev_driver, driver) > - > static inline void *mdev_get_drvdata(struct mdev_device *mdev) > { > return mdev->driver_data; > @@ -170,7 +166,7 @@ extern struct bus_type mdev_bus_type; > int mdev_register_device(struct device *dev, const struct mdev_parent_ops > *ops); > void mdev_unregister_device(struct device *dev); > > -int mdev_register_driver(struct mdev_driver *drv, struct module *owner); > +int mdev_register_driver(struct mdev_driver *drv); > void mdev_unregister_driver(struct mdev_driver *drv); > > struct device *mdev_parent_dev(struct mdev_device *mdev); > -- > 2.31.0
On Tue, Mar 23, 2021 at 08:14:15PM +0100, Christoph Hellwig wrote: > > static struct mdev_driver vfio_mdev_driver = { > > + .driver = { > > + .name = "vfio_mdev", > > + .owner = THIS_MODULE, > > + .mod_name = KBUILD_MODNAME, > > + }, > > What is the mod_name initialization for? It is usually hidden and works like this: /* pci_register_driver() must be a macro so KBUILD_MODNAME can be expanded */ #define pci_register_driver(driver) \ __pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME) int __pci_register_driver(struct pci_driver *drv, struct module *owner, const char *mod_name) { drv->driver.owner = owner; drv->driver.mod_name = mod_name; > I've not really seen that in anywere else, and the only user seems > to be module_add_driver for a rather odd case we shouldn't hit here. vfio_mdev could be compiled built in? AFAICT it handles the case where THIS_MODULE==NULL so we still need to create sysfs links to the built in module. If it is left NULL then a few sysfs files go missing for the built in mode but no harm done? I think it is correct to have it Thanks, Jason
On Fri, Mar 26, 2021 at 09:10:48AM -0300, Jason Gunthorpe wrote: > It is usually hidden and works like this: > > /* pci_register_driver() must be a macro so KBUILD_MODNAME can be expanded */ > #define pci_register_driver(driver) \ > __pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME) > > int __pci_register_driver(struct pci_driver *drv, struct module *owner, > const char *mod_name) > { > drv->driver.owner = owner; > drv->driver.mod_name = mod_name; Indeed, there seem to be about two handful of instance of that. > > I've not really seen that in anywere else, and the only user seems > > to be module_add_driver for a rather odd case we shouldn't hit here. > > vfio_mdev could be compiled built in? > > AFAICT it handles the case where THIS_MODULE==NULL so we still need to > create sysfs links to the built in module. > > If it is left NULL then a few sysfs files go missing for the built in > mode but no harm done? Yes, it seems to be needed for a few driver-specific files. So it looks ok, even if rather unexpected.
On Tue, 23 Mar 2021 14:55:20 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > This is only done once, we don't need to generate code to initialize a > structure stored in the ELF .data segment. Fill in the three required > .driver members directly instead of copying data into them during > mdev_register_driver(). > > Further the to_mdev_driver() function doesn't belong in a public header, > just inline it into the two places that need it. Finally, we can now > clearly see that 'drv' derived from dev->driver cannot be NULL, firstly > because the driver core forbids it, and secondly because NULL won't pass > through the container_of(). Remove the dead code. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > Documentation/driver-api/vfio-mediated-device.rst | 5 +---- > drivers/vfio/mdev/mdev_driver.c | 15 +++++++-------- > drivers/vfio/mdev/vfio_mdev.c | 8 ++++++-- > include/linux/mdev.h | 6 +----- > 4 files changed, 15 insertions(+), 19 deletions(-) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst index c43c1dc3333373..1779b85f014e2f 100644 --- a/Documentation/driver-api/vfio-mediated-device.rst +++ b/Documentation/driver-api/vfio-mediated-device.rst @@ -98,13 +98,11 @@ structure to represent a mediated device's driver:: /* * struct mdev_driver [2] - Mediated device's driver - * @name: driver name * @probe: called when new device created * @remove: called when device removed * @driver: device driver structure */ struct mdev_driver { - const char *name; int (*probe) (struct mdev_device *dev); void (*remove) (struct mdev_device *dev); struct device_driver driver; @@ -115,8 +113,7 @@ to register and unregister itself with the core driver: * Register:: - extern int mdev_register_driver(struct mdev_driver *drv, - struct module *owner); + extern int mdev_register_driver(struct mdev_driver *drv); * Unregister:: diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c index 44c3ba7e56d923..041699571b7e55 100644 --- a/drivers/vfio/mdev/mdev_driver.c +++ b/drivers/vfio/mdev/mdev_driver.c @@ -39,7 +39,8 @@ static void mdev_detach_iommu(struct mdev_device *mdev) static int mdev_probe(struct device *dev) { - struct mdev_driver *drv = to_mdev_driver(dev->driver); + struct mdev_driver *drv = + container_of(dev->driver, struct mdev_driver, driver); struct mdev_device *mdev = to_mdev_device(dev); int ret; @@ -47,7 +48,7 @@ static int mdev_probe(struct device *dev) if (ret) return ret; - if (drv && drv->probe) { + if (drv->probe) { ret = drv->probe(mdev); if (ret) mdev_detach_iommu(mdev); @@ -58,10 +59,11 @@ static int mdev_probe(struct device *dev) static int mdev_remove(struct device *dev) { - struct mdev_driver *drv = to_mdev_driver(dev->driver); + struct mdev_driver *drv = + container_of(dev->driver, struct mdev_driver, driver); struct mdev_device *mdev = to_mdev_device(dev); - if (drv && drv->remove) + if (drv->remove) drv->remove(mdev); mdev_detach_iommu(mdev); @@ -79,16 +81,13 @@ EXPORT_SYMBOL_GPL(mdev_bus_type); /** * mdev_register_driver - register a new MDEV driver * @drv: the driver to register - * @owner: module owner of driver to be registered * * Returns a negative value on error, otherwise 0. **/ -int mdev_register_driver(struct mdev_driver *drv, struct module *owner) +int mdev_register_driver(struct mdev_driver *drv) { /* initialize common driver fields */ - drv->driver.name = drv->name; drv->driver.bus = &mdev_bus_type; - drv->driver.owner = owner; /* register with core */ return driver_register(&drv->driver); diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c index 91b7b8b9eb9cb8..cc9507ed85a181 100644 --- a/drivers/vfio/mdev/vfio_mdev.c +++ b/drivers/vfio/mdev/vfio_mdev.c @@ -152,14 +152,18 @@ static void vfio_mdev_remove(struct mdev_device *mdev) } static struct mdev_driver vfio_mdev_driver = { - .name = "vfio_mdev", + .driver = { + .name = "vfio_mdev", + .owner = THIS_MODULE, + .mod_name = KBUILD_MODNAME, + }, .probe = vfio_mdev_probe, .remove = vfio_mdev_remove, }; static int __init vfio_mdev_init(void) { - return mdev_register_driver(&vfio_mdev_driver, THIS_MODULE); + return mdev_register_driver(&vfio_mdev_driver); } static void __exit vfio_mdev_exit(void) diff --git a/include/linux/mdev.h b/include/linux/mdev.h index 52f7ea19dd0f56..cb771c712da0f4 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -137,21 +137,17 @@ struct mdev_type_attribute mdev_type_attr_##_name = \ /** * struct mdev_driver - Mediated device driver - * @name: driver name * @probe: called when new device created * @remove: called when device removed * @driver: device driver structure * **/ struct mdev_driver { - const char *name; int (*probe)(struct mdev_device *dev); void (*remove)(struct mdev_device *dev); struct device_driver driver; }; -#define to_mdev_driver(drv) container_of(drv, struct mdev_driver, driver) - static inline void *mdev_get_drvdata(struct mdev_device *mdev) { return mdev->driver_data; @@ -170,7 +166,7 @@ extern struct bus_type mdev_bus_type; int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops); void mdev_unregister_device(struct device *dev); -int mdev_register_driver(struct mdev_driver *drv, struct module *owner); +int mdev_register_driver(struct mdev_driver *drv); void mdev_unregister_driver(struct mdev_driver *drv); struct device *mdev_parent_dev(struct mdev_device *mdev);
This is only done once, we don't need to generate code to initialize a structure stored in the ELF .data segment. Fill in the three required .driver members directly instead of copying data into them during mdev_register_driver(). Further the to_mdev_driver() function doesn't belong in a public header, just inline it into the two places that need it. Finally, we can now clearly see that 'drv' derived from dev->driver cannot be NULL, firstly because the driver core forbids it, and secondly because NULL won't pass through the container_of(). Remove the dead code. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- Documentation/driver-api/vfio-mediated-device.rst | 5 +---- drivers/vfio/mdev/mdev_driver.c | 15 +++++++-------- drivers/vfio/mdev/vfio_mdev.c | 8 ++++++-- include/linux/mdev.h | 6 +----- 4 files changed, 15 insertions(+), 19 deletions(-)