Do device node auto cleanup in drivers/soc/ti/

PATCH Superseded Kousik Sanagavarapu 2 participants 8 messages Jul 3, 2024 – Jul 5, 2024 TI Keystone multicore navigator drivers

lore

Share & tools

lore

Kousik Sanagavarapu

author

2024-07-03 06:58 UTC #0 lore raw

v2 cover Do device node auto cleanup in drivers/soc/ti/

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/[email protected]/

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(-)

Signature · 1 line

2.45.2.561.g66ac6e4bcd

Kousik Sanagavarapu

author

2024-07-03 06:58 UTC #1 lore raw

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

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.

Signed-off-by Kousik Sanagavarapu <[email protected]>

---
 drivers/soc/ti/pruss.c | 168 +++++++++++++++++++----------------------
 1 file changed, 78 insertions(+), 90 deletions(-)

drivers/soc/ti/pruss.c +78 ~~-90~~

index 24a42e0b645c..9767d129a8ea 100644
@@ -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);

Signature · 1 line

2.45.2.561.g66ac6e4bcd

Jonathan Cameron

2024-07-05 10:49 UTC #2 lore raw

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

Quoted 11 lines · On Wed, 3 Jul 2024 12:25:26 +0530 Kousik Sanagavarapu <[email protected]>…

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 <[email protected]>
Signed-off-by Kousik Sanagavarapu <[email protected]>

Hi.

A few minor comments inline.

Jonathan

Quoted 63 lines

---
 drivers/soc/ti/pruss.c | 168 +++++++++++++++++++----------------------
 1 file changed, 78 insertions(+), 90 deletions(-)

drivers/soc/ti/pruss.c +19 ~~-17~~

index 24a42e0b645c..9767d129a8ea 100644
@@ -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;
+

drivers/soc/ti/pruss.c static int pruss_clk_mux_setup(struct pruss *pruss, struct clk *clk_mux, · line 427

Quoted 10 lines

drivers/soc/ti/pruss.c +10 -0

+	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.

Quoted 83 lines

drivers/soc/ti/pruss.c +43 ~~-20~~

+		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),

drivers/soc/ti/pruss.c static int pruss_cfg_of_init(struct device *dev, struct pruss *pruss) · line 502

Quoted 10 lines

drivers/soc/ti/pruss.c +2 -3

+				     "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.

Quoted 57 lines

drivers/soc/ti/pruss.c +1 ~~-43~~

 }
 
 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;

drivers/soc/ti/pruss.c static int pruss_probe(struct platform_device *pdev) · line 526

Quoted 10 lines

drivers/soc/ti/pruss.c +3 -7

-		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.

Quoted 3 lines

drivers/soc/ti/pruss.c +0 -0
 	platform_set_drvdata(pdev, pruss);
 

Kousik Sanagavarapu

author

2024-07-03 06:58 UTC #3 lore raw

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

In reply to 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.

Signed-off-by Kousik Sanagavarapu <[email protected]>

---
 drivers/soc/ti/knav_qmss_queue.c | 85 +++++++++++++++++---------------
 1 file changed, 46 insertions(+), 39 deletions(-)

drivers/soc/ti/knav_qmss_queue.c +46 ~~-39~~

index 06fb5505c22c..767b9c49ea93 100644
@@ -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;

Signature · 1 line

2.45.2.561.g66ac6e4bcd

Jonathan Cameron

Reviewed-by on v2 2/3

2024-07-05 10:52 UTC #4 lore raw

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

Quoted 6 lines · On Wed, 3 Jul 2024 12:25:27 +0530 Kousik Sanagavarapu <[email protected]>…

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.

Quoted 6 lines

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 <[email protected]>
Signed-off-by Kousik Sanagavarapu <[email protected]>

One trivial thing inline

Reviewed-by Jonathan Cameron <[email protected]>

Quoted 6 lines

drivers/soc/ti/knav_qmss_queue.c +2 -0

@@ -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)

drivers/soc/ti/knav_qmss_queue.c static int knav_queue_start_pdsps(struct knav_device *kdev) · line 1700

Quoted 10 lines

drivers/soc/ti/knav_qmss_queue.c +10 -0

+{
+	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.

Quoted 6 lines

drivers/soc/ti/knav_qmss_queue.c +6 -0

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

Kousik Sanagavarapu

author

2024-07-03 06:58 UTC #5 lore raw

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

In reply to Kousik Sanagavarapu

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

Signed-off-by Kousik Sanagavarapu <[email protected]>

---
 drivers/soc/ti/pm33xx.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

drivers/soc/ti/pm33xx.c +7 ~~-13~~

index 8e983c3c4e03..40988c45ed00 100644
@@ -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)

Signature · 1 line

2.45.2.561.g66ac6e4bcd

Jonathan Cameron

2024-07-05 10:54 UTC #6 lore raw

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

Quoted 5 lines · On Wed, 3 Jul 2024 12:25:28 +0530 Kousik Sanagavarapu <[email protected]>…

Use scope based cleanup instead of manual of_node_put() calls, hence simplifying the handling of error paths.
Suggested-by Julia Lawall <[email protected]>
Signed-off-by Kousik Sanagavarapu <[email protected]>

I think you can make use of dev_err_probe() in here to further simplify things (a bit anyway!)

Jonathan

Quoted 36 lines

---
 drivers/soc/ti/pm33xx.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

drivers/soc/ti/pm33xx.c +3 -7

index 8e983c3c4e03..40988c45ed00 100644
@@ -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;

drivers/soc/ti/pm33xx.c static int am33xx_pm_alloc_sram(void) · line 416

Quoted 10 lines

drivers/soc/ti/pm33xx.c +2 -2

+		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.

drivers/soc/ti/pm33xx.c static int am33xx_pm_alloc_sram(void) · line 424

Quoted 9 lines

drivers/soc/ti/pm33xx.c +1 -1

 	}
 
 	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.

Quoted 9 lines

drivers/soc/ti/pm33xx.c +1 -3

 	}
 
-mpu_put_node:
-	of_node_put(np);
-	return ret;
+	return 0;
 }
 
 static int am33xx_pm_rtc_setup(void)

Kousik Sanagavarapu

author

2024-07-05 11:12 UTC #7 lore raw

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

Hi Jonathan,

Quoted 10 lines · Jonathan Cameron

Quoted 5 lines · On Wed, 3 Jul 2024 12:25:28 +0530 Kousik Sanagavarapu <[email protected]>…
Use scope based cleanup instead of manual of_node_put() calls, hence simplifying the handling of error paths.
Suggested-by Julia Lawall <[email protected]>
Signed-off-by Kousik Sanagavarapu <[email protected]>
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