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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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

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.


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 8e90071de6ed..da0f02c98bb2 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -439,7 +439,7 @@ int of_property_read_string(const struct device_node 
> > *np, const char *propname,
> >         const struct property *prop = of_find_property(np, propname, NULL);
> >         if (!prop)
> >                 return -EINVAL;
> > -       if (!prop->value)
> > +       if (!prop->value || !pp->length)
> >                 return -ENODATA;
> >         if (strnlen(prop->value, prop->length) >= prop->length)
> >                 return -EILSEQ;
>