Re: [PATCH 5/6] OF: Utility helper functions for dynamic nodes

[Grant Likely <grant.likely@secretlab.ca>]

On Tue, 24 Jun 2014 10:10:01 +0200, Alexander Sverdlin <alexander.sverdlin@nsn.com> wrote:
> Hi Pantelis, Grant,
> 
> On 23/06/14 20:33, Ioan Nicu wrote:
> >>> On 22/06/14 11:40, ext Pantelis Antoniou wrote:
> >>>> Introduce helper functions for working with the live DT tree,
> >>>> all of them related to dynamically adding/removing nodes and
> >>>> properties.
> >>>>
> >>>> __of_copy_property() copies a property dynamically
> >>>> __of_create_empty_node() creates an empty node
> >>>>
> >>>> Bug fix about prop->len == 0 by Ionut Nicu <ioan.nicu.ext@nsn.com>
> >>>
> >>> Are you sure about this? (see below...)
> >>>
> > 
> > Alexander is right, my fix was lost even though it's mentioned in this patch.
> > 
> >>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> >>>> ---
> >>
> >> [snip]
> >>>> +
> >>>> +	if (prop->length > 0) {
> >>>        ^^^^^^^^^^^^^^^^^^^^^
> >>> Seems, that length==0 case will still produce value==NULL results,
> >>> which will brake some checks in the kernel... Or am I missing something in
> >>> the new version?
> >>>
> >>
> >> prop->value will be set to NULL, and length will be set to zero (kzalloc).
> >> This is a normal zero length property.
> >>
> >> I don't know of any place in the kernel accessing the value if prop->length==0
> >>
> > 
> > We have a simple use case. We have an overlay which adds an interrupt controller.
> > If you look in drivers/of/irq.c, in of_irq_parse_raw():
> > 
> > [...]
> > 	/* Now start the actual "proper" walk of the interrupt tree */
> > 	while (ipar != NULL) {
> > 		/* Now check if cursor is an interrupt-controller and if it is
> > 		 * then we are done
> > 		 */
> > 		if (of_get_property(ipar, "interrupt-controller", NULL) !=
> 
> We have to define, if it's allowed for an empty property to have NULL value.
> Several places in the kernel use of_get_property() to check for property existence.
> We either have to make a tree-wide patch and replace of_get_property() with of_find_property() in those cases,
> or ensure value != NULL in this copy function...
> 
> Grant, what do you think?

I think it's a good cleanup, but until it happens, any new code needs to
match the behaviour of fdt.c. In that case, making the value point to an
empty string is a sane choice.  If you do the legwork of finding callers
and fixing them up, then I'll apply it. Once done we can make the
following change to fdt.c:

- 			pp->value = (__be32 *)p;
+ 			pp->value = sz ? (__be32 *)p : 0;

pdt.c similarly needs to be updated.

I'm not overly worried though. That code has been in place for a very
long time, so there is no rush. It can exist a while longer.

g.