Patchwork [2/4] powerpc: Add ppc_progress() wrapper

login
register
mail settings
Submitter Michael Ellerman
Date Jan. 15, 2009, 6:43 a.m.
Message ID <3922453a668745466cb291f889d2c067932e4894.1232001793.git.michael@ellerman.id.au>
Download mbox | patch
Permalink /patch/18582/
State Changes Requested
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Michael Ellerman - Jan. 15, 2009, 6:43 a.m.
There's quite a lot of code that does:

if (ppc_md.progress)
	ppc_md.progress(...)

So move that idiom into a wrapper. Having a wrapper also allows us
to have a fallback to printk if no progress routine is specified.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
 arch/powerpc/include/asm/machdep.h |    2 ++
 arch/powerpc/kernel/setup-common.c |    8 ++++++++
 2 files changed, 10 insertions(+), 0 deletions(-)
Grant Likely - Jan. 15, 2009, 7:23 a.m.
On Wed, Jan 14, 2009 at 11:43 PM, Michael Ellerman
<michael@ellerman.id.au> wrote:
> There's quite a lot of code that does:
>
> if (ppc_md.progress)
>        ppc_md.progress(...)
>
> So move that idiom into a wrapper. Having a wrapper also allows us
> to have a fallback to printk if no progress routine is specified.
>
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>

Good idea.

Acked-by: Grant Likely <grant.likely@secretlab.ca>

> ---
>  arch/powerpc/include/asm/machdep.h |    2 ++
>  arch/powerpc/kernel/setup-common.c |    8 ++++++++
>  2 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
> index 6c34a0d..9e4ab07 100644
> --- a/arch/powerpc/include/asm/machdep.h
> +++ b/arch/powerpc/include/asm/machdep.h
> @@ -328,6 +328,8 @@ extern void __devinit smp_generic_take_timebase(void);
>  /* Print a boot progress message. */
>  void ppc64_boot_msg(unsigned int src, const char *msg);
>
> +extern void ppc_progress(char *msg, unsigned short code);
> +
>  static inline void log_error(char *buf, unsigned int err_type, int fatal)
>  {
>        if (ppc_md.log_error)
> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> index 705fc4b..cc4997b 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -669,3 +669,11 @@ static int powerpc_debugfs_init(void)
>  }
>  arch_initcall(powerpc_debugfs_init);
>  #endif
> +
> +void ppc_progress(char *msg, unsigned short code)
> +{
> +       if (ppc_md.progress)
> +               ppc_md.progress(msg, code);
> +       else
> +               printk(KERN_DEBUG "*** %04x : %s\n", code, msg ? msg : "");
> +}
> --
> 1.5.5
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>
Paul Mackerras - Jan. 15, 2009, 11:34 a.m.
Michael Ellerman writes:

> There's quite a lot of code that does:
> 
> if (ppc_md.progress)
> 	ppc_md.progress(...)
> 
> So move that idiom into a wrapper. Having a wrapper also allows us
> to have a fallback to printk if no progress routine is specified.

It certainly used to be the case on ppc32 (and may still be) that
ppc_md.progress was called very very early, in some cases before the
MMU was set up, so we don't want to call printk from it.  Falling back
to udbg_printf would be more appropriate.

The idea of ppc_md.progress was that if you need to debug very early
boot and you have something that you can poke with absolutely minimal
setup and observe externally, you can hook that up to ppc_md.progress
and get an idea where the system is dying.  Once you get to the point
where printk works then you can just use printk.

Paul.
Benjamin Herrenschmidt - Jan. 15, 2009, 11:01 p.m.
> It certainly used to be the case on ppc32 (and may still be) that
> ppc_md.progress was called very very early, in some cases before the
> MMU was set up, so we don't want to call printk from it.  Falling back
> to udbg_printf would be more appropriate.
> 
> The idea of ppc_md.progress was that if you need to debug very early
> boot and you have something that you can poke with absolutely minimal
> setup and observe externally, you can hook that up to ppc_md.progress
> and get an idea where the system is dying.  Once you get to the point
> where printk works then you can just use printk.

Right, printk shouldn't be called before lockdep is initialized, though
I don't see a call to lockdep_init on ppc32 ..

Appart from that, printk is pretty safe as long as we have some kind
of translation from PAGE_OFFSET that covers the kernel text, data and
bss, which we do have fairly early.

I think the reasons we did that back then is that printk output will end
up nowhere. Nowadays however, we have udbg console which can possibly be
initialized very early too.

I'd say let's keep that patch on the back burner until we get a chance
to play a bit more with that stuff.

Among other things, I still want to change the early initialization
order of ppc32 to look more like ppc64 which would help in that are as
well so ...

Cheers,
Ben.
Michael Ellerman - Jan. 15, 2009, 11:09 p.m.
On Thu, 2009-01-15 at 22:34 +1100, Paul Mackerras wrote:
> Michael Ellerman writes:
> 
> > There's quite a lot of code that does:
> > 
> > if (ppc_md.progress)
> > 	ppc_md.progress(...)
> > 
> > So move that idiom into a wrapper. Having a wrapper also allows us
> > to have a fallback to printk if no progress routine is specified.
> 
> It certainly used to be the case on ppc32 (and may still be) that
> ppc_md.progress was called very very early, in some cases before the
> MMU was set up, so we don't want to call printk from it.  Falling back
> to udbg_printf would be more appropriate.

Except on some platforms udbg_printf() doesn't work. And all the
platforms that care still can and do use udbg_progress().

This just saves us having several implementations of progress that just
call printk().

cheers

Patch

diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
index 6c34a0d..9e4ab07 100644
--- a/arch/powerpc/include/asm/machdep.h
+++ b/arch/powerpc/include/asm/machdep.h
@@ -328,6 +328,8 @@  extern void __devinit smp_generic_take_timebase(void);
 /* Print a boot progress message. */
 void ppc64_boot_msg(unsigned int src, const char *msg);
 
+extern void ppc_progress(char *msg, unsigned short code);
+
 static inline void log_error(char *buf, unsigned int err_type, int fatal)
 {
 	if (ppc_md.log_error)
diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 705fc4b..cc4997b 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -669,3 +669,11 @@  static int powerpc_debugfs_init(void)
 }
 arch_initcall(powerpc_debugfs_init);
 #endif
+
+void ppc_progress(char *msg, unsigned short code)
+{
+	if (ppc_md.progress)
+		ppc_md.progress(msg, code);
+	else
+		printk(KERN_DEBUG "*** %04x : %s\n", code, msg ? msg : "");
+}