clk: __clk_set_parent: set uninitialized variable

PATCH New Marc Kleine-Budde 4 participants 7 messages Jul 2, 2012 – Jul 3, 2012 Common clk

lore

Share & tools

lore

Marc Kleine-Budde

author

2012-07-02 07:41 UTC #0 lore raw

PATCH v2 1/1 clk: __clk_set_parent: set uninitialized variable

This patch fixes the following warning:

drivers/clk/clk.c: In function '__clk_set_parent':
drivers/clk/clk.c:1083:5: warning: 'i' may be used uninitialized in this function [-Wuninitialized]

which has been introduced with commit:

    commit 7975059db572eb47f0fb272a62afeae272a4b209
    Author: Rajendra Nayak <[email protected]>
    Date:   Wed Jun 6 14:41:31 2012 +0530

        clk: Allow late cache allocation for clk->parents

This patch applies to linux-3.5-rc5

Cc Rajendra Nayak <[email protected]>

Signed-off-by Marc Kleine-Budde <[email protected]>

---
Hello,

here an updated version. Changes since v1:
- Set i to clk->num_parents as Uwe pointed out.

regards, Marc

 drivers/clk/clk.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/clk/clk.c +4 -2

index dcbe056..9a75635 100644
@@ -1068,13 +1068,15 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
 	old_parent = clk->parent;
 
 	/* find index of new parent clock using cached parent ptrs */
-	if (clk->parents)
+	if (clk->parents) {
 		for (i = 0; i < clk->num_parents; i++)
 			if (clk->parents[i] == parent)
 				break;
-	else
+	} else {
+		i = clk->num_parents
 		clk->parents = kzalloc((sizeof(struct clk*) * clk->num_parents),
 								GFP_KERNEL);
+	}
 
 	/*
 	 * find index of new parent clock using string name comparison

Signature · 1 line

1.7.10

Rajendra Nayak

2012-07-02 08:42 UTC #1 lore raw

PATCH v2 1/1 clk: __clk_set_parent: set uninitialized variable

Quoted 15 lines · Marc Kleine-Budde

This patch fixes the following warning:
drivers/clk/clk.c: In function '__clk_set_parent':
drivers/clk/clk.c:1083:5: warning: 'i' may be used uninitialized in this function [-Wuninitialized]
which has been introduced with commit:
     commit 7975059db572eb47f0fb272a62afeae272a4b209
     Author: Rajendra Nayak<[email protected]>
     Date:   Wed Jun 6 14:41:31 2012 +0530

         clk: Allow late cache allocation for clk->parents
This patch applies to linux-3.5-rc5

Quoted 7 lines

Cc Rajendra Nayak<[email protected]>
Signed-off-by Marc Kleine-Budde<[email protected]>
---
Hello,
here an updated version. Changes since v1:
- Set i to clk->num_parents as Uwe pointed out.

I started looking at how to avoid this initing of i to clk->parents (which is correct for the logic used below, but somehow seems error prone if someone happens to change the logic without noticing the init part)
This is what I came up with, not tested at all, but worth considering if Mike dislikes the idea of initing i to clk->parents.

drivers/clk/clk.c +0 -0

index dcbe056..af737cc 100644
@@ -1067,26 +1067,23 @@ static int __clk_set_parent(struct clk *clk,

struct clk *parent)

         old_parent = clk->parent;

- /* find index of new parent clock using cached parent ptrs */
- if (clk->parents)
- for (i = 0; i < clk->num_parents; i++)
- if (clk->parents[i] == parent)
- break;
- else
+ if (!clk->parents)

                 clk->parents = kzalloc((sizeof(struct clk*) * 
clk->num_parents), 
     GFP_KERNEL);
-
         /*
-        * find index of new parent clock using string name comparison
-        * also try to cache the parent to avoid future calls to 
__clk_lookup
+        * find index of new parent clock using cached parent ptrs,
+        * or if not yet cached, use string name comparison and cache
+        * them now to avoid future calls to __clk_lookup.
          */

- if (i == clk->num_parents)
- for (i = 0; i < clk->num_parents; i++)
- if (!strcmp(clk->parent_names[i], parent->name)) {
- if (clk->parents)
- clk->parents[i] =
__clk_lookup(parent->name);
- break;
- }
+ for (i = 0; i < clk->num_parents; i++) {
+ if (clk->parents && clk->parents[i] == parent)
+ break;
+ else if (!strcmp(clk->parent_names[i], parent->name)) {
+ if (clk->parents)
+ clk->parents[i] =
__clk_lookup(parent->name);
+ break;
+ }
+ }

if (i == clk->num_parents) {
pr_debug("%s: clock %s is not a possible parent of
clock %s\n",

regards,
Rajendra

Quoted 28 lines

regards, Marc

  drivers/clk/clk.c |    6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

drivers/clk/clk.c +4 -2

index dcbe056..9a75635 100644
@@ -1068,13 +1068,15 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
  	old_parent = clk->parent;
 
  	/* find index of new parent clock using cached parent ptrs */
-	if (clk->parents)
+	if (clk->parents) {
  		for (i = 0; i<  clk->num_parents; i++)
  			if (clk->parents[i] == parent)
  				break;
-	else
+	} else {
+		i = clk->num_parents
  		clk->parents = kzalloc((sizeof(struct clk*) * clk->num_parents),
  								GFP_KERNEL);
+	}
 
  	/*
  	 * find index of new parent clock using string name comparison

Rajendra Nayak

2012-07-02 08:45 UTC #2 lore raw

PATCH v2 1/1 clk: __clk_set_parent: set uninitialized variable

Quoted 8 lines · Rajendra Nayak

I started looking at how to avoid this initing of i to clk->parents (which is correct for the logic used below, but somehow seems error prone if someone happens to change the logic without noticing the init part)
This is what I came up with, not tested at all, but worth considering if Mike dislikes the idea of initing i to clk->parents.

s/clk->parents/clk->num_parents

Turquette, Mike

2012-07-02 23:29 UTC #3 lore raw

PATCH v2 1/1 clk: __clk_set_parent: set uninitialized variable

In reply to Rajendra Nayak

Quoted 27 lines · Rajendra Nayak

Quoted 17 lines · Marc Kleine-Budde
This patch fixes the following warning:
? ? ?drivers/clk/clk.c: In function '__clk_set_parent':
? ? ?drivers/clk/clk.c:1083:5: warning: 'i' may be used uninitialized in this function [-Wuninitialized]
which has been introduced with commit:
? ? ?commit 7975059db572eb47f0fb272a62afeae272a4b209 ? ? ?Author: Rajendra Nayak<[email protected]>
? ? ?Date: ? Wed Jun 6 14:41:31 2012 +0530
? ? ? ? ?clk: Allow late cache allocation for clk->parents
This patch applies to linux-3.5-rc5
Quoted 7 lines
Cc Rajendra Nayak<[email protected]>
Signed-off-by Marc Kleine-Budde<[email protected]>
---
Hello,
here an updated version. Changes since v1:
- Set i to clk->num_parents as Uwe pointed out.

Quoted 9 lines

I started looking at how to avoid this initing of i to clk->parents (which is correct for the logic used below, but somehow seems error prone if someone happens to change the logic without noticing the init part)
This is what I came up with, not tested at all, but worth considering if Mike dislikes the idea of initing i to clk->parents.

Hi Rajendra and Marc,

I prefer the code flow in Rajendra's change. It seems more readable and has a negative diffstat ;-)

$ git diff --stat
 drivers/clk/clk.c |   28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

Rajendra, can you test and send a proper patch for the same? Thanks Marc for sending your two previous patches. I don't think that I will Cc this one to stable since it falls under the category of "theoretical but not yet observed" bugs.

Thanks again,
Mike

Quoted 93 lines

drivers/clk/clk.c +1 -6
index dcbe056..af737cc 100644
@@ -1067,26 +1067,23 @@ static int __clk_set_parent(struct clk *clk, struct
 clk *parent)
 
 ? ? ? ? old_parent = clk->parent;
 
- ? ? ? /* find index of new parent clock using cached parent ptrs */
- ? ? ? if (clk->parents)
- ? ? ? ? ? ? ? for (i = 0; i < clk->num_parents; i++)
- ? ? ? ? ? ? ? ? ? ? ? if (clk->parents[i] == parent)
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
- ? ? ? else
+ ? ? ? if (!clk->parents)
 
? ? ? ? ? ? ? ? clk->parents = kzalloc((sizeof(struct clk*) * clk->num_parents), ? ? GFP_KERNEL);
-
? ? ? ? /*
- ? ? ? ?* find index of new parent clock using string name comparison
- ? ? ? ?* also try to cache the parent to avoid future calls to __clk_lookup
+ ? ? ? ?* find index of new parent clock using cached parent ptrs,
+ ? ? ? ?* or if not yet cached, use string name comparison and cache
+ ? ? ? ?* them now to avoid future calls to __clk_lookup.
? ? ? ? ?*/
- ? ? ? if (i == clk->num_parents)
- ? ? ? ? ? ? ? for (i = 0; i < clk->num_parents; i++)
- ? ? ? ? ? ? ? ? ? ? ? if (!strcmp(clk->parent_names[i], parent->name)) {
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (clk->parents)
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? clk->parents[i] = __clk_lookup(parent->name);
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
- ? ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? for (i = 0; i < clk->num_parents; i++) {
+ ? ? ? ? ? ? ? if (clk->parents && clk->parents[i] == parent)
+ ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? else if (!strcmp(clk->parent_names[i], parent->name)) {
+ ? ? ? ? ? ? ? ? ? ? ? if (clk->parents)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? clk->parents[i] = __clk_lookup(parent->name);
+ ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? }
+ ? ? ? }
? ? ? ? if (i == clk->num_parents) {
? ? ? ? ? ? ? ? pr_debug("%s: clock %s is not a possible parent of clock %s\n",
regards,
Rajendra
Quoted 26 lines
regards, Marc
? drivers/clk/clk.c | ? ?6 ++++--
? 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/clk/clk.c +0 -0
index dcbe056..9a75635 100644
@@ -1068,13 +1068,15 @@ static int __clk_set_parent(struct clk *clk,
 struct clk *parent)
 ? ? ? ? old_parent = clk->parent;
 
? ? ? ? /* find index of new parent clock using cached parent ptrs */
- ? ? ? if (clk->parents)
+ ? ? ? if (clk->parents) {
? ? ? ? ? ? ? ? for (i = 0; i< ?clk->num_parents; i++)
? ? ? ? ? ? ? ? ? ? ? ? if (clk->parents[i] == parent)
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
- ? ? ? else
+ ? ? ? } else {
+ ? ? ? ? ? ? ? i = clk->num_parents
? ? ? ? ? ? ? ? clk->parents = kzalloc((sizeof(struct clk*) * clk->num_parents),
Quoted 5 lines
GFP_KERNEL);
+ ? ? ? }
? ? ? ? /*
? ? ? ? ?* find index of new parent clock using string name comparison
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Rajendra Nayak

2012-07-03 10:29 UTC #4 lore raw

PATCH v2 1/1 clk: __clk_set_parent: set uninitialized variable

Quoted 10 lines · Mike

Hi Rajendra and Marc,
I prefer the code flow in Rajendra's change. It seems more readable and has a negative diffstat;-)
$ git diff --stat
  drivers/clk/clk.c |   28 +++++++++++++---------------
  1 file changed, 13 insertions(+), 15 deletions(-)
Rajendra, can you test and send a proper patch for the same?

Sure, will do.

Uwe Kleine-König

2012-07-03 10:50 UTC #5 lore raw

PATCH v2 1/1 clk: __clk_set_parent: set uninitialized variable

In reply to Turquette, Mike

Hello,

Quoted 43 lines · Mike

Quoted 27 lines · Rajendra Nayak
Quoted 17 lines · Marc Kleine-Budde
This patch fixes the following warning:
? ? ?drivers/clk/clk.c: In function '__clk_set_parent':
? ? ?drivers/clk/clk.c:1083:5: warning: 'i' may be used uninitialized in this function [-Wuninitialized]
which has been introduced with commit:
? ? ?commit 7975059db572eb47f0fb272a62afeae272a4b209 ? ? ?Author: Rajendra Nayak<[email protected]>
? ? ?Date: ? Wed Jun 6 14:41:31 2012 +0530
? ? ? ? ?clk: Allow late cache allocation for clk->parents
This patch applies to linux-3.5-rc5
Quoted 7 lines
Cc Rajendra Nayak<[email protected]>
Signed-off-by Marc Kleine-Budde<[email protected]>
---
Hello,
here an updated version. Changes since v1:
- Set i to clk->num_parents as Uwe pointed out.
Quoted 9 lines
I started looking at how to avoid this initing of i to clk->parents (which is correct for the logic used below, but somehow seems error prone if someone happens to change the logic without noticing the init part)
This is what I came up with, not tested at all, but worth considering if Mike dislikes the idea of initing i to clk->parents.
Hi Rajendra and Marc,
I prefer the code flow in Rajendra's change. It seems more readable and has a negative diffstat ;-)

Quoted 8 lines

$ git diff --stat
 drivers/clk/clk.c |   28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)
Rajendra, can you test and send a proper patch for the same? Thanks Marc for sending your two previous patches. I don't think that I will Cc this one to stable since it falls under the category of "theoretical but not yet observed" bugs.

Maybe it's not observed yet only because 7975059db572eb47f0fb272a62afeae272a4b209 isn't deeply tested yet? Don't know, just a guess.

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Mike Turquette

2012-07-03 23:30 UTC #6 lore raw

PATCH v2 1/1 clk: __clk_set_parent: set uninitialized variable

Quoted 50 lines · Uwe Kleine-K?nig

Hello,
Quoted 47 lines · Mike
Quoted 27 lines · Rajendra Nayak
Quoted 17 lines · Marc Kleine-Budde
This patch fixes the following warning:
? ? ?drivers/clk/clk.c: In function '__clk_set_parent':
? ? ?drivers/clk/clk.c:1083:5: warning: 'i' may be used uninitialized in this function [-Wuninitialized]
which has been introduced with commit:
? ? ?commit 7975059db572eb47f0fb272a62afeae272a4b209 ? ? ?Author: Rajendra Nayak<[email protected]>
? ? ?Date: ? Wed Jun 6 14:41:31 2012 +0530
? ? ? ? ?clk: Allow late cache allocation for clk->parents
This patch applies to linux-3.5-rc5
Quoted 7 lines
Cc Rajendra Nayak<[email protected]>
Signed-off-by Marc Kleine-Budde<[email protected]>
---
Hello,
here an updated version. Changes since v1:
- Set i to clk->num_parents as Uwe pointed out.
Quoted 9 lines
I started looking at how to avoid this initing of i to clk->parents (which is correct for the logic used below, but somehow seems error prone if someone happens to change the logic without noticing the init part)
This is what I came up with, not tested at all, but worth considering if Mike dislikes the idea of initing i to clk->parents.
Hi Rajendra and Marc,
I prefer the code flow in Rajendra's change. It seems more readable and has a negative diffstat ;-)
$ git diff --stat
 drivers/clk/clk.c |   28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

Quoted 8 lines

Quoted 4 lines
Rajendra, can you test and send a proper patch for the same? Thanks Marc for sending your two previous patches. I don't think that I will Cc this one to stable since it falls under the category of "theoretical but not yet observed" bugs.
Maybe it's not observed yet only because 7975059db572eb47f0fb272a62afeae272a4b209 isn't deeply tested yet? Don't know, just a guess.

Fair enough. I Cc'd stable on that patch and copied you on the fixes request to Linus for 3.5-rc6.

Thanks,
Mike

Quoted 6 lines

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |