[PATCH v3 0/4] Do device node auto cleanup in drivers/soc/ti/

[PATCH v3 0/4] 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.

Thanks Jonathan for the review on the previous round.

v2:

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

Changes since v2:
- Split v2 1/3 into v3 1/4 and v3 2/4.  The memory setup code is
  seperated out of the pruss_probe() function and put into 1/4 and the
  actual conversion to auto cleanup is done in 2/4.
- Replace dev_err() with dev_err_probe() in the code paths touched.

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.

Kousik Sanagavarapu (4):
  soc: ti: pruss: factor out memories setup
  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 | 100 +++++++++---------
 drivers/soc/ti/pm33xx.c          |  52 ++++-----
 drivers/soc/ti/pruss.c           | 176 ++++++++++++++-----------------
 3 files changed, 155 insertions(+), 173 deletions(-)

-- 
2.45.2.561.g66ac6e4bcd

[PATCH v3 1/4] soc: ti: pruss: factor out memories setup

[Kousik Sanagavarapu]

Factor out memories setup code from probe() into a new function
pruss_of_setup_memories().  This sets the stage for introducing auto
cleanup of the device node (done in the subsequent patch), since the
clean up depends on the scope of the pointer and factoring out
code into a seperate function obviously limits the scope of the various
variables used in that function.

Apart from the above, this change also has the advantage of making the
code look more neat.

While at it, use dev_err_probe() instead of plain dev_err() as this new
function is called by the probe().

Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
 drivers/soc/ti/pruss.c | 111 ++++++++++++++++++++++-------------------
 1 file changed, 61 insertions(+), 50 deletions(-)

diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index 24a42e0b645c..a3c55a291b0b 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -415,6 +415,63 @@ static int pruss_clk_init(struct pruss *pruss, struct device_node *cfg_node)
 	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;
+	const struct pruss_private_data *data = of_device_get_match_data(dev);
+	const char *mem_names[PRUSS_MEM_MAX] = { "dram0", "dram1", "shrdram2" };
+	int i;
+
+	child = of_get_child_by_name(np, "memories");
+	if (!child)
+		return dev_err_probe(dev, -ENODEV,
+				     "%pOF is missing its 'memories' node\n",
+				     child);
+
+	for (i = 0; i < PRUSS_MEM_MAX; i++) {
+		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) {
+			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) {
+			of_node_put(child);
+			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);
+	}
+	of_node_put(child);
+
+	return 0;
+}
+
 static struct regmap_config regmap_conf = {
 	.reg_bits = 32,
 	.val_bits = 32,
@@ -471,15 +528,8 @@ static int pruss_cfg_of_init(struct device *dev, struct pruss *pruss)
 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 +544,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 v3 2/4] 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, use dev_err_probe() instead of dev_err() in all the code
paths touched.

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

diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index a3c55a291b0b..a01eabb0ca26 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -380,50 +380,42 @@ 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);
+	struct device_node *clks_np __free(device_node) =
+			of_get_child_by_name(cfg_node, "clocks");
+	const struct pruss_private_data *data = of_device_get_match_data(dev);
+	int ret;
 
-	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;
-	}
+	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;
-	}
-
-put_clks_node:
-	of_node_put(clks_np);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to setup iepclk-mux\n");
 
-	return ret;
+	return 0;
 }
 
 static int pruss_of_setup_memories(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, "memories");
 	const struct pruss_private_data *data = of_device_get_match_data(dev);
 	const char *mem_names[PRUSS_MEM_MAX] = { "dram0", "dram1", "shrdram2" };
 	int i;
 
-	child = of_get_child_by_name(np, "memories");
 	if (!child)
 		return dev_err_probe(dev, -ENODEV,
 				     "%pOF is missing its 'memories' node\n",
@@ -442,24 +434,18 @@ static int pruss_of_setup_memories(struct device *dev, struct pruss *pruss)
 
 		index = of_property_match_string(child, "reg-names",
 						 mem_names[i]);
-		if (index < 0) {
-			of_node_put(child);
+		if (index < 0)
 			return index;
-		}
 
-		if (of_address_to_resource(child, index, &res)) {
-			of_node_put(child);
+		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) {
-			of_node_put(child);
+		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);
 
@@ -467,7 +453,6 @@ static int pruss_of_setup_memories(struct device *dev, struct pruss *pruss)
 			mem_names[i], &pruss->mem_regions[i].pa,
 			pruss->mem_regions[i].size, pruss->mem_regions[i].va);
 	}
-	of_node_put(child);
 
 	return 0;
 }
@@ -481,26 +466,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);
@@ -509,20 +489,15 @@ 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;
 }
 
 static int pruss_probe(struct platform_device *pdev)
-- 
2.45.2.561.g66ac6e4bcd

[PATCH v3 3/4] 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>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
---
 drivers/soc/ti/knav_qmss_queue.c | 100 ++++++++++++++++---------------
 1 file changed, 53 insertions(+), 47 deletions(-)

diff --git a/drivers/soc/ti/knav_qmss_queue.c b/drivers/soc/ti/knav_qmss_queue.c
index 06fb5505c22c..f4b603719055 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) {
@@ -1121,10 +1127,9 @@ static int knav_queue_setup_regions(struct knav_device *kdev,
 		INIT_LIST_HEAD(&region->pools);
 		list_add_tail(&region->list, &kdev->regions);
 	}
-	if (list_empty(&kdev->regions)) {
-		dev_err(dev, "no valid region information found\n");
-		return -ENODEV;
-	}
+	if (list_empty(&kdev->regions))
+		return dev_err_probe(dev, -ENODEV,
+				     "no valid region information found\n");
 
 	/* Next, we run through the regions and set things up */
 	for_each_region(kdev, region)
@@ -1306,10 +1311,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... */
@@ -1318,10 +1329,9 @@ static int knav_setup_queue_pools(struct knav_device *kdev,
 	}
 
 	/* ... and barf if they all failed! */
-	if (list_empty(&kdev->queue_ranges)) {
-		dev_err(kdev->dev, "no valid queue range found\n");
-		return -ENODEV;
-	}
+	if (list_empty(&kdev->queue_ranges))
+		return dev_err_probe(kdev->dev, -ENODEV,
+				     "no valid queue range found\n");
 	return 0;
 }
 
@@ -1389,14 +1399,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 +1684,26 @@ 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 +1791,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 +1834,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 +1866,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 v3 4/4] 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 | 52 +++++++++++++++++------------------------
 1 file changed, 21 insertions(+), 31 deletions(-)

diff --git a/drivers/soc/ti/pm33xx.c b/drivers/soc/ti/pm33xx.c
index 8e983c3c4e03..d0edce53c793 100644
--- a/drivers/soc/ti/pm33xx.c
+++ b/drivers/soc/ti/pm33xx.c
@@ -383,54 +383,44 @@ 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) {
-			dev_err(pm33xx_dev, "PM: %s: Unable to find device node for mpu\n",
-				__func__);
-			return -ENODEV;
-		}
+		if (!np)
+			return dev_err_probe(pm33xx_dev, -ENODEV,
+					     "PM: %s: Unable to find device node for mpu\n",
+					     __func__);
 	}
 
 	sram_pool = of_gen_pool_get(np, "pm-sram", 0);
-	if (!sram_pool) {
-		dev_err(pm33xx_dev, "PM: %s: Unable to get sram pool for ocmcram\n",
-			__func__);
-		ret = -ENODEV;
-		goto mpu_put_node;
-	}
+	if (!sram_pool)
+		return dev_err_probe(pm33xx_dev, -ENODEV,
+				     "PM: %s: Unable to get sram pool for ocmcram\n",
+				     __func__);
 
 	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;
-	}
+	if (!sram_pool_data)
+		return dev_err_probe(pm33xx_dev, -ENODEV,
+				     "PM: %s: Unable to get sram data pool for ocmcram\n",
+				     __func__);
 
 	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;
-	}
+	if (!ocmcram_location)
+		return dev_err_probe(pm33xx_dev, -ENOMEM,
+				     "PM: %s: Unable to allocate memory from ocmcram\n",
+				     __func__);
 
 	ocmcram_location_data = gen_pool_alloc(sram_pool_data,
 					       sizeof(struct emif_regs_amx3));
 	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 dev_err_probe(pm33xx_dev, -ENOMEM,
+				     "PM: Unable to allocate memory from ocmcram\n");
 	}
 
-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 v3 0/4] Do device node auto cleanup in drivers/soc/ti/

[Kousik Sanagavarapu]

On Sun, Jul 07, 2024 at 10:44:15AM +0530, Kousik Sanagavarapu wrote:
> Do "struct device_node" auto cleanup in soc/ti/.  This patch series takes
> care of all the cases where this is possible.
> 
> Thanks Jonathan for the review on the previous round.
> 
> v2:
> 
> 	https://lore.kernel.org/linux-arm-kernel/20240703065710.13786-1-five231003@gmail.com/
> 
> Changes since v2:
> - Split v2 1/3 into v3 1/4 and v3 2/4.  The memory setup code is
>   seperated out of the pruss_probe() function and put into 1/4 and the
>   actual conversion to auto cleanup is done in 2/4.
> - Replace dev_err() with dev_err_probe() in the code paths touched.
> 
> 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.
> 
> Kousik Sanagavarapu (4):
>   soc: ti: pruss: factor out memories setup
>   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 | 100 +++++++++---------
>  drivers/soc/ti/pm33xx.c          |  52 ++++-----
>  drivers/soc/ti/pruss.c           | 176 ++++++++++++++-----------------
>  3 files changed, 155 insertions(+), 173 deletions(-)

Ping

Re: [PATCH v3 0/4] Do device node auto cleanup in drivers/soc/ti/

[Nishanth Menon]

On 03:04-20240718, Kousik Sanagavarapu wrote:
> On Sun, Jul 07, 2024 at 10:44:15AM +0530, Kousik Sanagavarapu wrote:
> > Do "struct device_node" auto cleanup in soc/ti/.  This patch series takes
> > care of all the cases where this is possible.
> > 
> > Thanks Jonathan for the review on the previous round.
> > 
> > v2:
> > 
> > 	https://lore.kernel.org/linux-arm-kernel/20240703065710.13786-1-five231003@gmail.com/
> > 
> > Changes since v2:
> > - Split v2 1/3 into v3 1/4 and v3 2/4.  The memory setup code is
> >   seperated out of the pruss_probe() function and put into 1/4 and the
> >   actual conversion to auto cleanup is done in 2/4.
> > - Replace dev_err() with dev_err_probe() in the code paths touched.
> > 
> > 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.
> > 
> > Kousik Sanagavarapu (4):
> >   soc: ti: pruss: factor out memories setup
> >   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 | 100 +++++++++---------
> >  drivers/soc/ti/pm33xx.c          |  52 ++++-----
> >  drivers/soc/ti/pruss.c           | 176 ++++++++++++++-----------------
> >  3 files changed, 155 insertions(+), 173 deletions(-)
> 
> Ping

Umm... ping for what? for whom and why? In addition to reviews, I will
need someone to do tested-by as well - pruss/am33xx folks..? Further,
fyi, 6.12 collection cycle starts with 6.11 rc1 and I close mine around
rc4.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v3 0/4] Do device node auto cleanup in drivers/soc/ti/

[Kousik Sanagavarapu]

On Thu, Jul 18, 2024 at 06:21:34AM -0500, Nishanth Menon wrote:
> On 03:04-20240718, Kousik Sanagavarapu wrote:
>
> [...]
>
> > Ping
> 
> Umm... ping for what? for whom and why? In addition to reviews, I will
> need someone to do tested-by as well - pruss/am33xx folks..? Further,
> fyi, 6.12 collection cycle starts with 6.11 rc1 and I close mine around
> rc4.

Sorry for not being more specific.  Since the last version was reviewed
by Jonathan, I was wondering if he could review this round too.

And yes, it would be great if someone could test these patches as I don't
have access to the hardware - so RFT.

Also thanks for the heads up on the cycle, I didn't know.

Thanks

Re: [PATCH v3 1/4] soc: ti: pruss: factor out memories setup

[Nishanth Menon]

On 10:44-20240707, Kousik Sanagavarapu wrote:
> Factor out memories setup code from probe() into a new function
> pruss_of_setup_memories().  This sets the stage for introducing auto
> cleanup of the device node (done in the subsequent patch), since the
> clean up depends on the scope of the pointer and factoring out
> code into a seperate function obviously limits the scope of the various
typo s/seperate/separate - use --codespell with checkpatch to catch :)

A follow on patch has the same problem as well.

> variables used in that function.
> 
> Apart from the above, this change also has the advantage of making the
> code look more neat.
> 
> While at it, use dev_err_probe() instead of plain dev_err() as this new
> function is called by the probe().
> 
> Signed-off-by: Kousik Sanagavarapu <five231003@gmail.com>
> ---
>  drivers/soc/ti/pruss.c | 111 ++++++++++++++++++++++-------------------
>  1 file changed, 61 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
> index 24a42e0b645c..a3c55a291b0b 100644
> --- a/drivers/soc/ti/pruss.c
> +++ b/drivers/soc/ti/pruss.c
> @@ -415,6 +415,63 @@ static int pruss_clk_init(struct pruss *pruss, struct device_node *cfg_node)
>  	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;
> +	const struct pruss_private_data *data = of_device_get_match_data(dev);
> +	const char *mem_names[PRUSS_MEM_MAX] = { "dram0", "dram1", "shrdram2" };
> +	int i;
> +
> +	child = of_get_child_by_name(np, "memories");
> +	if (!child)
> +		return dev_err_probe(dev, -ENODEV,
> +				     "%pOF is missing its 'memories' node\n",
> +				     child);
> +
> +	for (i = 0; i < PRUSS_MEM_MAX; i++) {
> +		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) {
> +			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) {
> +			of_node_put(child);
> +			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);
> +	}
> +	of_node_put(child);
> +
> +	return 0;
> +}
> +
>  static struct regmap_config regmap_conf = {
>  	.reg_bits = 32,
>  	.val_bits = 32,
> @@ -471,15 +528,8 @@ static int pruss_cfg_of_init(struct device *dev, struct pruss *pruss)
>  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 +544,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;

Why? We have not called pm_runtime_enable at this point.

>  
>  	platform_set_drvdata(pdev, pruss);
>  
> -- 
> 2.45.2.561.g66ac6e4bcd
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v3 1/4] soc: ti: pruss: factor out memories setup

[Kousik Sanagavarapu]

On Sat, Aug 24, 2024 at 01:49:50PM -0500, Nishanth Menon wrote:
> On 10:44-20240707, Kousik Sanagavarapu wrote:
> > Factor out memories setup code from probe() into a new function
> > pruss_of_setup_memories().  This sets the stage for introducing auto
> > cleanup of the device node (done in the subsequent patch), since the
> > clean up depends on the scope of the pointer and factoring out
> > code into a seperate function obviously limits the scope of the various
> typo s/seperate/separate - use --codespell with checkpatch to catch :)
> 
> A follow on patch has the same problem as well.

Oh, yes should've used --codespell, my bad.  Thanks for spotting.

> > variables used in that function.

[...]

> > -	of_node_put(child);
> > +	ret = pruss_of_setup_memories(dev, pruss);
> > +	if (ret < 0)
> > +		goto rpm_put;
> 
> Why? We have not called pm_runtime_enable at this point.

Didn't catch this too, will change.

Thanks