[PATCH] rpmsg bus subsys_initcall initialization ordering

[PATCH] rpmsg bus subsys_initcall initialization ordering

[Federico Fuga]

omaprpc depends on the rpmsg bus.
When those two modules are compiled statically, the omaprpc is initialized before the rpmsg bus, triggering a BUG() in driver_register.
This of course can be prevented if they are compiled as modules and loaded in the right order.
This patch solves this problem by forcing the rpmsg module to be initialized at subsystem level.

Signed-off-by: Federico Fuga <fuga@studiofuga.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 210c4eb..589819f 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -1078,7 +1078,7 @@ static int __init rpmsg_init(void)
 
 	return ret;
 }
-module_init(rpmsg_init);
+subsys_initcall(rpmsg_init);
 
 static void __exit rpmsg_fini(void)
 {
-- 
1.7.9.5

Re: [PATCH] rpmsg bus subsys_initcall initialization ordering

[Ohad Ben-Cohen]

Hi Federico,

On Tue, Jul 10, 2012 at 7:07 PM, Federico Fuga <fuga@studiofuga.com> wrote:
> omaprpc depends on the rpmsg bus.

Sorry for the ignorance, but what's omaprpc ? :)

I guess it's some out-of-tree module (which I've never seen) so I'm
not sure we want change mainline code to fix issues with it.

If your code is anyway out-of-tree, it'd probably make more sense if
this patch stays out of tree too (plus I have no way to review this
patch, and understand the issue it fixes, without seeing the relevant
code).

If this really is about mainline kernel, let me know.

Thanks,
Ohad.

Re: [PATCH] rpmsg bus subsys_initcall initialization ordering

[Federico Fuga]

Hi Ohad,

Il giorno 10/lug/2012, alle ore 18:22, Ohad Ben-Cohen ha scritto:

> On Tue, Jul 10, 2012 at 7:07 PM, Federico Fuga <fuga@studiofuga.com> wrote:
>> omaprpc depends on the rpmsg bus.
> 
> Sorry for the ignorance, but what's omaprpc ? :)
> 


You're right, I should have explained this! :-)
omaprpc is the Omap Remote Procedure Call driver. Quoting the Kconfig file,

"An rpmsg driver that exposes the Remote Procedure Call API to User Space, in order to allow applications to distribute remote call to more power efficient remote processors on OMAP4+ systems".

It depends on rpmsg system.

> I guess it's some out-of-tree module (which I've never seen) so I'm
> not sure we want change mainline code to fix issues with it.
> 

omaprpc is out of the official mainline code, it's inside the official ti/omap branch/project.
I guessed that the easier solution had been to change the initialization level, in a similar way it's done by the i2c bus driver for example.
I don't know if this is the best solution to solve this, maybe the problem could be solved in the omaprpc driver (maybe this could be the right solution).


> If your code is anyway out-of-tree, it'd probably make more sense if
> this patch stays out of tree too (plus I have no way to review this
> patch, and understand the issue it fixes, without seeing the relevant
> code).
> 
> If this really is about mainline kernel, let me know.
> 

Really, I don't have enough experience to say if it should be solved this way or not. For what I can see, this seems absolutely logic that busses are initialized before other drivers, so subsys_initcall should be perfectly logic. But I don't have the complete view of the system, so I could be wrong.
On the other hand, I didn't find another way to solve this dependency problem. I know there is also a Makefile approach, but the dependent driver (omaprpc) is in the staging driver directory, so I think is not feasible.
Any other solution?

Thanks

Federico

Re: [PATCH] rpmsg bus subsys_initcall initialization ordering

[Ohad Ben-Cohen]

Hi Federico,

On Tue, Jul 10, 2012 at 7:43 PM, Federico Fuga <fuga@studiofuga.com> wrote:
> omaprpc is out of the official mainline code, it's inside the official ti/omap branch/project.

Ok, thanks for the explanation. In general, we don't change the
mainline kernel to solve issues with out-of-tree code.

> I guessed that the easier solution had been to change the initialization level, in a similar way it's done by the i2c bus driver for example.
> I don't know if this is the best solution to solve this, maybe the problem could be solved in the omaprpc driver (maybe this could be the right solution).
...
> Really, I don't have enough experience to say if it should be solved this way or not. For what I can see, this seems absolutely logic that busses are initialized before other drivers, so subsys_initcall should be perfectly logic.

I'm not convinced: there is no inherent reason for a bus and its
driver to live in separate initcalls, so this may all just be an
omaprpc issue.

> On the other hand, I didn't find another way to solve this dependency problem. I know there is also a Makefile approach, but the dependent driver (omaprpc) is in the staging driver directory, so I think is not feasible.
> Any other solution?

I'm not familiar with omaprpc, so I can't help much, sorry. The driver
doesn't seem to be in staging btw (or did you mean it lives in staging
in that out-of-tree repository?).

Thanks,
Ohad.

Re: [PATCH] rpmsg bus subsys_initcall initialization ordering

[Ohad Ben-Cohen]

Hi Federico,

On Wed, Jul 11, 2012 at 10:14 AM, Federico Fuga <fuga@studiofuga.com> wrote:
> it triggers a BUG() in drivers/base/driver.c line 169

Thanks for sharing this; it definitely makes sense now.

> The subsys_initcall indeed solve this issue

I agree. I'll take it, but will change the commit log to make it
omaprpc-agnostic.

Thanks,
Ohad.

Re: [PATCH] rpmsg bus subsys_initcall initialization ordering

[Ohad Ben-Cohen]

Hi Federico,

On Fri, Jul 13, 2012 at 9:00 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> I agree. I'll take it, but will change the commit log to make it
> omaprpc-agnostic.

Here's what I'm going to apply:

commit 913552b8c7a0f06cc1bff27f8e9953bffe6a1817
Author: Federico Fuga <fuga@studiofuga.com>
Date:   Mon Jul 16 10:36:51 2012 +0300

    rpmsg: fix dependency on initialization order

    When rpmsg drivers are built into the kernel, they must not initialize
    before the rpmsg bus does, otherwise they'd trigger a BUG() in
    drivers/base/driver.c line 169 (driver_register()).

    To fix that, and to stop depending on arbitrary linkage ordering of
    those built-in rpmsg driver, we let the rpmsg bus initialize at
    subsys_initcall.

    Signed-off-by: Federico Fuga <fuga@studiofuga.com>
    [ohad: rewrite the commit log]
    Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 0af7fd3..e564a01 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -1104,7 +1104,7 @@ static int __init rpmsg_init(void)

        return ret;
 }
-module_init(rpmsg_init);
+subsys_initcall(rpmsg_init);

 static void __exit rpmsg_fini(void)
 {