Re: [PATCH] of: of_property_read_string return -ENODATA when !length

[Rob Herring <robh+dt@xxxxxxxxxx>]

On Fri, Apr 1, 2022 at 3:49 PM Stefano Stabellini
<sstabellini@xxxxxxxxxx> wrote:
>
> On Fri, 1 Apr 2022, Rob Herring wrote:
> > On Thu, Mar 31, 2022 at 7:46 PM Stefano Stabellini
> > <sstabellini@xxxxxxxxxx> wrote:
> > >
> > > From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > >
> > > When the length of the string is zero of_property_read_string should
> > > return -ENODATA according to the description of the function.
> >
> > Perhaps it is a difference of:
> >
> > prop;
> >
> > vs.
> >
> > prop = "";
> >
> > Both are 0 length by some definition. The description, '-ENODATA if
> > property does not have a value', matches the first case.
> >
> > >
> > > However, of_property_read_string doesn't check pp->length. If pp->length
> > > is zero, return -ENODATA.
> > >
> > > Without this patch the following command in u-boot:
> > >
> > > fdt set /chosen/node property-name
> > >
> > > results in of_property_read_string returning -EILSEQ when attempting to
> > > read property-name. With this patch, it returns -ENODATA as expected.
> >
> > Why do you care? Do you have a user? There could be an in tree user
> > that doesn't like this change.
>
> During review of a Xen patch series (we have libfdt is Xen too, synced
> with the kernel) Julien noticed a check for -EILSEQ. I added the check
> so that Xen would behave correctly in cases like the u-boot example in
> the patch description.
>
> Looking more into it, it seemed to be a mismatch between the description
> of of_property_read_string and the behavior (e.g. -ENODATA would seem to
> be the right return value, not -EILSEQ.)
>
> I added a printk to confirm what was going on when -EILSEQ was returned:
>
> printk("DEBUG %s %d value=%s value[0]=%d length=%u 
> len=%lu\n",__func__,__LINE__,(char*)pp->value, *((char*)pp->value),pp->length,
> strlen(pp->value));
>
> This is the output:
> DEBUG of_property_read_string 205 value= value[0]=0 length=0 len=0

It turns out that we never set pp->value to NULL when unflattening
(and libfdt always returns a value). This function is assuming we do.
>
> As the description says:
>
>  *
>  * Return: 0 on success, -EINVAL if the property does not exist, -ENODATA if
>  * property does not have a value, and -EILSEQ if the string is not
>  * null-terminated within the length of the property data.
>  *
>
> It seems that this case matches "property does not have a value" which
> is expected to be -ENODATA instead of -EILSEQ. I guess one could also
> say that length is zero, so the string cannot be null-terminated,
> thus -EILSEQ?
>
> I am happy to go with your interpretation but -ENODATA seems to be the
> best match in my opinion.

I agree. I just think empty property should have a NULL value and 0
length, but we should only have to check one. I don't want check
length as that could be different for Sparc or non-FDT. So I think we
need this instead:

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index ec315b060cd5..d6b2b0d49d89 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -165,7 +165,7 @@ static void populate_properties(const void *blob,

                pp->name   = (char *)pname;
                pp->length = sz;
-               pp->value  = (__be32 *)val;
+               pp->value  = sz ? (__be32 *)val : NULL;
                *pprev     = pp;
                pprev      = &pp->next;
        }


It looks like setting 'value' has been like this at least since 2010.

Rob