Re: [EXTERNAL] Re: Array manipulation revisit

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6665 bytes --]

On Tue, Aug 06, 2024 at 04:29:45PM +0000, Hoover, Erich (Orion) wrote:
> > From: David Gibson
> > Sent: Monday, August 5, 2024 8:19 PM
> > ...
> > > It seems like string arrays are the only complicated case,
> > 
> > Not really.  Mixed properties that have both integers and strings in
> > them get even worse.
> 
> I was not thinking of this case, but if you want to handle this case in an initial implementation I would suggest:
>    PROPERTY[1*5+s*1+4*0] = /bits/ 32 0x0;
> (skip five bytes, skip a string, replace first 32-bit array element)

Already a substantial increase in the complexity of the overlay-time
parser.

Now.. the case where the number of integers before the strings isn't
know at overlay creation time, but comes from a separate property..

My point here isn't so much that these are super useful cases of
themselves.  But just as the string array replacement seemed an easy
and obvious extension to you, there's a whole series of "easy and
obvious" additional steps beyond that lead to something of horrific
complexity.  So far I'm not seeing an obvious boundary to set to limit
the scope of this.

> > Those aren't common, but they exist.  Or
> > properties that have a mixture of integer sizes - that's much more
> > common, since it can include 'reg', 'ranges' and 'interrupts'.
> 
> You could do something similar to the above or, for simplicity, just count bytes:
>    PROPERTY[1*5] = /bits/ 32 0x0;
> (replace 32-bit value starting at fifth byte)

*If* you know the byte offset when you're creating the overlay, which
is not always the case.

> > ...
> > Ah, so you're suggesting putting those annotations in the property
> > name itself inside the dtbo.  That's not a bad idea as encodings go.
> 
> Yes, in this case I'm thinking that the dtbo needs to be handled by libfdt
> (or a similar tool) before being passed to the kernel.  Though the kernel
> could conceivably support something like this directly.
> 
> > > For older utilities, or ones without the
> > > handling for this extra information, the utility generates an error
> > > or just adds a strangely named property that gets ignored (when I
> > > gave this a quick try by hex editing a DTBO, the kernel happily gave
> > > me a "gpio-line-names[s*148]").
> > 
> > Right, but then you silently get output that wasn't what you expected
> > which can be nasty to debug.
> 
> Well, libfdt does the right thing and generates an error.

Huh.  I didn't think we validated property names during overlay
application.  What error exactly is it?

> I would imagine
> that the version flag (or something similar) could be used to anger the
> kernel rather than creating strangely named properties.

That's harder than you'd think.  The version number is a dtb
construct, and as noted dtbo is a hack on top of that.  There's (alas)
no version number covering how overlay things are encoded in the dtb,
only for the low-level dtb encoding of properties and nodes.

> > ...
> > > Yeah, I understand why they did it the way they did - but it
> > > definitely makes this particular task harder.  Some of our systems
> > > are incredibly storage-constrained,
> >
> > Well, that raises another issue: this would add a substantial amount
> > of code to libfdt, which is supposed to be runnable in very
> > constrained environments.
> > ...
> 
> I did a quick prototype of the string case in dtmerge, for convenience, and it doesn't seem too bad to me:
> ===
>         dtoverlay_debug("  +prop(%s)", prop_name);
>         prop_name_len = strlen(prop_name);
>         array = memchr(prop_name, '[', prop_name_len);
>         array_end = memchr(prop_name, ']', prop_name_len);
>         if (array && array_end && array_end - array >= 4 && array[2] == '*')
>         {
>             if (array[1] != 's')
>             {
>                 dtoverlay_error("  unhandled overlay array type: %s", prop_name);
>                 err = -FDT_ERR_BADSTRUCTURE;
>                 break;
>             }
>             prop_name_local = malloc(prop_name_len);

Note that libfdt is designed to work in very constrained environments
and is not permitted to use an allocator (that's why there are all
those _namelen() variants).

>             strncpy(prop_name_local, prop_name, (array - prop_name));
>             prop_name = prop_name_local;
>         }
>         else if (array && array_end)
>         {
>             dtoverlay_error("  invalid overlay array: %s", prop_name);
>             err = -FDT_ERR_BADSTRUCTURE;

Hrm, that makes me think of another problem.  The more complex the
syntax allowed here, the more inadequate simple error return codes
become to communicate what's wrong, which is all we have in libfdt.

>             break;
>         }
> 
>         if (array && array_end &&
>             ((target_prop = fdt_get_property_w(base_dtb->fdt, target_off, prop_name, &target_len)) != NULL) &&
>             (target_len > 0))
>         {
>             int before_len, after_len;
>             char *old_val, *new_val;
>             int array_element;
>             const char *p;
> 
>             array_element = strtoul(&array[3], NULL, 10);
>             old_val = target_prop->data;
>             new_val = malloc(prop_len + target_len);
>             p = fdt_stringlist_get(base_dtb->fdt, target_off, prop_name, array_element, NULL);
>             if (!p)
>             {
>                 err = -FDT_ERR_NOTFOUND;
>                 break;
>             }
>             before_len = p - old_val;
>             p = fdt_stringlist_get(base_dtb->fdt, target_off, prop_name, array_element + 1, NULL);
>             after_len = (p ? target_len - (p - old_val) : 0);
>             memcpy(&new_val[0], old_val, before_len);
>             memcpy(&new_val[before_len], prop_val, prop_len);
>             memcpy(&new_val[before_len+prop_len], p, after_len);
>             err = fdt_setprop(base_dtb->fdt, target_off, prop_name, new_val, before_len + after_len + prop_len);
>             free(new_val);
>         }
>         else if ...
> 
>         free(prop_name_local);
> ===
> 
> Do you consider something like this to be too much?  I feel like
> adding support for the integer array cases is pretty trivial, but if
> you're wanting to support mixed property types then the error
> checking on that would get pretty complicated.

Yes, it would :/.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]