Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1660092 > unrolled thread

Re: [PATCH 03/17] base: fix order of OF initialization

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 03/17] base: fix order of OF initialization Wesley Terpstra <wesley@sifive.com> - 2017-06-07 20:50 +0200
    Re: [PATCH 03/17] base: fix order of OF initialization Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-06-07 23:20 +0200
    Re: [PATCH 03/17] base: fix order of OF initialization Frank Rowand <frowand.list@gmail.com> - 2017-06-08 06:00 +0200
      Re: [PATCH 03/17] base: fix order of OF initialization Mark Rutland <mark.rutland@arm.com> - 2017-06-08 11:10 +0200
        Re: [PATCH 03/17] base: fix order of OF initialization Frank Rowand <frowand.list@gmail.com> - 2017-06-09 02:40 +0200

#1660092 — Re: [PATCH 03/17] base: fix order of OF initialization

It was a while ago that I debugged this. I already reported this bug
to Benjamin Herrenschmidt (now in CC), and I believe he has a patch of
his own to fix the same issue.

As I understand it, of_core_init sets up the OF entries in
/sys/firmware/devicetree. During platform bringup, when the system
describes the cpu + cache hierarchy, it also makes an of_node symlink
into that directory. However, if it doesn't exist yet, you get the
warning.

# ls -l /sys/devices/system/cpu/cpu3/of_node
lrwxrwxrwx    1 root     root             0 Jan  1 00:00
/sys/devices/system/cpu/cpu3/of_node ->
../../../../firmware/devicetree/base/cpus/cpu@3

On Wed, Jun 7, 2017 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Jun 07, 2017 at 09:07:20AM +0200, Geert Uytterhoeven wrote:
>> CC devicetree folks
>>
>> On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> > From: "Wesley W. Terpstra" <wesley@sifive.com>
>> >
>> > This fixes: [    0.010000] cpu cpu0: Error -2 creating of_node link
>> > ... which you get for every CPU on all architectures with a OF cpu/ node.
>
> I take it this means a /cpus node? Or the /cpus/cpu@* nodes?
>
> I'm not seeing this on arm64 when booting v4.12-rc4 with DT, so clearly
> this doesn't affect all such architectures.
>
> What path are these errors happening in?
>
> Thanks,
> Mark.
>
>> >
>> > This affects riscv, nios, etc.
>> >
>> > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>> > ---
>> >  drivers/base/init.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/base/init.c b/drivers/base/init.c
>> > index 48c0e220acc0..0dcd17e561d0 100644
>> > --- a/drivers/base/init.c
>> > +++ b/drivers/base/init.c
>> > @@ -31,9 +31,9 @@ void __init driver_init(void)
>> >         /* These are also core pieces, but must come after the
>> >          * core core pieces.
>> >          */
>> > +       of_core_init();
>> >         platform_bus_init();
>> >         cpu_dev_init();
>> >         memory_dev_init();
>> >         container_dev_init();
>> > -       of_core_init();
>> >  }
>> > --
>> > 2.13.0
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [next] | [standalone]

#1660243

On Wed, 2017-06-07 at 11:39 -0700, Wesley Terpstra wrote:
> It was a while ago that I debugged this. I already reported this bug
> to Benjamin Herrenschmidt (now in CC), and I believe he has a patch of
> his own to fix the same issue.
> 
> As I understand it, of_core_init sets up the OF entries in
> /sys/firmware/devicetree. During platform bringup, when the system
> describes the cpu + cache hierarchy, it also makes an of_node symlink
> into that directory. However, if it doesn't exist yet, you get the
> warning.

Ugh... yes I did a patch for that and I think it fell through the
cracks, I can't even find it anymore...

The patch quoted here is fine I think. Everything in the device model
can potentially use OF bits these days, it makes sense to have them
initialized earlier.

Cheers,
Ben.

> # ls -l /sys/devices/system/cpu/cpu3/of_node
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00
> /sys/devices/system/cpu/cpu3/of_node ->
> ../../../../firmware/devicetree/base/cpus/cpu@3
> 
> On Wed, Jun 7, 2017 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Wed, Jun 07, 2017 at 09:07:20AM +0200, Geert Uytterhoeven wrote:
> > > CC devicetree folks
> > > 
> > > On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > > > From: "Wesley W. Terpstra" <wesley@sifive.com>
> > > > 
> > > > This fixes: [    0.010000] cpu cpu0: Error -2 creating of_node link
> > > > ... which you get for every CPU on all architectures with a OF cpu/ node.
> > 
> > I take it this means a /cpus node? Or the /cpus/cpu@* nodes?
> > 
> > I'm not seeing this on arm64 when booting v4.12-rc4 with DT, so clearly
> > this doesn't affect all such architectures.
> > 
> > What path are these errors happening in?
> > 
> > Thanks,
> > Mark.
> > 
> > > > 
> > > > This affects riscv, nios, etc.
> > > > 
> > > > Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> > > > ---
> > > >  drivers/base/init.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/base/init.c b/drivers/base/init.c
> > > > index 48c0e220acc0..0dcd17e561d0 100644
> > > > --- a/drivers/base/init.c
> > > > +++ b/drivers/base/init.c
> > > > @@ -31,9 +31,9 @@ void __init driver_init(void)
> > > >         /* These are also core pieces, but must come after the
> > > >          * core core pieces.
> > > >          */
> > > > +       of_core_init();
> > > >         platform_bus_init();
> > > >         cpu_dev_init();
> > > >         memory_dev_init();
> > > >         container_dev_init();
> > > > -       of_core_init();
> > > >  }
> > > > --
> > > > 2.13.0
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe devicetree" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1660715

On 06/07/17 11:39, Wesley Terpstra wrote:
> It was a while ago that I debugged this. I already reported this bug
> to Benjamin Herrenschmidt (now in CC), and I believe he has a patch of
> his own to fix the same issue.
> 
> As I understand it, of_core_init sets up the OF entries in
> /sys/firmware/devicetree. During platform bringup, when the system
> describes the cpu + cache hierarchy, it also makes an of_node symlink
> into that directory. However, if it doesn't exist yet, you get the
> warning.
> 
> # ls -l /sys/devices/system/cpu/cpu3/of_node
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00
> /sys/devices/system/cpu/cpu3/of_node ->
> ../../../../firmware/devicetree/base/cpus/cpu@3
> 
> On Wed, Jun 7, 2017 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> On Wed, Jun 07, 2017 at 09:07:20AM +0200, Geert Uytterhoeven wrote:
>>> CC devicetree folks
>>>
>>> On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>>> From: "Wesley W. Terpstra" <wesley@sifive.com>
>>>>
>>>> This fixes: [    0.010000] cpu cpu0: Error -2 creating of_node link
>>>> ... which you get for every CPU on all architectures with a OF cpu/ node.
>>
>> I take it this means a /cpus node? Or the /cpus/cpu@* nodes?
>>
>> I'm not seeing this on arm64 when booting v4.12-rc4 with DT, so clearly
>> this doesn't affect all such architectures.
>>
>> What path are these errors happening in?

On the surface, the patch looks reasonable.  But it is not obvious to me why
the error message is occurring.  I would like to understand the cause before
saying the patch is good.

What kernel version is showing the error?  For a specific architecture
(the patch lists 'riscv, nios, etc'), which config and device tree source?

And again, what is the calling path?

- Frank

>>
>> Thanks,
>> Mark.
>>
>>>>
>>>> This affects riscv, nios, etc.
>>>>
>>>> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
>>>> ---
>>>>  drivers/base/init.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/base/init.c b/drivers/base/init.c
>>>> index 48c0e220acc0..0dcd17e561d0 100644
>>>> --- a/drivers/base/init.c
>>>> +++ b/drivers/base/init.c
>>>> @@ -31,9 +31,9 @@ void __init driver_init(void)
>>>>         /* These are also core pieces, but must come after the
>>>>          * core core pieces.
>>>>          */
>>>> +       of_core_init();
>>>>         platform_bus_init();
>>>>         cpu_dev_init();
>>>>         memory_dev_init();
>>>>         container_dev_init();
>>>> -       of_core_init();
>>>>  }
>>>> --
>>>> 2.13.0
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [next] | [standalone]

#1660964

On Wed, Jun 07, 2017 at 08:49:43PM -0700, Frank Rowand wrote:
> On 06/07/17 11:39, Wesley Terpstra wrote:
> > It was a while ago that I debugged this. I already reported this bug
> > to Benjamin Herrenschmidt (now in CC), and I believe he has a patch of
> > his own to fix the same issue.
> > 
> > As I understand it, of_core_init sets up the OF entries in
> > /sys/firmware/devicetree. During platform bringup, when the system
> > describes the cpu + cache hierarchy, it also makes an of_node symlink
> > into that directory. However, if it doesn't exist yet, you get the
> > warning.
> > 
> > # ls -l /sys/devices/system/cpu/cpu3/of_node
> > lrwxrwxrwx    1 root     root             0 Jan  1 00:00
> > /sys/devices/system/cpu/cpu3/of_node ->
> > ../../../../firmware/devicetree/base/cpus/cpu@3
> > 
> > On Wed, Jun 7, 2017 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> On Wed, Jun 07, 2017 at 09:07:20AM +0200, Geert Uytterhoeven wrote:
> >>> CC devicetree folks
> >>>
> >>> On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >>>> From: "Wesley W. Terpstra" <wesley@sifive.com>
> >>>>
> >>>> This fixes: [    0.010000] cpu cpu0: Error -2 creating of_node link
> >>>> ... which you get for every CPU on all architectures with a OF cpu/ node.
> >>
> >> I take it this means a /cpus node? Or the /cpus/cpu@* nodes?
> >>
> >> I'm not seeing this on arm64 when booting v4.12-rc4 with DT, so clearly
> >> this doesn't affect all such architectures.
> >>
> >> What path are these errors happening in?
> 
> On the surface, the patch looks reasonable.  But it is not obvious to me why
> the error message is occurring.  I would like to understand the cause before
> saying the patch is good.
> 
> What kernel version is showing the error?  For a specific architecture
> (the patch lists 'riscv, nios, etc'), which config and device tree source?
> 
> And again, what is the calling path?

From having grepped around, I think this affects architectures which
select CONFIG_GENERIC_CPU_DEVICES, which includes nios2.

In that case, driver_init() calls cpu_dev_init() before calling
of_core_init(). Then we get the callchain:

   cpu_dev_init()
-> cpu_dev_register_generic()
-> register_cpu(cpu, i)
-> device_register(&cpu->dev)
-> device_add(dev)
-> device_add_class_symlinks(dev)

... in device_add_class_symlinks, we we dev->of_node, and call
sysfs_create_link(), which fails because we haven't called
of_core_init() to register the sysfs devicetree directory yet.

Given that, this patch makes sense to me.

FWIW, with the commit message updated to describe the particular
ordering problem:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1661842

On 06/08/17 02:05, Mark Rutland wrote:
> On Wed, Jun 07, 2017 at 08:49:43PM -0700, Frank Rowand wrote:
>> On 06/07/17 11:39, Wesley Terpstra wrote:
>>> It was a while ago that I debugged this. I already reported this bug
>>> to Benjamin Herrenschmidt (now in CC), and I believe he has a patch of
>>> his own to fix the same issue.
>>>
>>> As I understand it, of_core_init sets up the OF entries in
>>> /sys/firmware/devicetree. During platform bringup, when the system
>>> describes the cpu + cache hierarchy, it also makes an of_node symlink
>>> into that directory. However, if it doesn't exist yet, you get the
>>> warning.
>>>
>>> # ls -l /sys/devices/system/cpu/cpu3/of_node
>>> lrwxrwxrwx    1 root     root             0 Jan  1 00:00
>>> /sys/devices/system/cpu/cpu3/of_node ->
>>> ../../../../firmware/devicetree/base/cpus/cpu@3
>>>
>>> On Wed, Jun 7, 2017 at 2:35 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>> On Wed, Jun 07, 2017 at 09:07:20AM +0200, Geert Uytterhoeven wrote:
>>>>> CC devicetree folks
>>>>>
>>>>> On Wed, Jun 7, 2017 at 12:59 AM, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>>>>> From: "Wesley W. Terpstra" <wesley@sifive.com>
>>>>>>
>>>>>> This fixes: [    0.010000] cpu cpu0: Error -2 creating of_node link
>>>>>> ... which you get for every CPU on all architectures with a OF cpu/ node.
>>>>
>>>> I take it this means a /cpus node? Or the /cpus/cpu@* nodes?
>>>>
>>>> I'm not seeing this on arm64 when booting v4.12-rc4 with DT, so clearly
>>>> this doesn't affect all such architectures.
>>>>
>>>> What path are these errors happening in?
>>
>> On the surface, the patch looks reasonable.  But it is not obvious to me why
>> the error message is occurring.  I would like to understand the cause before
>> saying the patch is good.
>>
>> What kernel version is showing the error?  For a specific architecture
>> (the patch lists 'riscv, nios, etc'), which config and device tree source?
>>
>> And again, what is the calling path?
> 
>>From having grepped around, I think this affects architectures which
> select CONFIG_GENERIC_CPU_DEVICES, which includes nios2.

Thanks Mark!  The "#ifdef CONFIG_GENERIC_CPU_DEVICES" in cpu_dev_register_generic()
explains why we don't see the error on ARM, ARM64, etc.  Without the CONFIG
option, register_cpu(cpu, i) is not called.


> In that case, driver_init() calls cpu_dev_init() before calling
> of_core_init(). Then we get the callchain:
> 
>    cpu_dev_init()
> -> cpu_dev_register_generic()
> -> register_cpu(cpu, i)
> -> device_register(&cpu->dev)
> -> device_add(dev)
> -> device_add_class_symlinks(dev)
> 
> ... in device_add_class_symlinks, we we dev->of_node, and call
> sysfs_create_link(), which fails because we haven't called
> of_core_init() to register the sysfs devicetree directory yet.
> 
> Given that, this patch makes sense to me.
> 
> FWIW, with the commit message updated to describe the particular
> ordering problem:
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>

Agree with Mark's request to update the commit message, and also
I would like to see this shaken out in the -next tree.

Acked-by: Frank Rowand <frowand.list@gmail.com>

> 
> Thanks,
> Mark.
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web