| Message ID | 20240326-module-owner-amba-v1-1-4517b091385b@linaro.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | amba: store owner from modules with amba_driver_register() | expand |
Hi Krzysztof, ... > /** > - * amba_driver_register - register an AMBA device driver > + * __amba_driver_register - register an AMBA device driver > * @drv: amba device driver structure > + * @owner: owning module/driver > * > * Register an AMBA device driver with the Linux device model > * core. If devices pre-exist, the drivers probe function will > * be called. > */ > -int amba_driver_register(struct amba_driver *drv) > +int __amba_driver_register(struct amba_driver *drv, ... > +/* > + * use a macro to avoid include chaining to get THIS_MODULE > + */ Should the documentation be moved here? Well... I don't see any documentation linking this file yet, but in case it comes we want documented amba_driver_register() rather than __amba_driver_register(). Andi > +#define amba_driver_register(drv) \ > + __amba_driver_register(drv, THIS_MODULE) > +
On 27/03/2024 21:33, Andi Shyti wrote: > Hi Krzysztof, > > ... > >> /** >> - * amba_driver_register - register an AMBA device driver >> + * __amba_driver_register - register an AMBA device driver >> * @drv: amba device driver structure >> + * @owner: owning module/driver >> * >> * Register an AMBA device driver with the Linux device model >> * core. If devices pre-exist, the drivers probe function will >> * be called. >> */ >> -int amba_driver_register(struct amba_driver *drv) >> +int __amba_driver_register(struct amba_driver *drv, > > ... > >> +/* >> + * use a macro to avoid include chaining to get THIS_MODULE >> + */ > > Should the documentation be moved here? Well... I don't see any > documentation linking this file yet, but in case it comes we want > documented amba_driver_register() rather than > __amba_driver_register(). > That's just a wrapper, not API... why would we care to have kerneldoc for it? Best regards, Krzysztof
Hi Krzysztof, > >> /** > >> - * amba_driver_register - register an AMBA device driver > >> + * __amba_driver_register - register an AMBA device driver > >> * @drv: amba device driver structure > >> + * @owner: owning module/driver > >> * > >> * Register an AMBA device driver with the Linux device model > >> * core. If devices pre-exist, the drivers probe function will > >> * be called. > >> */ > >> -int amba_driver_register(struct amba_driver *drv) > >> +int __amba_driver_register(struct amba_driver *drv, > > > > ... > > > >> +/* > >> + * use a macro to avoid include chaining to get THIS_MODULE > >> + */ > > > > Should the documentation be moved here? Well... I don't see any > > documentation linking this file yet, but in case it comes we want > > documented amba_driver_register() rather than > > __amba_driver_register(). > > > > That's just a wrapper, not API... why would we care to have kerneldoc > for it? Because everyone should use the wrapper while the real function will be used only once or twice. I see also that this is a common practice which I don't surely like. In any case there is no documentation exported for AMBA so that this discussion does not bring any tangible benefit. So that, considering that it's a good improvement, Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Andi
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c index a24c152bfaac..aba3aa95b224 100644 --- a/drivers/amba/bus.c +++ b/drivers/amba/bus.c @@ -488,28 +488,31 @@ static int __init amba_stub_drv_init(void) * waiting on amba_match(). So, register a stub driver to make sure * amba_match() is called even if no amba driver has been registered. */ - return amba_driver_register(&amba_proxy_drv); + return __amba_driver_register(&amba_proxy_drv, NULL); } late_initcall_sync(amba_stub_drv_init); /** - * amba_driver_register - register an AMBA device driver + * __amba_driver_register - register an AMBA device driver * @drv: amba device driver structure + * @owner: owning module/driver * * Register an AMBA device driver with the Linux device model * core. If devices pre-exist, the drivers probe function will * be called. */ -int amba_driver_register(struct amba_driver *drv) +int __amba_driver_register(struct amba_driver *drv, + struct module *owner) { if (!drv->probe) return -EINVAL; + drv->drv.owner = owner; drv->drv.bus = &amba_bustype; return driver_register(&drv->drv); } -EXPORT_SYMBOL(amba_driver_register); +EXPORT_SYMBOL(__amba_driver_register); /** * amba_driver_unregister - remove an AMBA device driver diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h index c60a6a14638c..958a55bcc708 100644 --- a/include/linux/amba/bus.h +++ b/include/linux/amba/bus.h @@ -112,11 +112,18 @@ extern struct bus_type amba_bustype; #define amba_get_drvdata(d) dev_get_drvdata(&d->dev) #define amba_set_drvdata(d,p) dev_set_drvdata(&d->dev, p) +/* + * use a macro to avoid include chaining to get THIS_MODULE + */ +#define amba_driver_register(drv) \ + __amba_driver_register(drv, THIS_MODULE) + #ifdef CONFIG_ARM_AMBA -int amba_driver_register(struct amba_driver *); +int __amba_driver_register(struct amba_driver *, struct module *); void amba_driver_unregister(struct amba_driver *); #else -static inline int amba_driver_register(struct amba_driver *drv) +static inline int __amba_driver_register(struct amba_driver *drv, + struct module *owner) { return -EINVAL; }
Modules registering driver with amba_driver_register() often forget to set .owner field. 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> --- drivers/amba/bus.c | 11 +++++++---- include/linux/amba/bus.h | 11 +++++++++-- 2 files changed, 16 insertions(+), 6 deletions(-)