diff mbox series

[V3,2/2] of/platform: increase refcount of fwnode

Message ID 20230821023928.3324283-2-peng.fan@oss.nxp.com
State New
Headers show
Series None | expand

Commit Message

Peng Fan (OSS) Aug. 21, 2023, 2:39 a.m. UTC
From: Peng Fan <peng.fan@nxp.com>

commit 0f8e5651095b
("of/platform: Propagate firmware node by calling device_set_node()")
use of_fwnode_handle to replace of_node_get, which introduces a side
effect that the refcount is not increased. Then the out of tree
jailhouse hypervisor enable/disable test will trigger kernel dump in
of_overlay_remove, with the following sequence
"
   of_changeset_revert(&overlay_changeset);
   of_changeset_destroy(&overlay_changeset);
   of_overlay_remove(&overlay_id);
"

So increase the refcount to avoid issues.

This patch also release the refcount when releasing amba device to avoid
refcount leakage.

Fixes: 0f8e5651095b ("of/platform: Propagate firmware node by calling device_set_node()")
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V3:
 - Move amba device changes to patch 1/1

V2:
 - Per Andy's comment, use of_fwnode_handle(of_node_get(np))
 - release amba device of_node refcount when releasing amba device, this is
   done from code inspection, no test.

 drivers/of/platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Geert Uytterhoeven Aug. 22, 2023, 10:28 a.m. UTC | #1
Hi Rob,

On Tue, Aug 22, 2023 at 1:33 AM Rob Herring <robh@kernel.org> wrote:
> On Mon, 21 Aug 2023 10:39:28 +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > commit 0f8e5651095b
> > ("of/platform: Propagate firmware node by calling device_set_node()")
> > use of_fwnode_handle to replace of_node_get, which introduces a side
> > effect that the refcount is not increased. Then the out of tree
> > jailhouse hypervisor enable/disable test will trigger kernel dump in
> > of_overlay_remove, with the following sequence
> > "
> >    of_changeset_revert(&overlay_changeset);
> >    of_changeset_destroy(&overlay_changeset);
> >    of_overlay_remove(&overlay_id);
> > "
> >
> > So increase the refcount to avoid issues.
> >
> > This patch also release the refcount when releasing amba device to avoid
> > refcount leakage.
> >
> > Fixes: 0f8e5651095b ("of/platform: Propagate firmware node by calling device_set_node()")
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V3:
> >  - Move amba device changes to patch 1/1
> >
> > V2:
> >  - Per Andy's comment, use of_fwnode_handle(of_node_get(np))
> >  - release amba device of_node refcount when releasing amba device, this is
> >    done from code inspection, no test.
> >
> >  drivers/of/platform.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
>
> Applied, thanks!

Good to see this got applied!

I had missed this fix, and spent quite some time investigating why multiple
overlay add/removal sequences caused a crash.  Only after I had
written a reproducer unittest[1], I managed to bisect the issue,
and found Peng's fix. Thanks!

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

[1] "[PATCH] of: unittest: Run overlay apply/revert sequence three times"
    https://lore.kernel.org/all/a9fb4eb560c58d11a7f167bc78a137b46e76cf15.1692699743.git.geert+renesas@glider.be/

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 267d8c9a5612..d328bbb679c7 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -175,7 +175,7 @@  struct platform_device *of_device_alloc(struct device_node *np,
 	}
 
 	/* setup generic device info */
-	device_set_node(&dev->dev, of_fwnode_handle(np));
+	device_set_node(&dev->dev, of_fwnode_handle(of_node_get(np)));
 	dev->dev.parent = parent ? : &platform_bus;
 
 	if (bus_id)
@@ -273,7 +273,7 @@  static struct amba_device *of_amba_device_create(struct device_node *node,
 	dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
 
 	/* setup generic device info */
-	device_set_node(&dev->dev, of_fwnode_handle(node));
+	device_set_node(&dev->dev, of_fwnode_handle(of_node_get(node)));
 	dev->dev.parent = parent ? : &platform_bus;
 	dev->dev.platform_data = platform_data;
 	if (bus_id)