[PATCH 0/4] clk/Renesas-MSTP: Fine-tuning for two function implementations

[PATCH 0/4] clk/Renesas-MSTP: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 21:48:48 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Use kmalloc_array() in cpg_mstp_clocks_init()
  Delete an error message for a failed memory allocation
  Less function calls in cpg_mstp_clocks_init() after error detection
  Rename jump labels in cpg_mstp_attach_dev()

 drivers/clk/renesas/clk-mstp.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

-- 
2.10.0

[PATCH 1/4] clk/Renesas-MSTP: Use kmalloc_array() in cpg_mstp_clocks_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 21:10:47 +0200

A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/clk/renesas/clk-mstp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index 5093a25..9375777 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -167,7 +167,7 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
 	unsigned int i;
 
 	group = kzalloc(sizeof(*group), GFP_KERNEL);
-	clks = kmalloc(MSTP_MAX_CLOCKS * sizeof(*clks), GFP_KERNEL);
+	clks = kmalloc_array(MSTP_MAX_CLOCKS, sizeof(*clks), GFP_KERNEL);
 	if (group == NULL || clks == NULL) {
 		kfree(group);
 		kfree(clks);
-- 
2.10.0

Re: [PATCH 1/4] clk/Renesas-MSTP: Use kmalloc_array() in cpg_mstp_clocks_init()

[Geert Uytterhoeven]

On Wed, Sep 14, 2016 at 10:00 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 14 Sep 2016 21:10:47 +0200
>
> A multiplication for the size determination of a memory allocation
> indicated that an array data structure should be processed.
> Thus use the corresponding function "kmalloc_array".
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/4] clk/Renesas-MSTP: Use kmalloc_array() in cpg_mstp_clocks_init()

[Stephen Boyd]

On 09/14, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 14 Sep 2016 21:10:47 +0200
> 
> A multiplication for the size determination of a memory allocation
> indicated that an array data structure should be processed.
> Thus use the corresponding function "kmalloc_array".
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/4] clk/Renesas-MSTP: Delete an error message for a failed memory allocation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 21:17:18 +0200

Omit an extra message for a memory allocation failure in this function.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/clk/renesas/clk-mstp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index 9375777..1fdc44b 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -171,7 +171,6 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
 	if (group == NULL || clks == NULL) {
 		kfree(group);
 		kfree(clks);
-		pr_err("%s: failed to allocate group\n", __func__);
 		return;
 	}
 
-- 
2.10.0

Re: [PATCH 2/4] clk/Renesas-MSTP: Delete an error message for a failed memory allocation

[Geert Uytterhoeven]

On Wed, Sep 14, 2016 at 10:01 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 14 Sep 2016 21:17:18 +0200
>
> Omit an extra message for a memory allocation failure in this function.
>
> Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/4] clk/Renesas-MSTP: Delete an error message for a failed memory allocation

[Laurent Pinchart]

On Thursday 15 Sep 2016 21:07:17 Geert Uytterhoeven wrote:
> On Wed, Sep 14, 2016 at 10:01 PM, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Wed, 14 Sep 2016 21:17:18 +0200
> > 
> > Omit an extra message for a memory allocation failure in this function.
> > 
> > Link:
> > http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refacto
> > r_Strings-WSang_0.pdf
> > 
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

There are two other memory allocation failures printed by drivers in the same 
directory that, you'll get my ack if you extend this patch to remove the three 
messages in one go.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] clk/Renesas-MSTP: Delete an error message for a failed memory allocation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]

On Thu, Sep 15, 2016 at 10:13:17PM +0300, Laurent Pinchart wrote:
> On Thursday 15 Sep 2016 21:07:17 Geert Uytterhoeven wrote:
> > On Wed, Sep 14, 2016 at 10:01 PM, SF Markus Elfring wrote:
> > > From: Markus Elfring <elfring@users.sourceforge.net>
> > > Date: Wed, 14 Sep 2016 21:17:18 +0200
> > > 
> > > Omit an extra message for a memory allocation failure in this function.
> > > 
> > > Link:
> > > http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refacto
> > > r_Strings-WSang_0.pdf
> > > 
> > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > 
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> There are two other memory allocation failures printed by drivers in the same 
> directory that, you'll get my ack if you extend this patch to remove the three 
> messages in one go.

I agree.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: clk/Renesas-MSTP: Delete an error message for a failed memory allocation

[SF Markus Elfring]

>>> Omit an extra message for a memory allocation failure in this function.
>>>
>>> Link:
>>> http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refacto
>>> r_Strings-WSang_0.pdf
>>>
>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>>
>> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> There are two other memory allocation failures printed by drivers in the same 
> directory that, you'll get my ack

Does this kind of feedback express a general acceptance for the deletion
of similar error messages?


> if you extend this patch to remove the three messages in one go.

Does this wish influence the handling of suggested changes for the source file
"drivers/clk/renesas/clk-mstp.c" anyhow?
https://patchwork.kernel.org/patch/9332363/

Regards,
Markus

Re: clk/Renesas-MSTP: Delete an error message for a failed memory allocation

[Laurent Pinchart]

On Thursday 15 Sep 2016 22:17:41 SF Markus Elfring wrote:
> >>> Omit an extra message for a memory allocation failure in this function.
> >>> 
> >>> Link:
> >>> http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refact
> >>> or_Strings-WSang_0.pdf
> >>> 
> >>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> >> 
> >> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > 
> > There are two other memory allocation failures printed by drivers in the
> > same directory that, you'll get my ack
> 
> Does this kind of feedback express a general acceptance for the deletion
> of similar error messages?

No, it's my opinion only.

> > if you extend this patch to remove the three messages in one go.
> 
> Does this wish influence the handling of suggested changes for the source
> file "drivers/clk/renesas/clk-mstp.c" anyhow?
> https://patchwork.kernel.org/patch/9332363/

Yes, please submit a new version of this patch that removes all the memory 
allocation error messages from drivers/clk/renesas/ in one go.

-- 
Regards,

Laurent Pinchart

Re: clk/Renesas-MSTP: Delete an error message for a failed memory allocation

[SF Markus Elfring]

>> Does this wish influence the handling of suggested changes for the source
>> file "drivers/clk/renesas/clk-mstp.c" anyhow?
>> https://patchwork.kernel.org/patch/9332363/
> 
> Yes, please submit a new version of this patch that removes all the memory 
> allocation error messages from drivers/clk/renesas/ in one go.

I find this kind of feedback a bit surprising and it seems to be promising
to increase software development in this design direction.
Have you got any corresponding scripts for the semantic patch language
prepared to make such a source code adjustment safer (and a bit more convenient
with the help of the Coccinelle software)?

I am also curious on how the change acceptance will evolve for the other
three update steps from this patch series when this one indicates further
software update opportunities already.

Regards,
Markus

[PATCH] clk/Renesas: Delete an error message for a failed memory allocation in three functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 25 Sep 2017 10:10:51 +0200

The script "checkpatch.pl" pointed information out like the following.

WARNING: Possible unnecessary 'out of memory' message

Thus fix affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/clk/renesas/clk-mstp.c      | 5 +----
 drivers/clk/renesas/clk-rcar-gen2.c | 1 -
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index 500a9e4e03c4..c944cc421e30 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -156,10 +156,8 @@ static struct clk * __init cpg_mstp_clock_register(const char *name,
 	struct clk *clk;
 
 	clock = kzalloc(sizeof(*clock), GFP_KERNEL);
-	if (!clock) {
-		pr_err("%s: failed to allocate MSTP clock.\n", __func__);
+	if (!clock)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	init.name = name;
 	init.ops = &cpg_mstp_clock_ops;
@@ -196,7 +194,6 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
 	if (group == NULL || clks == NULL) {
 		kfree(group);
 		kfree(clks);
-		pr_err("%s: failed to allocate group\n", __func__);
 		return;
 	}
 
diff --git a/drivers/clk/renesas/clk-rcar-gen2.c b/drivers/clk/renesas/clk-rcar-gen2.c
index 0b2e56d0d94b..d14cbe1ca29a 100644
--- a/drivers/clk/renesas/clk-rcar-gen2.c
+++ b/drivers/clk/renesas/clk-rcar-gen2.c
@@ -423,7 +423,6 @@ static void __init rcar_gen2_cpg_clocks_init(struct device_node *np)
 		/* We're leaking memory on purpose, there's no point in cleaning
 		 * up as the system won't boot anyway.
 		 */
-		pr_err("%s: failed to allocate cpg\n", __func__);
 		return;
 	}
 
-- 
2.14.1

Re: [PATCH] clk/Renesas: Delete an error message for a failed memory allocation in three functions

[Geert Uytterhoeven]

Hi Markus,

On Mon, Sep 25, 2017 at 10:34 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 25 Sep 2017 10:10:51 +0200
>
> The script "checkpatch.pl" pointed information out like the following.
>
> WARNING: Possible unnecessary 'out of memory' message
>
> Thus fix affected source code places.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in clk-renesas-for-v4.15, split in two commits:
  - clk: renesas: rcar-gen2: Delete error message for failed memory allocation
  - clk: renesas: mstp: Delete error messages for failed memory allocations

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 3/4] clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 21:30:27 +0200

The kfree() function was called in up to two cases
by the cpg_mstp_clocks_init() function during error handling even if
the passed variable contained a null pointer.

* Split a condition check for memory allocation failures so that
  each pointer from these function calls will be checked immediately.

  See also background information:
  Topic "CWE-754: Improper check for unusual or exceptional conditions"
  Link: https://cwe.mitre.org/data/definitions/754.html

* Return directly after a call of the function "kzalloc" failed
  at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/clk/renesas/clk-mstp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index 1fdc44b..6c82e0e 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -167,10 +167,12 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
 	unsigned int i;
 
 	group = kzalloc(sizeof(*group), GFP_KERNEL);
+	if (!group)
+		return;
+
 	clks = kmalloc_array(MSTP_MAX_CLOCKS, sizeof(*clks), GFP_KERNEL);
-	if (group == NULL || clks == NULL) {
+	if (!clks) {
 		kfree(group);
-		kfree(clks);
 		return;
 	}
 
-- 
2.10.0

Re: [PATCH 3/4] clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection

[Geert Uytterhoeven]

Hi Markus,

On Wed, Sep 14, 2016 at 10:03 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 14 Sep 2016 21:30:27 +0200
>
> The kfree() function was called in up to two cases
> by the cpg_mstp_clocks_init() function during error handling even if
> the passed variable contained a null pointer.

It's perfectly legal to call kfree() on a NULL pointer.

> * Split a condition check for memory allocation failures so that
>   each pointer from these function calls will be checked immediately.
>
>   See also background information:
>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>   Link: https://cwe.mitre.org/data/definitions/754.html
>
> * Return directly after a call of the function "kzalloc" failed
>   at the beginning.

Both calls are already close together.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/clk/renesas/clk-mstp.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

In addition, your patch increases the LoC, IMHO without improving the code.

>
> diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
> index 1fdc44b..6c82e0e 100644
> --- a/drivers/clk/renesas/clk-mstp.c
> +++ b/drivers/clk/renesas/clk-mstp.c
> @@ -167,10 +167,12 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
>         unsigned int i;
>
>         group = kzalloc(sizeof(*group), GFP_KERNEL);
> +       if (!group)
> +               return;
> +
>         clks = kmalloc_array(MSTP_MAX_CLOCKS, sizeof(*clks), GFP_KERNEL);
> -       if (group == NULL || clks == NULL) {
> +       if (!clks) {
>                 kfree(group);
> -               kfree(clks);
>                 return;
>         }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/4] clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection

[SF Markus Elfring]

> It's perfectly legal to call kfree() on a NULL pointer.

I know this function property well.


>> * Split a condition check for memory allocation failures so that
>>   each pointer from these function calls will be checked immediately.
>>
>>   See also background information:
>>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>>   Link: https://cwe.mitre.org/data/definitions/754.html
>>
>> * Return directly after a call of the function "kzalloc" failed
>>   at the beginning.
> 
> Both calls are already close together.

Can it be that an other software development concern is eventually
overlooked because of this "neighbourship" (or is categorised with
a lower priority)?

I suggest to reconsider this design detail if it is really acceptable
for the safe implementation of such a software module.

* How much will it matter in general that one function call was performed
  in this use case without checking its return values immediately?

* Should it usually be determined quicker if a required resource like
  memory could be acquired before trying the next allocation?


> In addition, your patch increases the LoC, IMHO without improving the code.

I find this consequence still debatable.


>> diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
>> index 1fdc44b..6c82e0e 100644
>> --- a/drivers/clk/renesas/clk-mstp.c
>> +++ b/drivers/clk/renesas/clk-mstp.c
>> @@ -167,10 +167,12 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
>>         unsigned int i;
>>
>>         group = kzalloc(sizeof(*group), GFP_KERNEL);
>> +       if (!group)
>> +               return;
>> +
>>         clks = kmalloc_array(MSTP_MAX_CLOCKS, sizeof(*clks), GFP_KERNEL);
>> -       if (group == NULL || clks == NULL) {
>> +       if (!clks) {
>>                 kfree(group);
>> -               kfree(clks);
>>                 return;
>>         }

Is this update suggestion worth for another look?

Regards,
Markus

Re: [PATCH 3/4] clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection

[Geert Uytterhoeven]

On Thu, Sep 15, 2016 at 10:40 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> * Split a condition check for memory allocation failures so that
>>>   each pointer from these function calls will be checked immediately.
>>>
>>>   See also background information:
>>>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>>>   Link: https://cwe.mitre.org/data/definitions/754.html
>>>
>>> * Return directly after a call of the function "kzalloc" failed
>>>   at the beginning.
>>
>> Both calls are already close together.
>
> Can it be that an other software development concern is eventually
> overlooked because of this "neighbourship" (or is categorised with
> a lower priority)?
>
> I suggest to reconsider this design detail if it is really acceptable
> for the safe implementation of such a software module.
>
> * How much will it matter in general that one function call was performed
>   in this use case without checking its return values immediately?
>
> * Should it usually be determined quicker if a required resource like
>   memory could be acquired before trying the next allocation?

Note that if memory allocation fails in this driver, the system won't
boot at all. So even not checking for allocation failures at all could be
acceptable.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection

[SF Markus Elfring]

>> * Should it usually be determined quicker if a required resource like
>>   memory could be acquired before trying the next allocation?
> 
> Note that if memory allocation fails in this driver, the system won't
> boot at all.

Thanks for this information.


> So even not checking for allocation failures at all could be acceptable.

I find this opinion interesting somehow.

I would generally prefer to check return values from various function calls
immediately instead of keeping the discussed source code structure unchanged.

Regards,
Markus

[PATCH 4/4] clk/Renesas-MSTP: Rename jump labels in cpg_mstp_attach_dev()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 21:41:50 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/clk/renesas/clk-mstp.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index 6c82e0e..2f90718 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -256,19 +256,18 @@ int cpg_mstp_attach_dev(struct generic_pm_domain *unused, struct device *dev)
 					   &clkspec)) {
 		if (of_device_is_compatible(clkspec.np,
 					    "renesas,cpg-mstp-clocks"))
-			goto found;
+			goto get_clk;
 
 		/* BSC on r8a73a4/sh73a0 uses zb_clk instead of an mstp clock */
 		if (!strcmp(clkspec.np->name, "zb_clk"))
-			goto found;
+			goto get_clk;
 
 		of_node_put(clkspec.np);
 		i++;
 	}
 
 	return 0;
-
-found:
+ get_clk:
 	clk = of_clk_get_from_provider(&clkspec);
 	of_node_put(clkspec.np);
 
@@ -278,20 +277,19 @@ found:
 	error = pm_clk_create(dev);
 	if (error) {
 		dev_err(dev, "pm_clk_create failed %d\n", error);
-		goto fail_put;
+		goto put_clk;
 	}
 
 	error = pm_clk_add_clk(dev, clk);
 	if (error) {
 		dev_err(dev, "pm_clk_add_clk %pC failed %d\n", clk, error);
-		goto fail_destroy;
+		goto destroy_clk;
 	}
 
 	return 0;
-
-fail_destroy:
+ destroy_clk:
 	pm_clk_destroy(dev);
-fail_put:
+ put_clk:
 	clk_put(clk);
 	return error;
 }
-- 
2.10.0

Re: [PATCH 4/4] clk/Renesas-MSTP: Rename jump labels in cpg_mstp_attach_dev()

[Geert Uytterhoeven]

Hi Markus,

On Wed, Sep 14, 2016 at 10:04 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 14 Sep 2016 21:41:50 +0200
>
> Adjust jump labels according to the current Linux coding style convention.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/clk/renesas/clk-mstp.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
> index 6c82e0e..2f90718 100644
> --- a/drivers/clk/renesas/clk-mstp.c
> +++ b/drivers/clk/renesas/clk-mstp.c
> @@ -256,19 +256,18 @@ int cpg_mstp_attach_dev(struct generic_pm_domain *unused, struct device *dev)
>                                            &clkspec)) {
>                 if (of_device_is_compatible(clkspec.np,
>                                             "renesas,cpg-mstp-clocks"))
> -                       goto found;
> +                       goto get_clk;
>
>                 /* BSC on r8a73a4/sh73a0 uses zb_clk instead of an mstp clock */
>                 if (!strcmp(clkspec.np->name, "zb_clk"))
> -                       goto found;
> +                       goto get_clk;
>
>                 of_node_put(clkspec.np);
>                 i++;
>         }
>
>         return 0;
> -
> -found:
> + get_clk:

"Choose label names which say what the goto does or why the goto exists."

I prefer the "why" over the "what".

And the "indent labels with a single space" rule will be removed soon, as
there's no longer a technical reason for it after
https://lkml.org/lkml/2016/9/7/316

>         clk = of_clk_get_from_provider(&clkspec);
>         of_node_put(clkspec.np);
>
> @@ -278,20 +277,19 @@ found:
>         error = pm_clk_create(dev);
>         if (error) {
>                 dev_err(dev, "pm_clk_create failed %d\n", error);
> -               goto fail_put;
> +               goto put_clk;
>         }
>
>         error = pm_clk_add_clk(dev, clk);
>         if (error) {
>                 dev_err(dev, "pm_clk_add_clk %pC failed %d\n", clk, error);
> -               goto fail_destroy;
> +               goto destroy_clk;
>         }
>
>         return 0;
> -
> -fail_destroy:
> + destroy_clk:
>         pm_clk_destroy(dev);
> -fail_put:
> + put_clk:

Same here.

>         clk_put(clk);
>         return error;
>  }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: clk/Renesas-MSTP: Rename jump labels in cpg_mstp_attach_dev()

[SF Markus Elfring]

> "Choose label names which say what the goto does or why the goto exists."
> 
> I prefer the "why" over the "what".

Does your opinion indicate also that you would appreciate another adjustment
around the quoted sentence from "Chapter 7: Centralized exiting of functions"
of the document "CodingStyle"?

Would you like to achieve that the potential for confusion will be reduced
a bit more there?


> And the "indent labels with a single space" rule will be removed soon,

I am unsure on how this "story" will evolve further.

Will the indentation rules become any more precise for Linux source code?


> as there's no longer a technical reason for it after
> https://lkml.org/lkml/2016/9/7/316

The software update "Set git diff driver for C source code files" is also
interesting for current versions.

Will language-specific rules which are supported by recent Git software
influence any capabilities for the command "diff --show-c-function"?

Regards,
Markus