Re: [PATCH 1/3] of: overlay_adjust_phandles() - do not modify const field

[Frank Rowand]

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
>