From a1a2b7125e1079cfcc13a116aa3af3df2f9e002b Mon Sep 17 00:00:00 2001 From: Lad Prabhakar Date: Wed, 16 Mar 2022 20:06:33 +0000 Subject: of/platform: Drop static setup of IRQ resource from DT core Now that all the DT drivers have switched to platform_get_irq() we can now safely drop the static setup of IRQ resource from DT core code. With the above change hierarchical setup of irq domains is no longer bypassed and thus allowing hierarchical interrupt domains to describe interrupts using "interrupts" DT property. Signed-off-by: Lad Prabhakar Acked-by: Marc Zyngier Tested-by: Marc Zyngier Signed-off-by: Rob Herring Link: https://lore.kernel.org/r/20220316200633.28974-1-prabhakar.mahadev-lad.rj@bp.renesas.com --- drivers/of/platform.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) (limited to 'drivers/of') diff --git a/drivers/of/platform.c b/drivers/of/platform.c index a16b74f32aa9..fdc8491eed86 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -114,35 +114,31 @@ struct platform_device *of_device_alloc(struct device_node *np, struct device *parent) { struct platform_device *dev; - int rc, i, num_reg = 0, num_irq; + int rc, i, num_reg = 0; struct resource *res, temp_res; dev = platform_device_alloc("", PLATFORM_DEVID_NONE); if (!dev) return NULL; - /* count the io and irq resources */ + /* count the io resources */ while (of_address_to_resource(np, num_reg, &temp_res) == 0) num_reg++; - num_irq = of_irq_count(np); /* Populate the resource table */ - if (num_irq || num_reg) { - res = kcalloc(num_irq + num_reg, sizeof(*res), GFP_KERNEL); + if (num_reg) { + res = kcalloc(num_reg, sizeof(*res), GFP_KERNEL); if (!res) { platform_device_put(dev); return NULL; } - dev->num_resources = num_reg + num_irq; + dev->num_resources = num_reg; dev->resource = res; for (i = 0; i < num_reg; i++, res++) { rc = of_address_to_resource(np, i, res); WARN_ON(rc); } - if (of_irq_to_resource_table(np, res, num_irq) != num_irq) - pr_debug("not all legacy IRQ resources mapped for %pOFn\n", - np); } dev->dev.of_node = of_node_get(np); -- cgit v1.2.3 From e91033621d56e055803c4c4ba507fbbb2d145a7f Mon Sep 17 00:00:00 2001 From: Samuel Holland Date: Tue, 12 Apr 2022 00:15:28 -0500 Subject: of/irq: Use interrupts-extended to find parent The RISC-V PLIC binding uses interrupts-extended to specify its parent domain(s). That binding does not allow the interrupt-parent property to appear in the irqchip node. This prevents of_irq_init from properly detecting the irqchip hierarchy. If no interrupt-parent property is present in the enclosing bus or root node, then desc->interrupt_parent will be NULL for both the per-CPU RISC-V INTC (the actual root domain) and the RISC-V PLIC. Similarly, if the bus or root node specifies `interrupt-parent = <&plic>`, then of_irq_init will hit the `desc->interrupt_parent == np` check, and again all parents will be NULL. So things happen to work today for some boards due to Makefile ordering. However, things break when another irqchip ("foo") is stacked on top of the PLIC. The bus or root node will have `interrupt-parent = <&foo>`, since that is what all of the other peripherals need. When of_irq_init runs, it will try to find the PLIC's parent domain. of_irq_find_parent will fall back to using the interrupt-parent property of the PLIC's parent node (i.e. the bus or root node), and of_irq_init will see "foo" as the PLIC's parent domain. But this is wrong, because "foo" is actually the PLIC's child domain! So of_irq_init wrongly attempts to init the stacked irqchip before the PLIC. This fails and breaks booting. Fix this by using the first node referenced by interrupts-extended as the parent when that property is present. This allows of_irq_init to see the relationship between the PLIC and the per-CPU RISC-V INTC, and thus only the RISC-V INTC is (correctly) considered a root domain. Signed-off-by: Samuel Holland Signed-off-by: Rob Herring Link: https://lore.kernel.org/r/20220412051529.6293-1-samuel@sholland.org --- drivers/of/irq.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) (limited to 'drivers/of') diff --git a/drivers/of/irq.c b/drivers/of/irq.c index 2b07677a386b..aa485eb312ac 100644 --- a/drivers/of/irq.c +++ b/drivers/of/irq.c @@ -550,9 +550,18 @@ void __init of_irq_init(const struct of_device_id *matches) desc->irq_init_cb = match->data; desc->dev = of_node_get(np); - desc->interrupt_parent = of_irq_find_parent(np); - if (desc->interrupt_parent == np) + /* + * interrupts-extended can reference multiple parent domains. + * Arbitrarily pick the first one; assume any other parents + * are the same distance away from the root irq controller. + */ + desc->interrupt_parent = of_parse_phandle(np, "interrupts-extended", 0); + if (!desc->interrupt_parent) + desc->interrupt_parent = of_irq_find_parent(np); + if (desc->interrupt_parent == np) { + of_node_put(desc->interrupt_parent); desc->interrupt_parent = NULL; + } list_add_tail(&desc->list, &intc_desc_list); } -- cgit v1.2.3 From f688d61925f28699872307a66919bd164ba42ce0 Mon Sep 17 00:00:00 2001 From: Stefano Stabellini Date: Fri, 15 Apr 2022 17:30:28 -0700 Subject: of: of_property_read_string return -ENODATA when !length When the length of the string is zero of_property_read_string should return -ENODATA according to the description of the function. However, of_property_read_string doesn't check prop->length. If prop->length is zero, return -ENODATA. Without this patch the following command in u-boot: fdt set /chosen/node property-name results in of_property_read_string returning -EILSEQ when attempting to read property-name. With this patch, it returns -ENODATA as expected. Signed-off-by: Stefano Stabellini Signed-off-by: Rob Herring Link: https://lore.kernel.org/r/20220416003028.1315268-1-sstabellini@kernel.org --- drivers/of/property.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'drivers/of') diff --git a/drivers/of/property.c b/drivers/of/property.c index 8e90071de6ed..84903dad96a4 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -431,6 +431,9 @@ EXPORT_SYMBOL_GPL(of_property_read_variable_u64_array); * property does not have a value, and -EILSEQ if the string is not * null-terminated within the length of the property data. * + * Note that the empty string "" has length of 1, thus -ENODATA cannot + * be interpreted as an empty string. + * * The out_string pointer is modified only if a valid string can be decoded. */ int of_property_read_string(const struct device_node *np, const char *propname, @@ -439,7 +442,7 @@ int of_property_read_string(const struct device_node *np, const char *propname, const struct property *prop = of_find_property(np, propname, NULL); if (!prop) return -EINVAL; - if (!prop->value) + if (!prop->length) return -ENODATA; if (strnlen(prop->value, prop->length) >= prop->length) return -EILSEQ; -- cgit v1.2.3 From 5f756a2eaa4436d7d3dc1e040147f5e992ae34b5 Mon Sep 17 00:00:00 2001 From: Nuno Sá Date: Wed, 20 Apr 2022 15:02:05 +0200 Subject: of: overlay: do not break notify on NOTIFY_{OK|STOP} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We should not break overlay notifications on NOTIFY_{OK|STOP} otherwise we might break on the first fragment. We should only stop notifications if a *real* errno is returned by one of the listeners. Fixes: a1d19bd4cf1fe ("of: overlay: pr_err from return NOTIFY_OK to overlay apply/remove") Signed-off-by: Nuno Sá Signed-off-by: Rob Herring Link: https://lore.kernel.org/r/20220420130205.89435-1-nuno.sa@analog.com --- drivers/of/overlay.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'drivers/of') diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c index d80160cf34bb..d1187123c4fc 100644 --- a/drivers/of/overlay.c +++ b/drivers/of/overlay.c @@ -170,9 +170,7 @@ static int overlay_notify(struct overlay_changeset *ovcs, ret = blocking_notifier_call_chain(&overlay_notify_chain, action, &nd); - if (ret == NOTIFY_OK || ret == NOTIFY_STOP) - return 0; - if (ret) { + if (notifier_to_errno(ret)) { ret = notifier_to_errno(ret); pr_err("overlay changeset %s notifier error %d, target: %pOF\n", of_overlay_action_name[action], ret, nd.target); -- cgit v1.2.3 From 1e4089667c7c732dd1b92c4c6bc7bd240ca30213 Mon Sep 17 00:00:00 2001 From: Frank Rowand Date: Wed, 20 Apr 2022 17:25:04 -0500 Subject: of: overlay: rename variables to be consistent Variables change name across function calls when there is not a good reason to do so. Fix by changing "fdt" to "new_fdt" and "tree" to "overlay_root". The name disparity was confusing when creating the following commit. The name changes are in this separate commit to make review of the following commmit less complex. Signed-off-by: Frank Rowand Signed-off-by: Rob Herring Link: https://lore.kernel.org/r/20220420222505.928492-2-frowand.list@gmail.com --- drivers/of/overlay.c | 94 ++++++++++++++++++++++++++-------------------------- 1 file changed, 47 insertions(+), 47 deletions(-) (limited to 'drivers/of') diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c index d1187123c4fc..6df9ecdf16cf 100644 --- a/drivers/of/overlay.c +++ b/drivers/of/overlay.c @@ -57,8 +57,8 @@ struct fragment { * struct overlay_changeset * @id: changeset identifier * @ovcs_list: list on which we are located - * @fdt: base of memory allocated to hold aligned FDT that was unflattened to create @overlay_tree - * @overlay_tree: expanded device tree that contains the fragment nodes + * @new_fdt: Memory allocated to hold unflattened aligned FDT + * @overlay_root: expanded device tree that contains the fragment nodes * @count: count of fragment structures * @fragments: fragment nodes in the overlay expanded device tree * @symbols_fragment: last element of @fragments[] is the __symbols__ node @@ -67,8 +67,8 @@ struct fragment { struct overlay_changeset { int id; struct list_head ovcs_list; - const void *fdt; - struct device_node *overlay_tree; + const void *new_fdt; + struct device_node *overlay_root; int count; struct fragment *fragments; bool symbols_fragment; @@ -183,7 +183,7 @@ static int overlay_notify(struct overlay_changeset *ovcs, /* * The values of properties in the "/__symbols__" node are paths in - * the ovcs->overlay_tree. When duplicating the properties, the paths + * the ovcs->overlay_root. When duplicating the properties, the paths * need to be adjusted to be the correct path for the live device tree. * * The paths refer to a node in the subtree of a fragment node's "__overlay__" @@ -219,7 +219,7 @@ static struct property *dup_and_fixup_symbol_prop( if (path_len < 1) return NULL; - fragment_node = __of_find_node_by_path(ovcs->overlay_tree, path + 1); + fragment_node = __of_find_node_by_path(ovcs->overlay_root, path + 1); overlay_node = __of_find_node_by_path(fragment_node, "__overlay__/"); of_node_put(fragment_node); of_node_put(overlay_node); @@ -716,19 +716,20 @@ static struct device_node *find_target(struct device_node *info_node) /** * init_overlay_changeset() - initialize overlay changeset from overlay tree - * @ovcs: Overlay changeset to build - * @fdt: base of memory allocated to hold aligned FDT that was unflattened to create @tree - * @tree: Contains the overlay fragments and overlay fixup nodes + * @ovcs: Overlay changeset to build + * @new_fdt: Memory allocated to hold unflattened aligned FDT + * @overlay_root: Contains the overlay fragments and overlay fixup nodes * * Initialize @ovcs. Populate @ovcs->fragments with node information from - * the top level of @tree. The relevant top level nodes are the fragment - * nodes and the __symbols__ node. Any other top level node will be ignored. + * the top level of @overlay_root. The relevant top level nodes are the + * fragment nodes and the __symbols__ node. Any other top level node will + * be ignored. * * Return: 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error - * detected in @tree, or -ENOSPC if idr_alloc() error. + * detected in @overlay_root, or -ENOSPC if idr_alloc() error. */ static int init_overlay_changeset(struct overlay_changeset *ovcs, - const void *fdt, struct device_node *tree) + const void *new_fdt, struct device_node *overlay_root) { struct device_node *node, *overlay_node; struct fragment *fragment; @@ -739,17 +740,17 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, * Warn for some issues. Can not return -EINVAL for these until * of_unittest_apply_overlay() is fixed to pass these checks. */ - if (!of_node_check_flag(tree, OF_DYNAMIC)) - pr_debug("%s() tree is not dynamic\n", __func__); + if (!of_node_check_flag(overlay_root, OF_DYNAMIC)) + pr_debug("%s() overlay_root is not dynamic\n", __func__); - if (!of_node_check_flag(tree, OF_DETACHED)) - pr_debug("%s() tree is not detached\n", __func__); + if (!of_node_check_flag(overlay_root, OF_DETACHED)) + pr_debug("%s() overlay_root is not detached\n", __func__); - if (!of_node_is_root(tree)) - pr_debug("%s() tree is not root\n", __func__); + if (!of_node_is_root(overlay_root)) + pr_debug("%s() overlay_root is not root\n", __func__); - ovcs->overlay_tree = tree; - ovcs->fdt = fdt; + ovcs->overlay_root = overlay_root; + ovcs->new_fdt = new_fdt; INIT_LIST_HEAD(&ovcs->ovcs_list); @@ -762,7 +763,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, cnt = 0; /* fragment nodes */ - for_each_child_of_node(tree, node) { + for_each_child_of_node(overlay_root, node) { overlay_node = of_get_child_by_name(node, "__overlay__"); if (overlay_node) { cnt++; @@ -770,7 +771,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, } } - node = of_get_child_by_name(tree, "__symbols__"); + node = of_get_child_by_name(overlay_root, "__symbols__"); if (node) { cnt++; of_node_put(node); @@ -783,7 +784,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, } cnt = 0; - for_each_child_of_node(tree, node) { + for_each_child_of_node(overlay_root, node) { overlay_node = of_get_child_by_name(node, "__overlay__"); if (!overlay_node) continue; @@ -805,7 +806,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, * if there is a symbols fragment in ovcs->fragments[i] it is * the final element in the array */ - node = of_get_child_by_name(tree, "__symbols__"); + node = of_get_child_by_name(overlay_root, "__symbols__"); if (node) { ovcs->symbols_fragment = 1; fragment = &fragments[cnt]; @@ -859,12 +860,12 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs) } kfree(ovcs->fragments); /* - * There should be no live pointers into ovcs->overlay_tree and - * ovcs->fdt due to the policy that overlay notifiers are not allowed - * to retain pointers into the overlay devicetree. + * There should be no live pointers into ovcs->overlay_root and + * ovcs->new_fdt due to the policy that overlay notifiers are not + * allowed to retain pointers into the overlay devicetree. */ - kfree(ovcs->overlay_tree); - kfree(ovcs->fdt); + kfree(ovcs->overlay_root); + kfree(ovcs->new_fdt); kfree(ovcs); } @@ -872,16 +873,15 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs) * internal documentation * * of_overlay_apply() - Create and apply an overlay changeset - * @fdt: base of memory allocated to hold the aligned FDT - * @tree: Expanded overlay device tree - * @ovcs_id: Pointer to overlay changeset id + * @new_fdt: Memory allocated to hold the aligned FDT + * @overlay_root: Expanded overlay device tree + * @ovcs_id: Pointer to overlay changeset id * * Creates and applies an overlay changeset. * * If an error occurs in a pre-apply notifier, then no changes are made * to the device tree. * - * A non-zero return value will not have created the changeset if error is from: * - parameter checks * - building the changeset @@ -911,8 +911,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs) * id is returned to *ovcs_id. */ -static int of_overlay_apply(const void *fdt, struct device_node *tree, - int *ovcs_id) +static int of_overlay_apply(const void *new_fdt, + struct device_node *overlay_root, int *ovcs_id) { struct overlay_changeset *ovcs; int ret = 0, ret_revert, ret_tmp; @@ -924,16 +924,16 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree, if (devicetree_corrupt()) { pr_err("devicetree state suspect, refuse to apply overlay\n"); - kfree(fdt); - kfree(tree); + kfree(new_fdt); + kfree(overlay_root); ret = -EBUSY; goto out; } ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL); if (!ovcs) { - kfree(fdt); - kfree(tree); + kfree(new_fdt); + kfree(overlay_root); ret = -ENOMEM; goto out; } @@ -941,20 +941,20 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree, of_overlay_mutex_lock(); mutex_lock(&of_mutex); - ret = of_resolve_phandles(tree); + ret = of_resolve_phandles(overlay_root); if (ret) goto err_free_tree; - ret = init_overlay_changeset(ovcs, fdt, tree); + ret = init_overlay_changeset(ovcs, new_fdt, overlay_root); if (ret) goto err_free_tree; /* - * after overlay_notify(), ovcs->overlay_tree related pointers may have + * After overlay_notify(), ovcs->overlay_root related pointers may have * leaked to drivers, so can not kfree() tree, aka ovcs->overlay_tree; * and can not free memory containing aligned fdt. The aligned fdt - * is contained within the memory at ovcs->fdt, possibly at an offset - * from ovcs->fdt. + * is contained within the memory at ovcs->new_fdt, possibly at an + * offset from ovcs->new_fdt. */ ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY); if (ret) { @@ -996,8 +996,8 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree, goto out_unlock; err_free_tree: - kfree(fdt); - kfree(tree); + kfree(new_fdt); + kfree(overlay_root); err_free_overlay_changeset: free_overlay_changeset(ovcs); -- cgit v1.2.3 From 067c098766c6af667a9002d4e33cf1f3c998abbe Mon Sep 17 00:00:00 2001 From: Frank Rowand Date: Wed, 20 Apr 2022 17:25:05 -0500 Subject: of: overlay: rework overlay apply and remove kfree()s Fix various kfree() issues related to of_overlay_apply(). - Double kfree() of fdt and tree when init_overlay_changeset() returns an error. - free_overlay_changeset() free the root of the unflattened overlay (variable tree) instead of the memory that contains the unflattened overlay. - For the case of a failure during applying an overlay, move kfree() of new_fdt and overlay_mem into free_overlay_changeset(), which is called by the function that allocated them. - For the case of removing an overlay, the kfree() of new_fdt and overlay_mem remains in free_overlay_changeset(). - Check return value of of_fdt_unflatten_tree() for error instead of checking the returned value of overlay_root. - When storing pointers to allocated objects in ovcs, do so as near to the allocation as possible instead of in deeply layered function. More clearly document policy related to lifetime of pointers into overlay memory. Double kfree() Reported-by: Slawomir Stepien Signed-off-by: Frank Rowand Signed-off-by: Rob Herring Link: https://lore.kernel.org/r/20220420222505.928492-3-frowand.list@gmail.com --- Documentation/devicetree/overlay-notes.rst | 30 +++- drivers/of/overlay.c | 263 ++++++++++++++--------------- include/linux/of.h | 3 +- 3 files changed, 153 insertions(+), 143 deletions(-) (limited to 'drivers/of') diff --git a/Documentation/devicetree/overlay-notes.rst b/Documentation/devicetree/overlay-notes.rst index b2b8db765b8c..e139f22b363e 100644 --- a/Documentation/devicetree/overlay-notes.rst +++ b/Documentation/devicetree/overlay-notes.rst @@ -119,10 +119,32 @@ Finally, if you need to remove all overlays in one-go, just call of_overlay_remove_all() which will remove every single one in the correct order. -In addition, there is the option to register notifiers that get called on +There is the option to register notifiers that get called on overlay operations. See of_overlay_notifier_register/unregister and enum of_overlay_notify_action for details. -Note that a notifier callback is not supposed to store pointers to a device -tree node or its content beyond OF_OVERLAY_POST_REMOVE corresponding to the -respective node it received. +A notifier callback for OF_OVERLAY_PRE_APPLY, OF_OVERLAY_POST_APPLY, or +OF_OVERLAY_PRE_REMOVE may store pointers to a device tree node in the overlay +or its content but these pointers must not persist past the notifier callback +for OF_OVERLAY_POST_REMOVE. The memory containing the overlay will be +kfree()ed after OF_OVERLAY_POST_REMOVE notifiers are called. Note that the +memory will be kfree()ed even if the notifier for OF_OVERLAY_POST_REMOVE +returns an error. + +The changeset notifiers in drivers/of/dynamic.c are a second type of notifier +that could be triggered by applying or removing an overlay. These notifiers +are not allowed to store pointers to a device tree node in the overlay +or its content. The overlay code does not protect against such pointers +remaining active when the memory containing the overlay is freed as a result +of removing the overlay. + +Any other code that retains a pointer to the overlay nodes or data is +considered to be a bug because after removing the overlay the pointer +will refer to freed memory. + +Users of overlays must be especially aware of the overall operations that +occur on the system to ensure that other kernel code does not retain any +pointers to the overlay nodes or data. Any example of an inadvertent use +of such pointers is if a driver or subsystem module is loaded after an +overlay has been applied, and the driver or subsystem scans the entire +devicetree or a large portion of it, including the overlay nodes. diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c index 6df9ecdf16cf..bd522da7f76b 100644 --- a/drivers/of/overlay.c +++ b/drivers/of/overlay.c @@ -58,7 +58,9 @@ struct fragment { * @id: changeset identifier * @ovcs_list: list on which we are located * @new_fdt: Memory allocated to hold unflattened aligned FDT + * @overlay_mem: the memory chunk that contains @overlay_root * @overlay_root: expanded device tree that contains the fragment nodes + * @notify_state: most recent notify action used on overlay * @count: count of fragment structures * @fragments: fragment nodes in the overlay expanded device tree * @symbols_fragment: last element of @fragments[] is the __symbols__ node @@ -68,7 +70,9 @@ struct overlay_changeset { int id; struct list_head ovcs_list; const void *new_fdt; + const void *overlay_mem; struct device_node *overlay_root; + enum of_overlay_notify_action notify_state; int count; struct fragment *fragments; bool symbols_fragment; @@ -115,7 +119,6 @@ void of_overlay_mutex_unlock(void) mutex_unlock(&of_overlay_phandle_mutex); } - static LIST_HEAD(ovcs_list); static DEFINE_IDR(ovcs_idr); @@ -162,6 +165,8 @@ static int overlay_notify(struct overlay_changeset *ovcs, struct of_overlay_notify_data nd; int i, ret; + ovcs->notify_state = action; + for (i = 0; i < ovcs->count; i++) { struct fragment *fragment = &ovcs->fragments[i]; @@ -717,53 +722,49 @@ static struct device_node *find_target(struct device_node *info_node) /** * init_overlay_changeset() - initialize overlay changeset from overlay tree * @ovcs: Overlay changeset to build - * @new_fdt: Memory allocated to hold unflattened aligned FDT - * @overlay_root: Contains the overlay fragments and overlay fixup nodes * * Initialize @ovcs. Populate @ovcs->fragments with node information from * the top level of @overlay_root. The relevant top level nodes are the * fragment nodes and the __symbols__ node. Any other top level node will - * be ignored. + * be ignored. Populate other @ovcs fields. * * Return: 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error - * detected in @overlay_root, or -ENOSPC if idr_alloc() error. + * detected in @overlay_root. On error return, the caller of + * init_overlay_changeset() must call free_overlay_changeset(). */ -static int init_overlay_changeset(struct overlay_changeset *ovcs, - const void *new_fdt, struct device_node *overlay_root) +static int init_overlay_changeset(struct overlay_changeset *ovcs) { struct device_node *node, *overlay_node; struct fragment *fragment; struct fragment *fragments; - int cnt, id, ret; + int cnt, ret; + + /* + * None of the resources allocated by this function will be freed in + * the error paths. Instead the caller of this function is required + * to call free_overlay_changeset() (which will free the resources) + * if error return. + */ /* * Warn for some issues. Can not return -EINVAL for these until * of_unittest_apply_overlay() is fixed to pass these checks. */ - if (!of_node_check_flag(overlay_root, OF_DYNAMIC)) - pr_debug("%s() overlay_root is not dynamic\n", __func__); + if (!of_node_check_flag(ovcs->overlay_root, OF_DYNAMIC)) + pr_debug("%s() ovcs->overlay_root is not dynamic\n", __func__); - if (!of_node_check_flag(overlay_root, OF_DETACHED)) - pr_debug("%s() overlay_root is not detached\n", __func__); + if (!of_node_check_flag(ovcs->overlay_root, OF_DETACHED)) + pr_debug("%s() ovcs->overlay_root is not detached\n", __func__); - if (!of_node_is_root(overlay_root)) - pr_debug("%s() overlay_root is not root\n", __func__); - - ovcs->overlay_root = overlay_root; - ovcs->new_fdt = new_fdt; - - INIT_LIST_HEAD(&ovcs->ovcs_list); + if (!of_node_is_root(ovcs->overlay_root)) + pr_debug("%s() ovcs->overlay_root is not root\n", __func__); of_changeset_init(&ovcs->cset); - id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL); - if (id <= 0) - return id; - cnt = 0; /* fragment nodes */ - for_each_child_of_node(overlay_root, node) { + for_each_child_of_node(ovcs->overlay_root, node) { overlay_node = of_get_child_by_name(node, "__overlay__"); if (overlay_node) { cnt++; @@ -771,7 +772,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, } } - node = of_get_child_by_name(overlay_root, "__symbols__"); + node = of_get_child_by_name(ovcs->overlay_root, "__symbols__"); if (node) { cnt++; of_node_put(node); @@ -780,11 +781,12 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, fragments = kcalloc(cnt, sizeof(*fragments), GFP_KERNEL); if (!fragments) { ret = -ENOMEM; - goto err_free_idr; + goto err_out; } + ovcs->fragments = fragments; cnt = 0; - for_each_child_of_node(overlay_root, node) { + for_each_child_of_node(ovcs->overlay_root, node) { overlay_node = of_get_child_by_name(node, "__overlay__"); if (!overlay_node) continue; @@ -796,7 +798,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, of_node_put(fragment->overlay); ret = -EINVAL; of_node_put(node); - goto err_free_fragments; + goto err_out; } cnt++; @@ -806,7 +808,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, * if there is a symbols fragment in ovcs->fragments[i] it is * the final element in the array */ - node = of_get_child_by_name(overlay_root, "__symbols__"); + node = of_get_child_by_name(ovcs->overlay_root, "__symbols__"); if (node) { ovcs->symbols_fragment = 1; fragment = &fragments[cnt]; @@ -816,7 +818,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, if (!fragment->target) { pr_err("symbols in overlay, but not in live tree\n"); ret = -EINVAL; - goto err_free_fragments; + goto err_out; } cnt++; @@ -825,20 +827,14 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs, if (!cnt) { pr_err("no fragments or symbols in overlay\n"); ret = -EINVAL; - goto err_free_fragments; + goto err_out; } - ovcs->id = id; ovcs->count = cnt; - ovcs->fragments = fragments; return 0; -err_free_fragments: - kfree(fragments); -err_free_idr: - idr_remove(&ovcs_idr, id); - +err_out: pr_err("%s() failed, ret = %d\n", __func__, ret); return ret; @@ -851,21 +847,34 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs) if (ovcs->cset.entries.next) of_changeset_destroy(&ovcs->cset); - if (ovcs->id) + if (ovcs->id) { idr_remove(&ovcs_idr, ovcs->id); + list_del(&ovcs->ovcs_list); + ovcs->id = 0; + } + for (i = 0; i < ovcs->count; i++) { of_node_put(ovcs->fragments[i].target); of_node_put(ovcs->fragments[i].overlay); } kfree(ovcs->fragments); + /* - * There should be no live pointers into ovcs->overlay_root and + * There should be no live pointers into ovcs->overlay_mem and * ovcs->new_fdt due to the policy that overlay notifiers are not - * allowed to retain pointers into the overlay devicetree. + * allowed to retain pointers into the overlay devicetree other + * than during the window from OF_OVERLAY_PRE_APPLY overlay + * notifiers until the OF_OVERLAY_POST_REMOVE overlay notifiers. + * + * A memory leak will occur here if within the window. */ - kfree(ovcs->overlay_root); - kfree(ovcs->new_fdt); + + if (ovcs->notify_state == OF_OVERLAY_INIT || + ovcs->notify_state == OF_OVERLAY_POST_REMOVE) { + kfree(ovcs->overlay_mem); + kfree(ovcs->new_fdt); + } kfree(ovcs); } @@ -873,27 +882,13 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs) * internal documentation * * of_overlay_apply() - Create and apply an overlay changeset - * @new_fdt: Memory allocated to hold the aligned FDT - * @overlay_root: Expanded overlay device tree - * @ovcs_id: Pointer to overlay changeset id + * @ovcs: overlay changeset * * Creates and applies an overlay changeset. * - * If an error occurs in a pre-apply notifier, then no changes are made - * to the device tree. - * - * A non-zero return value will not have created the changeset if error is from: - * - parameter checks - * - building the changeset - * - overlay changeset pre-apply notifier - * * If an error is returned by an overlay changeset pre-apply notifier * then no further overlay changeset pre-apply notifier will be called. * - * A non-zero return value will have created the changeset if error is from: - * - overlay changeset entry notifier - * - overlay changeset post-apply notifier - * * If an error is returned by an overlay changeset post-apply notifier * then no further overlay changeset post-apply notifier will be called. * @@ -907,64 +902,37 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs) * following attempt to apply or remove an overlay changeset will be * refused. * - * Returns 0 on success, or a negative error number. Overlay changeset - * id is returned to *ovcs_id. + * Returns 0 on success, or a negative error number. On error return, + * the caller of of_overlay_apply() must call free_overlay_changeset(). */ -static int of_overlay_apply(const void *new_fdt, - struct device_node *overlay_root, int *ovcs_id) +static int of_overlay_apply(struct overlay_changeset *ovcs) { - struct overlay_changeset *ovcs; int ret = 0, ret_revert, ret_tmp; - /* - * As of this point, fdt and tree belong to the overlay changeset. - * overlay changeset code is responsible for freeing them. - */ - if (devicetree_corrupt()) { pr_err("devicetree state suspect, refuse to apply overlay\n"); - kfree(new_fdt); - kfree(overlay_root); ret = -EBUSY; goto out; } - ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL); - if (!ovcs) { - kfree(new_fdt); - kfree(overlay_root); - ret = -ENOMEM; - goto out; - } - - of_overlay_mutex_lock(); - mutex_lock(&of_mutex); - - ret = of_resolve_phandles(overlay_root); + ret = of_resolve_phandles(ovcs->overlay_root); if (ret) - goto err_free_tree; + goto out; - ret = init_overlay_changeset(ovcs, new_fdt, overlay_root); + ret = init_overlay_changeset(ovcs); if (ret) - goto err_free_tree; + goto out; - /* - * After overlay_notify(), ovcs->overlay_root related pointers may have - * leaked to drivers, so can not kfree() tree, aka ovcs->overlay_tree; - * and can not free memory containing aligned fdt. The aligned fdt - * is contained within the memory at ovcs->new_fdt, possibly at an - * offset from ovcs->new_fdt. - */ ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY); if (ret) { pr_err("overlay changeset pre-apply notify error %d\n", ret); - goto err_free_overlay_changeset; + goto out; } ret = build_changeset(ovcs); if (ret) - goto err_free_overlay_changeset; + goto out; ret_revert = 0; ret = __of_changeset_apply_entries(&ovcs->cset, &ret_revert); @@ -974,7 +942,7 @@ static int of_overlay_apply(const void *new_fdt, ret_revert); devicetree_state_flags |= DTSF_APPLY_FAIL; } - goto err_free_overlay_changeset; + goto out; } ret = __of_changeset_apply_notify(&ovcs->cset); @@ -982,9 +950,6 @@ static int of_overlay_apply(const void *new_fdt, pr_err("overlay apply changeset entry notify error %d\n", ret); /* notify failure is not fatal, continue */ - list_add_tail(&ovcs->ovcs_list, &ovcs_list); - *ovcs_id = ovcs->id; - ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_APPLY); if (ret_tmp) { pr_err("overlay changeset post-apply notify error %d\n", @@ -993,19 +958,6 @@ static int of_overlay_apply(const void *new_fdt, ret = ret_tmp; } - goto out_unlock; - -err_free_tree: - kfree(new_fdt); - kfree(overlay_root); - -err_free_overlay_changeset: - free_overlay_changeset(ovcs); - -out_unlock: - mutex_unlock(&of_mutex); - of_overlay_mutex_unlock(); - out: pr_debug("%s() err=%d\n", __func__, ret); @@ -1013,15 +965,16 @@ out: } int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size, - int *ovcs_id) + int *ret_ovcs_id) { void *new_fdt; void *new_fdt_align; + void *overlay_mem; int ret; u32 size; - struct device_node *overlay_root = NULL; + struct overlay_changeset *ovcs; - *ovcs_id = 0; + *ret_ovcs_id = 0; if (overlay_fdt_size < sizeof(struct fdt_header) || fdt_check_header(overlay_fdt)) { @@ -1033,41 +986,67 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size, if (overlay_fdt_size < size) return -EINVAL; + ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL); + if (!ovcs) + return -ENOMEM; + + of_overlay_mutex_lock(); + mutex_lock(&of_mutex); + + /* + * ovcs->notify_state must be set to OF_OVERLAY_INIT before allocating + * ovcs resources, implicitly set by kzalloc() of ovcs + */ + + ovcs->id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL); + if (ovcs->id <= 0) { + ret = ovcs->id; + goto err_free_ovcs; + } + + INIT_LIST_HEAD(&ovcs->ovcs_list); + list_add_tail(&ovcs->ovcs_list, &ovcs_list); + /* * Must create permanent copy of FDT because of_fdt_unflatten_tree() * will create pointers to the passed in FDT in the unflattened tree. */ new_fdt = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL); - if (!new_fdt) - return -ENOMEM; + if (!new_fdt) { + ret = -ENOMEM; + goto err_free_ovcs; + } + ovcs->new_fdt = new_fdt; new_fdt_align = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE); memcpy(new_fdt_align, overlay_fdt, size); - of_fdt_unflatten_tree(new_fdt_align, NULL, &overlay_root); - if (!overlay_root) { + overlay_mem = of_fdt_unflatten_tree(new_fdt_align, NULL, + &ovcs->overlay_root); + if (!overlay_mem) { pr_err("unable to unflatten overlay_fdt\n"); ret = -EINVAL; - goto out_free_new_fdt; + goto err_free_ovcs; } + ovcs->overlay_mem = overlay_mem; - ret = of_overlay_apply(new_fdt, overlay_root, ovcs_id); - if (ret < 0) { - /* - * new_fdt and overlay_root now belong to the overlay - * changeset. - * overlay changeset code is responsible for freeing them. - */ - goto out; - } + ret = of_overlay_apply(ovcs); + if (ret < 0) + goto err_free_ovcs; + + mutex_unlock(&of_mutex); + of_overlay_mutex_unlock(); + + *ret_ovcs_id = ovcs->id; return 0; +err_free_ovcs: + free_overlay_changeset(ovcs); -out_free_new_fdt: - kfree(new_fdt); + mutex_unlock(&of_mutex); + of_overlay_mutex_unlock(); -out: return ret; } EXPORT_SYMBOL_GPL(of_overlay_fdt_apply); @@ -1204,28 +1183,26 @@ int of_overlay_remove(int *ovcs_id) if (!ovcs) { ret = -ENODEV; pr_err("remove: Could not find overlay #%d\n", *ovcs_id); - goto out_unlock; + goto err_unlock; } if (!overlay_removal_is_ok(ovcs)) { ret = -EBUSY; - goto out_unlock; + goto err_unlock; } ret = overlay_notify(ovcs, OF_OVERLAY_PRE_REMOVE); if (ret) { pr_err("overlay changeset pre-remove notify error %d\n", ret); - goto out_unlock; + goto err_unlock; } - list_del(&ovcs->ovcs_list); - ret_apply = 0; ret = __of_changeset_revert_entries(&ovcs->cset, &ret_apply); if (ret) { if (ret_apply) devicetree_state_flags |= DTSF_REVERT_FAIL; - goto out_unlock; + goto err_unlock; } ret = __of_changeset_revert_notify(&ovcs->cset); @@ -1235,6 +1212,11 @@ int of_overlay_remove(int *ovcs_id) *ovcs_id = 0; + /* + * Note that the overlay memory will be kfree()ed by + * free_overlay_changeset() even if the notifier for + * OF_OVERLAY_POST_REMOVE returns an error. + */ ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE); if (ret_tmp) { pr_err("overlay changeset post-remove notify error %d\n", @@ -1245,7 +1227,12 @@ int of_overlay_remove(int *ovcs_id) free_overlay_changeset(ovcs); -out_unlock: +err_unlock: + /* + * If jumped over free_overlay_changeset(), then did not kfree() + * overlay related memory. This is a memory leak unless a subsequent + * of_overlay_remove() of this overlay is successful. + */ mutex_unlock(&of_mutex); out: diff --git a/include/linux/of.h b/include/linux/of.h index 04971e85fbc9..17741eee0ca4 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -1543,7 +1543,8 @@ static inline bool of_device_is_system_power_controller(const struct device_node */ enum of_overlay_notify_action { - OF_OVERLAY_PRE_APPLY = 0, + OF_OVERLAY_INIT = 0, /* kzalloc() of ovcs sets this value */ + OF_OVERLAY_PRE_APPLY, OF_OVERLAY_POST_APPLY, OF_OVERLAY_PRE_REMOVE, OF_OVERLAY_POST_REMOVE, -- cgit v1.2.3 From 1ac17586c950a2c129393f8a92901a2b357acf24 Mon Sep 17 00:00:00 2001 From: Frank Rowand Date: Mon, 2 May 2022 13:17:40 -0500 Subject: of: overlay: add entry to of_overlay_action_name[] The values of enum of_overlay_notify_action are used to index into array of_overlay_action_name. Add an entry to of_overlay_action_name for the value recently added to of_overlay_notify_action. Array of_overlay_action_name[] is moved into include/linux/of.h adjacent to enum of_overlay_notify_action to make the connection between the two more obvious if either is modified in the future. The only use of of_overlay_action_name is for error reporting in overlay_notify(). All callers of overlay_notify() report the same error, but with fewer details. Remove the redundant error reports in the callers. Fixes: 067c098766c6 ("of: overlay: rework overlay apply and remove kfree()s") Reported-by: Dan Carpenter Signed-off-by: Frank Rowand Signed-off-by: Rob Herring Link: https://lore.kernel.org/r/20220502181742.1402826-2-frowand.list@gmail.com --- drivers/of/overlay.c | 27 +++++---------------------- include/linux/of.h | 13 +++++++++++++ 2 files changed, 18 insertions(+), 22 deletions(-) (limited to 'drivers/of') diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c index bd522da7f76b..ae5ea5b1079b 100644 --- a/drivers/of/overlay.c +++ b/drivers/of/overlay.c @@ -152,13 +152,6 @@ int of_overlay_notifier_unregister(struct notifier_block *nb) } EXPORT_SYMBOL_GPL(of_overlay_notifier_unregister); -static char *of_overlay_action_name[] = { - "pre-apply", - "post-apply", - "pre-remove", - "post-remove", -}; - static int overlay_notify(struct overlay_changeset *ovcs, enum of_overlay_notify_action action) { @@ -178,7 +171,7 @@ static int overlay_notify(struct overlay_changeset *ovcs, if (notifier_to_errno(ret)) { ret = notifier_to_errno(ret); pr_err("overlay changeset %s notifier error %d, target: %pOF\n", - of_overlay_action_name[action], ret, nd.target); + of_overlay_action_name(action), ret, nd.target); return ret; } } @@ -925,10 +918,8 @@ static int of_overlay_apply(struct overlay_changeset *ovcs) goto out; ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY); - if (ret) { - pr_err("overlay changeset pre-apply notify error %d\n", ret); + if (ret) goto out; - } ret = build_changeset(ovcs); if (ret) @@ -951,12 +942,9 @@ static int of_overlay_apply(struct overlay_changeset *ovcs) /* notify failure is not fatal, continue */ ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_APPLY); - if (ret_tmp) { - pr_err("overlay changeset post-apply notify error %d\n", - ret_tmp); + if (ret_tmp) if (!ret) ret = ret_tmp; - } out: pr_debug("%s() err=%d\n", __func__, ret); @@ -1192,10 +1180,8 @@ int of_overlay_remove(int *ovcs_id) } ret = overlay_notify(ovcs, OF_OVERLAY_PRE_REMOVE); - if (ret) { - pr_err("overlay changeset pre-remove notify error %d\n", ret); + if (ret) goto err_unlock; - } ret_apply = 0; ret = __of_changeset_revert_entries(&ovcs->cset, &ret_apply); @@ -1218,12 +1204,9 @@ int of_overlay_remove(int *ovcs_id) * OF_OVERLAY_POST_REMOVE returns an error. */ ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE); - if (ret_tmp) { - pr_err("overlay changeset post-remove notify error %d\n", - ret_tmp); + if (ret_tmp) if (!ret) ret = ret_tmp; - } free_overlay_changeset(ovcs); diff --git a/include/linux/of.h b/include/linux/of.h index 17741eee0ca4..f0a5d6b10c5a 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -1550,6 +1550,19 @@ enum of_overlay_notify_action { OF_OVERLAY_POST_REMOVE, }; +static inline char *of_overlay_action_name(enum of_overlay_notify_action action) +{ + static char *of_overlay_action_name[] = { + "init", + "pre-apply", + "post-apply", + "pre-remove", + "post-remove", + }; + + return of_overlay_action_name[action]; +} + struct of_overlay_notify_data { struct device_node *overlay; struct device_node *target; -- cgit v1.2.3 From 992b0dc5c38a4e79dca17577960c5201275b83f1 Mon Sep 17 00:00:00 2001 From: Frank Rowand Date: Mon, 2 May 2022 13:17:41 -0500 Subject: of: overlay: unittest: add tests for overlay notifiers Add tests for overlay apply and remove notifiers. Trigger errors for each of the notifier actions. These tests will reveal a memory leak problem when a notifier returns an error for action OF_OVERLAY_POST_APPLY. The pr_err() message is: OF: ERROR: memory leak, expected refcount 1 instead of 3, of_node_get()/of_node_put() unbalanced - destroy cset entry: attach overlay node /testcase-data/overlay-node/test-bus/test-unittest17 Signed-off-by: Frank Rowand Signed-off-by: Rob Herring Link: https://lore.kernel.org/r/20220502181742.1402826-3-frowand.list@gmail.com --- drivers/of/unittest-data/Makefile | 10 ++ drivers/of/unittest-data/overlay_16.dts | 15 +++ drivers/of/unittest-data/overlay_17.dts | 15 +++ drivers/of/unittest-data/overlay_18.dts | 15 +++ drivers/of/unittest-data/overlay_19.dts | 15 +++ drivers/of/unittest-data/overlay_20.dts | 15 +++ drivers/of/unittest.c | 195 ++++++++++++++++++++++++++++++++ 7 files changed, 280 insertions(+) create mode 100644 drivers/of/unittest-data/overlay_16.dts create mode 100644 drivers/of/unittest-data/overlay_17.dts create mode 100644 drivers/of/unittest-data/overlay_18.dts create mode 100644 drivers/of/unittest-data/overlay_19.dts create mode 100644 drivers/of/unittest-data/overlay_20.dts (limited to 'drivers/of') diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile index fbded24c608c..d072f3ba3971 100644 --- a/drivers/of/unittest-data/Makefile +++ b/drivers/of/unittest-data/Makefile @@ -17,6 +17,11 @@ obj-$(CONFIG_OF_OVERLAY) += overlay.dtb.o \ overlay_12.dtb.o \ overlay_13.dtb.o \ overlay_15.dtb.o \ + overlay_16.dtb.o \ + overlay_17.dtb.o \ + overlay_18.dtb.o \ + overlay_19.dtb.o \ + overlay_20.dtb.o \ overlay_bad_add_dup_node.dtb.o \ overlay_bad_add_dup_prop.dtb.o \ overlay_bad_phandle.dtb.o \ @@ -75,6 +80,11 @@ apply_static_overlay_1 := overlay_0.dtbo \ overlay_12.dtbo \ overlay_13.dtbo \ overlay_15.dtbo \ + overlay_16.dtbo \ + overlay_17.dtbo \ + overlay_18.dtbo \ + overlay_19.dtbo \ + overlay_20.dtbo \ overlay_gpio_01.dtbo \ overlay_gpio_02a.dtbo \ overlay_gpio_02b.dtbo \ diff --git a/drivers/of/unittest-data/overlay_16.dts b/drivers/of/unittest-data/overlay_16.dts new file mode 100644 index 000000000000..80a46dc02581 --- /dev/null +++ b/drivers/of/unittest-data/overlay_16.dts @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-2.0 +/dts-v1/; +/plugin/; + +/* overlay_16 - notify test */ + +&unittest_test_bus { + #address-cells = <1>; + #size-cells = <0>; + + test-unittest16 { + compatible = "unittest"; + reg = <16>; + }; +}; diff --git a/drivers/of/unittest-data/overlay_17.dts b/drivers/of/unittest-data/overlay_17.dts new file mode 100644 index 000000000000..5b8a2209177f --- /dev/null +++ b/drivers/of/unittest-data/overlay_17.dts @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-2.0 +/dts-v1/; +/plugin/; + +/* overlay_17 - notify test */ + +&unittest_test_bus { + #address-cells = <1>; + #size-cells = <0>; + + test-unittest17 { + compatible = "unittest"; + reg = <17>; + }; +}; diff --git a/drivers/of/unittest-data/overlay_18.dts b/drivers/of/unittest-data/overlay_18.dts new file mode 100644 index 000000000000..dcddd5f6d301 --- /dev/null +++ b/drivers/of/unittest-data/overlay_18.dts @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-2.0 +/dts-v1/; +/plugin/; + +/* overlay_18 - notify test */ + +&unittest_test_bus { + #address-cells = <1>; + #size-cells = <0>; + + test-unittest18 { + compatible = "unittest"; + reg = <18>; + }; +}; diff --git a/drivers/of/unittest-data/overlay_19.dts b/drivers/of/unittest-data/overlay_19.dts new file mode 100644 index 000000000000..32b3ba0b66a3 --- /dev/null +++ b/drivers/of/unittest-data/overlay_19.dts @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-2.0 +/dts-v1/; +/plugin/; + +/* overlay_19 - notify test */ + +&unittest_test_bus { + #address-cells = <1>; + #size-cells = <0>; + + test-unittest19 { + compatible = "unittest"; + reg = <19>; + }; +}; diff --git a/drivers/of/unittest-data/overlay_20.dts b/drivers/of/unittest-data/overlay_20.dts new file mode 100644 index 000000000000..d4a4f2f32ec7 --- /dev/null +++ b/drivers/of/unittest-data/overlay_20.dts @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-2.0 +/dts-v1/; +/plugin/; + +/* overlay_20 - notify test */ + +&unittest_test_bus { + #address-cells = <1>; + #size-cells = <0>; + + test-unittest20 { + compatible = "unittest"; + reg = <20>; + }; +}; diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c index c4106de9f137..6747e8eb7c56 100644 --- a/drivers/of/unittest.c +++ b/drivers/of/unittest.c @@ -2741,6 +2741,189 @@ static inline void of_unittest_overlay_i2c_15(void) { } #endif +static int of_notify(struct notifier_block *nb, unsigned long action, + void *arg) +{ + struct of_overlay_notify_data *nd = arg; + struct device_node *found; + int ret; + + /* + * For overlay_16 .. overlay_19, check that returning an error + * works for each of the actions by setting an arbitrary return + * error number that matches the test number. e.g. for unittest16, + * ret = -EBUSY which is -16. + * + * OVERLAY_INFO() for the overlays is declared to expect the same + * error number, so overlay_data_apply() will return no error. + * + * overlay_20 will return NOTIFY_DONE + */ + + ret = 0; + of_node_get(nd->overlay); + + switch (action) { + + case OF_OVERLAY_PRE_APPLY: + found = of_find_node_by_name(nd->overlay, "test-unittest16"); + if (found) { + of_node_put(found); + ret = -EBUSY; + } + break; + + case OF_OVERLAY_POST_APPLY: + found = of_find_node_by_name(nd->overlay, "test-unittest17"); + if (found) { + of_node_put(found); + ret = -EEXIST; + } + break; + + case OF_OVERLAY_PRE_REMOVE: + found = of_find_node_by_name(nd->overlay, "test-unittest18"); + if (found) { + of_node_put(found); + ret = -EXDEV; + } + break; + + case OF_OVERLAY_POST_REMOVE: + found = of_find_node_by_name(nd->overlay, "test-unittest19"); + if (found) { + of_node_put(found); + ret = -ENODEV; + } + break; + + default: /* should not happen */ + of_node_put(nd->overlay); + ret = -EINVAL; + break; + } + + if (ret) + return notifier_from_errno(ret); + + return NOTIFY_DONE; +} + +static struct notifier_block of_nb = { + .notifier_call = of_notify, +}; + +static void __init of_unittest_overlay_notify(void) +{ + int ovcs_id; + int ret; + + ret = of_overlay_notifier_register(&of_nb); + unittest(!ret, + "of_overlay_notifier_register() failed, ret = %d\n", ret); + if (ret) + return; + + /* + * The overlays are applied by overlay_data_apply() + * instead of of_unittest_apply_overlay() so that they + * will not be tracked. Thus they will not be removed + * by of_unittest_remove_tracked_overlays(). + * + * Applying overlays 16 - 19 will each trigger an error for a + * different action in of_notify(). + * + * Applying overlay 20 will not trigger any error in of_notify(). + */ + + /* --- overlay 16 --- */ + + EXPECT_BEGIN(KERN_INFO, "OF: overlay: overlay changeset pre-apply notifier error -16, target: /testcase-data/overlay-node/test-bus"); + + unittest(overlay_data_apply("overlay_16", &ovcs_id), + "test OF_OVERLAY_PRE_APPLY notify injected error\n"); + + EXPECT_END(KERN_INFO, "OF: overlay: overlay changeset pre-apply notifier error -16, target: /testcase-data/overlay-node/test-bus"); + + unittest(!ovcs_id, "ovcs_id created for overlay_16\n"); + + /* --- overlay 17 --- */ + + EXPECT_BEGIN(KERN_INFO, "OF: overlay: overlay changeset post-apply notifier error -17, target: /testcase-data/overlay-node/test-bus"); + + unittest(overlay_data_apply("overlay_17", &ovcs_id), + "test OF_OVERLAY_POST_APPLY notify injected error\n"); + + EXPECT_END(KERN_INFO, "OF: overlay: overlay changeset post-apply notifier error -17, target: /testcase-data/overlay-node/test-bus"); + + unittest(!ovcs_id, "ovcs_id created for overlay_17\n"); + + /* --- overlay 18 --- */ + + unittest(overlay_data_apply("overlay_18", &ovcs_id), + "OF_OVERLAY_PRE_REMOVE notify injected error\n"); + + unittest(ovcs_id, "ovcs_id not created for overlay_18\n"); + + if (ovcs_id) { + EXPECT_BEGIN(KERN_INFO, "OF: overlay: overlay changeset pre-remove notifier error -18, target: /testcase-data/overlay-node/test-bus"); + + ret = of_overlay_remove(&ovcs_id); + EXPECT_END(KERN_INFO, "OF: overlay: overlay changeset pre-remove notifier error -18, target: /testcase-data/overlay-node/test-bus"); + if (ret == -EXDEV) { + /* + * change set ovcs_id should still exist + */ + unittest(1, "overlay_18 of_overlay_remove() injected error for OF_OVERLAY_PRE_REMOVE\n"); + } else { + unittest(0, "overlay_18 of_overlay_remove() injected error for OF_OVERLAY_PRE_REMOVE not returned\n"); + } + } else { + unittest(1, "ovcs_id not created for overlay_18\n"); + } + + unittest(ovcs_id, "ovcs_id removed for overlay_18\n"); + + /* --- overlay 19 --- */ + + unittest(overlay_data_apply("overlay_19", &ovcs_id), + "OF_OVERLAY_POST_REMOVE notify injected error\n"); + + unittest(ovcs_id, "ovcs_id not created for overlay_19\n"); + + if (ovcs_id) { + EXPECT_BEGIN(KERN_INFO, "OF: overlay: overlay changeset post-remove notifier error -19, target: /testcase-data/overlay-node/test-bus"); + ret = of_overlay_remove(&ovcs_id); + EXPECT_END(KERN_INFO, "OF: overlay: overlay changeset post-remove notifier error -19, target: /testcase-data/overlay-node/test-bus"); + if (ret == -ENODEV) + unittest(1, "overlay_19 of_overlay_remove() injected error for OF_OVERLAY_POST_REMOVE\n"); + else + unittest(0, "overlay_19 of_overlay_remove() injected error for OF_OVERLAY_POST_REMOVE not returned\n"); + } else { + unittest(1, "ovcs_id removed for overlay_19\n"); + } + + unittest(!ovcs_id, "changeset ovcs_id = %d not removed for overlay_19\n", + ovcs_id); + + /* --- overlay 20 --- */ + + unittest(overlay_data_apply("overlay_20", &ovcs_id), + "overlay notify no injected error\n"); + + if (ovcs_id) { + ret = of_overlay_remove(&ovcs_id); + if (ret) + unittest(1, "overlay_20 failed to be destroyed, ret = %d\n", + ret); + } else { + unittest(1, "ovcs_id not created for overlay_20\n"); + } + + unittest(!of_overlay_notifier_unregister(&of_nb), + "of_overlay_notifier_unregister() failed, ret = %d\n", ret); +} + static void __init of_unittest_overlay(void) { struct device_node *bus_np = NULL; @@ -2804,6 +2987,8 @@ static void __init of_unittest_overlay(void) of_unittest_remove_tracked_overlays(); + of_unittest_overlay_notify(); + out: of_node_put(bus_np); } @@ -2855,6 +3040,11 @@ OVERLAY_INFO_EXTERN(overlay_11); OVERLAY_INFO_EXTERN(overlay_12); OVERLAY_INFO_EXTERN(overlay_13); OVERLAY_INFO_EXTERN(overlay_15); +OVERLAY_INFO_EXTERN(overlay_16); +OVERLAY_INFO_EXTERN(overlay_17); +OVERLAY_INFO_EXTERN(overlay_18); +OVERLAY_INFO_EXTERN(overlay_19); +OVERLAY_INFO_EXTERN(overlay_20); OVERLAY_INFO_EXTERN(overlay_gpio_01); OVERLAY_INFO_EXTERN(overlay_gpio_02a); OVERLAY_INFO_EXTERN(overlay_gpio_02b); @@ -2885,6 +3075,11 @@ static struct overlay_info overlays[] = { OVERLAY_INFO(overlay_12, 0), OVERLAY_INFO(overlay_13, 0), OVERLAY_INFO(overlay_15, 0), + OVERLAY_INFO(overlay_16, -EBUSY), + OVERLAY_INFO(overlay_17, -EEXIST), + OVERLAY_INFO(overlay_18, 0), + OVERLAY_INFO(overlay_19, 0), + OVERLAY_INFO(overlay_20, 0), OVERLAY_INFO(overlay_gpio_01, 0), OVERLAY_INFO(overlay_gpio_02a, 0), OVERLAY_INFO(overlay_gpio_02b, 0), -- cgit v1.2.3 From 421f4d14bc037f708a3d11fbb01274b741767bf9 Mon Sep 17 00:00:00 2001 From: Frank Rowand Date: Mon, 2 May 2022 13:17:42 -0500 Subject: of: overlay: do not free changeset when of_overlay_apply returns error New unittests for overlay notifiers reveal a memory leak in of_overlay_apply() when a notifier returns an error for action OF_OVERLAY_POST_APPLY. The pr_err() message is: OF: ERROR: memory leak, expected refcount 1 instead of 3, of_node_get()/of_node_put() unbalanced - destroy cset entry: attach overlay node /testcase-data/overlay-node/test-bus/test-unittest17 Change the error path to no longer call free_overlay_changeset(), and document that the caller of of_overlay_fdt_apply() may choose to remove the overlay. Update the unittest that triggered the error to expect the changed return values and to call of_overlay_remove(). Signed-off-by: Frank Rowand Signed-off-by: Rob Herring Link: https://lore.kernel.org/r/20220502181742.1402826-4-frowand.list@gmail.com --- drivers/of/overlay.c | 29 ++++++++++++++++++++++++++--- drivers/of/unittest.c | 10 ++++++++-- 2 files changed, 34 insertions(+), 5 deletions(-) (limited to 'drivers/of') diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c index ae5ea5b1079b..4044ddcb02c6 100644 --- a/drivers/of/overlay.c +++ b/drivers/of/overlay.c @@ -952,6 +952,25 @@ out: return ret; } +/* + * of_overlay_fdt_apply() - Create and apply an overlay changeset + * @overlay_fdt: pointer to overlay FDT + * @overlay_fdt_size: number of bytes in @overlay_fdt + * @ret_ovcs_id: pointer for returning created changeset id + * + * Creates and applies an overlay changeset. + * + * See of_overlay_apply() for important behavior information. + * + * Return: 0 on success, or a negative error number. *@ret_ovcs_id is set to + * the value of overlay changeset id, which can be passed to of_overlay_remove() + * to remove the overlay. + * + * On error return, the changeset may be partially applied. This is especially + * likely if an OF_OVERLAY_POST_APPLY notifier returns an error. In this case + * the caller should call of_overlay_remove() with the value in *@ret_ovcs_id. + */ + int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size, int *ret_ovcs_id) { @@ -1019,15 +1038,19 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size, ovcs->overlay_mem = overlay_mem; ret = of_overlay_apply(ovcs); - if (ret < 0) - goto err_free_ovcs; + /* + * If of_overlay_apply() error, calling free_overlay_changeset() may + * result in a memory leak if the apply partly succeeded, so do NOT + * goto err_free_ovcs. Instead, the caller of of_overlay_fdt_apply() + * can call of_overlay_remove(); + */ mutex_unlock(&of_mutex); of_overlay_mutex_unlock(); *ret_ovcs_id = ovcs->id; - return 0; + return ret; err_free_ovcs: free_overlay_changeset(ovcs); diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c index 6747e8eb7c56..7f6bba18c515 100644 --- a/drivers/of/unittest.c +++ b/drivers/of/unittest.c @@ -2845,7 +2845,7 @@ static void __init of_unittest_overlay_notify(void) EXPECT_END(KERN_INFO, "OF: overlay: overlay changeset pre-apply notifier error -16, target: /testcase-data/overlay-node/test-bus"); - unittest(!ovcs_id, "ovcs_id created for overlay_16\n"); + unittest(ovcs_id, "ovcs_id not created for overlay_16\n"); /* --- overlay 17 --- */ @@ -2856,7 +2856,13 @@ static void __init of_unittest_overlay_notify(void) EXPECT_END(KERN_INFO, "OF: overlay: overlay changeset post-apply notifier error -17, target: /testcase-data/overlay-node/test-bus"); - unittest(!ovcs_id, "ovcs_id created for overlay_17\n"); + unittest(ovcs_id, "ovcs_id not created for overlay_17\n"); + + if (ovcs_id) { + ret = of_overlay_remove(&ovcs_id); + unittest(!ret, + "overlay_17 of_overlay_remove(), ret = %d\n", ret); + } /* --- overlay 18 --- */ -- cgit v1.2.3 From df5cd369876114f91f9ae60658fea80acfb15890 Mon Sep 17 00:00:00 2001 From: Andre Przywara Date: Tue, 17 May 2022 11:14:10 +0100 Subject: of/fdt: Ignore disabled memory nodes When we boot a machine using a devicetree, the generic DT code goes through all nodes with a 'device_type = "memory"' property, and collects all memory banks mentioned there. However it does not check for the status property, so any nodes which are explicitly "disabled" will still be added as a memblock. This ends up badly for QEMU, when booting with secure firmware on arm/arm64 machines, because QEMU adds a node describing secure-only memory: =================== secram@e000000 { secure-status = "okay"; status = "disabled"; reg = <0x00 0xe000000 0x00 0x1000000>; device_type = "memory"; }; =================== The kernel will eventually use that memory block (which is located below the main DRAM bank), but accesses to that will be answered with an SError: =================== [ 0.000000] Internal error: synchronous external abort: 96000050 [#1] PREEMPT SMP [ 0.000000] Modules linked in: [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0-rc6-00014-g10c8acb8b679 #524 [ 0.000000] Hardware name: linux,dummy-virt (DT) [ 0.000000] pstate: 200000c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 0.000000] pc : new_slab+0x190/0x340 [ 0.000000] lr : new_slab+0x184/0x340 [ 0.000000] sp : ffff80000a4b3d10 .... ================== The actual crash location and call stack will be somewhat random, and depend on the specific allocation of that physical memory range. As the DT spec[1] explicitly mentions standard properties, add a simple check to skip over disabled memory nodes, so that we only use memory that is meant for non-secure code to use. That fixes booting a QEMU arm64 VM with EL3 enabled ("secure=on"), when not using UEFI. In this case the QEMU generated DT will be handed on to the kernel, which will see the secram node. This issue is reproducible when using TF-A together with U-Boot as firmware, then booting with the "booti" command. When using U-Boot as an UEFI provider, the code there [2] explicitly filters for disabled nodes when generating the UEFI memory map, so we are safe. EDK/2 only reads the first bank of the first DT memory node [3] to learn about memory, so we got lucky there. [1] https://github.com/devicetree-org/devicetree-specification/blob/main/source/chapter3-devicenodes.rst#memory-node (after the table) [2] https://source.denx.de/u-boot/u-boot/-/blob/master/lib/fdtdec.c#L1061-1063 [3] https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/PrePi/FdtParser.c Reported-by: Ross Burton Signed-off-by: Andre Przywara Signed-off-by: Rob Herring Link: https://lore.kernel.org/r/20220517101410.3493781-1-andre.przywara@arm.com --- drivers/of/fdt.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'drivers/of') diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index ec315b060cd5..0f30496ce80b 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -1105,6 +1105,9 @@ int __init early_init_dt_scan_memory(void) if (type == NULL || strcmp(type, "memory") != 0) continue; + if (!of_fdt_device_is_available(fdt, node)) + continue; + reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l); if (reg == NULL) reg = of_get_flat_dt_prop(node, "reg", &l); -- cgit v1.2.3 From d036d915b61f23b9e80d93f8a606eebf3bfab73f Mon Sep 17 00:00:00 2001 From: Julia Lawall Date: Sat, 21 May 2022 13:11:09 +0200 Subject: of/irq: fix typo in comment Spelling mistake (triple letters) in comment. Detected with the help of Coccinelle. Signed-off-by: Julia Lawall Signed-off-by: Rob Herring Link: https://lore.kernel.org/r/20220521111145.81697-59-Julia.Lawall@inria.fr --- drivers/of/irq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/of') diff --git a/drivers/of/irq.c b/drivers/of/irq.c index aa485eb312ac..d22f605fa7ee 100644 --- a/drivers/of/irq.c +++ b/drivers/of/irq.c @@ -284,7 +284,7 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) } /* - * Successfully parsed an interrrupt-map translation; copy new + * Successfully parsed an interrupt-map translation; copy new * interrupt specifier into the out_irq structure */ match_array = imap - newaddrsize - newintsize; -- cgit v1.2.3