Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200528132541.463300-1-arch0.zheng@gmail.com>
 <ebbba4ac-ea65-472c-5a3a-201dfe59e402@alliedtelesis.co.nz> <CAL_Jsq+8TDagRWyxZHo+qeeV7zDok2mnq10B-B9-L2QoqMr=FQ@mail.gmail.com>
In-Reply-To: <CAL_Jsq+8TDagRWyxZHo+qeeV7zDok2mnq10B-B9-L2QoqMr=FQ@mail.gmail.com>
From:   Rob Herring <robh+dt@kernel.org>
Date:   Mon, 25 Jan 2021 20:34:49 -0600
Message-ID: <CAL_JsqLpy+8DdXjYAJJyHmcaQe8MBftWbzftaCAkvt9nr6pQEg@mail.gmail.com>
Subject: Re: [PATCH v2] of/fdt: Remove redundant kbasename function call
To:     Chris Packham <Chris.Packham@alliedtelesis.co.nz>
Cc:     Qi Zheng <arch0.zheng@gmail.com>,
        "frowand.list@gmail.com" <frowand.list@gmail.com>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Jan 25, 2021 at 8:06 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> +LAKML given it's an Arm issue
>
> On Mon, Jan 25, 2021 at 6:47 PM Chris Packham
> <Chris.Packham@alliedtelesis.co.nz> wrote:
> >
> > Hi All,
> >
> > On 29/05/20 1:25 am, Qi Zheng wrote:
> > > For version 1 to 3 of the device tree, this is the node full
> > > path as a zero terminated string, starting with "/". The
> > > following equation will not hold, since the node name has
> > > been processed in the fdt_get_name().
> > >
> > >       *pathp == '/'
> > >
> > > For version 16 and later, this is the node unit name only
> > > (or an empty string for the root node). So the above
> > > equation will still not hold.
> > >
> > > So the kbasename() is redundant, just remove it.
> > >
> > > Signed-off-by: Qi Zheng <arch0.zheng@gmail.com>
> > > ---
> > >
> > > Change in v2:
> > >       remove another kbasename() also.
> > >
> > >   drivers/of/fdt.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > >
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index 38619e9ef6b2..4602e467ca8b 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -643,8 +643,6 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node,
> > >            offset = fdt_next_node(blob, offset, &depth)) {
> > >
> > >               pathp = fdt_get_name(blob, offset, NULL);
> > > -             if (*pathp == '/')
> > > -                     pathp = kbasename(pathp);
> > >               rc = it(offset, pathp, depth, data);
> > >       }
> > >       return rc;
> > > @@ -671,8 +669,6 @@ int __init of_scan_flat_dt_subnodes(unsigned long parent,
> > >               int rc;
> > >
> > >               pathp = fdt_get_name(blob, node, NULL);
> > > -             if (*pathp == '/')
> > > -                     pathp = kbasename(pathp);
> > >               rc = it(node, pathp, data);
> > >               if (rc)
> > >                       return rc;
> >
> > I'm trying to keep our boards up to date with newer kernels.
> >
> > I've just hit a problem on an older board that uses
> > CONFIG_ARM_APPENDED_DTB and has a number of command line args passed up
> > from the bootloader that are required for a successful boot.
> >
> > I'm stepping through kernel versions in the hope that keeping things
> > running is easier in smaller increments I'm up to v5.8. I'm not
> > currently able to check a newer kernel on this board but looking at the
> > code the problem still seems to exist in the latest tree.
> >
> > early_init_dt_scan_chosen() searches for "chosen" prior to this change
> > the "/chosen" node that gets inserted by atags_to_fdt.c but with this
> > change it can't find it and fails to boot.
>
> Given this code works for normal cases, I'm guessing the problem is in
> atags_to_fdt.c or libfdt. Is it possible to add an empty chosen node
> to the DT and see if that makes any difference.

This will work. It's a atags_to_fdt.c bug. In node_offset(), we search
by path (/chosen) with fdt_path_offset. If not found, then we add a
subnode called '/chosen' (instead of 'chosen') with fdt_add_subnode().
Though arguably, libfdt could catch and handle this. I'll send a patch
in a minute.

Rob