[PATCH v3 0/3] fix a kernel panic on fsl corenet board when CONFIG_CLK_PPC_CORENET is enabled

[PATCH v3 0/3] fix a kernel panic on fsl corenet board when CONFIG_CLK_PPC_CORENET is enabled

[Kevin Hao]

I have done a boot test on p5040ds and mpc5121eads board.

v3:
  - Rebase on next.
  - CC stable for patch 1.
  - Add Scott and Michael's ack.

v2:
 - Revert the commit da788acb2838 first.
 - Invoke of_clk_init() from a common place.

v1
This tries to fix a kernel panic introduced by commit da788acb2838
("clk: ppc-corenet: Fix Section mismatch warning").


Kevin Hao (3):
  Revert "clk: ppc-corenet: Fix Section mismatch warning"
  powerpc: call of_clk_init() from time_init()
  clk: ppc-corenet: fix section mismatch warning

 arch/powerpc/kernel/time.c                    |  5 ++++
 arch/powerpc/platforms/512x/clock-commonclk.c | 11 ++++---
 drivers/clk/clk-ppc-corenet.c                 | 43 ++++-----------------------
 3 files changed, 16 insertions(+), 43 deletions(-)

-- 
1.9.3

[PATCH v3 1/3] Revert "clk: ppc-corenet: Fix Section mismatch warning"

[Kevin Hao]

This reverts commit da788acb28386aa896224e784954bb73c99ff26c.

That commit tried to fix the section mismatch warning by moving the
ppc_corenet_clk_driver struct to init section. This is definitely wrong
because the kernel would free the memories occupied by this struct
after boot while this driver is still registered in the driver core.
The kernel would panic when accessing this driver struct.

Cc: stable@vger.kernel.org # 3.17
Signed-off-by: Kevin Hao <haokexin@gmail.com>
Acked-by: Scott Wood <scottwood@freescale.com>
Acked-by: Michael Turquette <mturquette@linaro.org>
---
v3: Cc stable and add ack.

v2: A new patch in v2.

 drivers/clk/clk-ppc-corenet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk-ppc-corenet.c b/drivers/clk/clk-ppc-corenet.c
index b6e6c85507a5..0a47d6f49cd6 100644
--- a/drivers/clk/clk-ppc-corenet.c
+++ b/drivers/clk/clk-ppc-corenet.c
@@ -291,7 +291,7 @@ static const struct of_device_id ppc_clk_ids[] __initconst = {
 	{}
 };
 
-static struct platform_driver ppc_corenet_clk_driver __initdata = {
+static struct platform_driver ppc_corenet_clk_driver = {
 	.driver = {
 		.name = "ppc_corenet_clock",
 		.of_match_table = ppc_clk_ids,
-- 
1.9.3

Re: [PATCH v3 1/3] Revert "clk: ppc-corenet: Fix Section mismatch warning"

[Scott Wood]

On Wed, 2014-12-03 at 16:53 +0800, Kevin Hao wrote:
> This reverts commit da788acb28386aa896224e784954bb73c99ff26c.
> 
> That commit tried to fix the section mismatch warning by moving the
> ppc_corenet_clk_driver struct to init section. This is definitely wrong
> because the kernel would free the memories occupied by this struct
> after boot while this driver is still registered in the driver core.
> The kernel would panic when accessing this driver struct.
> 
> Cc: stable@vger.kernel.org # 3.17

I don't see stable@vger.kernel.org in the actual CC list.

-Scott

Re: [PATCH v3 1/3] Revert "clk: ppc-corenet: Fix Section mismatch warning"

[Kevin Hao]

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

On Wed, Dec 03, 2014 at 10:04:51PM -0600, Scott Wood wrote:
> On Wed, 2014-12-03 at 16:53 +0800, Kevin Hao wrote:
> > This reverts commit da788acb28386aa896224e784954bb73c99ff26c.
> > 
> > That commit tried to fix the section mismatch warning by moving the
> > ppc_corenet_clk_driver struct to init section. This is definitely wrong
> > because the kernel would free the memories occupied by this struct
> > after boot while this driver is still registered in the driver core.
> > The kernel would panic when accessing this driver struct.
> > 
> > Cc: stable@vger.kernel.org # 3.17
> 
> I don't see stable@vger.kernel.org in the actual CC list.

With the above cc tag, Won't this get automatically included in the stable
tree once it is merged into mainline? As I know, we only need to explicitly
send the patch to stable@vger.kernel.org only when that patch was already
merged into mainline without the corresponding cc stable tag and we also want
the stable tree include it. Did I miss something?

Thanks,
Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v3 1/3] Revert "clk: ppc-corenet: Fix Section mismatch warning"

[Scott Wood]

On Thu, 2014-12-04 at 12:38 +0800, Kevin Hao wrote:
> On Wed, Dec 03, 2014 at 10:04:51PM -0600, Scott Wood wrote:
> > On Wed, 2014-12-03 at 16:53 +0800, Kevin Hao wrote:
> > > This reverts commit da788acb28386aa896224e784954bb73c99ff26c.
> > > 
> > > That commit tried to fix the section mismatch warning by moving the
> > > ppc_corenet_clk_driver struct to init section. This is definitely wrong
> > > because the kernel would free the memories occupied by this struct
> > > after boot while this driver is still registered in the driver core.
> > > The kernel would panic when accessing this driver struct.
> > > 
> > > Cc: stable@vger.kernel.org # 3.17
> > 
> > I don't see stable@vger.kernel.org in the actual CC list.
> 
> With the above cc tag, Won't this get automatically included in the stable
> tree once it is merged into mainline? As I know, we only need to explicitly
> send the patch to stable@vger.kernel.org only when that patch was already
> merged into mainline without the corresponding cc stable tag and we also want
> the stable tree include it. Did I miss something?

I reread stable_kernel_rules.txt and it looks like you're right, though
usually it ends up on the real CC list anyway due to how git send-email
works.

Since only this first patch is a critical bugfix, and there's no
arch/powerpc content in that patch, I think it should go via Mike's tree
if it's to go in for 3.18 (if it's not already too late).  Or, to keep
things simple given the dependency of the following patches, we could
batch them all together for -next and add a # 3.18 stable request.

-Scott

Re: [PATCH v3 1/3] Revert "clk: ppc-corenet: Fix Section mismatch warning"

[Kevin Hao]

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

On Wed, Dec 03, 2014 at 10:46:24PM -0600, Scott Wood wrote:
> Since only this first patch is a critical bugfix, and there's no
> arch/powerpc content in that patch, I think it should go via Mike's tree
> if it's to go in for 3.18 (if it's not already too late).  Or, to keep
> things simple given the dependency of the following patches, we could
> batch them all together for -next and add a # 3.18 stable request.

We don't need to explicitly add a #3.18 stable request in this case. As I know,
we only need to indicate the first version which is affected by the bug fixed by
this commit in the cc tag. This would imply that the applicable stable kernel
version rang should be from 3.17 to the previous version of the kernel which
finally merge this commit.

Thanks,
Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v3 1/3] Revert "clk: ppc-corenet: Fix Section mismatch warning"

[Scott Wood]

On Thu, 2014-12-04 at 14:06 +0800, Kevin Hao wrote:
> On Wed, Dec 03, 2014 at 10:46:24PM -0600, Scott Wood wrote:
> > Since only this first patch is a critical bugfix, and there's no
> > arch/powerpc content in that patch, I think it should go via Mike's tree
> > if it's to go in for 3.18 (if it's not already too late).  Or, to keep
> > things simple given the dependency of the following patches, we could
> > batch them all together for -next and add a # 3.18 stable request.
> 
> We don't need to explicitly add a #3.18 stable request in this case. As I know,
> we only need to indicate the first version which is affected by the bug fixed by
> this commit in the cc tag. This would imply that the applicable stable kernel
> version rang should be from 3.17 to the previous version of the kernel which
> finally merge this commit.

OK.

-Scott

Re: [PATCH v3 1/3] Revert "clk: ppc-corenet: Fix Section mismatch warning"

[Scott Wood]

On Wed, 2014-12-03 at 16:53 +0800, Kevin Hao wrote:
> This reverts commit da788acb28386aa896224e784954bb73c99ff26c.
> 
> That commit tried to fix the section mismatch warning by moving the
> ppc_corenet_clk_driver struct to init section. This is definitely wrong
> because the kernel would free the memories occupied by this struct
> after boot while this driver is still registered in the driver core.
> The kernel would panic when accessing this driver struct.
> 
> Cc: stable@vger.kernel.org # 3.17
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> Acked-by: Scott Wood <scottwood@freescale.com>
> Acked-by: Michael Turquette <mturquette@linaro.org>
> ---
> v3: Cc stable and add ack.
> 
> v2: A new patch in v2.
> 
>  drivers/clk/clk-ppc-corenet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-ppc-corenet.c b/drivers/clk/clk-ppc-corenet.c
> index b6e6c85507a5..0a47d6f49cd6 100644
> --- a/drivers/clk/clk-ppc-corenet.c
> +++ b/drivers/clk/clk-ppc-corenet.c
> @@ -291,7 +291,7 @@ static const struct of_device_id ppc_clk_ids[] __initconst = {
>  	{}
>  };
>  
> -static struct platform_driver ppc_corenet_clk_driver __initdata = {
> +static struct platform_driver ppc_corenet_clk_driver = {
>  	.driver = {
>  		.name = "ppc_corenet_clock",
>  		.of_match_table = ppc_clk_ids,

This patch is going to conflict with commit a4ae8f3b0f7ac6ab3 "clk: drop
owner assignment from platform_drivers" in linux-next -- or rather,
you've based this on that patch, but it's not in mpe's next branch, so I
get a merge conflict and there'd be another merge conflict later on to
get back to the newer base.

I really think this should go via the clock tree.  That's where the
breakage was introduced in the first place...

-Scott

Re: [PATCH v3 1/3] Revert "clk: ppc-corenet: Fix Section mismatch warning"

[Kevin Hao]

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

On Thu, Dec 04, 2014 at 09:51:59PM -0600, Scott Wood wrote:
> This patch is going to conflict with commit a4ae8f3b0f7ac6ab3 "clk: drop
> owner assignment from platform_drivers" in linux-next -- or rather,
> you've based this on that patch, but it's not in mpe's next branch, so I
> get a merge conflict and there'd be another merge conflict later on to
> get back to the newer base.
> 
> I really think this should go via the clock tree.  That's where the
> breakage was introduced in the first place...

Hi Mike,

Could you pick up these patches?

Thanks,
Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v3 1/3] Revert "clk: ppc-corenet: Fix Section mismatch warning"

[Mike Turquette]

Quoting Kevin Hao (2014-12-08 17:34:15)
> On Thu, Dec 04, 2014 at 09:51:59PM -0600, Scott Wood wrote:
> > This patch is going to conflict with commit a4ae8f3b0f7ac6ab3 "clk: drop
> > owner assignment from platform_drivers" in linux-next -- or rather,
> > you've based this on that patch, but it's not in mpe's next branch, so I
> > get a merge conflict and there'd be another merge conflict later on to
> > get back to the newer base.
> > =

> > I really think this should go via the clock tree.  That's where the
> > breakage was introduced in the first place...
> =

> Hi Mike,
> =

> Could you pick up these patches?

Kevin,

I've taken patch #1 into clk-fixes (will be sent out later today) and
patches #2 & #3 into clk-next towards 3.20.

Regards,
Mike

> =

> Thanks,
> Kevin

RE: [PATCH v3 1/3] Revert "clk: ppc-corenet: Fix Section mismatch warning"

[Yuantian Tang]

PING!

Thanks,
Yuantian

> -----Original Message-----
> From: Kevin Hao [mailto:haokexin@gmail.com]
> Sent: Wednesday, December 03, 2014 4:54 PM
> To: linuxppc-dev@lists.ozlabs.org
> Cc: Benjamin Herrenschmidt; Wood Scott-B07421; Mike Turquette; Lu
> Jingchang-B35083; Gerhard Sittig; Tang Yuantian-B29983
> Subject: [PATCH v3 1/3] Revert "clk: ppc-corenet: Fix Section mismatch wa=
rning"
>=20
> This reverts commit da788acb28386aa896224e784954bb73c99ff26c.
>=20
> That commit tried to fix the section mismatch warning by moving the
> ppc_corenet_clk_driver struct to init section. This is definitely wrong b=
ecause the
> kernel would free the memories occupied by this struct after boot while t=
his
> driver is still registered in the driver core.
> The kernel would panic when accessing this driver struct.
>=20
> Cc: stable@vger.kernel.org # 3.17
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> Acked-by: Scott Wood <scottwood@freescale.com>
> Acked-by: Michael Turquette <mturquette@linaro.org>
> ---
> v3: Cc stable and add ack.
>=20
> v2: A new patch in v2.
>=20
>  drivers/clk/clk-ppc-corenet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>=20
> diff --git a/drivers/clk/clk-ppc-corenet.c b/drivers/clk/clk-ppc-corenet.=
c index
> b6e6c85507a5..0a47d6f49cd6 100644
> --- a/drivers/clk/clk-ppc-corenet.c
> +++ b/drivers/clk/clk-ppc-corenet.c
> @@ -291,7 +291,7 @@ static const struct of_device_id ppc_clk_ids[] __init=
const
> =3D {
>  	{}
>  };
>=20
> -static struct platform_driver ppc_corenet_clk_driver __initdata =3D {
> +static struct platform_driver ppc_corenet_clk_driver =3D {
>  	.driver =3D {
>  		.name =3D "ppc_corenet_clock",
>  		.of_match_table =3D ppc_clk_ids,
> --
> 1.9.3

[PATCH v3 2/3] powerpc: call of_clk_init() from time_init()

[Kevin Hao]

So the boards which has COMMON_CLK enabled don't have to
invoke this in its board specific file.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
Acked-by: Scott Wood <scottwood@freescale.com>
Acked-by: Michael Turquette <mturquette@linaro.org>
---
v3: Add ack.

v2: A new patch in v2.

 arch/powerpc/kernel/time.c                    |  5 +++++
 arch/powerpc/platforms/512x/clock-commonclk.c | 11 +++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index fa7c4f12104f..df9fa05b5fd3 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -54,6 +54,7 @@
 #include <linux/irq.h>
 #include <linux/delay.h>
 #include <linux/irq_work.h>
+#include <linux/clk-provider.h>
 #include <asm/trace.h>
 
 #include <asm/io.h>
@@ -943,6 +944,10 @@ void __init time_init(void)
 
 	init_decrementer_clockevent();
 	tick_setup_hrtimer_broadcast();
+
+#ifdef CONFIG_COMMON_CLK
+	of_clk_init(NULL);
+#endif
 }
 
 
diff --git a/arch/powerpc/platforms/512x/clock-commonclk.c b/arch/powerpc/platforms/512x/clock-commonclk.c
index 6eb614a271fb..f691bcabd710 100644
--- a/arch/powerpc/platforms/512x/clock-commonclk.c
+++ b/arch/powerpc/platforms/512x/clock-commonclk.c
@@ -1168,6 +1168,11 @@ static void mpc5121_clk_provide_backwards_compat(void)
 	}
 }
 
+/*
+ * The "fixed-clock" nodes (which includes the oscillator node if the board's
+ * DT provides one) has already been scanned by the of_clk_init() in
+ * time_init().
+ */
 int __init mpc5121_clk_init(void)
 {
 	struct device_node *clk_np;
@@ -1187,12 +1192,6 @@ int __init mpc5121_clk_init(void)
 	mpc512x_clk_preset_data();
 
 	/*
-	 * have the device tree scanned for "fixed-clock" nodes (which
-	 * includes the oscillator node if the board's DT provides one)
-	 */
-	of_clk_init(NULL);
-
-	/*
 	 * add a dummy clock for those situations where a clock spec is
 	 * required yet no real clock is involved
 	 */
-- 
1.9.3

[PATCH v3 3/3] clk: ppc-corenet: fix section mismatch warning

[Kevin Hao]

In order to fix the following section mismatch warning:
  WARNING: drivers/clk/built-in.o(.data+0xe4): Section mismatch in reference from the variable ppc_corenet_clk_driver to the function .init.text:ppc_corenet_clk_probe()
  The variable ppc_corenet_clk_driver references
  the function __init ppc_corenet_clk_probe()
  If the reference is valid then annotate the
  variable with __init* or __refdata (see linux/init.h) or name the variable:
  *_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console

  WARNING: drivers/clk/built-in.o(.data+0x10c): Section mismatch in reference from the variable ppc_corenet_clk_driver to the variable .init.rodata:ppc_clk_ids
  The variable ppc_corenet_clk_driver references
  the variable __initconst ppc_clk_ids
  If the reference is valid then annotate the
  variable with __init* or __refdata (see linux/init.h) or name the variable:
  *_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console

We can't just add the __init annotation to ppc_corenet_clk_driver or
remove the __init from ppc_corenet_clk_probe() and ppc_clk_ids.
So choose to use CLK_OF_DECLARE to scan and init the clock devices.

Signed-off-by: Kevin Hao <haokexin@gmail.com>
Acked-by: Scott Wood <scottwood@freescale.com>
Acked-by: Michael Turquette <mturquette@linaro.org>
---
v3: Rebase on next and add ack.

v2: Avoid to invoke of_clk_init() in a board specific file.

 drivers/clk/clk-ppc-corenet.c | 43 ++++++-------------------------------------
 1 file changed, 6 insertions(+), 37 deletions(-)

diff --git a/drivers/clk/clk-ppc-corenet.c b/drivers/clk/clk-ppc-corenet.c
index 0a47d6f49cd6..57a2de47d2cb 100644
--- a/drivers/clk/clk-ppc-corenet.c
+++ b/drivers/clk/clk-ppc-corenet.c
@@ -267,40 +267,9 @@ static void __init sysclk_init(struct device_node *node)
 	if (!IS_ERR(clk))
 		of_clk_add_provider(np, of_clk_src_simple_get, clk);
 }
-
-static const struct of_device_id clk_match[] __initconst = {
-	{ .compatible = "fsl,qoriq-sysclk-1.0", .data = sysclk_init, },
-	{ .compatible = "fsl,qoriq-sysclk-2.0", .data = sysclk_init, },
-	{ .compatible = "fsl,qoriq-core-pll-1.0", .data = core_pll_init, },
-	{ .compatible = "fsl,qoriq-core-pll-2.0", .data = core_pll_init, },
-	{ .compatible = "fsl,qoriq-core-mux-1.0", .data = core_mux_init, },
-	{ .compatible = "fsl,qoriq-core-mux-2.0", .data = core_mux_init, },
-	{}
-};
-
-static int __init ppc_corenet_clk_probe(struct platform_device *pdev)
-{
-	of_clk_init(clk_match);
-
-	return 0;
-}
-
-static const struct of_device_id ppc_clk_ids[] __initconst = {
-	{ .compatible = "fsl,qoriq-clockgen-1.0", },
-	{ .compatible = "fsl,qoriq-clockgen-2.0", },
-	{}
-};
-
-static struct platform_driver ppc_corenet_clk_driver = {
-	.driver = {
-		.name = "ppc_corenet_clock",
-		.of_match_table = ppc_clk_ids,
-	},
-	.probe = ppc_corenet_clk_probe,
-};
-
-static int __init ppc_corenet_clk_init(void)
-{
-	return platform_driver_register(&ppc_corenet_clk_driver);
-}
-subsys_initcall(ppc_corenet_clk_init);
+CLK_OF_DECLARE(qoriq_sysclk_1, "fsl,qoriq-sysclk-1.0", sysclk_init);
+CLK_OF_DECLARE(qoriq_sysclk_2, "fsl,qoriq-sysclk-2.0", sysclk_init);
+CLK_OF_DECLARE(qoriq_core_pll_1, "fsl,qoriq-core-pll-1.0", core_pll_init);
+CLK_OF_DECLARE(qoriq_core_pll_2, "fsl,qoriq-core-pll-2.0", core_pll_init);
+CLK_OF_DECLARE(qoriq_core_mux_1, "fsl,qoriq-core-mux-1.0", core_mux_init);
+CLK_OF_DECLARE(qoriq_core_mux_2, "fsl,qoriq-core-mux-2.0", core_mux_init);
-- 
1.9.3