* [PATCH v2] clk: __clk_set_parent: set uninitialized variable
@ 2012-07-02 7:41 Marc Kleine-Budde
2012-07-02 8:42 ` Rajendra Nayak
0 siblings, 1 reply; 7+ messages in thread
From: Marc Kleine-Budde @ 2012-07-02 7:41 UTC (permalink / raw)
To: linux-arm-kernel
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 <rnayak@ti.com>
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 <rnayak@ti.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
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(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index dcbe056..9a75635 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -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
--
1.7.10
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2] clk: __clk_set_parent: set uninitialized variable
2012-07-02 7:41 [PATCH v2] clk: __clk_set_parent: set uninitialized variable Marc Kleine-Budde
@ 2012-07-02 8:42 ` Rajendra Nayak
2012-07-02 8:45 ` Rajendra Nayak
2012-07-02 23:29 ` Turquette, Mike
0 siblings, 2 replies; 7+ messages in thread
From: Rajendra Nayak @ 2012-07-02 8:42 UTC (permalink / raw)
To: linux-arm-kernel
On Monday 02 July 2012 01:11 PM, Marc Kleine-Budde wrote:
> 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<rnayak@ti.com>
> 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<rnayak@ti.com>
> Signed-off-by: Marc Kleine-Budde<mkl@pengutronix.de>
> ---
> 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.
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index dcbe056..af737cc 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -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
>
> regards, Marc
>
> drivers/clk/clk.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index dcbe056..9a75635 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2] clk: __clk_set_parent: set uninitialized variable
2012-07-02 8:42 ` Rajendra Nayak
@ 2012-07-02 8:45 ` Rajendra Nayak
2012-07-02 23:29 ` Turquette, Mike
1 sibling, 0 replies; 7+ messages in thread
From: Rajendra Nayak @ 2012-07-02 8:45 UTC (permalink / raw)
To: linux-arm-kernel
On Monday 02 July 2012 02:12 PM, Rajendra Nayak wrote:
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] clk: __clk_set_parent: set uninitialized variable
2012-07-02 8:42 ` Rajendra Nayak
2012-07-02 8:45 ` Rajendra Nayak
@ 2012-07-02 23:29 ` Turquette, Mike
2012-07-03 10:29 ` Rajendra Nayak
2012-07-03 10:50 ` Uwe Kleine-König
1 sibling, 2 replies; 7+ messages in thread
From: Turquette, Mike @ 2012-07-02 23:29 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 2, 2012 at 1:42 AM, Rajendra Nayak <rnayak@ti.com> wrote:
> On Monday 02 July 2012 01:11 PM, Marc Kleine-Budde wrote:
>>
>> 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<rnayak@ti.com>
>> ? ? ?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<rnayak@ti.com>
>> Signed-off-by: Marc Kleine-Budde<mkl@pengutronix.de>
>> ---
>> 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.
>
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
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index dcbe056..af737cc 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -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
>
>
>>
>> regards, Marc
>>
>> ? drivers/clk/clk.c | ? ?6 ++++--
>> ? 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index dcbe056..9a75635 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -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
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] clk: __clk_set_parent: set uninitialized variable
2012-07-02 23:29 ` Turquette, Mike
@ 2012-07-03 10:29 ` Rajendra Nayak
2012-07-03 10:50 ` Uwe Kleine-König
1 sibling, 0 replies; 7+ messages in thread
From: Rajendra Nayak @ 2012-07-03 10:29 UTC (permalink / raw)
To: linux-arm-kernel
On Tuesday 03 July 2012 04:59 AM, Turquette, Mike wrote:
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] clk: __clk_set_parent: set uninitialized variable
2012-07-02 23:29 ` Turquette, Mike
2012-07-03 10:29 ` Rajendra Nayak
@ 2012-07-03 10:50 ` Uwe Kleine-König
2012-07-03 23:30 ` Mike Turquette
1 sibling, 1 reply; 7+ messages in thread
From: Uwe Kleine-König @ 2012-07-03 10:50 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On Mon, Jul 02, 2012 at 04:29:10PM -0700, Turquette, Mike wrote:
> On Mon, Jul 2, 2012 at 1:42 AM, Rajendra Nayak <rnayak@ti.com> wrote:
> > On Monday 02 July 2012 01:11 PM, Marc Kleine-Budde wrote:
> >>
> >> 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<rnayak@ti.com>
> >> ? ? ?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<rnayak@ti.com>
> >> Signed-off-by: Marc Kleine-Budde<mkl@pengutronix.de>
> >> ---
> >> 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.
> >
>
> 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.
Maybe it's not observed yet only because
7975059db572eb47f0fb272a62afeae272a4b209 isn't deeply tested yet? Don't
know, just a guess.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] clk: __clk_set_parent: set uninitialized variable
2012-07-03 10:50 ` Uwe Kleine-König
@ 2012-07-03 23:30 ` Mike Turquette
0 siblings, 0 replies; 7+ messages in thread
From: Mike Turquette @ 2012-07-03 23:30 UTC (permalink / raw)
To: linux-arm-kernel
On 20120703-12:50, Uwe Kleine-K?nig wrote:
> Hello,
>
> On Mon, Jul 02, 2012 at 04:29:10PM -0700, Turquette, Mike wrote:
> > On Mon, Jul 2, 2012 at 1:42 AM, Rajendra Nayak <rnayak@ti.com> wrote:
> > > On Monday 02 July 2012 01:11 PM, Marc Kleine-Budde wrote:
> > >>
> > >> 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<rnayak@ti.com>
> > >> ? ? ?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<rnayak@ti.com>
> > >> Signed-off-by: Marc Kleine-Budde<mkl@pengutronix.de>
> > >> ---
> > >> 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.
> > >
> >
> > 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.
> 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
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-07-03 23:30 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-02 7:41 [PATCH v2] clk: __clk_set_parent: set uninitialized variable Marc Kleine-Budde
2012-07-02 8:42 ` Rajendra Nayak
2012-07-02 8:45 ` Rajendra Nayak
2012-07-02 23:29 ` Turquette, Mike
2012-07-03 10:29 ` Rajendra Nayak
2012-07-03 10:50 ` Uwe Kleine-König
2012-07-03 23:30 ` Mike Turquette
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).