devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH][next] of: property: Remove calls to of_node_put
@ 2024-05-15 20:29 Shresth Prasad
  2024-05-15 21:22 ` Saravana Kannan
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Shresth Prasad @ 2024-05-15 20:29 UTC (permalink / raw)
  To: robh, saravanak
  Cc: devicetree, linux-kernel, skhan, javier.carrasco.cruz,
	julia.lawall, Shresth Prasad

Add __free cleanup handler to some variable initialisations, which
ensures that the resource is freed as soon as the variable goes out of
scope. Thus removing the need to manually free up the resource using
of_node_put.

Suggested-by: Julia Lawall <julia.lawall@inria.fr>
Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com>
---
I had submitted a similar patch a couple weeks ago addressing the same 
issue, but as it turns out I wasn't thorough enough and had left a couple
instances.

I hope this isn't too big an issue.
---
 drivers/of/property.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 17b294e16c56..96a74f6a8d64 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -773,15 +773,14 @@ EXPORT_SYMBOL(of_graph_get_port_parent);
 struct device_node *of_graph_get_remote_port_parent(
 			       const struct device_node *node)
 {
-	struct device_node *np, *pp;
+	struct device_node *pp;
 
 	/* Get remote endpoint node. */
-	np = of_graph_get_remote_endpoint(node);
+	struct device_node *np __free(device_node) =
+			    of_graph_get_remote_endpoint(node);
 
 	pp = of_graph_get_port_parent(np);
 
-	of_node_put(np);
-
 	return pp;
 }
 EXPORT_SYMBOL(of_graph_get_remote_port_parent);
@@ -835,17 +834,18 @@ EXPORT_SYMBOL(of_graph_get_endpoint_count);
 struct device_node *of_graph_get_remote_node(const struct device_node *node,
 					     u32 port, u32 endpoint)
 {
-	struct device_node *endpoint_node, *remote;
+	struct device_node *endpoint_node __free(device_node) =
+			    of_graph_get_endpoint_by_regs(node, port, endpoint);
+
+	struct device_node *remote __free(device_node) =
+			    of_graph_get_remote_port_parent(endpoint_node);
 
-	endpoint_node = of_graph_get_endpoint_by_regs(node, port, endpoint);
 	if (!endpoint_node) {
 		pr_debug("no valid endpoint (%d, %d) for node %pOF\n",
 			 port, endpoint, node);
 		return NULL;
 	}
 
-	remote = of_graph_get_remote_port_parent(endpoint_node);
-	of_node_put(endpoint_node);
 	if (!remote) {
 		pr_debug("no valid remote node\n");
 		return NULL;
@@ -853,7 +853,6 @@ struct device_node *of_graph_get_remote_node(const struct device_node *node,
 
 	if (!of_device_is_available(remote)) {
 		pr_debug("not available for remote node\n");
-		of_node_put(remote);
 		return NULL;
 	}
 
@@ -1064,19 +1063,15 @@ static void of_link_to_phandle(struct device_node *con_np,
 			      struct device_node *sup_np,
 			      u8 flags)
 {
-	struct device_node *tmp_np = of_node_get(sup_np);
+	struct device_node *tmp_np __free(device_node) = of_node_get(sup_np);
 
 	/* Check that sup_np and its ancestors are available. */
 	while (tmp_np) {
-		if (of_fwnode_handle(tmp_np)->dev) {
-			of_node_put(tmp_np);
+		if (of_fwnode_handle(tmp_np)->dev)
 			break;
-		}
 
-		if (!of_device_is_available(tmp_np)) {
-			of_node_put(tmp_np);
+		if (!of_device_is_available(tmp_np))
 			return;
-		}
 
 		tmp_np = of_get_next_parent(tmp_np);
 	}
-- 
2.45.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH][next] of: property: Remove calls to of_node_put
  2024-05-15 20:29 [PATCH][next] of: property: Remove calls to of_node_put Shresth Prasad
@ 2024-05-15 21:22 ` Saravana Kannan
  2024-05-20 19:20 ` Rob Herring (Arm)
       [not found] ` <CGME20240529101246eucas1p1266853c07f5178c7e3e4b8a264eb436e@eucas1p1.samsung.com>
  2 siblings, 0 replies; 5+ messages in thread
From: Saravana Kannan @ 2024-05-15 21:22 UTC (permalink / raw)
  To: Shresth Prasad
  Cc: robh, devicetree, linux-kernel, skhan, javier.carrasco.cruz,
	julia.lawall

On Wed, May 15, 2024 at 10:35 PM Shresth Prasad
<shresthprasad7@gmail.com> wrote:
>
> Add __free cleanup handler to some variable initialisations, which
> ensures that the resource is freed as soon as the variable goes out of
> scope. Thus removing the need to manually free up the resource using
> of_node_put.
>
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com>
> ---
> I had submitted a similar patch a couple weeks ago addressing the same
> issue, but as it turns out I wasn't thorough enough and had left a couple
> instances.
>
> I hope this isn't too big an issue.

I didn't see the previous patch from a couple weeks ago, but this
patch looks good.

Reviewed-by: Saravana Kannan <saravanak@google.com>

-Saravana

> ---
>  drivers/of/property.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 17b294e16c56..96a74f6a8d64 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -773,15 +773,14 @@ EXPORT_SYMBOL(of_graph_get_port_parent);
>  struct device_node *of_graph_get_remote_port_parent(
>                                const struct device_node *node)
>  {
> -       struct device_node *np, *pp;
> +       struct device_node *pp;
>
>         /* Get remote endpoint node. */
> -       np = of_graph_get_remote_endpoint(node);
> +       struct device_node *np __free(device_node) =
> +                           of_graph_get_remote_endpoint(node);
>
>         pp = of_graph_get_port_parent(np);
>
> -       of_node_put(np);
> -
>         return pp;
>  }
>  EXPORT_SYMBOL(of_graph_get_remote_port_parent);
> @@ -835,17 +834,18 @@ EXPORT_SYMBOL(of_graph_get_endpoint_count);
>  struct device_node *of_graph_get_remote_node(const struct device_node *node,
>                                              u32 port, u32 endpoint)
>  {
> -       struct device_node *endpoint_node, *remote;
> +       struct device_node *endpoint_node __free(device_node) =
> +                           of_graph_get_endpoint_by_regs(node, port, endpoint);
> +
> +       struct device_node *remote __free(device_node) =
> +                           of_graph_get_remote_port_parent(endpoint_node);
>
> -       endpoint_node = of_graph_get_endpoint_by_regs(node, port, endpoint);
>         if (!endpoint_node) {
>                 pr_debug("no valid endpoint (%d, %d) for node %pOF\n",
>                          port, endpoint, node);
>                 return NULL;
>         }
>
> -       remote = of_graph_get_remote_port_parent(endpoint_node);
> -       of_node_put(endpoint_node);
>         if (!remote) {
>                 pr_debug("no valid remote node\n");
>                 return NULL;
> @@ -853,7 +853,6 @@ struct device_node *of_graph_get_remote_node(const struct device_node *node,
>
>         if (!of_device_is_available(remote)) {
>                 pr_debug("not available for remote node\n");
> -               of_node_put(remote);
>                 return NULL;
>         }
>
> @@ -1064,19 +1063,15 @@ static void of_link_to_phandle(struct device_node *con_np,
>                               struct device_node *sup_np,
>                               u8 flags)
>  {
> -       struct device_node *tmp_np = of_node_get(sup_np);
> +       struct device_node *tmp_np __free(device_node) = of_node_get(sup_np);
>
>         /* Check that sup_np and its ancestors are available. */
>         while (tmp_np) {
> -               if (of_fwnode_handle(tmp_np)->dev) {
> -                       of_node_put(tmp_np);
> +               if (of_fwnode_handle(tmp_np)->dev)
>                         break;
> -               }
>
> -               if (!of_device_is_available(tmp_np)) {
> -                       of_node_put(tmp_np);
> +               if (!of_device_is_available(tmp_np))
>                         return;
> -               }
>
>                 tmp_np = of_get_next_parent(tmp_np);
>         }
> --
> 2.45.1
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH][next] of: property: Remove calls to of_node_put
  2024-05-15 20:29 [PATCH][next] of: property: Remove calls to of_node_put Shresth Prasad
  2024-05-15 21:22 ` Saravana Kannan
@ 2024-05-20 19:20 ` Rob Herring (Arm)
       [not found] ` <CGME20240529101246eucas1p1266853c07f5178c7e3e4b8a264eb436e@eucas1p1.samsung.com>
  2 siblings, 0 replies; 5+ messages in thread
From: Rob Herring (Arm) @ 2024-05-20 19:20 UTC (permalink / raw)
  To: Shresth Prasad
  Cc: julia.lawall, javier.carrasco.cruz, devicetree, saravanak,
	linux-kernel, skhan


On Thu, 16 May 2024 01:59:17 +0530, Shresth Prasad wrote:
> Add __free cleanup handler to some variable initialisations, which
> ensures that the resource is freed as soon as the variable goes out of
> scope. Thus removing the need to manually free up the resource using
> of_node_put.
> 
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com>
> ---
> I had submitted a similar patch a couple weeks ago addressing the same
> issue, but as it turns out I wasn't thorough enough and had left a couple
> instances.
> 
> I hope this isn't too big an issue.
> ---
>  drivers/of/property.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
> 

Applied, thanks!


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH][next] of: property: Remove calls to of_node_put
       [not found] ` <CGME20240529101246eucas1p1266853c07f5178c7e3e4b8a264eb436e@eucas1p1.samsung.com>
@ 2024-05-29 10:12   ` Marek Szyprowski
  2024-05-29 10:50     ` Shresth Prasad
  0 siblings, 1 reply; 5+ messages in thread
From: Marek Szyprowski @ 2024-05-29 10:12 UTC (permalink / raw)
  To: Shresth Prasad, robh, saravanak, DRI mailing list
  Cc: devicetree, linux-kernel, skhan, javier.carrasco.cruz,
	julia.lawall

On 15.05.2024 22:29, Shresth Prasad wrote:
> Add __free cleanup handler to some variable initialisations, which
> ensures that the resource is freed as soon as the variable goes out of
> scope. Thus removing the need to manually free up the resource using
> of_node_put.
>
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com>
> ---

This patch landed in today's linux-next as commit b94d24c08ee1 ("of: 
property: Remove calls to of_node_put"). I found that it triggers the 
following warning while booting of the Samsung Exynos5800 based Pi 
Chromebook (arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts):

OF: ERROR: of_node_release() detected bad of_node_put() on /panel
CPU: 2 PID: 68 Comm: kworker/u36:1 Not tainted 
6.10.0-rc1-00001-gb94d24c08ee1 #8619
Hardware name: Samsung Exynos (Flattened Device Tree)
Workqueue: events_unbound deferred_probe_work_func
tps65090 20-0048: No cache defaults, reading back from HW
Call trace:
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x50/0x64
  dump_stack_lvl from of_node_release+0x110/0x138
  of_node_release from kobject_put+0x98/0x108
  kobject_put from drm_of_find_panel_or_bridge+0x94/0xd8
  drm_of_find_panel_or_bridge from exynos_dp_probe+0xf0/0x158 [exynosdrm]
  exynos_dp_probe [exynosdrm] from platform_probe+0x80/0xc0
  platform_probe from really_probe+0xc8/0x288
  really_probe from __driver_probe_device+0x8c/0x190
  __driver_probe_device from driver_probe_device+0x30/0xd0
  driver_probe_device from __device_attach_driver+0x8c/0xbc
  __device_attach_driver from bus_for_each_drv+0x74/0xc0
  bus_for_each_drv from __device_attach+0xe8/0x184
  __device_attach from bus_probe_device+0x88/0x8c
  bus_probe_device from deferred_probe_work_func+0x7c/0xa8
  deferred_probe_work_func from process_scheduled_works+0xe8/0x41c
  process_scheduled_works from worker_thread+0x14c/0x35c
  worker_thread from kthread+0xd0/0x104
  kthread from ret_from_fork+0x14/0x28
Exception stack(0xf0a81fb0 to 0xf0a81ff8)

OF: ERROR: next of_node_put() on this node will result in a kobject 
warning 'refcount_t: underflow; use-after-free.'
------------[ cut here ]------------
WARNING: CPU: 3 PID: 68 at lib/refcount.c:25 kobject_get+0xa0/0xa4
refcount_t: addition on 0; use-after-free.
Modules linked in: i2c_cros_ec_tunnel(+) cros_ec_keyb cros_ec_dev 
cros_ec_spi cros_ec snd_soc_i2s snd_soc_idma snd_soc_max98090 
snd_soc_snow snd_soc_s3c_dma snd_soc_core tpm_i2c_infineon ac97_bus 
snd_pcm_dmaengine tpm exynosdrm libsha256 libaescfb snd_pcm analogix_dp 
ecdh_generic samsung_dsim ecc snd_timer atmel_mxt_ts snd libaes 
soundcore exynos_gsc s5p_jpeg s5p_mfc v4l2_mem2mem spi_s3c64xx 
videobuf2_dma_contig exynos_adc pwm_samsung videobuf2_memops 
videobuf2_v4l2 videodev phy_exynos_usb2 videobuf2_common ohci_exynos 
ehci_exynos mc exynos_ppmu rtc_s3c exynos_rng s3c2410_wdt s5p_sss 
phy_exynos_mipi_video phy_exynos_dp_video
CPU: 3 PID: 68 Comm: kworker/u36:1 Not tainted 
6.10.0-rc1-00001-gb94d24c08ee1 #8619
Hardware name: Samsung Exynos (Flattened Device Tree)
Workqueue: events_unbound deferred_probe_work_func
Call trace:
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x50/0x64
  dump_stack_lvl from __warn+0x108/0x12c
  __warn from warn_slowpath_fmt+0x118/0x17c
  warn_slowpath_fmt from kobject_get+0xa0/0xa4
  kobject_get from of_node_get+0x14/0x1c
  of_node_get from of_get_next_parent+0x24/0x50
  of_get_next_parent from of_graph_get_port_parent.part.1+0x58/0xa4
  of_graph_get_port_parent.part.1 from 
of_graph_get_remote_port_parent+0x1c/0x38
  of_graph_get_remote_port_parent from of_graph_get_remote_node+0x10/0x6c
  of_graph_get_remote_node from drm_of_find_panel_or_bridge+0x50/0xd8
  drm_of_find_panel_or_bridge from exynos_dp_probe+0xf0/0x158 [exynosdrm]
  exynos_dp_probe [exynosdrm] from platform_probe+0x80/0xc0
  platform_probe from really_probe+0xc8/0x288
  really_probe from __driver_probe_device+0x8c/0x190
  __driver_probe_device from driver_probe_device+0x30/0xd0
  driver_probe_device from __device_attach_driver+0x8c/0xbc
  __device_attach_driver from bus_for_each_drv+0x74/0xc0
  bus_for_each_drv from __device_attach+0xe8/0x184
  __device_attach from bus_probe_device+0x88/0x8c
  bus_probe_device from deferred_probe_work_func+0x7c/0xa8
  deferred_probe_work_func from process_scheduled_works+0xe8/0x41c
  process_scheduled_works from worker_thread+0x14c/0x35c
  worker_thread from kthread+0xd0/0x104
  kthread from ret_from_fork+0x14/0x28
Exception stack(0xf0a81fb0 to 0xf0a81ff8)

---[ end trace 0000000000000000 ]---
------------[ cut here ]------------

If I got this right, this points to the drm_of_find_panel_or_bridge() 
function. I briefly scanned it, but I don't see any obvious 
of_node_put() related issue there.


> I had submitted a similar patch a couple weeks ago addressing the same
> issue, but as it turns out I wasn't thorough enough and had left a couple
> instances.
>
> I hope this isn't too big an issue.
> ---
>   drivers/of/property.c | 27 +++++++++++----------------
>   1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 17b294e16c56..96a74f6a8d64 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -773,15 +773,14 @@ EXPORT_SYMBOL(of_graph_get_port_parent);
>   struct device_node *of_graph_get_remote_port_parent(
>   			       const struct device_node *node)
>   {
> -	struct device_node *np, *pp;
> +	struct device_node *pp;
>   
>   	/* Get remote endpoint node. */
> -	np = of_graph_get_remote_endpoint(node);
> +	struct device_node *np __free(device_node) =
> +			    of_graph_get_remote_endpoint(node);
>   
>   	pp = of_graph_get_port_parent(np);
>   
> -	of_node_put(np);
> -
>   	return pp;
>   }
>   EXPORT_SYMBOL(of_graph_get_remote_port_parent);
> @@ -835,17 +834,18 @@ EXPORT_SYMBOL(of_graph_get_endpoint_count);
>   struct device_node *of_graph_get_remote_node(const struct device_node *node,
>   					     u32 port, u32 endpoint)
>   {
> -	struct device_node *endpoint_node, *remote;
> +	struct device_node *endpoint_node __free(device_node) =
> +			    of_graph_get_endpoint_by_regs(node, port, endpoint);
> +
> +	struct device_node *remote __free(device_node) =
> +			    of_graph_get_remote_port_parent(endpoint_node);
>   
> -	endpoint_node = of_graph_get_endpoint_by_regs(node, port, endpoint);
>   	if (!endpoint_node) {
>   		pr_debug("no valid endpoint (%d, %d) for node %pOF\n",
>   			 port, endpoint, node);
>   		return NULL;
>   	}
>   
> -	remote = of_graph_get_remote_port_parent(endpoint_node);
> -	of_node_put(endpoint_node);
>   	if (!remote) {
>   		pr_debug("no valid remote node\n");
>   		return NULL;
> @@ -853,7 +853,6 @@ struct device_node *of_graph_get_remote_node(const struct device_node *node,
>   
>   	if (!of_device_is_available(remote)) {
>   		pr_debug("not available for remote node\n");
> -		of_node_put(remote);
>   		return NULL;
>   	}
>   
> @@ -1064,19 +1063,15 @@ static void of_link_to_phandle(struct device_node *con_np,
>   			      struct device_node *sup_np,
>   			      u8 flags)
>   {
> -	struct device_node *tmp_np = of_node_get(sup_np);
> +	struct device_node *tmp_np __free(device_node) = of_node_get(sup_np);
>   
>   	/* Check that sup_np and its ancestors are available. */
>   	while (tmp_np) {
> -		if (of_fwnode_handle(tmp_np)->dev) {
> -			of_node_put(tmp_np);
> +		if (of_fwnode_handle(tmp_np)->dev)
>   			break;
> -		}
>   
> -		if (!of_device_is_available(tmp_np)) {
> -			of_node_put(tmp_np);
> +		if (!of_device_is_available(tmp_np))
>   			return;
> -		}
>   
>   		tmp_np = of_get_next_parent(tmp_np);
>   	}

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH][next] of: property: Remove calls to of_node_put
  2024-05-29 10:12   ` Marek Szyprowski
@ 2024-05-29 10:50     ` Shresth Prasad
  0 siblings, 0 replies; 5+ messages in thread
From: Shresth Prasad @ 2024-05-29 10:50 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: robh, saravanak, DRI mailing list, devicetree, linux-kernel,
	skhan, javier.carrasco.cruz, julia.lawall

29 May 2024 3:42:48 pm Marek Szyprowski <m.szyprowski@samsung.com>:

> On 15.05.2024 22:29, Shresth Prasad wrote:
>> Add __free cleanup handler to some variable initialisations, which
>> ensures that the resource is freed as soon as the variable goes out of
>> scope. Thus removing the need to manually free up the resource using
>> of_node_put.
>> 
>> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
>> Signed-off-by: Shresth Prasad <shresthprasad7@gmail.com>
>> ---
> 
> This patch landed in today's linux-next as commit b94d24c08ee1 ("of:
> property: Remove calls to of_node_put"). I found that it triggers the
> following warning while booting of the Samsung Exynos5800 based Pi
> Chromebook (arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts):
> 
> OF: ERROR: of_node_release() detected bad of_node_put() on /panel
> CPU: 2 PID: 68 Comm: kworker/u36:1 Not tainted
> 6.10.0-rc1-00001-gb94d24c08ee1 #8619
> Hardware name: Samsung Exynos (Flattened Device Tree)
> Workqueue: events_unbound deferred_probe_work_func
> tps65090 20-0048: No cache defaults, reading back from HW
> Call trace:
>  unwind_backtrace from show_stack+0x10/0x14
>  show_stack from dump_stack_lvl+0x50/0x64
>  dump_stack_lvl from of_node_release+0x110/0x138
>  of_node_release from kobject_put+0x98/0x108
>  kobject_put from drm_of_find_panel_or_bridge+0x94/0xd8
>  drm_of_find_panel_or_bridge from exynos_dp_probe+0xf0/0x158 [exynosdrm]
>  exynos_dp_probe [exynosdrm] from platform_probe+0x80/0xc0
>  platform_probe from really_probe+0xc8/0x288
>  really_probe from __driver_probe_device+0x8c/0x190
>  __driver_probe_device from driver_probe_device+0x30/0xd0
>  driver_probe_device from __device_attach_driver+0x8c/0xbc
>  __device_attach_driver from bus_for_each_drv+0x74/0xc0
>  bus_for_each_drv from __device_attach+0xe8/0x184
>  __device_attach from bus_probe_device+0x88/0x8c
>  bus_probe_device from deferred_probe_work_func+0x7c/0xa8
>  deferred_probe_work_func from process_scheduled_works+0xe8/0x41c
>  process_scheduled_works from worker_thread+0x14c/0x35c
>  worker_thread from kthread+0xd0/0x104
>  kthread from ret_from_fork+0x14/0x28
> Exception stack(0xf0a81fb0 to 0xf0a81ff8)
> 
> OF: ERROR: next of_node_put() on this node will result in a kobject
> warning 'refcount_t: underflow; use-after-free.'
> ------------[ cut here ]------------
> WARNING: CPU: 3 PID: 68 at lib/refcount.c:25 kobject_get+0xa0/0xa4
> refcount_t: addition on 0; use-after-free.
> Modules linked in: i2c_cros_ec_tunnel(+) cros_ec_keyb cros_ec_dev
> cros_ec_spi cros_ec snd_soc_i2s snd_soc_idma snd_soc_max98090
> snd_soc_snow snd_soc_s3c_dma snd_soc_core tpm_i2c_infineon ac97_bus
> snd_pcm_dmaengine tpm exynosdrm libsha256 libaescfb snd_pcm analogix_dp
> ecdh_generic samsung_dsim ecc snd_timer atmel_mxt_ts snd libaes
> soundcore exynos_gsc s5p_jpeg s5p_mfc v4l2_mem2mem spi_s3c64xx
> videobuf2_dma_contig exynos_adc pwm_samsung videobuf2_memops
> videobuf2_v4l2 videodev phy_exynos_usb2 videobuf2_common ohci_exynos
> ehci_exynos mc exynos_ppmu rtc_s3c exynos_rng s3c2410_wdt s5p_sss
> phy_exynos_mipi_video phy_exynos_dp_video
> CPU: 3 PID: 68 Comm: kworker/u36:1 Not tainted
> 6.10.0-rc1-00001-gb94d24c08ee1 #8619
> Hardware name: Samsung Exynos (Flattened Device Tree)
> Workqueue: events_unbound deferred_probe_work_func
> Call trace:
>  unwind_backtrace from show_stack+0x10/0x14
>  show_stack from dump_stack_lvl+0x50/0x64
>  dump_stack_lvl from __warn+0x108/0x12c
>  __warn from warn_slowpath_fmt+0x118/0x17c
>  warn_slowpath_fmt from kobject_get+0xa0/0xa4
>  kobject_get from of_node_get+0x14/0x1c
>  of_node_get from of_get_next_parent+0x24/0x50
>  of_get_next_parent from of_graph_get_port_parent.part.1+0x58/0xa4
>  of_graph_get_port_parent.part.1 from
> of_graph_get_remote_port_parent+0x1c/0x38
>  of_graph_get_remote_port_parent from of_graph_get_remote_node+0x10/0x6c
>  of_graph_get_remote_node from drm_of_find_panel_or_bridge+0x50/0xd8
>  drm_of_find_panel_or_bridge from exynos_dp_probe+0xf0/0x158 [exynosdrm]
>  exynos_dp_probe [exynosdrm] from platform_probe+0x80/0xc0
>  platform_probe from really_probe+0xc8/0x288
>  really_probe from __driver_probe_device+0x8c/0x190
>  __driver_probe_device from driver_probe_device+0x30/0xd0
>  driver_probe_device from __device_attach_driver+0x8c/0xbc
>  __device_attach_driver from bus_for_each_drv+0x74/0xc0
>  bus_for_each_drv from __device_attach+0xe8/0x184
>  __device_attach from bus_probe_device+0x88/0x8c
>  bus_probe_device from deferred_probe_work_func+0x7c/0xa8
>  deferred_probe_work_func from process_scheduled_works+0xe8/0x41c
>  process_scheduled_works from worker_thread+0x14c/0x35c
>  worker_thread from kthread+0xd0/0x104
>  kthread from ret_from_fork+0x14/0x28
> Exception stack(0xf0a81fb0 to 0xf0a81ff8)
> 
> ---[ end trace 0000000000000000 ]---
> ------------[ cut here ]------------
> 
> If I got this right, this points to the drm_of_find_panel_or_bridge()
> function. I briefly scanned it, but I don't see any obvious
> of_node_put() related issue there.
> 
> 
>> I had submitted a similar patch a couple weeks ago addressing the same
>> issue, but as it turns out I wasn't thorough enough and had left a couple
>> instances.
>> 
>> I hope this isn't too big an issue.
>> ---
>>   drivers/of/property.c | 27 +++++++++++----------------
>>   1 file changed, 11 insertions(+), 16 deletions(-)
>> 
>> diff --git a/drivers/of/property.c b/drivers/of/property.c
>> index 17b294e16c56..96a74f6a8d64 100644
>> --- a/drivers/of/property.c
>> +++ b/drivers/of/property.c
>> @@ -773,15 +773,14 @@ EXPORT_SYMBOL(of_graph_get_port_parent);
>>   struct device_node *of_graph_get_remote_port_parent(
>>                    const struct device_node *node)
>>   {
>> -   struct device_node *np, *pp;
>> +   struct device_node *pp;
>>  
>>     /* Get remote endpoint node. */
>> -   np = of_graph_get_remote_endpoint(node);
>> +   struct device_node *np __free(device_node) =
>> +               of_graph_get_remote_endpoint(node);
>>  
>>     pp = of_graph_get_port_parent(np);
>>  
>> -   of_node_put(np);
>> -
>>     return pp;
>>   }
>>   EXPORT_SYMBOL(of_graph_get_remote_port_parent);
>> @@ -835,17 +834,18 @@ EXPORT_SYMBOL(of_graph_get_endpoint_count);
>>   struct device_node *of_graph_get_remote_node(const struct device_node *node,
>>                          u32 port, u32 endpoint)
>>   {
>> -   struct device_node *endpoint_node, *remote;
>> +   struct device_node *endpoint_node __free(device_node) =
>> +               of_graph_get_endpoint_by_regs(node, port, endpoint);
>> +
>> +   struct device_node *remote __free(device_node) =
>> +               of_graph_get_remote_port_parent(endpoint_node);
>>  
>> -   endpoint_node = of_graph_get_endpoint_by_regs(node, port, endpoint);
>>     if (!endpoint_node) {
>>         pr_debug("no valid endpoint (%d, %d) for node %pOF\n",
>>              port, endpoint, node);
>>         return NULL;
>>     }
>>  
>> -   remote = of_graph_get_remote_port_parent(endpoint_node);
>> -   of_node_put(endpoint_node);
>>     if (!remote) {
>>         pr_debug("no valid remote node\n");
>>         return NULL;
>> @@ -853,7 +853,6 @@ struct device_node *of_graph_get_remote_node(const struct device_node *node,
>>  
>>     if (!of_device_is_available(remote)) {
>>         pr_debug("not available for remote node\n");
>> -       of_node_put(remote);
>>         return NULL;
>>     }
>>  
>> @@ -1064,19 +1063,15 @@ static void of_link_to_phandle(struct device_node *con_np,
>>                   struct device_node *sup_np,
>>                   u8 flags)
>>   {
>> -   struct device_node *tmp_np = of_node_get(sup_np);
>> +   struct device_node *tmp_np __free(device_node) = of_node_get(sup_np);
>>  
>>     /* Check that sup_np and its ancestors are available. */
>>     while (tmp_np) {
>> -       if (of_fwnode_handle(tmp_np)->dev) {
>> -           of_node_put(tmp_np);
>> +       if (of_fwnode_handle(tmp_np)->dev)
>>             break;
>> -       }
>>  
>> -       if (!of_device_is_available(tmp_np)) {
>> -           of_node_put(tmp_np);
>> +       if (!of_device_is_available(tmp_np))
>>             return;
>> -       }
>>  
>>         tmp_np = of_get_next_parent(tmp_np);
>>     }
> 
> Best regards
> -- 
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland

Thanks for letting me know.

It seems this has already been fixed by Alexander Stein here:
https://lore.kernel.org/all/20240529073246.537459-1-alexander.stein@ew.tq-group.com/

Regards,
Shresth

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-05-29 10:50 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-15 20:29 [PATCH][next] of: property: Remove calls to of_node_put Shresth Prasad
2024-05-15 21:22 ` Saravana Kannan
2024-05-20 19:20 ` Rob Herring (Arm)
     [not found] ` <CGME20240529101246eucas1p1266853c07f5178c7e3e4b8a264eb436e@eucas1p1.samsung.com>
2024-05-29 10:12   ` Marek Szyprowski
2024-05-29 10:50     ` Shresth Prasad

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).