Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S978787AbdDXWRF (ORCPT ); Mon, 24 Apr 2017 18:17:05 -0400 Received: from mail-io0-f195.google.com ([209.85.223.195]:34165 "EHLO mail-io0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S978732AbdDXWQ6 (ORCPT ); Mon, 24 Apr 2017 18:16:58 -0400 Subject: Re: [PATCH 1/3] of: overlay_adjust_phandles() - do not modify const field To: Rob Herring , Benjamin Herrenschmidt References: <1493011204-27635-1-git-send-email-frowand.list@gmail.com> <1493011204-27635-2-git-send-email-frowand.list@gmail.com> <58FE49F4.7060600@gmail.com> Cc: Stephen Boyd , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" From: Frank Rowand Message-ID: <58FE793D.7020908@gmail.com> Date: Mon, 24 Apr 2017 15:16:29 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5042 Lines: 116 On 04/24/17 14:40, Rob Herring wrote: > +Ben H > > On Mon, Apr 24, 2017 at 1:54 PM, Frank Rowand wrote: >> On 04/24/17 09:56, Rob Herring wrote: >>> On Mon, Apr 24, 2017 at 12:20 AM, wrote: >>>> From: Frank Rowand >>>> >>>> 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 >