> 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)
> 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)
> ...
> 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. I would imagine
that the version flag (or something similar) could be used to anger the
kernel rather than creating strangely named properties.
> ...
> > 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);
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;
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.
Best,
Erich E. Hoover, Ph.D.
erich.hoover@xxxxxxxxxxxx
