* Dynamic overlay failure in 4.19 & 4.20 @ 2019-06-04 12:15 Phil Elwell 2019-06-04 18:20 ` Frank Rowand 0 siblings, 1 reply; 8+ messages in thread From: Phil Elwell @ 2019-06-04 12:15 UTC (permalink / raw) To: Rob Herring, Frank Rowand, Pantelis Antoniou, devicetree, linux-kernel Hi, In the downstream Raspberry Pi kernel we are using configfs to apply overlays at runtime, using a patchset from Pantelis that hasn't been accepted upstream yet. Apart from the occasional need to adapt to upstream changes, this has been working well for us. A Raspberry Pi user recently noticed that this mechanism was failing for an overlay in 4.19. Although the overlay appeared to be applied successfully, pinctrl was reporting that one of the two fragments contained an invalid phandle, and an examination of the live DT agreed - the target of the reference, which was in the other fragment, was missing the phandle property. 5.0 added two patches - [1] to stop blindly copying properties from the overlay fragments into the live tree, and [2] to explicitly copy across the name and phandle properties. These two commits should be treated as a pair; the former requires the properties that are legitimately defined by an overlay to be added via a changeset, but this mechanism deliberately skips the name and phandle; the latter addresses this shortcoming. However, [1] was back-ported to 4.19 and 4.20 but [2] wasn't, hence the problem. The effect can be seen in the "overlay" overlay in the unittest data. Although the overlay appears to apply correctly, the hvac-large-1 node is lacking the phandle it should have as a result of the hvac_2 label, and that leaves the hvac-provider property of ride@200 with an unresolved phandle. The obvious fix is to also back-port [2] to 4.19, but that leaves open the question of whether either the overlay application mechanism or the unit test framework should have detected the missing phandle. Phil [1] 8814dc46bd9e ("of: overlay: do not duplicate properties from overlay for new nodes") [2] f96278810150 ("of: overlay: set node fields from properties when add new overlay node") ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Dynamic overlay failure in 4.19 & 4.20 2019-06-04 12:15 Dynamic overlay failure in 4.19 & 4.20 Phil Elwell @ 2019-06-04 18:20 ` Frank Rowand 2019-06-04 19:39 ` Phil Elwell 0 siblings, 1 reply; 8+ messages in thread From: Frank Rowand @ 2019-06-04 18:20 UTC (permalink / raw) To: Phil Elwell, Rob Herring, Pantelis Antoniou, devicetree, linux-kernel Hi Phil, On 6/4/19 5:15 AM, Phil Elwell wrote: > Hi, > > In the downstream Raspberry Pi kernel we are using configfs to apply overlays at > runtime, using a patchset from Pantelis that hasn't been accepted upstream yet. > Apart from the occasional need to adapt to upstream changes, this has been working > well for us. > > A Raspberry Pi user recently noticed that this mechanism was failing for an overlay in > 4.19. Although the overlay appeared to be applied successfully, pinctrl was reporting > that one of the two fragments contained an invalid phandle, and an examination of the > live DT agreed - the target of the reference, which was in the other fragment, was > missing the phandle property. > > 5.0 added two patches - [1] to stop blindly copying properties from the overlay fragments > into the live tree, and [2] to explicitly copy across the name and phandle properties. > These two commits should be treated as a pair; the former requires the properties that > are legitimately defined by an overlay to be added via a changeset, but this mechanism > deliberately skips the name and phandle; the latter addresses this shortcoming. However, > [1] was back-ported to 4.19 and 4.20 but [2] wasn't, hence the problem. I have relied upon Greg's statement that he would handle the stable kernels, and that the process of doing so would not impact (or would minimally impact) maintainers. If I think something should go into stable, I will tag it as such, but otherwise I ignore the stable branches. For overlay related code specifically, my base standard is that overlay support is an under development, not yet ready for prime time feature and thus I do not tag my overlay patches for stable. Your research and analysis above sound like there are on target (thanks for providing the clear and detailed explanation!), so if you want the stable branches to work for overlays (out of tree, as you mentioned) I would suggest you email Greg, asking that the second patch be added to the stable branches. Since the two patches you pointed out are put of a larger series, you might also want to check which of the other patches in that series were included in stable or left out from stable. My suggestion that you request Greg add the second patch continues to rely on the concept that stable does not add to my workload, so I have not carefully analyzed whether adding the patch actually is the correct and full fix, but instead am relying on your good judgment that it is. To give you some context on the patch series, I started with 144552c78692 ("of: overlay: add tests to validate kfrees from overlay removal") due to my concerns with how memory is allocated and freed for overlays. The added tests exposed issues, and the rest of the patches in the series were dealing with the issues that were exposed. You can see the full series (with maybe an extra patch or so) with git log --oneline 144552c78692^..eeb07c573ec3 > The effect can be seen in the "overlay" overlay in the unittest data. Although the > overlay appears to apply correctly, the hvac-large-1 node is lacking the phandle it > should have as a result of the hvac_2 label, and that leaves the hvac-provider property > of ride@200 with an unresolved phandle. > > The obvious fix is to also back-port [2] to 4.19, but that leaves open the question of > whether either the overlay application mechanism or the unit test framework should have > detected the missing phandle. Yes, unittest showed the missing phandle, as you described in the /proc view of overlay.dts above. That portion of unittest depends on someone looking at the result of what is in /proc/device-tree and is not automated and is not documented. I intend to automate that check someday, but the task is still languishing on my todo list. > > Phil > > [1] 8814dc46bd9e ("of: overlay: do not duplicate properties from overlay for new nodes") > [2] f96278810150 ("of: overlay: set node fields from properties when add new overlay node") > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Dynamic overlay failure in 4.19 & 4.20 2019-06-04 18:20 ` Frank Rowand @ 2019-06-04 19:39 ` Phil Elwell 0 siblings, 0 replies; 8+ messages in thread From: Phil Elwell @ 2019-06-04 19:39 UTC (permalink / raw) To: Frank Rowand, Rob Herring, Pantelis Antoniou, devicetree, linux-kernel Hi Frank, On 04/06/2019 19:20, Frank Rowand wrote: > Hi Phil, > > On 6/4/19 5:15 AM, Phil Elwell wrote: >> Hi, >> >> In the downstream Raspberry Pi kernel we are using configfs to apply overlays at >> runtime, using a patchset from Pantelis that hasn't been accepted upstream yet. >> Apart from the occasional need to adapt to upstream changes, this has been working >> well for us. >> >> A Raspberry Pi user recently noticed that this mechanism was failing for an overlay in >> 4.19. Although the overlay appeared to be applied successfully, pinctrl was reporting >> that one of the two fragments contained an invalid phandle, and an examination of the >> live DT agreed - the target of the reference, which was in the other fragment, was >> missing the phandle property. >> >> 5.0 added two patches - [1] to stop blindly copying properties from the overlay fragments >> into the live tree, and [2] to explicitly copy across the name and phandle properties. >> These two commits should be treated as a pair; the former requires the properties that >> are legitimately defined by an overlay to be added via a changeset, but this mechanism >> deliberately skips the name and phandle; the latter addresses this shortcoming. However, >> [1] was back-ported to 4.19 and 4.20 but [2] wasn't, hence the problem. > > I have relied upon Greg's statement that he would handle the stable kernels, and that > the process of doing so would not impact (or would minimally impact) maintainers. If > I think something should go into stable, I will tag it as such, but otherwise I ignore > the stable branches. For overlay related code specifically, my base standard is that > overlay support is an under development, not yet ready for prime time feature and thus > I do not tag my overlay patches for stable. > > Your research and analysis above sound like there are on target (thanks for providing > the clear and detailed explanation!), so if you want the stable branches to work for > overlays (out of tree, as you mentioned) I would suggest you email Greg, asking that > the second patch be added to the stable branches. Since the two patches you pointed > out are put of a larger series, you might also want to check which of the other > patches in that series were included in stable or left out from stable. My suggestion > that you request Greg add the second patch continues to rely on the concept that > stable does not add to my workload, so I have not carefully analyzed whether adding > the patch actually is the correct and full fix, but instead am relying on your good > judgment that it is. <useful context snipped> Thank you - I'll email Greg directly as you suggest, with your answer as supporting evidence. Phil ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <45e99a24-efb9-5473-2e57-14411537dc9f@raspberrypi.org>]
* Dynamic overlay failure in 4.19 & 4.20 [not found] <45e99a24-efb9-5473-2e57-14411537dc9f@raspberrypi.org> @ 2019-06-05 12:02 ` Phil Elwell 2019-06-05 17:50 ` Greg Kroah-Hartman 0 siblings, 1 reply; 8+ messages in thread From: Phil Elwell @ 2019-06-05 12:02 UTC (permalink / raw) To: Greg Kroah-Hartman, stable Hi, I think patch f96278810150 ("of: overlay: set node fields from properties when add new overlay node") should be back-ported to 4.19, for the reasons outlined below (briefly: without it, overlay fragments that define phandles will appear to merged successfully, but they do so without those phandles, causing any references to them to break). Frank Rowand agrees that a back-port is necessary. I can verify that it solves the issue of the missing phandles, and as far as I know it doesn't cause any regressions. However, the code in question isn't really used in mainline kernels, so I understand if you prefer to leave things as they stand now rather than risk further breaking a stable kernel. Thanks, Phil Elwell, Raspberry Pi -------- Forwarded Message -------- Subject: Re: Dynamic overlay failure in 4.19 & 4.20 Date: Tue, 4 Jun 2019 11:20:43 -0700 From: Frank Rowand <frowand.list@gmail.com> To: Phil Elwell <phil@raspberrypi.org>, Rob Herring <robh+dt@kernel.org>, Pantelis Antoniou <pantelis.antoniou@konsulko.com>, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> Hi Phil, On 6/4/19 5:15 AM, Phil Elwell wrote: > Hi, > > In the downstream Raspberry Pi kernel we are using configfs to apply overlays at > runtime, using a patchset from Pantelis that hasn't been accepted upstream yet. > Apart from the occasional need to adapt to upstream changes, this has been working > well for us. > > A Raspberry Pi user recently noticed that this mechanism was failing for an overlay in > 4.19. Although the overlay appeared to be applied successfully, pinctrl was reporting > that one of the two fragments contained an invalid phandle, and an examination of the > live DT agreed - the target of the reference, which was in the other fragment, was > missing the phandle property. > > 5.0 added two patches - [1] to stop blindly copying properties from the overlay fragments > into the live tree, and [2] to explicitly copy across the name and phandle properties. > These two commits should be treated as a pair; the former requires the properties that > are legitimately defined by an overlay to be added via a changeset, but this mechanism > deliberately skips the name and phandle; the latter addresses this shortcoming. However, > [1] was back-ported to 4.19 and 4.20 but [2] wasn't, hence the problem. I have relied upon Greg's statement that he would handle the stable kernels, and that the process of doing so would not impact (or would minimally impact) maintainers. If I think something should go into stable, I will tag it as such, but otherwise I ignore the stable branches. For overlay related code specifically, my base standard is that overlay support is an under development, not yet ready for prime time feature and thus I do not tag my overlay patches for stable. Your research and analysis above sound like there are on target (thanks for providing the clear and detailed explanation!), so if you want the stable branches to work for overlays (out of tree, as you mentioned) I would suggest you email Greg, asking that the second patch be added to the stable branches. Since the two patches you pointed out are put of a larger series, you might also want to check which of the other patches in that series were included in stable or left out from stable. My suggestion that you request Greg add the second patch continues to rely on the concept that stable does not add to my workload, so I have not carefully analyzed whether adding the patch actually is the correct and full fix, but instead am relying on your good judgment that it is. To give you some context on the patch series, I started with 144552c78692 ("of: overlay: add tests to validate kfrees from overlay removal") due to my concerns with how memory is allocated and freed for overlays. The added tests exposed issues, and the rest of the patches in the series were dealing with the issues that were exposed. You can see the full series (with maybe an extra patch or so) with git log --oneline 144552c78692^..eeb07c573ec3 > The effect can be seen in the "overlay" overlay in the unittest data. Although the > overlay appears to apply correctly, the hvac-large-1 node is lacking the phandle it > should have as a result of the hvac_2 label, and that leaves the hvac-provider property > of ride@200 with an unresolved phandle. > > The obvious fix is to also back-port [2] to 4.19, but that leaves open the question of > whether either the overlay application mechanism or the unit test framework should have > detected the missing phandle. Yes, unittest showed the missing phandle, as you described in the /proc view of overlay.dts above. That portion of unittest depends on someone looking at the result of what is in /proc/device-tree and is not automated and is not documented. I intend to automate that check someday, but the task is still languishing on my todo list. > > Phil > > [1] 8814dc46bd9e ("of: overlay: do not duplicate properties from overlay for new nodes") > [2] f96278810150 ("of: overlay: set node fields from properties when add new overlay node") > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Dynamic overlay failure in 4.19 & 4.20 2019-06-05 12:02 ` Phil Elwell @ 2019-06-05 17:50 ` Greg Kroah-Hartman 2019-06-05 18:36 ` Phil Elwell 2019-06-06 1:14 ` Sasha Levin 0 siblings, 2 replies; 8+ messages in thread From: Greg Kroah-Hartman @ 2019-06-05 17:50 UTC (permalink / raw) To: Phil Elwell; +Cc: stable On Wed, Jun 05, 2019 at 01:02:18PM +0100, Phil Elwell wrote: > Hi, > > I think patch f96278810150 ("of: overlay: set node fields from > properties when add new overlay node") should be back-ported to 4.19, > for the reasons outlined below (briefly: without it, overlay fragments > that define phandles will appear to merged successfully, but they do > so without those phandles, causing any references to them to break). That patch does not properly apply to the 4.19.y tree. Can you provide a working backport that I can queue up to resolve this? thanks, greg k-h ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Dynamic overlay failure in 4.19 & 4.20 2019-06-05 17:50 ` Greg Kroah-Hartman @ 2019-06-05 18:36 ` Phil Elwell 2019-06-06 1:14 ` Sasha Levin 1 sibling, 0 replies; 8+ messages in thread From: Phil Elwell @ 2019-06-05 18:36 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: stable On 05/06/2019 18:50, Greg Kroah-Hartman wrote: > On Wed, Jun 05, 2019 at 01:02:18PM +0100, Phil Elwell wrote: >> Hi, >> >> I think patch f96278810150 ("of: overlay: set node fields from >> properties when add new overlay node") should be back-ported to 4.19, >> for the reasons outlined below (briefly: without it, overlay fragments >> that define phandles will appear to merged successfully, but they do >> so without those phandles, causing any references to them to break). > > That patch does not properly apply to the 4.19.y tree. Can you provide > a working backport that I can queue up to resolve this? > > thanks, > > greg k-h > Yes, of course: ---- From 714e9cd6d2ceb7716bcc60e86c540f9e9b5e76f0 Mon Sep 17 00:00:00 2001 From: Frank Rowand <frank.rowand@sony.com> Date: Fri, 12 Oct 2018 19:21:16 -0700 Subject: [PATCH] of: overlay: set node fields from properties when add new overlay node commit f96278810150fc39085d1872e5b39ea06366d03e upstream. Overlay nodes added by add_changeset_node() do not have the node fields name, phandle, and type set. The node passed to __of_attach_node() when the add node changeset entry is processed does not contain any properties. The node's properties are located in add property changeset entries that will be processed after the add node changeset is applied. Set the node's fields in the node contained in the add node changeset entry and do not set them to incorrect values in add_changeset_node(). A visible symptom that is fixed by this patch is the names of nodes added by overlays that have an entry in /sys/bus/platform/drivers/*/ will contain the unit-address but the node-name will be <NULL>, for example, "fc4ab000.<NULL>". After applying the patch the name, in this example, for node restart@fc4ab000 is "fc4ab000.restart". Tested-by: Alan Tull <atull@kernel.org> Signed-off-by: Frank Rowand <frank.rowand@sony.com> --- drivers/of/dynamic.c | 27 ++++++++++++++++++--------- drivers/of/overlay.c | 34 +++++++++++++++++++++++++++------- 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c index 45c0b1f..a09c1c3 100644 --- a/drivers/of/dynamic.c +++ b/drivers/of/dynamic.c @@ -205,15 +205,24 @@ static void __of_attach_node(struct device_node *np) const __be32 *phandle; int sz; - np->name = __of_get_property(np, "name", NULL) ? : "<NULL>"; - np->type = __of_get_property(np, "device_type", NULL) ? : "<NULL>"; - - phandle = __of_get_property(np, "phandle", &sz); - if (!phandle) - phandle = __of_get_property(np, "linux,phandle", &sz); - if (IS_ENABLED(CONFIG_PPC_PSERIES) && !phandle) - phandle = __of_get_property(np, "ibm,phandle", &sz); - np->phandle = (phandle && (sz >= 4)) ? be32_to_cpup(phandle) : 0; + if (!of_node_check_flag(np, OF_OVERLAY)) { + np->name = __of_get_property(np, "name", NULL); + np->type = __of_get_property(np, "device_type", NULL); + if (!np->name) + np->name = "<NULL>"; + if (!np->type) + np->type = "<NULL>"; + + phandle = __of_get_property(np, "phandle", &sz); + if (!phandle) + phandle = __of_get_property(np, "linux,phandle", &sz); + if (IS_ENABLED(CONFIG_PPC_PSERIES) && !phandle) + phandle = __of_get_property(np, "ibm,phandle", &sz); + if (phandle && (sz >= 4)) + np->phandle = be32_to_cpup(phandle); + else + np->phandle = 0; + } np->child = NULL; np->sibling = np->parent->child; diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c index 9808aae..b2704ba 100644 --- a/drivers/of/overlay.c +++ b/drivers/of/overlay.c @@ -301,10 +301,11 @@ static int add_changeset_property(struct overlay_changeset *ovcs, struct property *new_prop = NULL, *prop; int ret = 0; - if (!of_prop_cmp(overlay_prop->name, "name") || - !of_prop_cmp(overlay_prop->name, "phandle") || - !of_prop_cmp(overlay_prop->name, "linux,phandle")) - return 0; + if (target->in_livetree) + if (!of_prop_cmp(overlay_prop->name, "name") || + !of_prop_cmp(overlay_prop->name, "phandle") || + !of_prop_cmp(overlay_prop->name, "linux,phandle")) + return 0; if (target->in_livetree) prop = of_find_property(target->np, overlay_prop->name, NULL); @@ -322,12 +323,17 @@ static int add_changeset_property(struct overlay_changeset *ovcs, if (!new_prop) return -ENOMEM; - if (!prop) + if (!prop) { + if (!target->in_livetree) { + new_prop->next = target->np->deadprops; + target->np->deadprops = new_prop; + } ret = of_changeset_add_property(&ovcs->cset, target->np, new_prop); - else + } else { ret = of_changeset_update_property(&ovcs->cset, target->np, new_prop); + } if (ret) { kfree(new_prop->name); @@ -382,9 +388,10 @@ static int add_changeset_node(struct overlay_changeset *ovcs, struct target *target, struct device_node *node) { const char *node_kbasename; + const __be32 *phandle; struct device_node *tchild; struct target target_child; - int ret = 0; + int ret = 0, size; node_kbasename = kbasename(node->full_name); @@ -398,6 +405,19 @@ static int add_changeset_node(struct overlay_changeset *ovcs, return -ENOMEM; tchild->parent = target->np; + tchild->name = __of_get_property(node, "name", NULL); + tchild->type = __of_get_property(node, "device_type", NULL); + + if (!tchild->name) + tchild->name = "<NULL>"; + if (!tchild->type) + tchild->type = "<NULL>"; + + /* ignore obsolete "linux,phandle" */ + phandle = __of_get_property(node, "phandle", &size); + if (phandle && (size == 4)) + tchild->phandle = be32_to_cpup(phandle); + of_node_set_flag(tchild, OF_OVERLAY); ret = of_changeset_attach_node(&ovcs->cset, tchild); -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Dynamic overlay failure in 4.19 & 4.20 2019-06-05 17:50 ` Greg Kroah-Hartman 2019-06-05 18:36 ` Phil Elwell @ 2019-06-06 1:14 ` Sasha Levin 2019-06-06 19:54 ` Greg Kroah-Hartman 1 sibling, 1 reply; 8+ messages in thread From: Sasha Levin @ 2019-06-06 1:14 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: Phil Elwell, stable On Wed, Jun 05, 2019 at 07:50:59PM +0200, Greg Kroah-Hartman wrote: >On Wed, Jun 05, 2019 at 01:02:18PM +0100, Phil Elwell wrote: >> Hi, >> >> I think patch f96278810150 ("of: overlay: set node fields from >> properties when add new overlay node") should be back-ported to 4.19, >> for the reasons outlined below (briefly: without it, overlay fragments >> that define phandles will appear to merged successfully, but they do >> so without those phandles, causing any references to them to break). > >That patch does not properly apply to the 4.19.y tree. Can you provide >a working backport that I can queue up to resolve this? Greg, That patch has contextual dependencies on 6f75118800 ("of: overlay: validate overlay properties #address-cells and #size-cells"), I think it would make sense to take 6f75118800 and then f96278810150 rather than a backport. -- Thanks, Sasha ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Dynamic overlay failure in 4.19 & 4.20 2019-06-06 1:14 ` Sasha Levin @ 2019-06-06 19:54 ` Greg Kroah-Hartman 0 siblings, 0 replies; 8+ messages in thread From: Greg Kroah-Hartman @ 2019-06-06 19:54 UTC (permalink / raw) To: Sasha Levin; +Cc: Phil Elwell, stable On Wed, Jun 05, 2019 at 09:14:22PM -0400, Sasha Levin wrote: > On Wed, Jun 05, 2019 at 07:50:59PM +0200, Greg Kroah-Hartman wrote: > > On Wed, Jun 05, 2019 at 01:02:18PM +0100, Phil Elwell wrote: > > > Hi, > > > > > > I think patch f96278810150 ("of: overlay: set node fields from > > > properties when add new overlay node") should be back-ported to 4.19, > > > for the reasons outlined below (briefly: without it, overlay fragments > > > that define phandles will appear to merged successfully, but they do > > > so without those phandles, causing any references to them to break). > > > > That patch does not properly apply to the 4.19.y tree. Can you provide > > a working backport that I can queue up to resolve this? > > Greg, > > That patch has contextual dependencies on 6f75118800 ("of: overlay: > validate overlay properties #address-cells and #size-cells"), I think it > would make sense to take 6f75118800 and then f96278810150 rather than a > backport. That works better, I have now done this, thanks. greg k-h ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-06-06 19:54 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-04 12:15 Dynamic overlay failure in 4.19 & 4.20 Phil Elwell 2019-06-04 18:20 ` Frank Rowand 2019-06-04 19:39 ` Phil Elwell [not found] <45e99a24-efb9-5473-2e57-14411537dc9f@raspberrypi.org> 2019-06-05 12:02 ` Phil Elwell 2019-06-05 17:50 ` Greg Kroah-Hartman 2019-06-05 18:36 ` Phil Elwell 2019-06-06 1:14 ` Sasha Levin 2019-06-06 19:54 ` Greg Kroah-Hartman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.