| Message ID | 20240327-module-owner-virtio-v1-0-0feffab77d99@linaro.org |
|---|---|
| Headers | show |
| Series | virtio: store owner from modules with register_virtio_driver() | expand |
On Wed, Mar 27, 2024 at 01:40:54PM +0100, Krzysztof Kozlowski wrote: >Modules registering driver with register_virtio_driver() might forget to >set .owner field. i2c-virtio.c for example has it missing. The field >is used by some of other kernel parts for reference counting >(try_module_get()), so it is expected that drivers will set it. > >Solve the problem by moving this task away from the drivers to the core >amba bus code, just like we did for platform_driver in >commit 9447057eaff8 ("platform_device: use a macro instead of >platform_driver_register"). > >Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >--- > Documentation/driver-api/virtio/writing_virtio_drivers.rst | 1 - > drivers/virtio/virtio.c | 6 ++++-- > include/linux/virtio.h | 7 +++++-- > 3 files changed, 9 insertions(+), 5 deletions(-) > >diff --git a/Documentation/driver-api/virtio/writing_virtio_drivers.rst b/Documentation/driver-api/virtio/writing_virtio_drivers.rst >index e14c58796d25..e5de6f5d061a 100644 >--- a/Documentation/driver-api/virtio/writing_virtio_drivers.rst >+++ b/Documentation/driver-api/virtio/writing_virtio_drivers.rst >@@ -97,7 +97,6 @@ like this:: > > static struct virtio_driver virtio_dummy_driver = { > .driver.name = KBUILD_MODNAME, >- .driver.owner = THIS_MODULE, > .id_table = id_table, > .probe = virtio_dummy_probe, > .remove = virtio_dummy_remove, >diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c >index f173587893cb..9510c551dce8 100644 >--- a/drivers/virtio/virtio.c >+++ b/drivers/virtio/virtio.c >@@ -362,14 +362,16 @@ static const struct bus_type virtio_bus = { > .remove = virtio_dev_remove, > }; > >-int register_virtio_driver(struct virtio_driver *driver) >+int __register_virtio_driver(struct virtio_driver *driver, struct module *owner) > { > /* Catch this early. */ > BUG_ON(driver->feature_table_size && !driver->feature_table); > driver->driver.bus = &virtio_bus; >+ driver->driver.owner = owner; >+ `.driver.name = KBUILD_MODNAME` also seems very common, should we put that in the macro as well? > return driver_register(&driver->driver); > } >-EXPORT_SYMBOL_GPL(register_virtio_driver); >+EXPORT_SYMBOL_GPL(__register_virtio_driver); > > void unregister_virtio_driver(struct virtio_driver *driver) > { >diff --git a/include/linux/virtio.h b/include/linux/virtio.h >index b0201747a263..26c4325aa373 100644 >--- a/include/linux/virtio.h >+++ b/include/linux/virtio.h >@@ -170,7 +170,7 @@ size_t virtio_max_dma_size(const struct virtio_device *vdev); > > /** > * struct virtio_driver - operations for a virtio I/O driver >- * @driver: underlying device driver (populate name and owner). >+ * @driver: underlying device driver (populate name). > * @id_table: the ids serviced by this driver. > * @feature_table: an array of feature numbers supported by this driver. > * @feature_table_size: number of entries in the feature table array. >@@ -208,7 +208,10 @@ static inline struct virtio_driver *drv_to_virtio(struct device_driver *drv) > return container_of(drv, struct virtio_driver, driver); > } > >-int register_virtio_driver(struct virtio_driver *drv); >+/* use a macro to avoid include chaining to get THIS_MODULE */ >+#define register_virtio_driver(drv) \ >+ __register_virtio_driver(drv, THIS_MODULE) >+int __register_virtio_driver(struct virtio_driver *drv, struct module *owner); > void unregister_virtio_driver(struct virtio_driver *drv); > > /* module_virtio_driver() - Helper macro for drivers that don't do > >-- >2.34.1 >
On Wed, Mar 27, 2024 at 01:41:09PM +0100, Krzysztof Kozlowski wrote: >virtio core already sets the .owner, so driver does not need to. > >Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >--- > >Depends on the first patch. >--- > net/vmw_vsock/virtio_transport.c | 1 - > 1 file changed, 1 deletion(-) Acked-by: Stefano Garzarella <sgarzare@redhat.com> Nit: you can use "vsock/virtio: " as prefix for the commit title. > >diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c >index 1748268e0694..13f42a62b034 100644 >--- a/net/vmw_vsock/virtio_transport.c >+++ b/net/vmw_vsock/virtio_transport.c >@@ -858,7 +858,6 @@ static struct virtio_driver virtio_vsock_driver = { > .feature_table = features, > .feature_table_size = ARRAY_SIZE(features), > .driver.name = KBUILD_MODNAME, >- .driver.owner = THIS_MODULE, > .id_table = id_table, > .probe = virtio_vsock_probe, > .remove = virtio_vsock_remove, > >-- >2.34.1 >
On Fri, Mar 29, 2024 at 01:07:31PM +0100, Krzysztof Kozlowski wrote: >On 29/03/2024 12:42, Stefano Garzarella wrote: >>> }; >>> >>> -int register_virtio_driver(struct virtio_driver *driver) >>> +int __register_virtio_driver(struct virtio_driver *driver, struct module *owner) >>> { >>> /* Catch this early. */ >>> BUG_ON(driver->feature_table_size && !driver->feature_table); >>> driver->driver.bus = &virtio_bus; >>> + driver->driver.owner = owner; >>> + >> >> `.driver.name = KBUILD_MODNAME` also seems very common, should we put >> that in the macro as well? > >This is a bit different thing. Every driver is expected to set owner to >itself (THIS_MODULE), but is every driver name KBUILD_MODNAME? Nope, IIUC we have 2 exceptions: - drivers/firmware/arm_scmi/virtio.c - arch/um/drivers/virt-pci.c >Remember that this overrides whatever driver actually put there. They can call __register_virtio_driver() where we can add the `name` parameter. That said, I don't have a strong opinion, we can leave it as it is. Thanks, Stefano
Merging ======= All further patches depend on the first virtio patch, therefore please ack and this should go via one tree: virtio? Description =========== Modules registering driver with register_virtio_driver() often forget to set .owner field. Solve the problem by moving this task away from the drivers to the core amba bus code, just like we did for platform_driver in commit 9447057eaff8 ("platform_device: use a macro instead of platform_driver_register"). Best regards, Krzysztof Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- Krzysztof Kozlowski (22): virtio: store owner from modules with register_virtio_driver() um: virt-pci: drop owner assignment virtio_blk: drop owner assignment bluetooth: virtio: drop owner assignment hwrng: virtio: drop owner assignment virtio_console: drop owner assignment crypto: virtio - drop owner assignment firmware: arm_scmi: virtio: drop owner assignment gpio: virtio: drop owner assignment drm/virtio: drop owner assignment iommu: virtio: drop owner assignment misc: nsm: drop owner assignment net: caif: virtio: drop owner assignment net: virtio: drop owner assignment net: 9p: virtio: drop owner assignment net: vmw_vsock: virtio: drop owner assignment wireless: mac80211_hwsim: drop owner assignment nvdimm: virtio_pmem: drop owner assignment rpmsg: virtio: drop owner assignment scsi: virtio: drop owner assignment fuse: virtio: drop owner assignment sound: virtio: drop owner assignment Documentation/driver-api/virtio/writing_virtio_drivers.rst | 1 - arch/um/drivers/virt-pci.c | 1 - drivers/block/virtio_blk.c | 1 - drivers/bluetooth/virtio_bt.c | 1 - drivers/char/hw_random/virtio-rng.c | 1 - drivers/char/virtio_console.c | 2 -- drivers/crypto/virtio/virtio_crypto_core.c | 1 - drivers/firmware/arm_scmi/virtio.c | 1 - drivers/gpio/gpio-virtio.c | 1 - drivers/gpu/drm/virtio/virtgpu_drv.c | 1 - drivers/iommu/virtio-iommu.c | 1 - drivers/misc/nsm.c | 1 - drivers/net/caif/caif_virtio.c | 1 - drivers/net/virtio_net.c | 1 - drivers/net/wireless/virtual/mac80211_hwsim.c | 1 - drivers/nvdimm/virtio_pmem.c | 1 - drivers/rpmsg/virtio_rpmsg_bus.c | 1 - drivers/scsi/virtio_scsi.c | 1 - drivers/virtio/virtio.c | 6 ++++-- fs/fuse/virtio_fs.c | 1 - include/linux/virtio.h | 7 +++++-- net/9p/trans_virtio.c | 1 - net/vmw_vsock/virtio_transport.c | 1 - sound/virtio/virtio_card.c | 1 - 24 files changed, 9 insertions(+), 27 deletions(-) --- base-commit: 7fdcff3312e16ba8d1419f8a18f465c5cc235ecf change-id: 20240327-module-owner-virtio-546763b3ca22 Best regards,