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