Message ID | 20240327-module-owner-virtio-v1-1-0feffab77d99@linaro.org (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | virtio: store owner from modules with register_virtio_driver() | expand |
Context | Check | Description |
---|---|---|
tedd_an/pre-ci_am | success | Success |
tedd_an/CheckPatch | success | CheckPatch PASS |
tedd_an/GitLint | success | Gitlint PASS |
tedd_an/SubjectPrefix | fail | "Bluetooth: " prefix is not specified in the subject |
tedd_an/BuildKernel | success | BuildKernel PASS |
tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
tedd_an/CheckSparse | success | CheckSparse PASS |
tedd_an/CheckSmatch | success | CheckSparse PASS |
tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
tedd_an/TestRunner_iso-tester | success | TestRunner PASS |
tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
tedd_an/TestRunner_mgmt-tester | success | TestRunner PASS |
tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
tedd_an/IncrementalBuild | success | Incremental Build PASS |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=838871 ---Test result--- Test Summary: CheckPatch PASS 11.02 seconds GitLint PASS 5.32 seconds SubjectPrefix FAIL 2.18 seconds BuildKernel PASS 28.39 seconds CheckAllWarning PASS 30.91 seconds CheckSparse PASS 38.98 seconds CheckSmatch PASS 102.80 seconds BuildKernel32 PASS 27.28 seconds TestRunnerSetup PASS 520.10 seconds TestRunner_l2cap-tester PASS 20.37 seconds TestRunner_iso-tester PASS 30.80 seconds TestRunner_bnep-tester PASS 4.73 seconds TestRunner_mgmt-tester PASS 114.76 seconds TestRunner_rfcomm-tester PASS 7.32 seconds TestRunner_sco-tester PASS 14.95 seconds TestRunner_ioctl-tester PASS 7.84 seconds TestRunner_mesh-tester PASS 6.00 seconds TestRunner_smp-tester PASS 6.85 seconds TestRunner_userchan-tester PASS 4.96 seconds IncrementalBuild PASS 109.17 seconds Details ############################## Test: SubjectPrefix - FAIL Desc: Check subject contains "Bluetooth" prefix Output: "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject "Bluetooth: " prefix is not specified in the subject --- Regards, Linux Bluetooth
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 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
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
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(-)