On 04/24/17 14:40, Rob Herring wrote: > +Ben H > > On Mon, Apr 24, 2017 at 1:54 PM, Frank Rowand <frowand.list@xxxxxxxxx> wrote: >> On 04/24/17 09:56, Rob Herring wrote: >>> On Mon, Apr 24, 2017 at 12:20 AM, <frowand.list@xxxxxxxxx> wrote: >>>> From: Frank Rowand <frank.rowand@xxxxxxxx> >>>> >>>> When adjusting overlay phandles to apply to the live device tree, can >>>> not modify the property value because it is type const. >>>> >>>> This is to resolve the issue found by Stephen Boyd [1] when he changed >>>> the type of struct property.value from void * to const void *. As >>>> a result of the type change, the overlay code had compile errors >>>> where the resolver updates phandle values. >>> >>> Conceptually, I prefer your first version. phandles are special and >>> there's little reason to expose them except to generate a dts or dtb >>> from /proc/device-tree. We could still generate the phandle file in >>> that case, but I don't know if special casing phandle is worth it. >> >> The biggest thing that makes me wary about my first version is PPC >> and Sparc. I can read their code, but do not know what the firmware >> is feeding into the kernel, so I felt like there might be some >> incorrect assumptions or fundamental misunderstandings that I >> may have. > > I never let that stop me. ;) I guess one question is does > "ibm,phandle" need to be exposed to userspace. Maybe Ben has some > thoughts. > >> If we do remove the phandle properties from the live tree, I think >> that phandle still needs to be exposed in /proc/device-tree >> because that is important information for being able to understand >> (or debug) code via reading the source. It isn't a lot code. >> >> One factor I was not sure of to help choose between the first version >> and this approach is net memory size of the device tree: >> >> first version: >> >> Adds struct bin_attribute (28 bytes on 32 bit arm) to every node > > You could do a pointer to the struct instead. > >> Removes "linux,phandle" and "phandle" properties from nodes that >> have a phandle (64 + 72 = 136 bytes) > > I don't think it is that high because we don't copy the property names > and values. It's just 2x sizeof(struct property) plus whatever > overhead each unflatten_dt_alloc has. > >> second version plus subsequent "linux,phandle" removal: >> >> Removes "linux,phandle" properties from nodes >> that have a phandle (72 bytes) >> >> I do not have a feel of how many nodes have phandles in the many >> different device trees, so I'm not sure of the end result for the >> first version. > > Probably well under half. Though in theory dtc could create a phandle > for every node. > >> I do not have a strong preference between my first approach and second >> approach. But now that I have done both, a choice can be made. Let me >> know which way you want to go and I'll respin the one you prefer. >> For this version I'll make the change you suggested. For the first >> version, I'll modify of_attach_mode() slightly more to remove any >> "phandle", "linux,phandle", and "ibm,phandle" property from the node >> before attaching it, and add the call to add the phandle sysfs >> file: __of_add_phandle_sysfs(np); > > I still lean toward the former, but I guess it depends if there are > really any size savings. OK, I'll respin the first approach. > > Why would you remove properties in of_attach_mode? You would want to > convert populate_properties() to store the phandle from the start and > never create the properties. For a tree that gets created by unflatten, yes, the phandle property is never created with this patch applied. But PPC adds nodes (and their properties) through of_attach_node(), so this is where I can avoid copying phandle properties into the live tree. > [...] > >>>> + prop = __of_find_property(overlay, "phandle", NULL); >>>> + newprop = __of_prop_alloc(prop->name, &phandle, sizeof(phandle), >>>> + GFP_KERNEL); >>>> + if (!newprop) >>>> + return -ENOMEM; >>>> + __of_update_property(overlay, newprop, &prop); >>>> + >>>> + prop = __of_find_property(overlay, "linux,phandle", NULL); >>>> + newprop = __of_prop_alloc(prop->name, &phandle, sizeof(phandle), >>>> + GFP_KERNEL); >>> >>> There is no reason to support "linux,phandle" for overlays. That is >>> legacy (pre ePAPR) which predates any overlays by a long time. >> >> I would like to have the same behavior for non-overlays as for overlays. >> The driver is the same whether the device tree description comes from >> the initial device tree or from an overlay. > > Agreed. You only need to store one of them for both base and overlays. > You could go further and just ignore them altogether for overlays as > we should never have any with linux,phandle only, but that doesn't > really matter as it won't affect the memory usage. > > Rob > -- 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
