Re: [PATCH 09/16] of: overlay: validate overlay properties #address-cells and #size-cells

[Alan Tull]

On Mon, Oct 8, 2018 at 10:57 AM Alan Tull <atull@xxxxxxxxxx> wrote:
>
> On Thu, Oct 4, 2018 at 11:14 PM <frowand.list@xxxxxxxxx> wrote:
> >
> > From: Frank Rowand <frank.rowand@xxxxxxxx>
> >
> > If overlay properties #address-cells or #size-cells are already in
> > the live devicetree for any given node, then the values in the
> > overlay must match the values in the live tree.
>
> Hi Frank,
>
> I'm starting some FPGA testing on this patchset applied to v4.19-rc7.
> That applied cleanly; if that's not the best base to test against,
> please let me know.
>
> On a very simple overlay, I'm seeing this patch's warning catching
> things other than #address-cells or #size-cells.

What it's warning about are new properties being added to an existing
node.  So !prop is true and !of_node_check_flag(target->np,
OF_OVERLAY) also is true.  Is that a potential memory leak as you are
warning?  If so, your code is working as planned and you'll just need
to document that also in the header.

> I'm just getting
> started looking at this, will spend time understanding this better and
> I'll test other overlays.  The warnings were:
>
> Applying dtbo: socfpga_overlay.dtb
> [   33.117881] fpga_manager fpga0: writing soc_system.rbf to Altera
> SOCFPGA FPGA Manager
> [   33.575223] OF: overlay: WARNING: add_changeset_property(), memory
> leak will occur if overlay removed.  Property:
> /soc/base-fpga-region/firmware-name
> [   33.588584] OF: overlay: WARNING: add_changeset_property(), memory
> leak will occur if overlay removed.  Property:
> /soc/base-fpga-region/fpga-bridges
> [   33.601856] OF: overlay: WARNING: add_changeset_property(), memory
> leak will occur if overlay removed.  Property:
> /soc/base-fpga-region/ranges
>
> Here's part of that overlay including the properties it's complaining about:
>
> /dts-v1/;
> /plugin/;
> / {
>         fragment@0 {
>                 target = <&base_fpga_region>;
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 __overlay__ {
>                         #address-cells = <1>;
>                         #size-cells = <1>;
>
>                         firmware-name = "soc_system.rbf";
>                         fpga-bridges = <&fpga_bridge1>;
>                         ranges = <0x20000 0xff200000 0x100000>,
>                         <0x0 0xc0000000 0x20000000>;
>
>                         gpio@10040 {
> so on...
>
> By the way, I didn't get any warnings when I subsequently removed this overlay.
>
> Alan
>
> >
> > If the properties are already in the live tree then there is no
> > need to create a changeset entry to add them since they must
> > have the same value.  This reduces the memory used by the
> > changeset and eliminates a possible memory leak.  This is
> > verified by 12 fewer warnings during the devicetree unittest,
> > as the possible memory leak warnings about #address-cells and
> >
> > Signed-off-by: Frank Rowand <frank.rowand@xxxxxxxx>
> > ---
> >  drivers/of/overlay.c | 38 +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > index 29c33a5c533f..e6fb3ffe9d93 100644
> > --- a/drivers/of/overlay.c
> > +++ b/drivers/of/overlay.c
> > @@ -287,7 +287,12 @@ static struct property *dup_and_fixup_symbol_prop(
> >   * @target may be either in the live devicetree or in a new subtree that
> >   * is contained in the changeset.
> >   *
> > - * Some special properties are not updated (no error returned).
> > + * Some special properties are not added or updated (no error returned):
> > + * "name", "phandle", "linux,phandle".
> > + *
> > + * Properties "#address-cells" and "#size-cells" are not updated if they
> > + * are already in the live tree, but if present in the live tree, the values
> > + * in the overlay must match the values in the live tree.
> >   *
> >   * Update of property in symbols node is not allowed.
> >   *
> > @@ -300,6 +305,7 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> >  {
> >         struct property *new_prop = NULL, *prop;
> >         int ret = 0;
> > +       bool check_for_non_overlay_node = false;
> >
> >         if (!of_prop_cmp(overlay_prop->name, "name") ||
> >             !of_prop_cmp(overlay_prop->name, "phandle") ||
> > @@ -322,13 +328,39 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
> >         if (!new_prop)
> >                 return -ENOMEM;
> >
> > -       if (!prop)
> > +       if (!prop) {
> > +
> > +               check_for_non_overlay_node = true;
> >                 ret = of_changeset_add_property(&ovcs->cset, target->np,
> >                                                 new_prop);
> > -       else
> > +
> > +       } else if (!of_prop_cmp(prop->name, "#address-cells")) {
> > +
> > +               if (prop->length != 4 || new_prop->length != 4 ||
> > +                   *(u32 *)prop->value != *(u32 *)new_prop->value)
> > +                       pr_err("ERROR: overlay and/or live tree #address-cells invalid in node %pOF\n",
> > +                              target->np);
> > +
> > +       } else if (!of_prop_cmp(prop->name, "#size-cells")) {
> > +
> > +               if (prop->length != 4 || new_prop->length != 4 ||
> > +                   *(u32 *)prop->value != *(u32 *)new_prop->value)
> > +                       pr_err("ERROR: overlay and/or live tree #size-cells invalid in node %pOF\n",
> > +                              target->np);
> > +
> > +       } else {
> > +
> > +               check_for_non_overlay_node = true;
> >                 ret = of_changeset_update_property(&ovcs->cset, target->np,
> >                                                    new_prop);
> >
> > +       }
> > +
> > +       if (check_for_non_overlay_node &&
> > +           !of_node_check_flag(target->np, OF_OVERLAY))
> > +               pr_err("WARNING: %s(), memory leak will occur if overlay removed.  Property: %pOF/%s\n",
> > +                      __func__, target->np, new_prop->name);
> > +
> >         if (ret) {
> >                 kfree(new_prop->name);
> >                 kfree(new_prop->value);
> > --
> > Frank Rowand <frank.rowand@xxxxxxxx>
> >