On Fri, 1 Apr 2022, Rob Herring wrote:
> 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.
Yep, fixing old bugs nobody cares about, that's me :-)
I made the corresponding change to Xen to check that it fixes the
original issue (I am using Xen only for convenience because I already
have a reproducer setup for it.)
Unfortunately it breaks the boot: the reason is that of_get_property is
implemented as:
return pp ? pp->value : NULL;
and at least in Xen (maybe in Linux too) there are instances of callers
doing:
if (!of_get_property(node, "interrupt-controller", NULL))
continue;
This would now take the wrong code path because value is returned as
NULL.
So, although your patch is technically correct, it comes with higher
risk of breaking existing code. How do you want to proceed?
