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>
> >
