Re: 2.6.23-rc7-mm1 -- powerpc rtas panic

[Michael Ellerman <michael@ellerman.id.au>]

On Wed, 2007-10-03 at 11:19 +1000, Tony Breeds wrote:
> On Wed, Oct 03, 2007 at 10:30:16AM +1000, Michael Ellerman wrote:
>  
> > I realise it'll make the patch bigger, but this doesn't seem like a
> > particularly good name for the variable anymore.
> 
> Sure, what about?

Better .. but  ..   :D

> diff --git a/arch/powerpc/platforms/pseries/rtasd.c b/arch/powerpc/platforms/pseries/rtasd.c
> index 30925d2..73401c8 100644
> --- a/arch/powerpc/platforms/pseries/rtasd.c
> +++ b/arch/powerpc/platforms/pseries/rtasd.c
> @@ -54,8 +54,9 @@ static unsigned int rtas_event_scan_rate;
>  static int full_rtas_msgs = 0;
>  
>  /* Stop logging to nvram after first fatal error */
> -static int no_more_logging;
> -
> +static int logging_enabled; /* Until we initialize everything,
> +                             * make sure we don't try logging
> +                             * anything */

Until we initialise what exactly?

>  static int error_log_cnt;
>  
>  /*
> @@ -217,7 +218,7 @@ void pSeries_log_error(char *buf, unsigned int err_type, int fatal)
>  	}
>  
>  	/* Write error to NVRAM */
> -	if (!no_more_logging && !(err_type & ERR_FLAG_BOOT))
> +	if (logging_enabled && !(err_type & ERR_FLAG_BOOT))
>  		nvram_write_error_log(buf, len, err_type, error_log_cnt);
>  
>  	/*
> @@ -229,8 +230,8 @@ void pSeries_log_error(char *buf, unsigned int err_type, int fatal)
>  		printk_log_rtas(buf, len);
>  
>  	/* Check to see if we need to or have stopped logging */
> -	if (fatal || no_more_logging) {
> -		no_more_logging = 1;
> +	if (fatal || !logging_enabled) {
> +		logging_enabled = 0;
>  		spin_unlock_irqrestore(&rtasd_log_lock, s);
>  		return;
>  	}

Hmmm, this routine has 4 separate lock-dropping exit paths ..

> @@ -302,7 +303,7 @@ static ssize_t rtas_log_read(struct file * file, char __user * buf,
>  
>  	spin_lock_irqsave(&rtasd_log_lock, s);
>  	/* if it's 0, then we know we got the last one (the one in NVRAM) */
> -	if (rtas_log_size == 0 && !no_more_logging)
> +	if (rtas_log_size == 0 && logging_enabled)
>  		nvram_clear_error_log();
>  	spin_unlock_irqrestore(&rtasd_log_lock, s);
>  
> @@ -414,6 +415,8 @@ static int rtasd(void *unused)
>  	memset(logdata, 0, rtas_error_log_max);
>  	rc = nvram_read_error_log(logdata, rtas_error_log_max,
>  	                          &err_type, &error_log_cnt);
> +	/* We can use rtas_log_buf now */
> +	logging_enabled = 1;
>  
>  	if (!rc) {
>  		if (err_type != ERR_FLAG_ALREADY_LOGGED) {

What exactly happens that allows us to do logging? I don't see any
ordering between anything else and the setting of the flag, and AFAICT
we're not inside a spinlock or anything here.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

Attachment: signature.asc
Description: This is a digitally signed message part