Message ID | 1572938372-7006-1-git-send-email-peng.fan@nxp.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | clk: imx: pll14xx: initialize flags to 0 | expand |
On 05/11/2019 08:21, Peng Fan wrote: > From: Peng Fan <peng.fan@nxp.com> > > init.flags is initialized with value from pll_clk->flags, however > imx_1443x_pll and imx_1416x_pll are not static structure, so flags > might be random value. So let's initialize flags as 0 now. This is incorrect. When using an initializer list, struct members not explicitly specified are initialized to 0. https://port70.net/~nsz/c/c11/n1570.html#6.7.9p19 > The initialization shall occur in initializer list order, each > initializer provided for a particular subobject overriding any > previously listed initializer for the same subobject; all > subobjects that are not initialized explicitly shall be initialized > implicitly the same as objects that have static storage duration. (You might point out that the kernel is compiled with -std=gnu89 not C11, but GCC's semantics are the same.) https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html#Designated-Inits > diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c > index fa76e04251c4..a7f1c1abe664 100644 > --- a/drivers/clk/imx/clk-pll14xx.c > +++ b/drivers/clk/imx/clk-pll14xx.c > @@ -65,12 +65,14 @@ struct imx_pll14xx_clk imx_1443x_pll = { > .type = PLL_1443X, > .rate_table = imx_pll1443x_tbl, > .rate_count = ARRAY_SIZE(imx_pll1443x_tbl), > + .flags = 0, > }; What tree is this patch based on? https://elixir.bootlin.com/linux/v5.4-rc1/source/drivers/clk/imx/clk-pll14xx.c#L65 Regards.
> Subject: Re: [PATCH] clk: imx: pll14xx: initialize flags to 0 > > On 05/11/2019 08:21, Peng Fan wrote: > > > From: Peng Fan <peng.fan@nxp.com> > > > > init.flags is initialized with value from pll_clk->flags, however > > imx_1443x_pll and imx_1416x_pll are not static structure, so flags > > might be random value. So let's initialize flags as 0 now. > > This is incorrect. When using an initializer list, struct members not explicitly > specified are initialized to 0. > > https://eur01.safelinks.protection.outlook.com/?url=https:%2F%2Fport70.net > %2F~nsz%2Fc%2Fc11%2Fn1570.html%236.7.9p19&data=02%7C01%7C > peng.fan%40nxp.com%7Ca94356700d9c4f61614a08d761d53936%7C686ea1 > d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637085440717516996&s > data=cUQmXzdbaK6oZqWtJc3Vnh0FJBD1v1b%2FJqb4lRPis5w%3D&rese > rved=0 > > > The initialization shall occur in initializer list order, each > > initializer provided for a particular subobject overriding any > > previously listed initializer for the same subobject; all subobjects > > that are not initialized explicitly shall be initialized implicitly > > the same as objects that have static storage duration. > (You might point out that the kernel is compiled with -std=gnu89 not C11, but > GCC's semantics are the same.) > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgcc.gn > u.org%2Fonlinedocs%2Fgcc%2FDesignated-Inits.html%23Designated-Inits&a > mp;data=02%7C01%7Cpeng.fan%40nxp.com%7Ca94356700d9c4f61614a08d > 761d53936%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C637085 > 440717516996&sdata=nsiLE%2B91lfZ15zJtvg8D8nJbIpKJkPyaoKBtmMoa > AwQ%3D&reserved=0 Thanks for the info. My understanding is wrong. The patch was initially try to address what [1] did. In [1], static was discarded and moved to a common place. So I thought flags should be initialized as 0. > > > diff --git a/drivers/clk/imx/clk-pll14xx.c > > b/drivers/clk/imx/clk-pll14xx.c index fa76e04251c4..a7f1c1abe664 > > 100644 > > --- a/drivers/clk/imx/clk-pll14xx.c > > +++ b/drivers/clk/imx/clk-pll14xx.c > > @@ -65,12 +65,14 @@ struct imx_pll14xx_clk imx_1443x_pll = { > > .type = PLL_1443X, > > .rate_table = imx_pll1443x_tbl, > > .rate_count = ARRAY_SIZE(imx_pll1443x_tbl), > > + .flags = 0, > > }; > > What tree is this patch based on? https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/log/?h=for-next [1] https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git/commit/?h=for-next&id=43cdaa1567ad3931fbde438853947d45238cc040 Thanks, Peng. > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.b > ootlin.com%2Flinux%2Fv5.4-rc1%2Fsource%2Fdrivers%2Fclk%2Fimx%2Fclk-p > ll14xx.c%23L65&data=02%7C01%7Cpeng.fan%40nxp.com%7Ca9435670 > 0d9c4f61614a08d761d53936%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C > 0%7C1%7C637085440717516996&sdata=fPeaCJQKsdWZKVG431KNL8v > hqT0y2TUMMntf1C66YNg%3D&reserved=0 > > Regards.
On 05/11/2019 11:04, Peng Fan wrote: > The patch was initially try to address what [1] did. > In [1], static was discarded and moved to a common place. > So I thought flags should be initialized as 0. A small additional (pedantic) note :-) Actually, in your case, imx_1443x_pll has "static storage duration". The "static" keyword ("storage-class specifier") merely limits the symbol's visibility outside a compilation unit. Basically, static => "static storage duration" But the reverse is not true, see 6.2.4p3 https://port70.net/~nsz/c/c11/n1570.html#6.2.4 6.2.4 Storage durations of objects Regards.
On 19-11-05 07:21:08, Peng Fan wrote: > From: Peng Fan <peng.fan@nxp.com> > > init.flags is initialized with value from pll_clk->flags, however > imx_1443x_pll and imx_1416x_pll are not static structure, so flags > might be random value. So let's initialize flags as 0 now. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Abel Vesa <abel.vesa@nxp.com> > --- > drivers/clk/imx/clk-pll14xx.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c > index fa76e04251c4..a7f1c1abe664 100644 > --- a/drivers/clk/imx/clk-pll14xx.c > +++ b/drivers/clk/imx/clk-pll14xx.c > @@ -65,12 +65,14 @@ struct imx_pll14xx_clk imx_1443x_pll = { > .type = PLL_1443X, > .rate_table = imx_pll1443x_tbl, > .rate_count = ARRAY_SIZE(imx_pll1443x_tbl), > + .flags = 0, > }; > > struct imx_pll14xx_clk imx_1416x_pll = { > .type = PLL_1416X, > .rate_table = imx_pll1416x_tbl, > .rate_count = ARRAY_SIZE(imx_pll1416x_tbl), > + .flags = 0, > }; > > static const struct imx_pll14xx_rate_table *imx_get_pll_settings( > -- > 2.16.4 >
Hello Peng, On 11/5/19 8:21 AM, Peng Fan wrote: > From: Peng Fan <peng.fan@nxp.com> > > init.flags is initialized with value from pll_clk->flags, however > imx_1443x_pll and imx_1416x_pll are not static structure, They don't have a static in front of them, but they still have static storage duration. > so flags > might be random value. So let's initialize flags as 0 now. I fail to see how. Members not listed in the { initializer-list } are implicitly initialized as if they were static objects, so flags should already be zero. (I assumed this patch is based on Shawn's imx-clk-5.5 tag) Cheers Ahmad > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/clk/imx/clk-pll14xx.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c > index fa76e04251c4..a7f1c1abe664 100644 > --- a/drivers/clk/imx/clk-pll14xx.c > +++ b/drivers/clk/imx/clk-pll14xx.c > @@ -65,12 +65,14 @@ struct imx_pll14xx_clk imx_1443x_pll = { > .type = PLL_1443X, > .rate_table = imx_pll1443x_tbl, > .rate_count = ARRAY_SIZE(imx_pll1443x_tbl), > + .flags = 0, > }; > > struct imx_pll14xx_clk imx_1416x_pll = { > .type = PLL_1416X, > .rate_table = imx_pll1416x_tbl, > .rate_count = ARRAY_SIZE(imx_pll1416x_tbl), > + .flags = 0, > }; > > static const struct imx_pll14xx_rate_table *imx_get_pll_settings( >
> Subject: Re: [PATCH] clk: imx: pll14xx: initialize flags to 0 > > Hello Peng, > > On 11/5/19 8:21 AM, Peng Fan wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > init.flags is initialized with value from pll_clk->flags, however > > imx_1443x_pll and imx_1416x_pll are not static structure, > > They don't have a static in front of them, but they still have static storage > duration. Yes. I am wrong. > > > so flags > > might be random value. So let's initialize flags as 0 now. > > I fail to see how. Members not listed in the { initializer-list } are implicitly > initialized as if they were static objects, so flags should already be zero. Understand. > > (I assumed this patch is based on Shawn's imx-clk-5.5 tag) Yes. Drop this patch. Thanks, Peng. > > Cheers > Ahmad > > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > drivers/clk/imx/clk-pll14xx.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/clk/imx/clk-pll14xx.c > > b/drivers/clk/imx/clk-pll14xx.c index fa76e04251c4..a7f1c1abe664 > > 100644 > > --- a/drivers/clk/imx/clk-pll14xx.c > > +++ b/drivers/clk/imx/clk-pll14xx.c > > @@ -65,12 +65,14 @@ struct imx_pll14xx_clk imx_1443x_pll = { > > .type = PLL_1443X, > > .rate_table = imx_pll1443x_tbl, > > .rate_count = ARRAY_SIZE(imx_pll1443x_tbl), > > + .flags = 0, > > }; > > > > struct imx_pll14xx_clk imx_1416x_pll = { > > .type = PLL_1416X, > > .rate_table = imx_pll1416x_tbl, > > .rate_count = ARRAY_SIZE(imx_pll1416x_tbl), > > + .flags = 0, > > }; > > > > static const struct imx_pll14xx_rate_table *imx_get_pll_settings( > > > > -- > Pengutronix e.K. | > | > Industrial Linux Solutions | > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p > engutronix.de%2F&data=02%7C01%7Cpeng.fan%40nxp.com%7Cd19f6f7 > 6f49e40ed516108d761eff88d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C > 0%7C1%7C637085555604797300&sdata=MVUvIPUFpkhLj6KDs1Za2sBU > FNPMrWvS9vA9BuxqQ3k%3D&reserved=0 | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 > | > Amtsgericht Hildesheim, HRA 2686 | Fax: > +49-5121-206917-5555 |
diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c index fa76e04251c4..a7f1c1abe664 100644 --- a/drivers/clk/imx/clk-pll14xx.c +++ b/drivers/clk/imx/clk-pll14xx.c @@ -65,12 +65,14 @@ struct imx_pll14xx_clk imx_1443x_pll = { .type = PLL_1443X, .rate_table = imx_pll1443x_tbl, .rate_count = ARRAY_SIZE(imx_pll1443x_tbl), + .flags = 0, }; struct imx_pll14xx_clk imx_1416x_pll = { .type = PLL_1416X, .rate_table = imx_pll1416x_tbl, .rate_count = ARRAY_SIZE(imx_pll1416x_tbl), + .flags = 0, }; static const struct imx_pll14xx_rate_table *imx_get_pll_settings(