[PATCH v3 0/9] misc: sram: minor fixes and clean up

[PATCH v3 0/9] misc: sram: minor fixes and clean up

[Vladimir Zapolskiy]

The series contains a number of minor fixes (move to phys_addr_t
from u32, fix ups on error path etc.) and overall driver clean up,
no functional change.

Changes since v2:
- immediately return -ENOMEM on kmalloc() failure after probe
  error path simplification (7/9)
- fixes a variable may be uninitialized warning

Changes since v1:
- report size of SRAM in decimal format '%zu' instead of '%zx'
- replacement of denominator '1024' to SZ_1K requires explicit
  include of linux/sizes.h on some platforms, keep it as a number

Vladimir Zapolskiy (9):
  misc: sram: fix enabled clock leak on error path
  misc: sram: fix device node reference leak on error
  misc: sram: use phys_addr_t instead of u32 for physical address
  misc: sram: bump error message level on unclean driver unbinding
  misc: sram: report correct SRAM pool size
  misc: sram: add private struct device and virt_base members
  misc: sram: simplify probe error path
  misc: sram: move reserved block logic out of probe function
  misc: sram: sort and clean up included headers

 drivers/misc/sram.c | 153 ++++++++++++++++++++++++++++------------------------
 1 file changed, 82 insertions(+), 71 deletions(-)

-- 
2.1.4

[PATCH v3 1/9] misc: sram: fix enabled clock leak on error path

[Vladimir Zapolskiy]

If devm_gen_pool_create() fails, the previously enabled sram->clk is
not disabled on probe() exit.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/misc/sram.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index eeaaf5f..b44a423 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -90,16 +90,17 @@ static int sram_probe(struct platform_device *pdev)
 	if (!sram)
 		return -ENOMEM;
 
+	sram->pool = devm_gen_pool_create(&pdev->dev,
+					  ilog2(SRAM_GRANULARITY), -1);
+	if (!sram->pool)
+		return -ENOMEM;
+
 	sram->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(sram->clk))
 		sram->clk = NULL;
 	else
 		clk_prepare_enable(sram->clk);
 
-	sram->pool = devm_gen_pool_create(&pdev->dev, ilog2(SRAM_GRANULARITY), -1);
-	if (!sram->pool)
-		return -ENOMEM;
-
 	/*
 	 * We need an additional block to mark the end of the memory region
 	 * after the reserved blocks from the dt are processed.
-- 
2.1.4

[PATCH v3 2/9] misc: sram: fix device node reference leak on error

[Vladimir Zapolskiy]

A pointer device node reference should be decremented on manual exit
from for_each_available_child_of_node() loop.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/misc/sram.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index b44a423..999684a 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -121,6 +121,7 @@ static int sram_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev,
 				"could not get address for node %s\n",
 				child->full_name);
+			of_node_put(child);
 			goto err_chunks;
 		}
 
@@ -129,6 +130,7 @@ static int sram_probe(struct platform_device *pdev)
 				"reserved block %s outside the sram area\n",
 				child->full_name);
 			ret = -EINVAL;
+			of_node_put(child);
 			goto err_chunks;
 		}
 
-- 
2.1.4

[PATCH v3 3/9] misc: sram: use phys_addr_t instead of u32 for physical address

[Vladimir Zapolskiy]

To avoid any problems on non 32-bit platforms get and store memory
addresses under phys_addr_t type.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
Changes from v1 to v2:
- report size of SRAM in decimal format '%zu' instead of '%zx'
- replacement of denominator '1024' to SZ_1K requires explicit
  include of linux/sizes.h on some platforms, keep it as a number

 drivers/misc/sram.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 999684a..b7a3a24 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -41,8 +41,8 @@ struct sram_dev {
 
 struct sram_reserve {
 	struct list_head list;
-	u32 start;
-	u32 size;
+	phys_addr_t start;
+	size_t size;
 };
 
 static int sram_reserve_cmp(void *priv, struct list_head *a,
@@ -60,7 +60,8 @@ static int sram_probe(struct platform_device *pdev)
 	struct sram_dev *sram;
 	struct resource *res;
 	struct device_node *np = pdev->dev.of_node, *child;
-	unsigned long size, cur_start, cur_size;
+	phys_addr_t cur_start;
+	size_t size, cur_size;
 	struct sram_reserve *rblocks, *block;
 	struct list_head reserve_list;
 	unsigned int nblocks;
@@ -138,9 +139,9 @@ static int sram_probe(struct platform_device *pdev)
 		block->size = resource_size(&child_res);
 		list_add_tail(&block->list, &reserve_list);
 
-		dev_dbg(&pdev->dev, "found reserved block 0x%x-0x%x\n",
-			block->start,
-			block->start + block->size);
+		dev_dbg(&pdev->dev, "found reserved block 0x%llx-0x%llx\n",
+			(unsigned long long)block->start,
+			(unsigned long long)block->start + block->size);
 
 		block++;
 	}
@@ -158,8 +159,9 @@ static int sram_probe(struct platform_device *pdev)
 		/* can only happen if sections overlap */
 		if (block->start < cur_start) {
 			dev_err(&pdev->dev,
-				"block at 0x%x starts after current offset 0x%lx\n",
-				block->start, cur_start);
+				"block at 0x%llx starts after current offset 0x%llx\n",
+				(unsigned long long)block->start,
+				(unsigned long long)cur_start);
 			ret = -EINVAL;
 			goto err_chunks;
 		}
@@ -177,8 +179,9 @@ static int sram_probe(struct platform_device *pdev)
 		 */
 		cur_size = block->start - cur_start;
 
-		dev_dbg(&pdev->dev, "adding chunk 0x%lx-0x%lx\n",
-			cur_start, cur_start + cur_size);
+		dev_dbg(&pdev->dev, "adding chunk 0x%llx-0x%llx\n",
+			(unsigned long long)cur_start,
+			(unsigned long long)cur_start + cur_size);
 		ret = gen_pool_add_virt(sram->pool,
 				(unsigned long)virt_base + cur_start,
 				res->start + cur_start, cur_size, -1);
@@ -193,7 +196,8 @@ static int sram_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, sram);
 
-	dev_dbg(&pdev->dev, "SRAM pool: %ld KiB @ 0x%p\n", size / 1024, virt_base);
+	dev_dbg(&pdev->dev, "SRAM pool: %zu KiB @ 0x%p\n",
+		size / 1024, virt_base);
 
 	return 0;
 
-- 
2.1.4

[PATCH v3 4/9] misc: sram: bump error message level on unclean driver unbinding

[Vladimir Zapolskiy]

Report an error level message to a user, if the driver is unbound
while there are still some pool allocations.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/misc/sram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index b7a3a24..201cc28 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -214,7 +214,7 @@ static int sram_remove(struct platform_device *pdev)
 	struct sram_dev *sram = platform_get_drvdata(pdev);
 
 	if (gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
-		dev_dbg(&pdev->dev, "removed while SRAM allocated\n");
+		dev_err(&pdev->dev, "removed while SRAM allocated\n");
 
 	if (sram->clk)
 		clk_disable_unprepare(sram->clk);
-- 
2.1.4

[PATCH v3 5/9] misc: sram: report correct SRAM pool size

[Vladimir Zapolskiy]

Since some space in SRAM may be reserved, report the left free space
in the allocated memory pool instead of total physical size of the
SRAM device.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
Changes from v1 to v2:
- rebased on top of v2 3/9 misc: sram: use phys_addr_t instead of u32
  for physical address

 drivers/misc/sram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 201cc28..75e6245 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -197,7 +197,7 @@ static int sram_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, sram);
 
 	dev_dbg(&pdev->dev, "SRAM pool: %zu KiB @ 0x%p\n",
-		size / 1024, virt_base);
+		gen_pool_size(sram->pool) / 1024, virt_base);
 
 	return 0;
 
-- 
2.1.4

[PATCH v3 6/9] misc: sram: add private struct device and virt_base members

[Vladimir Zapolskiy]

No functional change, this is a preceding change to simplify
separation of reserved partition handling logic from probe()
function.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
Changes from v1 to v2:
- rebased on top of v2 3/9 misc: sram: use phys_addr_t instead of u32
  for physical address

 drivers/misc/sram.c | 46 +++++++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 75e6245..1ad979f 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -35,6 +35,9 @@
 #define SRAM_GRANULARITY	32
 
 struct sram_dev {
+	struct device *dev;
+	void __iomem *virt_base;
+
 	struct gen_pool *pool;
 	struct clk *clk;
 };
@@ -56,7 +59,6 @@ static int sram_reserve_cmp(void *priv, struct list_head *a,
 
 static int sram_probe(struct platform_device *pdev)
 {
-	void __iomem *virt_base;
 	struct sram_dev *sram;
 	struct resource *res;
 	struct device_node *np = pdev->dev.of_node, *child;
@@ -69,34 +71,36 @@ static int sram_probe(struct platform_device *pdev)
 
 	INIT_LIST_HEAD(&reserve_list);
 
+	sram = devm_kzalloc(&pdev->dev, sizeof(*sram), GFP_KERNEL);
+	if (!sram)
+		return -ENOMEM;
+
+	sram->dev = &pdev->dev;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
-		dev_err(&pdev->dev, "found no memory resource\n");
+		dev_err(sram->dev, "found no memory resource\n");
 		return -EINVAL;
 	}
 
 	size = resource_size(res);
 
-	if (!devm_request_mem_region(&pdev->dev,
+	if (!devm_request_mem_region(sram->dev,
 			res->start, size, pdev->name)) {
-		dev_err(&pdev->dev, "could not request region for resource\n");
+		dev_err(sram->dev, "could not request region for resource\n");
 		return -EBUSY;
 	}
 
-	virt_base = devm_ioremap_wc(&pdev->dev, res->start, size);
-	if (IS_ERR(virt_base))
-		return PTR_ERR(virt_base);
-
-	sram = devm_kzalloc(&pdev->dev, sizeof(*sram), GFP_KERNEL);
-	if (!sram)
-		return -ENOMEM;
+	sram->virt_base = devm_ioremap_wc(sram->dev, res->start, size);
+	if (IS_ERR(sram->virt_base))
+		return PTR_ERR(sram->virt_base);
 
-	sram->pool = devm_gen_pool_create(&pdev->dev,
+	sram->pool = devm_gen_pool_create(sram->dev,
 					  ilog2(SRAM_GRANULARITY), -1);
 	if (!sram->pool)
 		return -ENOMEM;
 
-	sram->clk = devm_clk_get(&pdev->dev, NULL);
+	sram->clk = devm_clk_get(sram->dev, NULL);
 	if (IS_ERR(sram->clk))
 		sram->clk = NULL;
 	else
@@ -119,7 +123,7 @@ static int sram_probe(struct platform_device *pdev)
 
 		ret = of_address_to_resource(child, 0, &child_res);
 		if (ret < 0) {
-			dev_err(&pdev->dev,
+			dev_err(sram->dev,
 				"could not get address for node %s\n",
 				child->full_name);
 			of_node_put(child);
@@ -127,7 +131,7 @@ static int sram_probe(struct platform_device *pdev)
 		}
 
 		if (child_res.start < res->start || child_res.end > res->end) {
-			dev_err(&pdev->dev,
+			dev_err(sram->dev,
 				"reserved block %s outside the sram area\n",
 				child->full_name);
 			ret = -EINVAL;
@@ -139,7 +143,7 @@ static int sram_probe(struct platform_device *pdev)
 		block->size = resource_size(&child_res);
 		list_add_tail(&block->list, &reserve_list);
 
-		dev_dbg(&pdev->dev, "found reserved block 0x%llx-0x%llx\n",
+		dev_dbg(sram->dev, "found reserved block 0x%llx-0x%llx\n",
 			(unsigned long long)block->start,
 			(unsigned long long)block->start + block->size);
 
@@ -158,7 +162,7 @@ static int sram_probe(struct platform_device *pdev)
 	list_for_each_entry(block, &reserve_list, list) {
 		/* can only happen if sections overlap */
 		if (block->start < cur_start) {
-			dev_err(&pdev->dev,
+			dev_err(sram->dev,
 				"block at 0x%llx starts after current offset 0x%llx\n",
 				(unsigned long long)block->start,
 				(unsigned long long)cur_start);
@@ -179,11 +183,11 @@ static int sram_probe(struct platform_device *pdev)
 		 */
 		cur_size = block->start - cur_start;
 
-		dev_dbg(&pdev->dev, "adding chunk 0x%llx-0x%llx\n",
+		dev_dbg(sram->dev, "adding chunk 0x%llx-0x%llx\n",
 			(unsigned long long)cur_start,
 			(unsigned long long)cur_start + cur_size);
 		ret = gen_pool_add_virt(sram->pool,
-				(unsigned long)virt_base + cur_start,
+				(unsigned long)sram->virt_base + cur_start,
 				res->start + cur_start, cur_size, -1);
 		if (ret < 0)
 			goto err_chunks;
@@ -197,7 +201,7 @@ static int sram_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, sram);
 
 	dev_dbg(&pdev->dev, "SRAM pool: %zu KiB @ 0x%p\n",
-		gen_pool_size(sram->pool) / 1024, virt_base);
+		gen_pool_size(sram->pool) / 1024, sram->virt_base);
 
 	return 0;
 
@@ -214,7 +218,7 @@ static int sram_remove(struct platform_device *pdev)
 	struct sram_dev *sram = platform_get_drvdata(pdev);
 
 	if (gen_pool_avail(sram->pool) < gen_pool_size(sram->pool))
-		dev_err(&pdev->dev, "removed while SRAM allocated\n");
+		dev_err(sram->dev, "removed while SRAM allocated\n");
 
 	if (sram->clk)
 		clk_disable_unprepare(sram->clk);
-- 
2.1.4

[PATCH v3 7/9] misc: sram: simplify probe error path

[Vladimir Zapolskiy]

Reserved block logic relies only on information from device tree,
there is no need to get and enable device clock in advance, especially
because not provided clock is not considered as an error. No
functional change.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
Changes from v1 to v3:
- immediately return -ENOMEM on kmalloc()

 drivers/misc/sram.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 1ad979f..8e5346e 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -100,22 +100,14 @@ static int sram_probe(struct platform_device *pdev)
 	if (!sram->pool)
 		return -ENOMEM;
 
-	sram->clk = devm_clk_get(sram->dev, NULL);
-	if (IS_ERR(sram->clk))
-		sram->clk = NULL;
-	else
-		clk_prepare_enable(sram->clk);
-
 	/*
 	 * We need an additional block to mark the end of the memory region
 	 * after the reserved blocks from the dt are processed.
 	 */
 	nblocks = (np) ? of_get_available_child_count(np) + 1 : 1;
 	rblocks = kmalloc((nblocks) * sizeof(*rblocks), GFP_KERNEL);
-	if (!rblocks) {
-		ret = -ENOMEM;
-		goto err_alloc;
-	}
+	if (!rblocks)
+		return -ENOMEM;
 
 	block = &rblocks[0];
 	for_each_available_child_of_node(np, child) {
@@ -198,6 +190,12 @@ static int sram_probe(struct platform_device *pdev)
 
 	kfree(rblocks);
 
+	sram->clk = devm_clk_get(sram->dev, NULL);
+	if (IS_ERR(sram->clk))
+		sram->clk = NULL;
+	else
+		clk_prepare_enable(sram->clk);
+
 	platform_set_drvdata(pdev, sram);
 
 	dev_dbg(&pdev->dev, "SRAM pool: %zu KiB @ 0x%p\n",
@@ -207,9 +205,7 @@ static int sram_probe(struct platform_device *pdev)
 
 err_chunks:
 	kfree(rblocks);
-err_alloc:
-	if (sram->clk)
-		clk_disable_unprepare(sram->clk);
+
 	return ret;
 }
 
-- 
2.1.4

[PATCH v3 8/9] misc: sram: move reserved block logic out of probe function

[Vladimir Zapolskiy]

No functional change, but now previously overloaded sram_probe() is
greatly simplified and perceptible, reserved regions logic also has
its own space.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
Changes from v1/v2 to v3:
- fixes a variable may be uninitialized warning

 drivers/misc/sram.c | 84 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 47 insertions(+), 37 deletions(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 8e5346e..582a893 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -57,49 +57,20 @@ static int sram_reserve_cmp(void *priv, struct list_head *a,
 	return ra->start - rb->start;
 }
 
-static int sram_probe(struct platform_device *pdev)
+static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
 {
-	struct sram_dev *sram;
-	struct resource *res;
-	struct device_node *np = pdev->dev.of_node, *child;
+	struct device_node *np = sram->dev->of_node, *child;
 	phys_addr_t cur_start;
 	size_t size, cur_size;
 	struct sram_reserve *rblocks, *block;
 	struct list_head reserve_list;
 	unsigned int nblocks;
-	int ret;
+	int ret = 0;
 
 	INIT_LIST_HEAD(&reserve_list);
 
-	sram = devm_kzalloc(&pdev->dev, sizeof(*sram), GFP_KERNEL);
-	if (!sram)
-		return -ENOMEM;
-
-	sram->dev = &pdev->dev;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		dev_err(sram->dev, "found no memory resource\n");
-		return -EINVAL;
-	}
-
 	size = resource_size(res);
 
-	if (!devm_request_mem_region(sram->dev,
-			res->start, size, pdev->name)) {
-		dev_err(sram->dev, "could not request region for resource\n");
-		return -EBUSY;
-	}
-
-	sram->virt_base = devm_ioremap_wc(sram->dev, res->start, size);
-	if (IS_ERR(sram->virt_base))
-		return PTR_ERR(sram->virt_base);
-
-	sram->pool = devm_gen_pool_create(sram->dev,
-					  ilog2(SRAM_GRANULARITY), -1);
-	if (!sram->pool)
-		return -ENOMEM;
-
 	/*
 	 * We need an additional block to mark the end of the memory region
 	 * after the reserved blocks from the dt are processed.
@@ -188,8 +159,52 @@ static int sram_probe(struct platform_device *pdev)
 		cur_start = block->start + block->size;
 	}
 
+ err_chunks:
 	kfree(rblocks);
 
+	return ret;
+}
+
+static int sram_probe(struct platform_device *pdev)
+{
+	struct sram_dev *sram;
+	struct resource *res;
+	size_t size;
+	int ret;
+
+	sram = devm_kzalloc(&pdev->dev, sizeof(*sram), GFP_KERNEL);
+	if (!sram)
+		return -ENOMEM;
+
+	sram->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(sram->dev, "found no memory resource\n");
+		return -EINVAL;
+	}
+
+	size = resource_size(res);
+
+	if (!devm_request_mem_region(sram->dev, res->start, size,
+				     pdev->name)) {
+		dev_err(sram->dev, "could not request region for resource\n");
+		return -EBUSY;
+	}
+
+	sram->virt_base = devm_ioremap_wc(sram->dev, res->start, size);
+	if (IS_ERR(sram->virt_base))
+		return PTR_ERR(sram->virt_base);
+
+	sram->pool = devm_gen_pool_create(sram->dev,
+					  ilog2(SRAM_GRANULARITY), -1);
+	if (!sram->pool)
+		return -ENOMEM;
+
+	ret = sram_reserve_regions(sram, res);
+	if (ret)
+		return ret;
+
 	sram->clk = devm_clk_get(sram->dev, NULL);
 	if (IS_ERR(sram->clk))
 		sram->clk = NULL;
@@ -202,11 +217,6 @@ static int sram_probe(struct platform_device *pdev)
 		gen_pool_size(sram->pool) / 1024, sram->virt_base);
 
 	return 0;
-
-err_chunks:
-	kfree(rblocks);
-
-	return ret;
 }
 
 static int sram_remove(struct platform_device *pdev)
-- 
2.1.4

[PATCH v3 9/9] misc: sram: sort and clean up included headers

[Vladimir Zapolskiy]

Most of the included header files are already included as
dependencies.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/misc/sram.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 582a893..2791def 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -18,19 +18,13 @@
  * MA 02110-1301, USA.
  */
 
-#include <linux/kernel.h>
-#include <linux/init.h>
 #include <linux/clk.h>
-#include <linux/err.h>
+#include <linux/genalloc.h>
 #include <linux/io.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-#include <linux/list.h>
 #include <linux/list_sort.h>
+#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
-#include <linux/spinlock.h>
-#include <linux/genalloc.h>
 
 #define SRAM_GRANULARITY	32
 
-- 
2.1.4

Re: [PATCH v3 3/9] misc: sram: use phys_addr_t instead of u32 for physical address

[Philipp Zabel]

Hi Vladimir,

Am Montag, den 18.05.2015, 22:08 +0300 schrieb Vladimir Zapolskiy:
> To avoid any problems on non 32-bit platforms get and store memory
> addresses under phys_addr_t type.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> ---
> Changes from v1 to v2:
> - report size of SRAM in decimal format '%zu' instead of '%zx'
> - replacement of denominator '1024' to SZ_1K requires explicit
>   include of linux/sizes.h on some platforms, keep it as a number
> 
>  drivers/misc/sram.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> index 999684a..b7a3a24 100644
> --- a/drivers/misc/sram.c
> +++ b/drivers/misc/sram.c
> @@ -41,8 +41,8 @@ struct sram_dev {
>  
>  struct sram_reserve {
>  	struct list_head list;
> -	u32 start;
> -	u32 size;
> +	phys_addr_t start;
> +	size_t size;
>  };
>  
>  static int sram_reserve_cmp(void *priv, struct list_head *a,
> @@ -60,7 +60,8 @@ static int sram_probe(struct platform_device *pdev)
>  	struct sram_dev *sram;
>  	struct resource *res;
>  	struct device_node *np = pdev->dev.of_node, *child;
> -	unsigned long size, cur_start, cur_size;
> +	phys_addr_t cur_start;
> +	size_t size, cur_size;
>  	struct sram_reserve *rblocks, *block;
>  	struct list_head reserve_list;
>  	unsigned int nblocks;
> @@ -138,9 +139,9 @@ static int sram_probe(struct platform_device *pdev)
>  		block->size = resource_size(&child_res);
>  		list_add_tail(&block->list, &reserve_list);
>  
> -		dev_dbg(&pdev->dev, "found reserved block 0x%x-0x%x\n",
> -			block->start,
> -			block->start + block->size);
> +		dev_dbg(&pdev->dev, "found reserved block 0x%llx-0x%llx\n",
> +			(unsigned long long)block->start,

Now that block->start is of type phys_addr_t, is there a reason not to
use %pa ?

> +			(unsigned long long)block->start + block->size);

		phys_addr_t end = block->start + block->size;

		dev_dbg(&pdev->dev, "found reserved block 0x%pa-0x%pa\n",
			&block->start, &end);
 
>  		block++;
>  	}
> @@ -158,8 +159,9 @@ static int sram_probe(struct platform_device *pdev)
>  		/* can only happen if sections overlap */
>  		if (block->start < cur_start) {
>  			dev_err(&pdev->dev,
> -				"block at 0x%x starts after current offset 0x%lx\n",
> -				block->start, cur_start);
> +				"block at 0x%llx starts after current offset 0x%llx\n",
> +				(unsigned long long)block->start,
> +				(unsigned long long)cur_start);

			dev_err(&pdev->dev,
				"block at 0x%pa starts after current offset 0x%pa\n",
				&block->start, &cur_start);

>  			ret = -EINVAL;
>  			goto err_chunks;
>  		}
> @@ -177,8 +179,9 @@ static int sram_probe(struct platform_device *pdev)
>  		 */
>  		cur_size = block->start - cur_start;
>  
> -		dev_dbg(&pdev->dev, "adding chunk 0x%lx-0x%lx\n",
> -			cur_start, cur_start + cur_size);
> +		dev_dbg(&pdev->dev, "adding chunk 0x%llx-0x%llx\n",
> +			(unsigned long long)cur_start,
> +			(unsigned long long)cur_start + cur_size);

		dev_dbg(&pdev->dev, "adding chunk 0x%pa-0x%pa\n",
			&cur_start, &block->start);

>  		ret = gen_pool_add_virt(sram->pool,
>  				(unsigned long)virt_base + cur_start,
>  				res->start + cur_start, cur_size, -1);
> @@ -193,7 +196,8 @@ static int sram_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, sram);
>  
> -	dev_dbg(&pdev->dev, "SRAM pool: %ld KiB @ 0x%p\n", size / 1024, virt_base);
> +	dev_dbg(&pdev->dev, "SRAM pool: %zu KiB @ 0x%p\n",
> +		size / 1024, virt_base);
>  
>  	return 0;

regards
Philipp

Re: [PATCH v3 1/9] misc: sram: fix enabled clock leak on error path

[Philipp Zabel]

Am Montag, den 18.05.2015, 22:08 +0300 schrieb Vladimir Zapolskiy:
> If devm_gen_pool_create() fails, the previously enabled sram->clk is
> not disabled on probe() exit.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> ---
>  drivers/misc/sram.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> index eeaaf5f..b44a423 100644
> --- a/drivers/misc/sram.c
> +++ b/drivers/misc/sram.c
> @@ -90,16 +90,17 @@ static int sram_probe(struct platform_device *pdev)
>  	if (!sram)
>  		return -ENOMEM;
>  
> +	sram->pool = devm_gen_pool_create(&pdev->dev,
> +					  ilog2(SRAM_GRANULARITY), -1);
> +	if (!sram->pool)
> +		return -ENOMEM;
> +
>  	sram->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(sram->clk))
>  		sram->clk = NULL;
>  	else
>  		clk_prepare_enable(sram->clk);

Here you move sram->clk around, and later in patch 7 it gets moved
again. To me it looks like the two should be squashed together.

>  
> -	sram->pool = devm_gen_pool_create(&pdev->dev, ilog2(SRAM_GRANULARITY), -1);
> -	if (!sram->pool)
> -		return -ENOMEM;
> -
>  	/*
>  	 * We need an additional block to mark the end of the memory region
>  	 * after the reserved blocks from the dt are processed.

regards
Philipp

Re: [PATCH v3 3/9] misc: sram: use phys_addr_t instead of u32 for physical address

[Vladimir Zapolskiy]

Hi Philipp,

thank you for review.

On 19.05.2015 13:38, Philipp Zabel wrote:
> Hi Vladimir,
> 
> Am Montag, den 18.05.2015, 22:08 +0300 schrieb Vladimir Zapolskiy:
>> To avoid any problems on non 32-bit platforms get and store memory
>> addresses under phys_addr_t type.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>> ---
>> Changes from v1 to v2:
>> - report size of SRAM in decimal format '%zu' instead of '%zx'
>> - replacement of denominator '1024' to SZ_1K requires explicit
>>   include of linux/sizes.h on some platforms, keep it as a number
>>
>>  drivers/misc/sram.c | 26 +++++++++++++++-----------
>>  1 file changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
>> index 999684a..b7a3a24 100644
>> --- a/drivers/misc/sram.c
>> +++ b/drivers/misc/sram.c
>> @@ -41,8 +41,8 @@ struct sram_dev {
>>  
>>  struct sram_reserve {
>>  	struct list_head list;
>> -	u32 start;
>> -	u32 size;
>> +	phys_addr_t start;
>> +	size_t size;
>>  };
>>  
>>  static int sram_reserve_cmp(void *priv, struct list_head *a,
>> @@ -60,7 +60,8 @@ static int sram_probe(struct platform_device *pdev)
>>  	struct sram_dev *sram;
>>  	struct resource *res;
>>  	struct device_node *np = pdev->dev.of_node, *child;
>> -	unsigned long size, cur_start, cur_size;
>> +	phys_addr_t cur_start;
>> +	size_t size, cur_size;
>>  	struct sram_reserve *rblocks, *block;
>>  	struct list_head reserve_list;
>>  	unsigned int nblocks;
>> @@ -138,9 +139,9 @@ static int sram_probe(struct platform_device *pdev)
>>  		block->size = resource_size(&child_res);
>>  		list_add_tail(&block->list, &reserve_list);
>>  
>> -		dev_dbg(&pdev->dev, "found reserved block 0x%x-0x%x\n",
>> -			block->start,
>> -			block->start + block->size);
>> +		dev_dbg(&pdev->dev, "found reserved block 0x%llx-0x%llx\n",
>> +			(unsigned long long)block->start,
> 
> Now that block->start is of type phys_addr_t, is there a reason not to
> use %pa ?

the only reason why I decided not to use %pa in the change is because
the value should be passed over a pointer then.

For instance right here it is possible to change '"0x%llx", (unsigned
long long)block->start' to '"%pa", &block->start', however some lines
below the same trick can not be done for '(unsigned long long)cur_start
+ cur_size' value. Among alternatives to introduce a local variable, use
mixed "0x%llx" and "%pa" or use unified "0x%llx" I selected the latter one.

>> +			(unsigned long long)block->start + block->size);
> 
> 		phys_addr_t end = block->start + block->size;
> 
> 		dev_dbg(&pdev->dev, "found reserved block 0x%pa-0x%pa\n",
> 			&block->start, &end);
>  
>>  		block++;
>>  	}
>> @@ -158,8 +159,9 @@ static int sram_probe(struct platform_device *pdev)
>>  		/* can only happen if sections overlap */
>>  		if (block->start < cur_start) {
>>  			dev_err(&pdev->dev,
>> -				"block at 0x%x starts after current offset 0x%lx\n",
>> -				block->start, cur_start);
>> +				"block at 0x%llx starts after current offset 0x%llx\n",
>> +				(unsigned long long)block->start,
>> +				(unsigned long long)cur_start);
> 
> 			dev_err(&pdev->dev,
> 				"block at 0x%pa starts after current offset 0x%pa\n",
> 				&block->start, &cur_start);
> 
>>  			ret = -EINVAL;
>>  			goto err_chunks;
>>  		}
>> @@ -177,8 +179,9 @@ static int sram_probe(struct platform_device *pdev)
>>  		 */
>>  		cur_size = block->start - cur_start;
>>  
>> -		dev_dbg(&pdev->dev, "adding chunk 0x%lx-0x%lx\n",
>> -			cur_start, cur_start + cur_size);
>> +		dev_dbg(&pdev->dev, "adding chunk 0x%llx-0x%llx\n",
>> +			(unsigned long long)cur_start,
>> +			(unsigned long long)cur_start + cur_size);
> 
> 		dev_dbg(&pdev->dev, "adding chunk 0x%pa-0x%pa\n",
> 			&cur_start, &block->start);
> 
>>  		ret = gen_pool_add_virt(sram->pool,
>>  				(unsigned long)virt_base + cur_start,
>>  				res->start + cur_start, cur_size, -1);
>> @@ -193,7 +196,8 @@ static int sram_probe(struct platform_device *pdev)
>>  
>>  	platform_set_drvdata(pdev, sram);
>>  
>> -	dev_dbg(&pdev->dev, "SRAM pool: %ld KiB @ 0x%p\n", size / 1024, virt_base);
>> +	dev_dbg(&pdev->dev, "SRAM pool: %zu KiB @ 0x%p\n",
>> +		size / 1024, virt_base);
>>  
>>  	return 0;
> 
> regards
> Philipp
> 

--
With best wishes,
Vladimir

Re: [PATCH v3 1/9] misc: sram: fix enabled clock leak on error path

[Vladimir Zapolskiy]

Hi Philipp,

On 19.05.2015 13:41, Philipp Zabel wrote:
> Am Montag, den 18.05.2015, 22:08 +0300 schrieb Vladimir Zapolskiy:
>> If devm_gen_pool_create() fails, the previously enabled sram->clk is
>> not disabled on probe() exit.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>> ---
>>  drivers/misc/sram.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
>> index eeaaf5f..b44a423 100644
>> --- a/drivers/misc/sram.c
>> +++ b/drivers/misc/sram.c
>> @@ -90,16 +90,17 @@ static int sram_probe(struct platform_device *pdev)
>>  	if (!sram)
>>  		return -ENOMEM;
>>  
>> +	sram->pool = devm_gen_pool_create(&pdev->dev,
>> +					  ilog2(SRAM_GRANULARITY), -1);
>> +	if (!sram->pool)
>> +		return -ENOMEM;
>> +
>>  	sram->clk = devm_clk_get(&pdev->dev, NULL);
>>  	if (IS_ERR(sram->clk))
>>  		sram->clk = NULL;
>>  	else
>>  		clk_prepare_enable(sram->clk);
> 
> Here you move sram->clk around, and later in patch 7 it gets moved
> again. To me it looks like the two should be squashed together.

I agree with you, instead of moving sram->pool up it is better to place
sram->clk right at the end of probe(), in other words this patch can be
safely merged with patch 7 and the series becomes a bit shorter.

Thank you for the finding, I'm going to resend the change, please let me
know your opinion about "%pa" vs "0x%llx", if it is needed to be changed
or not.

>>  
>> -	sram->pool = devm_gen_pool_create(&pdev->dev, ilog2(SRAM_GRANULARITY), -1);
>> -	if (!sram->pool)
>> -		return -ENOMEM;
>> -
>>  	/*
>>  	 * We need an additional block to mark the end of the memory region
>>  	 * after the reserved blocks from the dt are processed.
> 
> regards
> Philipp
> 

--
With best wishes,
Vladimir

Re: [PATCH v3 1/9] misc: sram: fix enabled clock leak on error path

[Philipp Zabel]

Hi Vladimir,

Am Dienstag, den 19.05.2015, 16:11 +0300 schrieb Vladimir Zapolskiy:
> Hi Philipp,
> 
> On 19.05.2015 13:41, Philipp Zabel wrote:
> > Am Montag, den 18.05.2015, 22:08 +0300 schrieb Vladimir Zapolskiy:
> >> If devm_gen_pool_create() fails, the previously enabled sram->clk is
> >> not disabled on probe() exit.
> >>
> >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> >> ---
> >>  drivers/misc/sram.c | 9 +++++----
> >>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> >> index eeaaf5f..b44a423 100644
> >> --- a/drivers/misc/sram.c
> >> +++ b/drivers/misc/sram.c
> >> @@ -90,16 +90,17 @@ static int sram_probe(struct platform_device *pdev)
> >>  	if (!sram)
> >>  		return -ENOMEM;
> >>  
> >> +	sram->pool = devm_gen_pool_create(&pdev->dev,
> >> +					  ilog2(SRAM_GRANULARITY), -1);
> >> +	if (!sram->pool)
> >> +		return -ENOMEM;
> >> +
> >>  	sram->clk = devm_clk_get(&pdev->dev, NULL);
> >>  	if (IS_ERR(sram->clk))
> >>  		sram->clk = NULL;
> >>  	else
> >>  		clk_prepare_enable(sram->clk);
> > 
> > Here you move sram->clk around, and later in patch 7 it gets moved
> > again. To me it looks like the two should be squashed together.
> 
> I agree with you, instead of moving sram->pool up it is better to place
> sram->clk right at the end of probe(), in other words this patch can be
> safely merged with patch 7 and the series becomes a bit shorter.
> 
> Thank you for the finding, I'm going to resend the change, please let me
> know your opinion about "%pa" vs "0x%llx", if it is needed to be changed
> or not.

I'd prefer to use %pa for the phys_addr_t types. You could argue that
%pa is inappropriate as those are addresses relative to the SRAM region,
not physical addresses as seen by the CPU. But following that argument,
using phys_addr_t in the first place would not be correct either. Which
leads me to question whether we will see larger than 4 GiB SRAM regions
in the foreseeable future?

regards
Philipp

Re: [PATCH v3 1/9] misc: sram: fix enabled clock leak on error path

[Vladimir Zapolskiy]

Hi Philipp,

On 20.05.2015 14:30, Philipp Zabel wrote:
> Hi Vladimir,
> 
> Am Dienstag, den 19.05.2015, 16:11 +0300 schrieb Vladimir Zapolskiy:
>> Hi Philipp,
>>
>> On 19.05.2015 13:41, Philipp Zabel wrote:
>>> Am Montag, den 18.05.2015, 22:08 +0300 schrieb Vladimir Zapolskiy:
>>>> If devm_gen_pool_create() fails, the previously enabled sram->clk is
>>>> not disabled on probe() exit.
>>>>
>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>>> ---
>>>>  drivers/misc/sram.c | 9 +++++----
>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
>>>> index eeaaf5f..b44a423 100644
>>>> --- a/drivers/misc/sram.c
>>>> +++ b/drivers/misc/sram.c
>>>> @@ -90,16 +90,17 @@ static int sram_probe(struct platform_device *pdev)
>>>>  	if (!sram)
>>>>  		return -ENOMEM;
>>>>  
>>>> +	sram->pool = devm_gen_pool_create(&pdev->dev,
>>>> +					  ilog2(SRAM_GRANULARITY), -1);
>>>> +	if (!sram->pool)
>>>> +		return -ENOMEM;
>>>> +
>>>>  	sram->clk = devm_clk_get(&pdev->dev, NULL);
>>>>  	if (IS_ERR(sram->clk))
>>>>  		sram->clk = NULL;
>>>>  	else
>>>>  		clk_prepare_enable(sram->clk);
>>>
>>> Here you move sram->clk around, and later in patch 7 it gets moved
>>> again. To me it looks like the two should be squashed together.
>>
>> I agree with you, instead of moving sram->pool up it is better to place
>> sram->clk right at the end of probe(), in other words this patch can be
>> safely merged with patch 7 and the series becomes a bit shorter.
>>
>> Thank you for the finding, I'm going to resend the change, please let me
>> know your opinion about "%pa" vs "0x%llx", if it is needed to be changed
>> or not.
> 
> I'd prefer to use %pa for the phys_addr_t types. You could argue that
> %pa is inappropriate as those are addresses relative to the SRAM region,
> not physical addresses as seen by the CPU. But following that argument,
> using phys_addr_t in the first place would not be correct either.

The driver utilizes genalloc gen_pool_add_virt() function, whose
corresponding argument is of phys_addr_t type (and it is correct in my
opinion), the interface to this function dictates the type of its
arguments on client's side.

> Which leads me to question whether we will see larger than 4 GiB SRAM
> regions in the foreseeable future?

The question is not only about SRAM region size, but mainly it is about
the base address of SRAM, and don't want to exclude a situation, when
some kind of SRAM device is found outside of u32 addressable memory
space. Actually I believe an arbitrary physical memory region may be
claimed as it were a "SRAM device" and the driver still works fine.

If phys_addr_t arguments are accepted, then back to "%pa" vs "0x%llx"
question:

From: Philipp Zabel <p.zabel@pengutronix.de>
Date: Tue, 19 May 2015 12:38:37 +0200
In-Reply-To:
<1431976122-4228-4-git-send-email-vladimir_zapolskiy@mentor.com>

> Now that block->start is of type phys_addr_t, is there a reason
> not to use %pa ?

[snip]

>> -		dev_dbg(&pdev->dev, "adding chunk 0x%lx-0x%lx\n",
>> -			cur_start, cur_start + cur_size);
>> +		dev_dbg(&pdev->dev, "adding chunk 0x%llx-0x%llx\n",
>> +			(unsigned long long)cur_start,
>> +			(unsigned long long)cur_start + cur_size);
>
>		dev_dbg(&pdev->dev, "adding chunk 0x%pa-0x%pa\n",
>			&cur_start, &block->start);
>

I see that the change preserves the functionality, so I'll change printk
format to "%pa", will resend the series tomorrow.

--
With best wishes,
Vladimir

Re: [PATCH v3 1/9] misc: sram: fix enabled clock leak on error path

[Philipp Zabel]

Hi Vladimir,

Am Sonntag, den 24.05.2015, 23:12 +0300 schrieb Vladimir Zapolskiy:
> Hi Philipp,
> 
> On 20.05.2015 14:30, Philipp Zabel wrote:
> > Hi Vladimir,
> > 
> > Am Dienstag, den 19.05.2015, 16:11 +0300 schrieb Vladimir Zapolskiy:
> >> Hi Philipp,
> >>
> >> On 19.05.2015 13:41, Philipp Zabel wrote:
> >>> Am Montag, den 18.05.2015, 22:08 +0300 schrieb Vladimir Zapolskiy:
> >>>> If devm_gen_pool_create() fails, the previously enabled sram->clk is
> >>>> not disabled on probe() exit.
> >>>>
> >>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> >>>> ---
> >>>>  drivers/misc/sram.c | 9 +++++----
> >>>>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> >>>> index eeaaf5f..b44a423 100644
> >>>> --- a/drivers/misc/sram.c
> >>>> +++ b/drivers/misc/sram.c
> >>>> @@ -90,16 +90,17 @@ static int sram_probe(struct platform_device *pdev)
> >>>>  	if (!sram)
> >>>>  		return -ENOMEM;
> >>>>  
> >>>> +	sram->pool = devm_gen_pool_create(&pdev->dev,
> >>>> +					  ilog2(SRAM_GRANULARITY), -1);
> >>>> +	if (!sram->pool)
> >>>> +		return -ENOMEM;
> >>>> +
> >>>>  	sram->clk = devm_clk_get(&pdev->dev, NULL);
> >>>>  	if (IS_ERR(sram->clk))
> >>>>  		sram->clk = NULL;
> >>>>  	else
> >>>>  		clk_prepare_enable(sram->clk);
> >>>
> >>> Here you move sram->clk around, and later in patch 7 it gets moved
> >>> again. To me it looks like the two should be squashed together.
> >>
> >> I agree with you, instead of moving sram->pool up it is better to place
> >> sram->clk right at the end of probe(), in other words this patch can be
> >> safely merged with patch 7 and the series becomes a bit shorter.
> >>
> >> Thank you for the finding, I'm going to resend the change, please let me
> >> know your opinion about "%pa" vs "0x%llx", if it is needed to be changed
> >> or not.
> > 
> > I'd prefer to use %pa for the phys_addr_t types. You could argue that
> > %pa is inappropriate as those are addresses relative to the SRAM region,
> > not physical addresses as seen by the CPU. But following that argument,
> > using phys_addr_t in the first place would not be correct either.
> 
> The driver utilizes genalloc gen_pool_add_virt() function, whose
> corresponding argument is of phys_addr_t type (and it is correct in my
> opinion), the interface to this function dictates the type of its
> arguments on client's side.

res->start is of type phys_addr_t (well, resource_size_t) already.
block->start/size and cur_start/size are just offsets added to it.
I wonder if it wouldn't be more appropriate to use resource_size_t for
the sram_reserve .start field.

> > Which leads me to question whether we will see larger than 4 GiB SRAM
> > regions in the foreseeable future?
> 
> The question is not only about SRAM region size, but mainly it is about
> the base address of SRAM, and don't want to exclude a situation, when
> some kind of SRAM device is found outside of u32 addressable memory
> space. Actually I believe an arbitrary physical memory region may be
> claimed as it were a "SRAM device" and the driver still works fine.

See above, start addresses above 4 GiB should be supported even without
extending the offsets to 64-bit.

> If phys_addr_t arguments are accepted, then back to "%pa" vs "0x%llx"
> question:
> 
[...]
> I see that the change preserves the functionality, so I'll change printk
> format to "%pa", will resend the series tomorrow.

Thanks.

regards
Philipp

Re: [PATCH v3 1/9] misc: sram: fix enabled clock leak on error path

[Vladimir Zapolskiy]

Hi Philipp,

On 26.05.2015 11:54, Philipp Zabel wrote:
> Hi Vladimir,
> 
> Am Sonntag, den 24.05.2015, 23:12 +0300 schrieb Vladimir Zapolskiy:
>> Hi Philipp,
>>
>> On 20.05.2015 14:30, Philipp Zabel wrote:
>>> Hi Vladimir,
>>>
>>> Am Dienstag, den 19.05.2015, 16:11 +0300 schrieb Vladimir Zapolskiy:
>>>> Hi Philipp,
>>>>
>>>> On 19.05.2015 13:41, Philipp Zabel wrote:
>>>>> Am Montag, den 18.05.2015, 22:08 +0300 schrieb Vladimir Zapolskiy:
>>>>>> If devm_gen_pool_create() fails, the previously enabled sram->clk is
>>>>>> not disabled on probe() exit.
>>>>>>
>>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>>>>> ---
>>>>>>  drivers/misc/sram.c | 9 +++++----
>>>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
>>>>>> index eeaaf5f..b44a423 100644
>>>>>> --- a/drivers/misc/sram.c
>>>>>> +++ b/drivers/misc/sram.c
>>>>>> @@ -90,16 +90,17 @@ static int sram_probe(struct platform_device *pdev)
>>>>>>  	if (!sram)
>>>>>>  		return -ENOMEM;
>>>>>>  
>>>>>> +	sram->pool = devm_gen_pool_create(&pdev->dev,
>>>>>> +					  ilog2(SRAM_GRANULARITY), -1);
>>>>>> +	if (!sram->pool)
>>>>>> +		return -ENOMEM;
>>>>>> +
>>>>>>  	sram->clk = devm_clk_get(&pdev->dev, NULL);
>>>>>>  	if (IS_ERR(sram->clk))
>>>>>>  		sram->clk = NULL;
>>>>>>  	else
>>>>>>  		clk_prepare_enable(sram->clk);
>>>>>
>>>>> Here you move sram->clk around, and later in patch 7 it gets moved
>>>>> again. To me it looks like the two should be squashed together.
>>>>
>>>> I agree with you, instead of moving sram->pool up it is better to place
>>>> sram->clk right at the end of probe(), in other words this patch can be
>>>> safely merged with patch 7 and the series becomes a bit shorter.
>>>>
>>>> Thank you for the finding, I'm going to resend the change, please let me
>>>> know your opinion about "%pa" vs "0x%llx", if it is needed to be changed
>>>> or not.
>>>
>>> I'd prefer to use %pa for the phys_addr_t types. You could argue that
>>> %pa is inappropriate as those are addresses relative to the SRAM region,
>>> not physical addresses as seen by the CPU. But following that argument,
>>> using phys_addr_t in the first place would not be correct either.
>>
>> The driver utilizes genalloc gen_pool_add_virt() function, whose
>> corresponding argument is of phys_addr_t type (and it is correct in my
>> opinion), the interface to this function dictates the type of its
>> arguments on client's side.
> 
> res->start is of type phys_addr_t (well, resource_size_t) already.
> block->start/size and cur_start/size are just offsets added to it.

I agree.

> I wonder if it wouldn't be more appropriate to use resource_size_t for
> the sram_reserve .start field.

Assuming that the sram_reserve .start field represents only the difference
of two res->start and this difference fits into u32 storage, it should be
safe to keep it as is.

In my opinion integer overflow case should not be considered or handled
by the driver, so probably the best option would be just to drop
phys_addr_t commit, since it attempts to solve a nonexistent problem.

Please let me know your opinion, if it is fine with you, I'll remove
"use phys_addr_t instead of u32 for physical address" commit and resend
the series.

>>> Which leads me to question whether we will see larger than 4 GiB SRAM
>>> regions in the foreseeable future?
>>
>> The question is not only about SRAM region size, but mainly it is about
>> the base address of SRAM, and don't want to exclude a situation, when
>> some kind of SRAM device is found outside of u32 addressable memory
>> space. Actually I believe an arbitrary physical memory region may be
>> claimed as it were a "SRAM device" and the driver still works fine.
> 
> See above, start addresses above 4 GiB should be supported even without
> extending the offsets to 64-bit.
> 
>> If phys_addr_t arguments are accepted, then back to "%pa" vs "0x%llx"
>> question:
>>
> [...]
>> I see that the change preserves the functionality, so I'll change printk
>> format to "%pa", will resend the series tomorrow.
> 

--
With best wishes,
Vladimir

Re: [PATCH v3 1/9] misc: sram: fix enabled clock leak on error path

[Philipp Zabel]

Hi Vladimir,

Am Freitag, den 29.05.2015, 14:31 +0300 schrieb Vladimir Zapolskiy:
[...]
> > res->start is of type phys_addr_t (well, resource_size_t) already.
> > block->start/size and cur_start/size are just offsets added to it.
> 
> I agree.
> 
> > I wonder if it wouldn't be more appropriate to use resource_size_t for
> > the sram_reserve .start field.
> 
> Assuming that the sram_reserve .start field represents only the difference
> of two res->start and this difference fits into u32 storage, it should be
> safe to keep it as is.
> 
> In my opinion integer overflow case should not be considered or handled
> by the driver, so probably the best option would be just to drop
> phys_addr_t commit, since it attempts to solve a nonexistent problem.
> 
> Please let me know your opinion, if it is fine with you, I'll remove
> "use phys_addr_t instead of u32 for physical address" commit and resend
> the series.

Yes, that's fine with me.

regards
Philipp