[PATCH v2 0/3] Do device node auto cleanup in drivers/soc/ti/

[PATCH v2 0/3] Do device node auto cleanup in drivers/soc/ti/

[Kousik Sanagavarapu]

Do "struct device_node" auto cleanup in soc/ti/.  This patch series takes
care of all the cases where this is possible.

v1:

	https://lore.kernel.org/linux-arm-kernel/20240510071432.62913-1-five231003@gmail.com/

Changes since v1:
- Refactor code so that it the scope of the pointers touched is reduced,
  making the code look more clean.
- The above also the side-effect of fixing the errors that clang emitted
  (but my local version of gcc didn't) for PATCH 2/3 during v1.

Sorry for sending the v2 so late.  I was busy with my semester exams.

Kousik Sanagavarapu (3):
  soc: ti: pruss: do device_node auto cleanup
  soc: ti: knav_qmss_queue: do device_node auto cleanup
  soc: ti: pm33xx: do device_node auto cleanup

 drivers/soc/ti/knav_qmss_queue.c |  85 +++++++++-------
 drivers/soc/ti/pm33xx.c          |  20 ++--
 drivers/soc/ti/pruss.c           | 168 ++++++++++++++-----------------
 3 files changed, 131 insertions(+), 142 deletions(-)

-- 
2.45.2.561.g66ac6e4bcd

[PATCH v2 1/3] soc: ti: pruss: do device_node auto cleanup

[Kousik Sanagavarapu]

Use scope based cleanup instead of manual of_node_put() calls, hence
simplifying the handling of error paths at various places.

While at it, refactor the setup code for device node "memories", from
pruss_probe() into a new function pruss_of_setup_memories().  It is
worth noticing that this step is a direct consequence of trying to
minimize the scope of the "struct device_node" pointer to make auto
cleanup read more cleanly.

Suggested-by: Julia Lawall <julia.lawall@inria.fr>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
 drivers/soc/ti/pruss.c | 168 +++++++++++++++++++----------------------
 1 file changed, 78 insertions(+), 90 deletions(-)

diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index 24a42e0b645c..9767d129a8ea 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -381,38 +381,82 @@ static int pruss_clk_mux_setup(struct pruss *pruss, struct clk *clk_mux,
 static int pruss_clk_init(struct pruss *pruss, struct device_node *cfg_node)
 {
 	const struct pruss_private_data *data;
-	struct device_node *clks_np;
 	struct device *dev = pruss->dev;
 	int ret = 0;
 
 	data = of_device_get_match_data(dev);
 
-	clks_np = of_get_child_by_name(cfg_node, "clocks");
-	if (!clks_np) {
-		dev_err(dev, "%pOF is missing its 'clocks' node\n", cfg_node);
-		return -ENODEV;
-	}
+	struct device_node *clks_np __free(device_node) =
+			of_get_child_by_name(cfg_node, "clocks");
+	if (!clks_np)
+		return dev_err_probe(dev, -ENODEV,
+				     "%pOF is missing its 'clocks' node\n", cfg_node);
 
 	if (data && data->has_core_mux_clock) {
 		ret = pruss_clk_mux_setup(pruss, pruss->core_clk_mux,
 					  "coreclk-mux", clks_np);
-		if (ret) {
-			dev_err(dev, "failed to setup coreclk-mux\n");
-			goto put_clks_node;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "failed to setup coreclk-mux\n");
 	}
 
 	ret = pruss_clk_mux_setup(pruss, pruss->iep_clk_mux, "iepclk-mux",
 				  clks_np);
-	if (ret) {
-		dev_err(dev, "failed to setup iepclk-mux\n");
-		goto put_clks_node;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to setup iepclk-mux\n");
 
-put_clks_node:
-	of_node_put(clks_np);
+	return 0;
+}
 
-	return ret;
+static int pruss_of_setup_memories(struct device *dev, struct pruss *pruss)
+{
+	struct device_node *np = dev_of_node(dev);
+	struct device_node *child __free(device_node) =
+			of_get_child_by_name(np, "memories");
+	int i;
+
+	if (!child)
+		return dev_err_probe(dev, -ENODEV,
+				     "%pOF is missing its 'memories' node\n",
+				     child);
+
+	for (i = 0; i < PRUSS_MEM_MAX; i++) {
+		const struct pruss_private_data *data =
+				of_device_get_match_data(dev);
+		const char *mem_names[PRUSS_MEM_MAX] =
+				{ "dram0", "dram1", "shrdram2" };
+		struct resource res;
+		int index;
+
+		/*
+		 * On AM437x one of two PRUSS units don't contain Shared RAM,
+		 * skip it
+		 */
+		if (data && data->has_no_sharedram && i == PRUSS_MEM_SHRD_RAM2)
+			continue;
+
+		index = of_property_match_string(child, "reg-names",
+						 mem_names[i]);
+		if (index < 0)
+			return index;
+
+		if (of_address_to_resource(child, index, &res))
+			return -EINVAL;
+
+		pruss->mem_regions[i].va = devm_ioremap(dev, res.start,
+							resource_size(&res));
+		if (!pruss->mem_regions[i].va)
+			return dev_err_probe(dev, -ENOMEM,
+					     "failed to parse and map memory resource %d %s\n",
+					     i, mem_names[i]);
+		pruss->mem_regions[i].pa = res.start;
+		pruss->mem_regions[i].size = resource_size(&res);
+
+		dev_dbg(dev, "memory %8s: pa %pa size 0x%zx va %pK\n",
+			mem_names[i], &pruss->mem_regions[i].pa,
+			pruss->mem_regions[i].size, pruss->mem_regions[i].va);
+	}
+	return 0;
 }
 
 static struct regmap_config regmap_conf = {
@@ -424,26 +468,21 @@ static struct regmap_config regmap_conf = {
 static int pruss_cfg_of_init(struct device *dev, struct pruss *pruss)
 {
 	struct device_node *np = dev_of_node(dev);
-	struct device_node *child;
+	struct device_node *child __free(device_node) =
+			of_get_child_by_name(np, "cfg");
 	struct resource res;
 	int ret;
 
-	child = of_get_child_by_name(np, "cfg");
-	if (!child) {
-		dev_err(dev, "%pOF is missing its 'cfg' node\n", child);
-		return -ENODEV;
-	}
+	if (!child)
+		return dev_err_probe(dev, -ENODEV,
+				     "%pOF is missing its 'cfg' node\n", child);
 
-	if (of_address_to_resource(child, 0, &res)) {
-		ret = -ENOMEM;
-		goto node_put;
-	}
+	if (of_address_to_resource(child, 0, &res))
+		return -ENOMEM;
 
 	pruss->cfg_base = devm_ioremap(dev, res.start, resource_size(&res));
-	if (!pruss->cfg_base) {
-		ret = -ENOMEM;
-		goto node_put;
-	}
+	if (!pruss->cfg_base)
+		return -ENOMEM;
 
 	regmap_conf.name = kasprintf(GFP_KERNEL, "%pOFn@%llx", child,
 				     (u64)res.start);
@@ -452,34 +491,22 @@ static int pruss_cfg_of_init(struct device *dev, struct pruss *pruss)
 	pruss->cfg_regmap = devm_regmap_init_mmio(dev, pruss->cfg_base,
 						  &regmap_conf);
 	kfree(regmap_conf.name);
-	if (IS_ERR(pruss->cfg_regmap)) {
-		dev_err(dev, "regmap_init_mmio failed for cfg, ret = %ld\n",
-			PTR_ERR(pruss->cfg_regmap));
-		ret = PTR_ERR(pruss->cfg_regmap);
-		goto node_put;
-	}
+	if (IS_ERR(pruss->cfg_regmap))
+		return dev_err_probe(dev, PTR_ERR(pruss->cfg_regmap),
+				     "regmap_init_mmio failed for cfg\n");
 
 	ret = pruss_clk_init(pruss, child);
 	if (ret)
-		dev_err(dev, "pruss_clk_init failed, ret = %d\n", ret);
+		return dev_err_probe(dev, ret, "pruss_clk_init failed\n");
 
-node_put:
-	of_node_put(child);
 	return ret;
 }
 
 static int pruss_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *np = dev_of_node(dev);
-	struct device_node *child;
 	struct pruss *pruss;
-	struct resource res;
-	int ret, i, index;
-	const struct pruss_private_data *data;
-	const char *mem_names[PRUSS_MEM_MAX] = { "dram0", "dram1", "shrdram2" };
-
-	data = of_device_get_match_data(&pdev->dev);
+	int ret;
 
 	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
 	if (ret) {
@@ -494,48 +521,9 @@ static int pruss_probe(struct platform_device *pdev)
 	pruss->dev = dev;
 	mutex_init(&pruss->lock);
 
-	child = of_get_child_by_name(np, "memories");
-	if (!child) {
-		dev_err(dev, "%pOF is missing its 'memories' node\n", child);
-		return -ENODEV;
-	}
-
-	for (i = 0; i < PRUSS_MEM_MAX; i++) {
-		/*
-		 * On AM437x one of two PRUSS units don't contain Shared RAM,
-		 * skip it
-		 */
-		if (data && data->has_no_sharedram && i == PRUSS_MEM_SHRD_RAM2)
-			continue;
-
-		index = of_property_match_string(child, "reg-names",
-						 mem_names[i]);
-		if (index < 0) {
-			of_node_put(child);
-			return index;
-		}
-
-		if (of_address_to_resource(child, index, &res)) {
-			of_node_put(child);
-			return -EINVAL;
-		}
-
-		pruss->mem_regions[i].va = devm_ioremap(dev, res.start,
-							resource_size(&res));
-		if (!pruss->mem_regions[i].va) {
-			dev_err(dev, "failed to parse and map memory resource %d %s\n",
-				i, mem_names[i]);
-			of_node_put(child);
-			return -ENOMEM;
-		}
-		pruss->mem_regions[i].pa = res.start;
-		pruss->mem_regions[i].size = resource_size(&res);
-
-		dev_dbg(dev, "memory %8s: pa %pa size 0x%zx va %pK\n",
-			mem_names[i], &pruss->mem_regions[i].pa,
-			pruss->mem_regions[i].size, pruss->mem_regions[i].va);
-	}
-	of_node_put(child);
+	ret = pruss_of_setup_memories(dev, pruss);
+	if (ret < 0)
+		goto rpm_put;
 
 	platform_set_drvdata(pdev, pruss);
 
-- 
2.45.2.561.g66ac6e4bcd

[PATCH v2 2/3] soc: ti: knav_qmss_queue: do device_node auto cleanup

[Kousik Sanagavarapu]

Use scope based cleanup, instead of manual of_node_put() calls, which
automatically free()s "struct device_node".

While at it, refactor the code from knav_queue_probe() into the seperate
functions to make auto cleanup look more neat.

Doing the cleanup this way has the advantage of reducing the chance of
memory leaks in case we need to read from new OF nodes in the future
when we probe.

Suggested-by: Julia Lawall <julia.lawall@inria.fr>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
 drivers/soc/ti/knav_qmss_queue.c | 85 +++++++++++++++++---------------
 1 file changed, 46 insertions(+), 39 deletions(-)

diff --git a/drivers/soc/ti/knav_qmss_queue.c b/drivers/soc/ti/knav_qmss_queue.c
index 06fb5505c22c..767b9c49ea93 100644
--- a/drivers/soc/ti/knav_qmss_queue.c
+++ b/drivers/soc/ti/knav_qmss_queue.c
@@ -1076,14 +1076,20 @@ static const char *knav_queue_find_name(struct device_node *node)
 }
 
 static int knav_queue_setup_regions(struct knav_device *kdev,
-					struct device_node *regions)
+				    struct device_node *node)
 {
 	struct device *dev = kdev->dev;
+	struct device_node *regions __free(device_node) =
+			of_get_child_by_name(node, "descriptor-regions");
 	struct knav_region *region;
 	struct device_node *child;
 	u32 temp[2];
 	int ret;
 
+	if (!regions)
+		return dev_err_probe(dev, -ENODEV,
+				     "descriptor-regions not specified\n");
+
 	for_each_child_of_node(regions, child) {
 		region = devm_kzalloc(dev, sizeof(*region), GFP_KERNEL);
 		if (!region) {
@@ -1306,10 +1312,16 @@ static int knav_setup_queue_range(struct knav_device *kdev,
 }
 
 static int knav_setup_queue_pools(struct knav_device *kdev,
-				   struct device_node *queue_pools)
+				  struct device_node *node)
 {
+	struct device_node *queue_pools __free(device_node) =
+			of_get_child_by_name(node, "queue-pools");
 	struct device_node *type, *range;
 
+	if (!queue_pools)
+		return dev_err_probe(kdev->dev, -ENODEV,
+				     "queue-pools not specified\n");
+
 	for_each_child_of_node(queue_pools, type) {
 		for_each_child_of_node(type, range) {
 			/* return value ignored, we init the rest... */
@@ -1389,14 +1401,20 @@ static void __iomem *knav_queue_map_reg(struct knav_device *kdev,
 }
 
 static int knav_queue_init_qmgrs(struct knav_device *kdev,
-					struct device_node *qmgrs)
+				 struct device_node *node)
 {
 	struct device *dev = kdev->dev;
+	struct device_node *qmgrs __free(device_node) =
+			of_get_child_by_name(node, "qmgrs");
 	struct knav_qmgr_info *qmgr;
 	struct device_node *child;
 	u32 temp[2];
 	int ret;
 
+	if (!qmgrs)
+		return dev_err_probe(dev, -ENODEV,
+				     "queue manager info not specified\n");
+
 	for_each_child_of_node(qmgrs, child) {
 		qmgr = devm_kzalloc(dev, sizeof(*qmgr), GFP_KERNEL);
 		if (!qmgr) {
@@ -1668,6 +1686,25 @@ static int knav_queue_start_pdsps(struct knav_device *kdev)
 	return 0;
 }
 
+static int knav_queue_setup_pdsps(struct knav_device *kdev,
+				  struct device_node *node)
+{
+	struct device_node *pdsps __free(device_node) =
+			of_get_child_by_name(node, "pdsps");
+
+	if (pdsps) {
+		int ret;
+
+		ret = knav_queue_init_pdsps(kdev, pdsps);
+		if (ret)
+			return ret;
+		ret = knav_queue_start_pdsps(kdev);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
 static inline struct knav_qmgr_info *knav_find_qmgr(unsigned id)
 {
 	struct knav_qmgr_info *qmgr;
@@ -1755,7 +1792,6 @@ MODULE_DEVICE_TABLE(of, keystone_qmss_of_match);
 static int knav_queue_probe(struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
-	struct device_node *qmgrs, *queue_pools, *regions, *pdsps;
 	struct device *dev = &pdev->dev;
 	u32 temp[2];
 	int ret;
@@ -1799,39 +1835,17 @@ static int knav_queue_probe(struct platform_device *pdev)
 	kdev->num_queues = temp[1];
 
 	/* Initialize queue managers using device tree configuration */
-	qmgrs =  of_get_child_by_name(node, "qmgrs");
-	if (!qmgrs) {
-		dev_err(dev, "queue manager info not specified\n");
-		ret = -ENODEV;
-		goto err;
-	}
-	ret = knav_queue_init_qmgrs(kdev, qmgrs);
-	of_node_put(qmgrs);
+	ret = knav_queue_init_qmgrs(kdev, node);
 	if (ret)
 		goto err;
 
 	/* get pdsp configuration values from device tree */
-	pdsps =  of_get_child_by_name(node, "pdsps");
-	if (pdsps) {
-		ret = knav_queue_init_pdsps(kdev, pdsps);
-		if (ret)
-			goto err;
-
-		ret = knav_queue_start_pdsps(kdev);
-		if (ret)
-			goto err;
-	}
-	of_node_put(pdsps);
+	ret = knav_queue_setup_pdsps(kdev, node);
+	if (ret)
+		goto err;
 
 	/* get usable queue range values from device tree */
-	queue_pools = of_get_child_by_name(node, "queue-pools");
-	if (!queue_pools) {
-		dev_err(dev, "queue-pools not specified\n");
-		ret = -ENODEV;
-		goto err;
-	}
-	ret = knav_setup_queue_pools(kdev, queue_pools);
-	of_node_put(queue_pools);
+	ret = knav_setup_queue_pools(kdev, node);
 	if (ret)
 		goto err;
 
@@ -1853,14 +1867,7 @@ static int knav_queue_probe(struct platform_device *pdev)
 	if (ret)
 		goto err;
 
-	regions = of_get_child_by_name(node, "descriptor-regions");
-	if (!regions) {
-		dev_err(dev, "descriptor-regions not specified\n");
-		ret = -ENODEV;
-		goto err;
-	}
-	ret = knav_queue_setup_regions(kdev, regions);
-	of_node_put(regions);
+	ret = knav_queue_setup_regions(kdev, node);
 	if (ret)
 		goto err;
 
-- 
2.45.2.561.g66ac6e4bcd

[PATCH v2 3/3] soc: ti: pm33xx: do device_node auto cleanup

[Kousik Sanagavarapu]

Use scope based cleanup instead of manual of_node_put() calls, hence
simplifying the handling of error paths.

Suggested-by: Julia Lawall <julia.lawall@inria.fr>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
 drivers/soc/ti/pm33xx.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/soc/ti/pm33xx.c b/drivers/soc/ti/pm33xx.c
index 8e983c3c4e03..40988c45ed00 100644
--- a/drivers/soc/ti/pm33xx.c
+++ b/drivers/soc/ti/pm33xx.c
@@ -383,10 +383,9 @@ static void am33xx_pm_free_sram(void)
  */
 static int am33xx_pm_alloc_sram(void)
 {
-	struct device_node *np;
-	int ret = 0;
+	struct device_node *np __free(device_node) =
+			of_find_compatible_node(NULL, NULL, "ti,omap3-mpu");
 
-	np = of_find_compatible_node(NULL, NULL, "ti,omap3-mpu");
 	if (!np) {
 		np = of_find_compatible_node(NULL, NULL, "ti,omap4-mpu");
 		if (!np) {
@@ -400,24 +399,21 @@ static int am33xx_pm_alloc_sram(void)
 	if (!sram_pool) {
 		dev_err(pm33xx_dev, "PM: %s: Unable to get sram pool for ocmcram\n",
 			__func__);
-		ret = -ENODEV;
-		goto mpu_put_node;
+		return -ENODEV;
 	}
 
 	sram_pool_data = of_gen_pool_get(np, "pm-sram", 1);
 	if (!sram_pool_data) {
 		dev_err(pm33xx_dev, "PM: %s: Unable to get sram data pool for ocmcram\n",
 			__func__);
-		ret = -ENODEV;
-		goto mpu_put_node;
+		return -ENODEV;
 	}
 
 	ocmcram_location = gen_pool_alloc(sram_pool, *pm_sram->do_wfi_sz);
 	if (!ocmcram_location) {
 		dev_err(pm33xx_dev, "PM: %s: Unable to allocate memory from ocmcram\n",
 			__func__);
-		ret = -ENOMEM;
-		goto mpu_put_node;
+		return -ENOMEM;
 	}
 
 	ocmcram_location_data = gen_pool_alloc(sram_pool_data,
@@ -425,12 +421,10 @@ static int am33xx_pm_alloc_sram(void)
 	if (!ocmcram_location_data) {
 		dev_err(pm33xx_dev, "PM: Unable to allocate memory from ocmcram\n");
 		gen_pool_free(sram_pool, ocmcram_location, *pm_sram->do_wfi_sz);
-		ret = -ENOMEM;
+		return -ENOMEM;
 	}
 
-mpu_put_node:
-	of_node_put(np);
-	return ret;
+	return 0;
 }
 
 static int am33xx_pm_rtc_setup(void)
-- 
2.45.2.561.g66ac6e4bcd

Re: [PATCH v2 1/3] soc: ti: pruss: do device_node auto cleanup

[Jonathan Cameron]

On Wed,  3 Jul 2024 12:25:26 +0530
Kousik Sanagavarapu <five231003@gmail.com> wrote:

> Use scope based cleanup instead of manual of_node_put() calls, hence
> simplifying the handling of error paths at various places.
> 
> While at it, refactor the setup code for device node "memories", from
> pruss_probe() into a new function pruss_of_setup_memories().  It is
> worth noticing that this step is a direct consequence of trying to
> minimize the scope of the "struct device_node" pointer to make auto
> cleanup read more cleanly.
> 
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
Hi.

A few minor comments inline.

Jonathan

> ---
>  drivers/soc/ti/pruss.c | 168 +++++++++++++++++++----------------------
>  1 file changed, 78 insertions(+), 90 deletions(-)
> 
> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> index 24a42e0b645c..9767d129a8ea 100644
> --- a/drivers/soc/ti/pruss.c
> +++ b/drivers/soc/ti/pruss.c
> @@ -381,38 +381,82 @@ static int pruss_clk_mux_setup(struct pruss *pruss, struct clk *clk_mux,
>  static int pruss_clk_init(struct pruss *pruss, struct device_node *cfg_node)
>  {
>  	const struct pruss_private_data *data;
> -	struct device_node *clks_np;
>  	struct device *dev = pruss->dev;
>  	int ret = 0;
>  
>  	data = of_device_get_match_data(dev);
>  
> -	clks_np = of_get_child_by_name(cfg_node, "clocks");
> -	if (!clks_np) {
> -		dev_err(dev, "%pOF is missing its 'clocks' node\n", cfg_node);
> -		return -ENODEV;
> -	}
> +	struct device_node *clks_np __free(device_node) =
> +			of_get_child_by_name(cfg_node, "clocks");
> +	if (!clks_np)
> +		return dev_err_probe(dev, -ENODEV,
> +				     "%pOF is missing its 'clocks' node\n", cfg_node);
>  
>  	if (data && data->has_core_mux_clock) {
>  		ret = pruss_clk_mux_setup(pruss, pruss->core_clk_mux,
>  					  "coreclk-mux", clks_np);
> -		if (ret) {
> -			dev_err(dev, "failed to setup coreclk-mux\n");
> -			goto put_clks_node;
> -		}
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to setup coreclk-mux\n");
>  	}
>  
>  	ret = pruss_clk_mux_setup(pruss, pruss->iep_clk_mux, "iepclk-mux",
>  				  clks_np);
> -	if (ret) {
> -		dev_err(dev, "failed to setup iepclk-mux\n");
> -		goto put_clks_node;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to setup iepclk-mux\n");
>  
> -put_clks_node:
> -	of_node_put(clks_np);
> +	return 0;
> +}
>  
> -	return ret;
> +static int pruss_of_setup_memories(struct device *dev, struct pruss *pruss)
> +{
> +	struct device_node *np = dev_of_node(dev);
> +	struct device_node *child __free(device_node) =
> +			of_get_child_by_name(np, "memories");
> +	int i;
> +
> +	if (!child)
> +		return dev_err_probe(dev, -ENODEV,
> +				     "%pOF is missing its 'memories' node\n",
> +				     child);
> +
> +	for (i = 0; i < PRUSS_MEM_MAX; i++) {
> +		const struct pruss_private_data *data =
> +				of_device_get_match_data(dev);
> +		const char *mem_names[PRUSS_MEM_MAX] =
> +				{ "dram0", "dram1", "shrdram2" };

These are not per loop iteration. I'd pull them out of here.
Sure the compiler will probably figure it out, but the code will
more readable if it doesn't look like dev might change from
one iteration to the next.


> +		struct resource res;
> +		int index;
> +
> +		/*
> +		 * On AM437x one of two PRUSS units don't contain Shared RAM,
> +		 * skip it
> +		 */
> +		if (data && data->has_no_sharedram && i == PRUSS_MEM_SHRD_RAM2)
> +			continue;
> +
> +		index = of_property_match_string(child, "reg-names",
> +						 mem_names[i]);
> +		if (index < 0)
> +			return index;
> +
> +		if (of_address_to_resource(child, index, &res))
> +			return -EINVAL;
> +
> +		pruss->mem_regions[i].va = devm_ioremap(dev, res.start,
> +							resource_size(&res));
> +		if (!pruss->mem_regions[i].va)
> +			return dev_err_probe(dev, -ENOMEM,
> +					     "failed to parse and map memory resource %d %s\n",
> +					     i, mem_names[i]);
> +		pruss->mem_regions[i].pa = res.start;
> +		pruss->mem_regions[i].size = resource_size(&res);
> +
> +		dev_dbg(dev, "memory %8s: pa %pa size 0x%zx va %pK\n",
> +			mem_names[i], &pruss->mem_regions[i].pa,
> +			pruss->mem_regions[i].size, pruss->mem_regions[i].va);
> +	}
> +	return 0;
>  }
>  
>  static struct regmap_config regmap_conf = {
> @@ -424,26 +468,21 @@ static struct regmap_config regmap_conf = {
>  static int pruss_cfg_of_init(struct device *dev, struct pruss *pruss)
>  {
>  	struct device_node *np = dev_of_node(dev);
> -	struct device_node *child;
> +	struct device_node *child __free(device_node) =
> +			of_get_child_by_name(np, "cfg");
>  	struct resource res;
>  	int ret;
>  
> -	child = of_get_child_by_name(np, "cfg");
> -	if (!child) {
> -		dev_err(dev, "%pOF is missing its 'cfg' node\n", child);
> -		return -ENODEV;
> -	}
> +	if (!child)
> +		return dev_err_probe(dev, -ENODEV,
> +				     "%pOF is missing its 'cfg' node\n", child);
>  
> -	if (of_address_to_resource(child, 0, &res)) {
> -		ret = -ENOMEM;
> -		goto node_put;
> -	}
> +	if (of_address_to_resource(child, 0, &res))
> +		return -ENOMEM;
>  
>  	pruss->cfg_base = devm_ioremap(dev, res.start, resource_size(&res));
> -	if (!pruss->cfg_base) {
> -		ret = -ENOMEM;
> -		goto node_put;
> -	}
> +	if (!pruss->cfg_base)
> +		return -ENOMEM;
>  
>  	regmap_conf.name = kasprintf(GFP_KERNEL, "%pOFn@%llx", child,
>  				     (u64)res.start);
> @@ -452,34 +491,22 @@ static int pruss_cfg_of_init(struct device *dev, struct pruss *pruss)
>  	pruss->cfg_regmap = devm_regmap_init_mmio(dev, pruss->cfg_base,
>  						  &regmap_conf);
>  	kfree(regmap_conf.name);
> -	if (IS_ERR(pruss->cfg_regmap)) {
> -		dev_err(dev, "regmap_init_mmio failed for cfg, ret = %ld\n",
> -			PTR_ERR(pruss->cfg_regmap));
> -		ret = PTR_ERR(pruss->cfg_regmap);
> -		goto node_put;
> -	}
> +	if (IS_ERR(pruss->cfg_regmap))
> +		return dev_err_probe(dev, PTR_ERR(pruss->cfg_regmap),
> +				     "regmap_init_mmio failed for cfg\n");
>  
>  	ret = pruss_clk_init(pruss, child);
>  	if (ret)
> -		dev_err(dev, "pruss_clk_init failed, ret = %d\n", ret);
> +		return dev_err_probe(dev, ret, "pruss_clk_init failed\n");
>  
> -node_put:
> -	of_node_put(child);
>  	return ret;
return 0;

Make it obvious that it can't be anything else and that if we reach
here we have succeeded.

>  }
>  
>  static int pruss_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct device_node *np = dev_of_node(dev);
> -	struct device_node *child;
>  	struct pruss *pruss;
> -	struct resource res;
> -	int ret, i, index;
> -	const struct pruss_private_data *data;
> -	const char *mem_names[PRUSS_MEM_MAX] = { "dram0", "dram1", "shrdram2" };
> -
> -	data = of_device_get_match_data(&pdev->dev);
> +	int ret;
>  
>  	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>  	if (ret) {
> @@ -494,48 +521,9 @@ static int pruss_probe(struct platform_device *pdev)
>  	pruss->dev = dev;
>  	mutex_init(&pruss->lock);
>  
> -	child = of_get_child_by_name(np, "memories");
> -	if (!child) {
> -		dev_err(dev, "%pOF is missing its 'memories' node\n", child);
> -		return -ENODEV;
> -	}
> -
> -	for (i = 0; i < PRUSS_MEM_MAX; i++) {
> -		/*
> -		 * On AM437x one of two PRUSS units don't contain Shared RAM,
> -		 * skip it
> -		 */
> -		if (data && data->has_no_sharedram && i == PRUSS_MEM_SHRD_RAM2)
> -			continue;
> -
> -		index = of_property_match_string(child, "reg-names",
> -						 mem_names[i]);
> -		if (index < 0) {
> -			of_node_put(child);
> -			return index;
> -		}
> -
> -		if (of_address_to_resource(child, index, &res)) {
> -			of_node_put(child);
> -			return -EINVAL;
> -		}
> -
> -		pruss->mem_regions[i].va = devm_ioremap(dev, res.start,
> -							resource_size(&res));
> -		if (!pruss->mem_regions[i].va) {
> -			dev_err(dev, "failed to parse and map memory resource %d %s\n",
> -				i, mem_names[i]);
> -			of_node_put(child);
> -			return -ENOMEM;
> -		}
> -		pruss->mem_regions[i].pa = res.start;
> -		pruss->mem_regions[i].size = resource_size(&res);
> -
> -		dev_dbg(dev, "memory %8s: pa %pa size 0x%zx va %pK\n",
> -			mem_names[i], &pruss->mem_regions[i].pa,
> -			pruss->mem_regions[i].size, pruss->mem_regions[i].va);
> -	}
> -	of_node_put(child);
> +	ret = pruss_of_setup_memories(dev, pruss);
> +	if (ret < 0)
> +		goto rpm_put;
I'd have preferred a precursor patch that was just code movement then
a main one using the auto cleanup.

>  
>  	platform_set_drvdata(pdev, pruss);
>  

Re: [PATCH v2 2/3] soc: ti: knav_qmss_queue: do device_node auto cleanup

[Jonathan Cameron]

On Wed,  3 Jul 2024 12:25:27 +0530
Kousik Sanagavarapu <five231003@gmail.com> wrote:

> Use scope based cleanup, instead of manual of_node_put() calls, which
> automatically free()s "struct device_node".
> 
> While at it, refactor the code from knav_queue_probe() into the seperate
> functions to make auto cleanup look more neat.
> 
> Doing the cleanup this way has the advantage of reducing the chance of
> memory leaks in case we need to read from new OF nodes in the future
> when we probe.
> 
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>

One trivial thing inline
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> @@ -1668,6 +1686,25 @@ static int knav_queue_start_pdsps(struct knav_device *kdev)
>  	return 0;
>  }
>  
> +static int knav_queue_setup_pdsps(struct knav_device *kdev,
> +				  struct device_node *node)
> +{
> +	struct device_node *pdsps __free(device_node) =
> +			of_get_child_by_name(node, "pdsps");
> +
> +	if (pdsps) {
> +		int ret;
> +
> +		ret = knav_queue_init_pdsps(kdev, pdsps);
> +		if (ret)
> +			return ret;
As per original style, readability slightly helped by
a blank line here.

> +		ret = knav_queue_start_pdsps(kdev);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}

Re: [PATCH v2 3/3] soc: ti: pm33xx: do device_node auto cleanup

[Jonathan Cameron]

On Wed,  3 Jul 2024 12:25:28 +0530
Kousik Sanagavarapu <five231003@gmail.com> wrote:

> Use scope based cleanup instead of manual of_node_put() calls, hence
> simplifying the handling of error paths.
> 
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
I think you can make use of dev_err_probe() in here to
further simplify things (a bit anyway!)

Jonathan

> ---
>  drivers/soc/ti/pm33xx.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/soc/ti/pm33xx.c b/drivers/soc/ti/pm33xx.c
> index 8e983c3c4e03..40988c45ed00 100644
> --- a/drivers/soc/ti/pm33xx.c
> +++ b/drivers/soc/ti/pm33xx.c
> @@ -383,10 +383,9 @@ static void am33xx_pm_free_sram(void)
>   */
>  static int am33xx_pm_alloc_sram(void)
>  {
> -	struct device_node *np;
> -	int ret = 0;
> +	struct device_node *np __free(device_node) =
> +			of_find_compatible_node(NULL, NULL, "ti,omap3-mpu");
>  
> -	np = of_find_compatible_node(NULL, NULL, "ti,omap3-mpu");
>  	if (!np) {
>  		np = of_find_compatible_node(NULL, NULL, "ti,omap4-mpu");
>  		if (!np) {
> @@ -400,24 +399,21 @@ static int am33xx_pm_alloc_sram(void)
>  	if (!sram_pool) {
>  		dev_err(pm33xx_dev, "PM: %s: Unable to get sram pool for ocmcram\n",
>  			__func__);
> -		ret = -ENODEV;
> -		goto mpu_put_node;
> +		return -ENODEV;
>  	}
>  
>  	sram_pool_data = of_gen_pool_get(np, "pm-sram", 1);
>  	if (!sram_pool_data) {
>  		dev_err(pm33xx_dev, "PM: %s: Unable to get sram data pool for ocmcram\n",
>  			__func__);
> -		ret = -ENODEV;
> -		goto mpu_put_node;
> +		return -ENODEV;
>  	}
>  
>  	ocmcram_location = gen_pool_alloc(sram_pool, *pm_sram->do_wfi_sz);
>  	if (!ocmcram_location) {
>  		dev_err(pm33xx_dev, "PM: %s: Unable to allocate memory from ocmcram\n",
>  			__func__);
> -		ret = -ENOMEM;
> -		goto mpu_put_node;
> +		return -ENOMEM;
Why not dev_err_probe()?
Seems to only be called from a probe() callback.

>  	}
>  
>  	ocmcram_location_data = gen_pool_alloc(sram_pool_data,
> @@ -425,12 +421,10 @@ static int am33xx_pm_alloc_sram(void)
>  	if (!ocmcram_location_data) {
>  		dev_err(pm33xx_dev, "PM: Unable to allocate memory from ocmcram\n");
>  		gen_pool_free(sram_pool, ocmcram_location, *pm_sram->do_wfi_sz);
> -		ret = -ENOMEM;
> +		return -ENOMEM;
I doubt the ordering matters so can probably do dev_err_probe() in here.
>  	}
>  
> -mpu_put_node:
> -	of_node_put(np);
> -	return ret;
> +	return 0;
>  }
>  
>  static int am33xx_pm_rtc_setup(void)

Re: [PATCH v2 3/3] soc: ti: pm33xx: do device_node auto cleanup

[Kousik Sanagavarapu]

Hi Jonathan,

On Fri, Jul 05, 2024 at 11:54:13AM +0100, Jonathan Cameron wrote:
> On Wed,  3 Jul 2024 12:25:28 +0530
> Kousik Sanagavarapu <five231003@gmail.com> wrote:
> 
> > Use scope based cleanup instead of manual of_node_put() calls, hence
> > simplifying the handling of error paths.
> > 
> > Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> > Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
> I think you can make use of dev_err_probe() in here to
> further simplify things (a bit anyway!)

The changes proposed on all the patches make sense.  I'll send out v3
soon.

I hadn't caught the dev_err_probe() change in this patch ;) while
preparing this series.  I'll make the changes.

Thanks for the review