lkml.org 
[lkml]   [2004]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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;
}
\
 
 \ /
  Last update: 2005-03-22 14:04    [W:0.176 / U:0.384 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site