Messages in this thread Patch in this message | | | Date | Wed, 30 Jun 2004 12:36:37 -0500 | From | linas@austin ... | Subject | Re: [PATCH] PPC64: lockfix for rtas error log |
| |
On Tue, Jun 29, 2004 at 08:44:26PM -0500, Benjamin Herrenschmidt wrote: > On Tue, 2004-06-29 at 17:50, linas@austin.ibm.com wrote: > > Paul, > > > > Could you please apply the following path to the ameslab tree, and/or > > forward it to the main 2.6 kernel maintainers. > > > > This patch moves the location of a lock in order to protect > > the contents of a buffer until it has been copied to its final > > destination. Prior to this, a race existed whereby the buffer > > could be filled even while it was being emptied. > > Hrm.... > > That's bad, I moved that out of the lock on purpose to avoid deadlocks,
I looked for deadlocks, but didn't see anything obvious. I'll take your word, though.
> I think ppc_md.log_error can take the rtas lock again (nvram). We need to > take a separate lock for the err buf if that function can be called > concurrently I suppose.
Well, the problem was that there is no lock that is protecting the use of the single, global buffer. Adding yet another lock is bad; it makes hunting for deadlocks that much more tedious and difficult; already, finding deadlocks is error-prone, and subject to bit-rot as future hackers update the code. So instead, the problem can be easily avoided by not using a global buffer. The code below mallocs/frees. Its not perf-critcal, so I don't mind malloc overhead. Would this work for you? Patch attached below.
Signed-off-by: Linas Vepstas <linas@linas.org>
--linas
--- arch/ppc64/kernel/rtas.c.orig-pre-lockfix 2004-06-29 17:02:12.000000000 -0500 +++ arch/ppc64/kernel/rtas.c 2004-06-30 12:26:51.000000000 -0500 @@ -131,12 +131,20 @@ log_rtas_error(void) { unsigned long s; int rc; + char * buff_copy = NULL; spin_lock_irqsave(&rtas.lock, s); rc = __log_rtas_error(); + if (rc == 0) { + buff_copy = kmalloc (RTAS_ERROR_LOG_MAX, GFP_ATOMIC); + memcpy (buff_copy, rtas_err_buf, RTAS_ERROR_LOG_MAX); + } spin_unlock_irqrestore(&rtas.lock, s); - if (rc == 0) - log_error(rtas_err_buf, ERR_TYPE_RTAS_LOG, 0); + + if (buff_copy) { + log_error(buff_copy, ERR_TYPE_RTAS_LOG, 0); + kfree (buff_copy); + } } int @@ -147,6 +155,7 @@ rtas_call(int token, int nargs, int nret int i, logit = 0; unsigned long s; struct rtas_args *rtas_args; + char * buff_copy = NULL; int ret; PPCDBG(PPCDBG_RTAS, "Entering rtas_call\n"); @@ -193,12 +202,18 @@ rtas_call(int token, int nargs, int nret outputs[i] = rtas_args->rets[i+1]; ret = (int)((nret > 0) ? rtas_args->rets[0] : 0); + if (logit) { + buff_copy = kmalloc (RTAS_ERROR_LOG_MAX, GFP_ATOMIC); + memcpy (buff_copy, rtas_err_buf, RTAS_ERROR_LOG_MAX); + } + /* Gotta do something different here, use global lock for now... */ spin_unlock_irqrestore(&rtas.lock, s); - if (logit) - log_error(rtas_err_buf, ERR_TYPE_RTAS_LOG, 0); - + if (buff_copy) { + log_error(buff_copy, ERR_TYPE_RTAS_LOG, 0); + kfree (buff_copy); + } return ret; } | |