diff mbox

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

Message ID 1478259816-24965-3-git-send-email-yamada.masahiro@socionext.com (mailing list archive)
State New, archived
Headers show

Commit Message

Masahiro Yamada Nov. 4, 2016, 11:43 a.m. UTC
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(-)

Comments

Russell King - ARM Linux admin Nov. 4, 2016, 12:23 p.m. UTC | #1
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.
Masahiro Yamada Nov. 4, 2016, 12:50 p.m. UTC | #2
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.
Russell King - ARM Linux admin Nov. 4, 2016, 1:32 p.m. UTC | #3
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.
Masahiro Yamada Nov. 5, 2016, 3:17 a.m. UTC | #4
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.
diff mbox

Patch

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;