Re: [PATCH] powerpc: populate the default bus with machine_arch_initcall

Re: [PATCH] powerpc: populate the default bus with machine_arch_initcall

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 5228 bytes --]

On Fri, Aug 12, 2016 at 02:39:32PM +1000, Michael Ellerman wrote:
> Kevin Hao <haokexin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
> 
> > With the commit 44a7185c2ae6 ("of/platform: Add common method to
> > populate default bus"), a default function is introduced to populate
> > the default bus and this function is invoked at the arch_initcall_sync
> > level. This will override the arch specific population of default bus
> > which run at a lower level than arch_initcall_sync. Since not all
> > powerpc specific buses are added to the of_default_bus_match_table[],
> > this causes some powerpc specific bus are not probed. Fix this by
> > using a more preceding initcall.
> >
> > Signed-off-by: Kevin Hao <haokexin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> > Of course we can adjust the powerpc arch codes to use the
> > of_platform_default_populate_init(), but it has high risk to break
> > other boards given the complicated powerpc specific buses. So I would
> > like just to fix the broken boards in the current release, and cook 
> > a patch to change to of_platform_default_populate_init() for linux-next.
> >
> > Only boot test on a mpc8315erdb board.
> >
> >  arch/powerpc/platforms/40x/ep405.c               | 2 +-
> >  arch/powerpc/platforms/40x/ppc40x_simple.c       | 2 +-
> >  arch/powerpc/platforms/40x/virtex.c              | 2 +-
> >  arch/powerpc/platforms/40x/walnut.c              | 2 +-
> >  arch/powerpc/platforms/44x/canyonlands.c         | 2 +-
> >  arch/powerpc/platforms/44x/ebony.c               | 2 +-
> >  arch/powerpc/platforms/44x/iss4xx.c              | 2 +-
> >  arch/powerpc/platforms/44x/ppc44x_simple.c       | 2 +-
> >  arch/powerpc/platforms/44x/ppc476.c              | 2 +-
> >  arch/powerpc/platforms/44x/sam440ep.c            | 2 +-
> >  arch/powerpc/platforms/44x/virtex.c              | 2 +-
> >  arch/powerpc/platforms/44x/warp.c                | 2 +-
> >  arch/powerpc/platforms/82xx/ep8248e.c            | 2 +-
> >  arch/powerpc/platforms/82xx/km82xx.c             | 2 +-
> >  arch/powerpc/platforms/82xx/mpc8272_ads.c        | 2 +-
> >  arch/powerpc/platforms/82xx/pq2fads.c            | 2 +-
> >  arch/powerpc/platforms/83xx/mpc831x_rdb.c        | 2 +-
> >  arch/powerpc/platforms/83xx/mpc834x_itx.c        | 2 +-
> >  arch/powerpc/platforms/85xx/ppa8548.c            | 2 +-
> >  arch/powerpc/platforms/8xx/adder875.c            | 2 +-
> >  arch/powerpc/platforms/8xx/ep88xc.c              | 2 +-
> >  arch/powerpc/platforms/8xx/mpc86xads_setup.c     | 2 +-
> >  arch/powerpc/platforms/8xx/mpc885ads_setup.c     | 2 +-
> >  arch/powerpc/platforms/8xx/tqm8xx_setup.c        | 2 +-
> >  arch/powerpc/platforms/cell/setup.c              | 2 +-
> >  arch/powerpc/platforms/embedded6xx/gamecube.c    | 2 +-
> >  arch/powerpc/platforms/embedded6xx/linkstation.c | 2 +-
> >  arch/powerpc/platforms/embedded6xx/mvme5100.c    | 2 +-
> >  arch/powerpc/platforms/embedded6xx/storcenter.c  | 2 +-
> >  arch/powerpc/platforms/embedded6xx/wii.c         | 2 +-
> >  arch/powerpc/platforms/pasemi/setup.c            | 2 +-
> 
> That's not a very minimal fix.
> 
> Every one of those initcall changes could be introducing a bug, by
> changing the order vs other init calls.
> 
> Can we just go back to the old behaviour on ppc?

Sure. How about this one?

From 4362b4cdd8a6198df4cc46c628473f0d44e03fa8 Mon Sep 17 00:00:00 2001
From: Kevin Hao <haokexin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Fri, 12 Aug 2016 13:30:03 +0800
Subject: [PATCH v2] of/platform: disable the
 of_platform_default_populate_init() for all the ppc boards

With the commit 44a7185c2ae6 ("of/platform: Add common method to
populate default bus"), a default function is introduced to populate
the default bus and this function is invoked at the arch_initcall_sync
level. But a lot of ppc boards use machine_device_initcall() to
populate the default bus. This means that the default populate function
has higher priority and would override the arch specific population of
the bus. The side effect is that some arch specific bus are not probed,
then cause various malfunction due to the miss of some devices. Since
it is very possible to introduce bugs if we simply change the initcall
level for all these boards(about 30+). This just disable this default
function for all the ppc boards.

Signed-off-by: Kevin Hao <haokexin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/of/platform.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 8aa197691074..f39ccd5aa701 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -497,6 +497,7 @@ int of_platform_default_populate(struct device_node *root,
 }
 EXPORT_SYMBOL_GPL(of_platform_default_populate);
 
+#ifndef CONFIG_PPC
 static int __init of_platform_default_populate_init(void)
 {
 	struct device_node *node;
@@ -521,6 +522,7 @@ static int __init of_platform_default_populate_init(void)
 	return 0;
 }
 arch_initcall_sync(of_platform_default_populate_init);
+#endif
 
 static int of_platform_device_destroy(struct device *dev, void *data)
 {
-- 
2.8.1

Thanks,
Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] powerpc: populate the default bus with machine_arch_initcall

[Rob Herring]

On Fri, Aug 12, 2016 at 12:49 AM, Kevin Hao <haokexin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Aug 12, 2016 at 02:39:32PM +1000, Michael Ellerman wrote:
>> Kevin Hao <haokexin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

[...]

>> Every one of those initcall changes could be introducing a bug, by
>> changing the order vs other init calls.
>>
>> Can we just go back to the old behaviour on ppc?
>
> Sure. How about this one?
>
> From 4362b4cdd8a6198df4cc46c628473f0d44e03fa8 Mon Sep 17 00:00:00 2001
> From: Kevin Hao <haokexin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Date: Fri, 12 Aug 2016 13:30:03 +0800
> Subject: [PATCH v2] of/platform: disable the
>  of_platform_default_populate_init() for all the ppc boards
>
> With the commit 44a7185c2ae6 ("of/platform: Add common method to
> populate default bus"), a default function is introduced to populate
> the default bus and this function is invoked at the arch_initcall_sync
> level. But a lot of ppc boards use machine_device_initcall() to
> populate the default bus. This means that the default populate function
> has higher priority and would override the arch specific population of
> the bus. The side effect is that some arch specific bus are not probed,
> then cause various malfunction due to the miss of some devices. Since
> it is very possible to introduce bugs if we simply change the initcall
> level for all these boards(about 30+). This just disable this default
> function for all the ppc boards.
>
> Signed-off-by: Kevin Hao <haokexin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/of/platform.c | 2 ++
>  1 file changed, 2 insertions(+)

I've applied this one.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] powerpc: populate the default bus with machine_arch_initcall

[Benjamin Herrenschmidt]

On Fri, 2016-08-12 at 14:30 -0500, Rob Herring wrote:
> 
> > With the commit 44a7185c2ae6 ("of/platform: Add common method to
> > populate default bus"), a default function is introduced to
> > populate
> > the default bus and this function is invoked at the
> > arch_initcall_sync
> > level. But a lot of ppc boards use machine_device_initcall() to
> > populate the default bus. This means that the default populate
> > function
> > has higher priority and would override the arch specific population
> > of
> > the bus. The side effect is that some arch specific bus are not
> > probed,
> > then cause various malfunction due to the miss of some devices.
> > Since
> > it is very possible to introduce bugs if we simply change the
> > initcall
> > level for all these boards(about 30+). This just disable this
> > default
> > function for all the ppc boards.
> > 
> > Signed-off-by: Kevin Hao <haokexin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/of/platform.c | 2 ++
> >  1 file changed, 2 insertions(+)
> 
> I've applied this one.

Not fan of the hard #ifdef at all... it will make it hard to convert
platforms one by one. Why not an arch_want_default_of_probe() or
something like this which we can then plumb into ppc_md. ?

Cheers,
Ben.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] powerpc: populate the default bus with machine_arch_initcall

[Rob Herring]

On Sun, Aug 14, 2016 at 4:21 PM, Benjamin Herrenschmidt
<benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> wrote:
> On Fri, 2016-08-12 at 14:30 -0500, Rob Herring wrote:
>>
>> > With the commit 44a7185c2ae6 ("of/platform: Add common method to
>> > populate default bus"), a default function is introduced to
>> > populate
>> > the default bus and this function is invoked at the
>> > arch_initcall_sync
>> > level. But a lot of ppc boards use machine_device_initcall() to
>> > populate the default bus. This means that the default populate
>> > function
>> > has higher priority and would override the arch specific population
>> > of
>> > the bus. The side effect is that some arch specific bus are not
>> > probed,
>> > then cause various malfunction due to the miss of some devices.
>> > Since
>> > it is very possible to introduce bugs if we simply change the
>> > initcall
>> > level for all these boards(about 30+). This just disable this
>> > default
>> > function for all the ppc boards.
>> >
>> > Signed-off-by: Kevin Hao <haokexin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> > ---
>> >  drivers/of/platform.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>>
>> I've applied this one.
>
> Not fan of the hard #ifdef at all... it will make it hard to convert
> platforms one by one. Why not an arch_want_default_of_probe() or
> something like this which we can then plumb into ppc_md. ?

Yeah, I'm not either really. I didn't propose using IS_ENABLED here
either as it does add a needless initcall.

Do you have any idea which platforms are not broken and could be
switched over now. If not, I think we should just leave as is for 4.8
and make it per platform selectable for 4.9.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] powerpc: populate the default bus with machine_arch_initcall

[Benjamin Herrenschmidt]

On Sun, 2016-08-14 at 17:42 -0500, Rob Herring wrote:
> > Not fan of the hard #ifdef at all... it will make it hard to
> convert
> > platforms one by one. Why not an arch_want_default_of_probe() or
> > something like this which we can then plumb into ppc_md. ?
> 
> Yeah, I'm not either really. I didn't propose using IS_ENABLED here
> either as it does add a needless initcall.
> 
> Do you have any idea which platforms are not broken and could be
> switched over now. If not, I think we should just leave as is for 4.8
> and make it per platform selectable for 4.9.

Not really... the non-embedded platforms (mac, pseries, etc...) don't
really use the OF bus probing iirc, so it's going to be a matter of
scrubbing the embedded ones. I agree we can do that in 4.9

Cheers,
Ben.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html