| 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 27-03-24, 13:41, 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. > --- > drivers/gpio/gpio-virtio.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpio/gpio-virtio.c b/drivers/gpio/gpio-virtio.c > index fcc5e8c08973..9fae8e396c58 100644 > --- a/drivers/gpio/gpio-virtio.c > +++ b/drivers/gpio/gpio-virtio.c > @@ -653,7 +653,6 @@ static struct virtio_driver virtio_gpio_driver = { > .remove = virtio_gpio_remove, > .driver = { > .name = KBUILD_MODNAME, > - .owner = THIS_MODULE, > }, > }; > module_virtio_driver(virtio_gpio_driver); Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On Wed, Mar 27, 2024 at 01:41:12PM +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. > --- > drivers/rpmsg/virtio_rpmsg_bus.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c > index 1062939c3264..e9e8c1f7829f 100644 > --- a/drivers/rpmsg/virtio_rpmsg_bus.c > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c > @@ -1053,7 +1053,6 @@ static struct virtio_driver virtio_ipc_driver = { > .feature_table = features, > .feature_table_size = ARRAY_SIZE(features), > .driver.name = KBUILD_MODNAME, > - .driver.owner = THIS_MODULE, > .id_table = id_table, > .probe = rpmsg_probe, > .remove = rpmsg_remove, Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > -- > 2.34.1 >
On Wed, Mar 27, 2024 at 1:45 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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. > --- > drivers/gpio/gpio-virtio.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpio/gpio-virtio.c b/drivers/gpio/gpio-virtio.c > index fcc5e8c08973..9fae8e396c58 100644 > --- a/drivers/gpio/gpio-virtio.c > +++ b/drivers/gpio/gpio-virtio.c > @@ -653,7 +653,6 @@ static struct virtio_driver virtio_gpio_driver = { > .remove = virtio_gpio_remove, > .driver = { > .name = KBUILD_MODNAME, > - .owner = THIS_MODULE, > }, > }; > module_virtio_driver(virtio_gpio_driver); > > -- > 2.34.1 > Applied, thanks! Bart
On Fri, Mar 29, 2024 at 11:27:19AM +0100, Bartosz Golaszewski wrote: >On Wed, Mar 27, 2024 at 1:45 PM Krzysztof Kozlowski ><krzysztof.kozlowski@linaro.org> 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. >> --- >> drivers/gpio/gpio-virtio.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/drivers/gpio/gpio-virtio.c b/drivers/gpio/gpio-virtio.c >> index fcc5e8c08973..9fae8e396c58 100644 >> --- a/drivers/gpio/gpio-virtio.c >> +++ b/drivers/gpio/gpio-virtio.c >> @@ -653,7 +653,6 @@ static struct virtio_driver virtio_gpio_driver = { >> .remove = virtio_gpio_remove, >> .driver = { >> .name = KBUILD_MODNAME, >> - .owner = THIS_MODULE, >> }, >> }; >> module_virtio_driver(virtio_gpio_driver); >> >> -- >> 2.34.1 >> > >Applied, thanks! Did you also applied the first patch of this series? Without that I'm not sure it's a good idea to apply this patch as also Krzysztof mentioned after ---. Thanks, Stefano
On 29/03/2024 11:27, Bartosz Golaszewski wrote: > On Wed, Mar 27, 2024 at 1:45 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> 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. >> --- >> drivers/gpio/gpio-virtio.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/drivers/gpio/gpio-virtio.c b/drivers/gpio/gpio-virtio.c >> index fcc5e8c08973..9fae8e396c58 100644 >> --- a/drivers/gpio/gpio-virtio.c >> +++ b/drivers/gpio/gpio-virtio.c >> @@ -653,7 +653,6 @@ static struct virtio_driver virtio_gpio_driver = { >> .remove = virtio_gpio_remove, >> .driver = { >> .name = KBUILD_MODNAME, >> - .owner = THIS_MODULE, >> }, >> }; >> module_virtio_driver(virtio_gpio_driver); >> >> -- >> 2.34.1 >> > > Applied, thanks! I expressed dependency in two places: cover letter and this patch. Please drop it, because without dependency this won't work. Patch could go with the dependency and with your ack or next cycle. Best regards, Krzysztof
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 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? Remember that this overrides whatever driver actually put there. Best regards, Krzysztof
On Fri, Mar 29, 2024 at 12:35 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 29/03/2024 11:27, Bartosz Golaszewski wrote: > > On Wed, Mar 27, 2024 at 1:45 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> 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. > >> --- > >> drivers/gpio/gpio-virtio.c | 1 - > >> 1 file changed, 1 deletion(-) > >> > >> diff --git a/drivers/gpio/gpio-virtio.c b/drivers/gpio/gpio-virtio.c > >> index fcc5e8c08973..9fae8e396c58 100644 > >> --- a/drivers/gpio/gpio-virtio.c > >> +++ b/drivers/gpio/gpio-virtio.c > >> @@ -653,7 +653,6 @@ static struct virtio_driver virtio_gpio_driver = { > >> .remove = virtio_gpio_remove, > >> .driver = { > >> .name = KBUILD_MODNAME, > >> - .owner = THIS_MODULE, > >> }, > >> }; > >> module_virtio_driver(virtio_gpio_driver); > >> > >> -- > >> 2.34.1 > >> > > > > Applied, thanks! > > I expressed dependency in two places: cover letter and this patch. > Please drop it, because without dependency this won't work. Patch could > go with the dependency and with your ack or next cycle. > > Best regards, > Krzysztof > Dropped, and: Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
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
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> This makes sense. So this will be: Fixes: 3cfc88380413 ("i2c: virtio: add a virtio i2c frontend driver") Cc: "Jie Deng" <jie.deng@intel.com> and I think I will pick this patch for this cycle to fix the bug. The cleanups can go in the next cycle. > --- > 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; > + > 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
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,