On Tue, 24 Jun 2014 10:10:01 +0200, Alexander Sverdlin <alexander.sverdlin@xxxxxxx> 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@xxxxxxx> > >>> > >>> 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@xxxxxxxxxxxx> > >>>> --- > >> > >> [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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html