* [PATCH 0/4] clk/Renesas-MSTP: Fine-tuning for two function implementations
[not found] <566ABCD9.1060404@users.sourceforge.net>
@ 2016-09-14 19:56 ` SF Markus Elfring
2016-09-14 20:00 ` [PATCH 1/4] clk/Renesas-MSTP: Use kmalloc_array() in cpg_mstp_clocks_init() SF Markus Elfring
` (3 more replies)
0 siblings, 4 replies; 21+ messages in thread
From: SF Markus Elfring @ 2016-09-14 19:56 UTC (permalink / raw)
To: linux-clk, Geert Uytterhoeven, Laurent Pinchart,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson
Cc: LKML, kernel-janitors, Julia Lawall
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/4] clk/Renesas-MSTP: Use kmalloc_array() in cpg_mstp_clocks_init()
2016-09-14 19:56 ` [PATCH 0/4] clk/Renesas-MSTP: Fine-tuning for two function implementations SF Markus Elfring
@ 2016-09-14 20:00 ` SF Markus Elfring
2016-09-15 19:11 ` Geert Uytterhoeven
2016-09-16 23:13 ` Stephen Boyd
2016-09-14 20:01 ` [PATCH 2/4] clk/Renesas-MSTP: Delete an error message for a failed memory allocation SF Markus Elfring
` (2 subsequent siblings)
3 siblings, 2 replies; 21+ messages in thread
From: SF Markus Elfring @ 2016-09-14 20:00 UTC (permalink / raw)
To: linux-clk, Geert Uytterhoeven, Laurent Pinchart,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson
Cc: LKML, kernel-janitors, Julia Lawall
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/4] clk/Renesas-MSTP: Delete an error message for a failed memory allocation
2016-09-14 19:56 ` [PATCH 0/4] clk/Renesas-MSTP: Fine-tuning for two function implementations SF Markus Elfring
2016-09-14 20:00 ` [PATCH 1/4] clk/Renesas-MSTP: Use kmalloc_array() in cpg_mstp_clocks_init() SF Markus Elfring
@ 2016-09-14 20:01 ` SF Markus Elfring
2016-09-15 19:07 ` Geert Uytterhoeven
2016-09-14 20:03 ` [PATCH 3/4] clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection SF Markus Elfring
2016-09-14 20:04 ` [PATCH 4/4] clk/Renesas-MSTP: Rename jump labels in cpg_mstp_attach_dev() SF Markus Elfring
3 siblings, 1 reply; 21+ messages in thread
From: SF Markus Elfring @ 2016-09-14 20:01 UTC (permalink / raw)
To: linux-clk, Geert Uytterhoeven, Laurent Pinchart,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson
Cc: LKML, kernel-janitors, Julia Lawall, Wolfram Sang
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 3/4] clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection
2016-09-14 19:56 ` [PATCH 0/4] clk/Renesas-MSTP: Fine-tuning for two function implementations SF Markus Elfring
2016-09-14 20:00 ` [PATCH 1/4] clk/Renesas-MSTP: Use kmalloc_array() in cpg_mstp_clocks_init() SF Markus Elfring
2016-09-14 20:01 ` [PATCH 2/4] clk/Renesas-MSTP: Delete an error message for a failed memory allocation SF Markus Elfring
@ 2016-09-14 20:03 ` SF Markus Elfring
2016-09-15 19:11 ` Geert Uytterhoeven
2016-09-14 20:04 ` [PATCH 4/4] clk/Renesas-MSTP: Rename jump labels in cpg_mstp_attach_dev() SF Markus Elfring
3 siblings, 1 reply; 21+ messages in thread
From: SF Markus Elfring @ 2016-09-14 20:03 UTC (permalink / raw)
To: linux-clk, Geert Uytterhoeven, Laurent Pinchart,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson
Cc: LKML, kernel-janitors, Julia Lawall
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 4/4] clk/Renesas-MSTP: Rename jump labels in cpg_mstp_attach_dev()
2016-09-14 19:56 ` [PATCH 0/4] clk/Renesas-MSTP: Fine-tuning for two function implementations SF Markus Elfring
` (2 preceding siblings ...)
2016-09-14 20:03 ` [PATCH 3/4] clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection SF Markus Elfring
@ 2016-09-14 20:04 ` SF Markus Elfring
2016-09-15 19:18 ` Geert Uytterhoeven
3 siblings, 1 reply; 21+ messages in thread
From: SF Markus Elfring @ 2016-09-14 20:04 UTC (permalink / raw)
To: linux-clk, Geert Uytterhoeven, Laurent Pinchart,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson
Cc: LKML, kernel-janitors, Julia Lawall
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/4] clk/Renesas-MSTP: Delete an error message for a failed memory allocation
2016-09-14 20:01 ` [PATCH 2/4] clk/Renesas-MSTP: Delete an error message for a failed memory allocation SF Markus Elfring
@ 2016-09-15 19:07 ` Geert Uytterhoeven
2016-09-15 19:13 ` Laurent Pinchart
0 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2016-09-15 19:07 UTC (permalink / raw)
To: SF Markus Elfring
Cc: linux-clk, Geert Uytterhoeven, Laurent Pinchart,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson, LKML,
kernel-janitors@vger.kernel.org, Julia Lawall, Wolfram Sang
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection
2016-09-14 20:03 ` [PATCH 3/4] clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection SF Markus Elfring
@ 2016-09-15 19:11 ` Geert Uytterhoeven
2016-09-15 20:40 ` SF Markus Elfring
0 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2016-09-15 19:11 UTC (permalink / raw)
To: SF Markus Elfring
Cc: linux-clk, Geert Uytterhoeven, Laurent Pinchart,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson, LKML,
kernel-janitors@vger.kernel.org, Julia Lawall
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] clk/Renesas-MSTP: Use kmalloc_array() in cpg_mstp_clocks_init()
2016-09-14 20:00 ` [PATCH 1/4] clk/Renesas-MSTP: Use kmalloc_array() in cpg_mstp_clocks_init() SF Markus Elfring
@ 2016-09-15 19:11 ` Geert Uytterhoeven
2016-09-16 23:13 ` Stephen Boyd
1 sibling, 0 replies; 21+ messages in thread
From: Geert Uytterhoeven @ 2016-09-15 19:11 UTC (permalink / raw)
To: SF Markus Elfring
Cc: linux-clk, Geert Uytterhoeven, Laurent Pinchart,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson, LKML,
kernel-janitors@vger.kernel.org, Julia Lawall
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/4] clk/Renesas-MSTP: Delete an error message for a failed memory allocation
2016-09-15 19:07 ` Geert Uytterhoeven
@ 2016-09-15 19:13 ` Laurent Pinchart
2016-09-15 19:41 ` Wolfram Sang
2016-09-15 20:17 ` SF Markus Elfring
0 siblings, 2 replies; 21+ messages in thread
From: Laurent Pinchart @ 2016-09-15 19:13 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: SF Markus Elfring, linux-clk, Geert Uytterhoeven,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson, LKML,
kernel-janitors@vger.kernel.org, Julia Lawall, Wolfram Sang
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] clk/Renesas-MSTP: Rename jump labels in cpg_mstp_attach_dev()
2016-09-14 20:04 ` [PATCH 4/4] clk/Renesas-MSTP: Rename jump labels in cpg_mstp_attach_dev() SF Markus Elfring
@ 2016-09-15 19:18 ` Geert Uytterhoeven
2016-09-16 6:00 ` SF Markus Elfring
0 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2016-09-15 19:18 UTC (permalink / raw)
To: SF Markus Elfring
Cc: linux-clk, Geert Uytterhoeven, Laurent Pinchart,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson, LKML,
kernel-janitors@vger.kernel.org, Julia Lawall
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/4] clk/Renesas-MSTP: Delete an error message for a failed memory allocation
2016-09-15 19:13 ` Laurent Pinchart
@ 2016-09-15 19:41 ` Wolfram Sang
2016-09-15 20:17 ` SF Markus Elfring
1 sibling, 0 replies; 21+ messages in thread
From: Wolfram Sang @ 2016-09-15 19:41 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Geert Uytterhoeven, SF Markus Elfring, linux-clk,
Geert Uytterhoeven, Michael Turquette, Simon Horman, Stephen Boyd,
Ulf Hansson, LKML, kernel-janitors@vger.kernel.org, Julia Lawall
[-- 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 --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: clk/Renesas-MSTP: Delete an error message for a failed memory allocation
2016-09-15 19:13 ` Laurent Pinchart
2016-09-15 19:41 ` Wolfram Sang
@ 2016-09-15 20:17 ` SF Markus Elfring
2016-09-15 22:55 ` Laurent Pinchart
1 sibling, 1 reply; 21+ messages in thread
From: SF Markus Elfring @ 2016-09-15 20:17 UTC (permalink / raw)
To: Laurent Pinchart, Geert Uytterhoeven
Cc: linux-clk, Geert Uytterhoeven, Michael Turquette, Simon Horman,
Stephen Boyd, Ulf Hansson, LKML, kernel-janitors@vger.kernel.org,
Julia Lawall, Wolfram Sang
>>> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection
2016-09-15 19:11 ` Geert Uytterhoeven
@ 2016-09-15 20:40 ` SF Markus Elfring
2016-09-15 20:48 ` Geert Uytterhoeven
0 siblings, 1 reply; 21+ messages in thread
From: SF Markus Elfring @ 2016-09-15 20:40 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-clk, Geert Uytterhoeven, Laurent Pinchart,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson, LKML,
kernel-janitors@vger.kernel.org, Julia Lawall
> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection
2016-09-15 20:40 ` SF Markus Elfring
@ 2016-09-15 20:48 ` Geert Uytterhoeven
2016-09-16 5:32 ` SF Markus Elfring
0 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2016-09-15 20:48 UTC (permalink / raw)
To: SF Markus Elfring
Cc: linux-clk, Geert Uytterhoeven, Laurent Pinchart,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson, LKML,
kernel-janitors@vger.kernel.org, Julia Lawall
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: clk/Renesas-MSTP: Delete an error message for a failed memory allocation
2016-09-15 20:17 ` SF Markus Elfring
@ 2016-09-15 22:55 ` Laurent Pinchart
2016-09-16 5:21 ` SF Markus Elfring
2017-09-25 8:34 ` [PATCH] clk/Renesas: Delete an error message for a failed memory allocation in three functions SF Markus Elfring
0 siblings, 2 replies; 21+ messages in thread
From: Laurent Pinchart @ 2016-09-15 22:55 UTC (permalink / raw)
To: SF Markus Elfring
Cc: Geert Uytterhoeven, linux-clk, Geert Uytterhoeven,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson, LKML,
kernel-janitors@vger.kernel.org, Julia Lawall, Wolfram Sang
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: clk/Renesas-MSTP: Delete an error message for a failed memory allocation
2016-09-15 22:55 ` Laurent Pinchart
@ 2016-09-16 5:21 ` SF Markus Elfring
2017-09-25 8:34 ` [PATCH] clk/Renesas: Delete an error message for a failed memory allocation in three functions SF Markus Elfring
1 sibling, 0 replies; 21+ messages in thread
From: SF Markus Elfring @ 2016-09-16 5:21 UTC (permalink / raw)
To: Laurent Pinchart, Wolfram Sang
Cc: Geert Uytterhoeven, linux-clk, Geert Uytterhoeven,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson, LKML,
kernel-janitors@vger.kernel.org, Julia Lawall
>> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection
2016-09-15 20:48 ` Geert Uytterhoeven
@ 2016-09-16 5:32 ` SF Markus Elfring
0 siblings, 0 replies; 21+ messages in thread
From: SF Markus Elfring @ 2016-09-16 5:32 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-clk, Geert Uytterhoeven, Laurent Pinchart,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson, LKML,
kernel-janitors@vger.kernel.org, Julia Lawall
>> * 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: clk/Renesas-MSTP: Rename jump labels in cpg_mstp_attach_dev()
2016-09-15 19:18 ` Geert Uytterhoeven
@ 2016-09-16 6:00 ` SF Markus Elfring
0 siblings, 0 replies; 21+ messages in thread
From: SF Markus Elfring @ 2016-09-16 6:00 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-clk, Geert Uytterhoeven, Laurent Pinchart,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson, LKML,
kernel-janitors@vger.kernel.org, Julia Lawall, linux-doc
> "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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] clk/Renesas-MSTP: Use kmalloc_array() in cpg_mstp_clocks_init()
2016-09-14 20:00 ` [PATCH 1/4] clk/Renesas-MSTP: Use kmalloc_array() in cpg_mstp_clocks_init() SF Markus Elfring
2016-09-15 19:11 ` Geert Uytterhoeven
@ 2016-09-16 23:13 ` Stephen Boyd
1 sibling, 0 replies; 21+ messages in thread
From: Stephen Boyd @ 2016-09-16 23:13 UTC (permalink / raw)
To: SF Markus Elfring
Cc: linux-clk, Geert Uytterhoeven, Laurent Pinchart,
Michael Turquette, Simon Horman, Ulf Hansson, LKML,
kernel-janitors, Julia Lawall
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] clk/Renesas: Delete an error message for a failed memory allocation in three functions
2016-09-15 22:55 ` Laurent Pinchart
2016-09-16 5:21 ` SF Markus Elfring
@ 2017-09-25 8:34 ` SF Markus Elfring
2017-09-25 10:02 ` Geert Uytterhoeven
1 sibling, 1 reply; 21+ messages in thread
From: SF Markus Elfring @ 2017-09-25 8:34 UTC (permalink / raw)
To: linux-clk, linux-renesas-soc, Geert Uytterhoeven,
Laurent Pinchart, Michael Turquette, Simon Horman, Stephen Boyd,
Ulf Hansson
Cc: LKML, kernel-janitors, Wolfram Sang
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] clk/Renesas: Delete an error message for a failed memory allocation in three functions
2017-09-25 8:34 ` [PATCH] clk/Renesas: Delete an error message for a failed memory allocation in three functions SF Markus Elfring
@ 2017-09-25 10:02 ` Geert Uytterhoeven
0 siblings, 0 replies; 21+ messages in thread
From: Geert Uytterhoeven @ 2017-09-25 10:02 UTC (permalink / raw)
To: SF Markus Elfring
Cc: linux-clk, Linux-Renesas, Geert Uytterhoeven, Laurent Pinchart,
Michael Turquette, Simon Horman, Stephen Boyd, Ulf Hansson, LKML,
kernel-janitors@vger.kernel.org, Wolfram Sang
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
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-09-25 10:02 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <566ABCD9.1060404@users.sourceforge.net>
2016-09-14 19:56 ` [PATCH 0/4] clk/Renesas-MSTP: Fine-tuning for two function implementations SF Markus Elfring
2016-09-14 20:00 ` [PATCH 1/4] clk/Renesas-MSTP: Use kmalloc_array() in cpg_mstp_clocks_init() SF Markus Elfring
2016-09-15 19:11 ` Geert Uytterhoeven
2016-09-16 23:13 ` Stephen Boyd
2016-09-14 20:01 ` [PATCH 2/4] clk/Renesas-MSTP: Delete an error message for a failed memory allocation SF Markus Elfring
2016-09-15 19:07 ` Geert Uytterhoeven
2016-09-15 19:13 ` Laurent Pinchart
2016-09-15 19:41 ` Wolfram Sang
2016-09-15 20:17 ` SF Markus Elfring
2016-09-15 22:55 ` Laurent Pinchart
2016-09-16 5:21 ` SF Markus Elfring
2017-09-25 8:34 ` [PATCH] clk/Renesas: Delete an error message for a failed memory allocation in three functions SF Markus Elfring
2017-09-25 10:02 ` Geert Uytterhoeven
2016-09-14 20:03 ` [PATCH 3/4] clk/Renesas-MSTP: Less function calls in cpg_mstp_clocks_init() after error detection SF Markus Elfring
2016-09-15 19:11 ` Geert Uytterhoeven
2016-09-15 20:40 ` SF Markus Elfring
2016-09-15 20:48 ` Geert Uytterhoeven
2016-09-16 5:32 ` SF Markus Elfring
2016-09-14 20:04 ` [PATCH 4/4] clk/Renesas-MSTP: Rename jump labels in cpg_mstp_attach_dev() SF Markus Elfring
2016-09-15 19:18 ` Geert Uytterhoeven
2016-09-16 6:00 ` SF Markus Elfring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).