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
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
