[PATCH 0/3] ARM: cache-uniphier: slight cleanups and trivial coding style fix

[PATCH 0/3] ARM: cache-uniphier: slight cleanups and trivial coding style fix

[Masahiro Yamada]

The cache-uniphier is a full-custom outer-cache for
Socionext UniPhier SoC family.

This series includes SoC-specific cleanups and
a trivial coding style fix suggested by Documentation/CodingStyle.



Masahiro Yamada (3):
  ARM: cache-uniphier: call kzalloc() after DT property parsing
  ARM: cache-uniphier: refactor jump label to follow coding style
    guideline
  ARM: cache-uniphier: clean up active way setup code

 arch/arm/mm/cache-uniphier.c | 64 ++++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 35 deletions(-)

-- 
1.9.1

[PATCH 1/3] ARM: cache-uniphier: call kzalloc() after DT property parsing

[Masahiro Yamada]

Allocate memory after DT property parsing that has more possibility
of failure.  This will decrease the number of bail-out points for
kfree().

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 arch/arm/mm/cache-uniphier.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/arch/arm/mm/cache-uniphier.c b/arch/arm/mm/cache-uniphier.c
index dfe97b4..58f2ddb 100644
--- a/arch/arm/mm/cache-uniphier.c
+++ b/arch/arm/mm/cache-uniphier.c
@@ -328,7 +328,7 @@ static int __init __uniphier_cache_init(struct device_node *np,
 					unsigned int *cache_level)
 {
 	struct uniphier_cache_data *data;
-	u32 level, cache_size;
+	u32 level, line_size, nsets, cache_size;
 	struct device_node *next_np;
 	int ret = 0;
 
@@ -354,36 +354,34 @@ static int __init __uniphier_cache_init(struct device_node *np,
 		return -EINVAL;
 	}
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-
-	if (of_property_read_u32(np, "cache-line-size", &data->line_size) ||
-	    !is_power_of_2(data->line_size)) {
+	if (of_property_read_u32(np, "cache-line-size", &line_size) |
+	    !is_power_of_2(line_size)) {
 		pr_err("L%d: cache-line-size is unspecified or invalid\n",
 		       *cache_level);
-		ret = -EINVAL;
-		goto err;
+		return -EINVAL;
 	}
 
-	if (of_property_read_u32(np, "cache-sets", &data->nsets) ||
-	    !is_power_of_2(data->nsets)) {
+	if (of_property_read_u32(np, "cache-sets", &nsets) ||
+	    !is_power_of_2(nsets)) {
 		pr_err("L%d: cache-sets is unspecified or invalid\n",
 		       *cache_level);
-		ret = -EINVAL;
-		goto err;
+		return -EINVAL;
 	}
 
 	if (of_property_read_u32(np, "cache-size", &cache_size) ||
-	    cache_size == 0 || cache_size % (data->nsets * data->line_size)) {
+	    cache_size == 0 || cache_size % (nsets * line_size)) {
 		pr_err("L%d: cache-size is unspecified or invalid\n",
 		       *cache_level);
-		ret = -EINVAL;
-		goto err;
+		return -EINVAL;
 	}
 
-	data->way_present_mask =
-		((u32)1 << cache_size / data->nsets / data->line_size) - 1;
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->line_size = line_size;
+	data->nsets = nsets;
+	data->way_present_mask = ((u32)1 << cache_size / nsets / line_size) - 1;
 
 	data->ctrl_base = of_iomap(np, 0);
 	if (!data->ctrl_base) {
-- 
1.9.1

[PATCH 2/3] ARM: cache-uniphier: refactor jump label to follow coding style guideline

[Masahiro Yamada]

Documentation/CodingStyle recommends to use label names which say
what the goto does or why the goto exists.

Just in case, split it up into three labels because the CodingStyle
says "one err bugs" is a common type of bug (although, I do not
believe the current code includes such a bug).

During the refactoring, iounmap(data->op_base) turned out to have no
corresponding bail-out point, so remove it.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 arch/arm/mm/cache-uniphier.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mm/cache-uniphier.c b/arch/arm/mm/cache-uniphier.c
index 58f2ddb..c71ab7c 100644
--- a/arch/arm/mm/cache-uniphier.c
+++ b/arch/arm/mm/cache-uniphier.c
@@ -387,21 +387,21 @@ static int __init __uniphier_cache_init(struct device_node *np,
 	if (!data->ctrl_base) {
 		pr_err("L%d: failed to map control register\n", *cache_level);
 		ret = -ENOMEM;
-		goto err;
+		goto free_mem;
 	}
 
 	data->rev_base = of_iomap(np, 1);
 	if (!data->rev_base) {
 		pr_err("L%d: failed to map revision register\n", *cache_level);
 		ret = -ENOMEM;
-		goto err;
+		goto unmap_ctrl;
 	}
 
 	data->op_base = of_iomap(np, 2);
 	if (!data->op_base) {
 		pr_err("L%d: failed to map operation register\n", *cache_level);
 		ret = -ENOMEM;
-		goto err;
+		goto unmap_rev;
 	}
 
 	data->way_ctrl_base = data->ctrl_base + 0xc00;
@@ -451,10 +451,12 @@ static int __init __uniphier_cache_init(struct device_node *np,
 	of_node_put(next_np);
 
 	return ret;
-err:
-	iounmap(data->op_base);
+
+unmap_rev:
 	iounmap(data->rev_base);
+unmap_ctrl:
 	iounmap(data->ctrl_base);
+free_mem:
 	kfree(data);
 
 	return ret;
-- 
1.9.1

Re: [PATCH 2/3] ARM: cache-uniphier: refactor jump label to follow coding style guideline

[Russell King - ARM Linux]

On Fri, Nov 04, 2016 at 08:43:35PM +0900, Masahiro Yamada wrote:
> Documentation/CodingStyle recommends to use label names which say
> what the goto does or why the goto exists.
> 
> Just in case, split it up into three labels because the CodingStyle
> says "one err bugs" is a common type of bug (although, I do not
> believe the current code includes such a bug).

However, this has the effect of making the code unnecessarily more
complicated, which is a bad thing.  Avoiding unnecessary code
complexity wins over style rules.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/3] ARM: cache-uniphier: refactor jump label to follow coding style guideline

[Masahiro Yamada]

Hi Russell,

2016-11-04 21:23 GMT+09:00 Russell King - ARM Linux <linux@armlinux.org.uk>:
> On Fri, Nov 04, 2016 at 08:43:35PM +0900, Masahiro Yamada wrote:
>> Documentation/CodingStyle recommends to use label names which say
>> what the goto does or why the goto exists.
>>
>> Just in case, split it up into three labels because the CodingStyle
>> says "one err bugs" is a common type of bug (although, I do not
>> believe the current code includes such a bug).
>
> However, this has the effect of making the code unnecessarily more
> complicated, which is a bad thing.  Avoiding unnecessary code
> complexity wins over style rules.


I thought this patch is stupid, but makes the code more straight-forward;
the failure path only calls really needed iounmap/kfree()
without exploiting that NULL input makes them no-op.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/3] ARM: cache-uniphier: refactor jump label to follow coding style guideline

[Russell King - ARM Linux]

On Fri, Nov 04, 2016 at 09:50:56PM +0900, Masahiro Yamada wrote:
> Hi Russell,
> 
> 2016-11-04 21:23 GMT+09:00 Russell King - ARM Linux <linux@armlinux.org.uk>:
> > On Fri, Nov 04, 2016 at 08:43:35PM +0900, Masahiro Yamada wrote:
> >> Documentation/CodingStyle recommends to use label names which say
> >> what the goto does or why the goto exists.
> >>
> >> Just in case, split it up into three labels because the CodingStyle
> >> says "one err bugs" is a common type of bug (although, I do not
> >> believe the current code includes such a bug).
> >
> > However, this has the effect of making the code unnecessarily more
> > complicated, which is a bad thing.  Avoiding unnecessary code
> > complexity wins over style rules.
> 
> 
> I thought this patch is stupid, but makes the code more straight-forward;
> the failure path only calls really needed iounmap/kfree()
> without exploiting that NULL input makes them no-op.

... while making it more fragile, because we're going back to a
situation where the right places need to jump to the right label
in the cleanup, so that the right functions are called.

This is a backwards step.

The reason that iounmap() and kfree() check for NULL pointers is to
allow the cleanup paths to be simple, and that's very important as
many cleanup paths are simply _not_ tested.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/3] ARM: cache-uniphier: refactor jump label to follow coding style guideline

[Masahiro Yamada]

2016-11-04 22:32 GMT+09:00 Russell King - ARM Linux <linux@armlinux.org.uk>:
> On Fri, Nov 04, 2016 at 09:50:56PM +0900, Masahiro Yamada wrote:
>> Hi Russell,
>>
>> 2016-11-04 21:23 GMT+09:00 Russell King - ARM Linux <linux@armlinux.org.uk>:
>> > On Fri, Nov 04, 2016 at 08:43:35PM +0900, Masahiro Yamada wrote:
>> >> Documentation/CodingStyle recommends to use label names which say
>> >> what the goto does or why the goto exists.
>> >>
>> >> Just in case, split it up into three labels because the CodingStyle
>> >> says "one err bugs" is a common type of bug (although, I do not
>> >> believe the current code includes such a bug).
>> >
>> > However, this has the effect of making the code unnecessarily more
>> > complicated, which is a bad thing.  Avoiding unnecessary code
>> > complexity wins over style rules.
>>
>>
>> I thought this patch is stupid, but makes the code more straight-forward;
>> the failure path only calls really needed iounmap/kfree()
>> without exploiting that NULL input makes them no-op.
>
> ... while making it more fragile, because we're going back to a
> situation where the right places need to jump to the right label
> in the cleanup, so that the right functions are called.
>
> This is a backwards step.
>
> The reason that iounmap() and kfree() check for NULL pointers is to
> allow the cleanup paths to be simple, and that's very important as
> many cleanup paths are simply _not_ tested.


OK, fair enough.



-- 
Best Regards
Masahiro Yamada

[PATCH 3/3] ARM: cache-uniphier: clean up active way setup code

[Masahiro Yamada]

Now, the active way setup function is called with a fixed value zero
for the second argument only when enabling the outer-cache.
The code can be simpler.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 arch/arm/mm/cache-uniphier.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/arch/arm/mm/cache-uniphier.c b/arch/arm/mm/cache-uniphier.c
index c71ab7c..58ba2bd 100644
--- a/arch/arm/mm/cache-uniphier.c
+++ b/arch/arm/mm/cache-uniphier.c
@@ -71,8 +71,7 @@
  * @ctrl_base: virtual base address of control registers
  * @rev_base: virtual base address of revision registers
  * @op_base: virtual base address of operation registers
- * @way_present_mask: each bit specifies if the way is present
- * @way_locked_mask: each bit specifies if the way is locked
+ * @way_mask: each bit specifies if the way is present
  * @nsets: number of associativity sets
  * @line_size: line size in bytes
  * @range_op_max_size: max size that can be handled by a single range operation
@@ -83,8 +82,7 @@ struct uniphier_cache_data {
 	void __iomem *rev_base;
 	void __iomem *op_base;
 	void __iomem *way_ctrl_base;
-	u32 way_present_mask;
-	u32 way_locked_mask;
+	u32 way_mask;
 	u32 nsets;
 	u32 line_size;
 	u32 range_op_max_size;
@@ -234,17 +232,13 @@ static void __uniphier_cache_enable(struct uniphier_cache_data *data, bool on)
 	writel_relaxed(val, data->ctrl_base + UNIPHIER_SSCC);
 }
 
-static void __init __uniphier_cache_set_locked_ways(
-					struct uniphier_cache_data *data,
-					u32 way_mask)
+static void __init __uniphier_cache_set_active_ways(
+					struct uniphier_cache_data *data)
 {
 	unsigned int cpu;
 
-	data->way_locked_mask = way_mask & data->way_present_mask;
-
 	for_each_possible_cpu(cpu)
-		writel_relaxed(~data->way_locked_mask & data->way_present_mask,
-			       data->way_ctrl_base + 4 * cpu);
+		writel_relaxed(data->way_mask, data->way_ctrl_base + 4 * cpu);
 }
 
 static void uniphier_cache_maint_range(unsigned long start, unsigned long end,
@@ -307,7 +301,7 @@ static void __init uniphier_cache_enable(void)
 
 	list_for_each_entry(data, &uniphier_cache_list, list) {
 		__uniphier_cache_enable(data, true);
-		__uniphier_cache_set_locked_ways(data, 0);
+		__uniphier_cache_set_active_ways(data);
 	}
 }
 
@@ -381,7 +375,7 @@ static int __init __uniphier_cache_init(struct device_node *np,
 
 	data->line_size = line_size;
 	data->nsets = nsets;
-	data->way_present_mask = ((u32)1 << cache_size / nsets / line_size) - 1;
+	data->way_mask = GENMASK(cache_size / nsets / line_size - 1, 0);
 
 	data->ctrl_base = of_iomap(np, 0);
 	if (!data->ctrl_base) {
-- 
1.9.1