Re: [S390] cio: fix stsch_reset.

Re: [S390] cio: fix stsch_reset.

[Chuck Ebbert]

In-Reply-To: <20061228103925.GB6270@skybase>

On Thu, 28 Dec 2006 11:39:25 +0100, Martin Schwidefsky wrote:

> @@ -881,10 +880,18 @@ static void cio_reset_pgm_check_handler(
>  static int stsch_reset(struct subchannel_id schid, volatile struct schib *addr)
>  {
>       int rc;
> +     register struct subchannel_id reg1 asm ("1") = schid;
>  
>       pgm_check_occured = 0;
>       s390_reset_pgm_handler = cio_reset_pgm_check_handler;
> -     rc = stsch(schid, addr);
> +
> +     asm volatile(
> +             "       stsch   0(%2)\n"
> +             "       ipm     %0\n"
> +             "       srl     %0,28"
> +             : "=d" (rc)
> +             : "d" (reg1), "a" (addr), "m" (*addr) : "memory", "cc");
> +
>       s390_reset_pgm_handler = NULL;
>       if (pgm_check_occured)
>               return -EIO;


Can't you just put a barrier() before the stsch() call?

-- 
MBTI: IXTP

Re: [S390] cio: fix stsch_reset.

[Martin Schwidefsky]

On Sun, 2006-12-31 at 01:31 -0500, Chuck Ebbert wrote:
> > @@ -881,10 +880,18 @@ static void cio_reset_pgm_check_handler(
> >  static int stsch_reset(struct subchannel_id schid, volatile struct schib *addr)
> >  {
> >       int rc;
> > +     register struct subchannel_id reg1 asm ("1") = schid;
> >
> >       pgm_check_occured = 0;
> >       s390_reset_pgm_handler = cio_reset_pgm_check_handler;
> > -     rc = stsch(schid, addr);
> > +
> > +     asm volatile(
> > +             "       stsch   0(%2)\n"
> > +             "       ipm     %0\n"
> > +             "       srl     %0,28"
> > +             : "=d" (rc)
> > +             : "d" (reg1), "a" (addr), "m" (*addr) : "memory", "cc");
> > +
> >       s390_reset_pgm_handler = NULL;
> >       if (pgm_check_occured)
> >               return -EIO;
> 
> 
> Can't you just put a barrier() before the stsch() call?

You mean after. We have to prevent the sequence
1) load pgm_check_occured,
2) stsch and the execution of cio_reset_pgm_check_handler
3) use of the the value loaded in 1)
A barrier before 2) forces a reload of pgm_check_occured but it does not
force the reload to be before the stsch call.

But the basic idea is valid. The standard stsch() inline followed by a
barrier() is equivalent to the new inline assembly.

-- 
blue skies,
  Martin.

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

"Reality continues to ruin my life." - Calvin.

Re: [S390] cio: fix stsch_reset.

[Heiko Carstens]

On Sun, Dec 31, 2006 at 01:31:43AM -0500, Chuck Ebbert wrote:
> In-Reply-To: <20061228103925.GB6270@skybase>
> 
> On Thu, 28 Dec 2006 11:39:25 +0100, Martin Schwidefsky wrote:
> 
> > @@ -881,10 +880,18 @@ static void cio_reset_pgm_check_handler(
> >  static int stsch_reset(struct subchannel_id schid, volatile struct schib *addr)
> >  {
> >       int rc;
> > +     register struct subchannel_id reg1 asm ("1") = schid;
> >
> >       pgm_check_occured = 0;
> >       s390_reset_pgm_handler = cio_reset_pgm_check_handler;
> > -     rc = stsch(schid, addr);
> > +
> > +     asm volatile(
> > +             "       stsch   0(%2)\n"
> > +             "       ipm     %0\n"
> > +             "       srl     %0,28"
> > +             : "=d" (rc)
> > +             : "d" (reg1), "a" (addr), "m" (*addr) : "memory", "cc");
> > +
> >       s390_reset_pgm_handler = NULL;
> >       if (pgm_check_occured)
> >               return -EIO;
> 
> 
> Can't you just put a barrier() before the stsch() call?

Yes, that would work too and would look much nicer.

I think we should change the reset program check handler, so that it searches
the exception tables. At least for in-kernel addresses that should work.
For addresses within modules this might cause deadlocks, since the module
code takes spinlocks and we are in a context where we just killed all cpus
not knowing what they executed... Hmm..

Re: [S390] cio: fix stsch_reset.

[Michael Holzheu]

heicars2@linux.vnet.ibm.com wrote on 12/31/2006 01:22:03 PM:

> On Sun, Dec 31, 2006 at 01:31:43AM -0500, Chuck Ebbert wrote:
> > In-Reply-To: <20061228103925.GB6270@skybase>
> >
> > On Thu, 28 Dec 2006 11:39:25 +0100, Martin Schwidefsky wrote:
> >
> > > @@ -881,10 +880,18 @@ static void cio_reset_pgm_check_handler(
> > >  static int stsch_reset(struct subchannel_id schid, volatile
> struct schib *addr)
> > >  {
> > >       int rc;
> > > +     register struct subchannel_id reg1 asm ("1") = schid;
> > >
> > >       pgm_check_occured = 0;
> > >       s390_reset_pgm_handler = cio_reset_pgm_check_handler;
> > > -     rc = stsch(schid, addr);
> > > +
> > > +     asm volatile(
> > > +             "       stsch   0(%2)\n"
> > > +             "       ipm     %0\n"
> > > +             "       srl     %0,28"
> > > +             : "=d" (rc)
> > > +             : "d" (reg1), "a" (addr), "m" (*addr) : "memory",
"cc");
> > > +
> > >       s390_reset_pgm_handler = NULL;
> > >       if (pgm_check_occured)
> > >               return -EIO;
> >
> >
> > Can't you just put a barrier() before the stsch() call?
>
> Yes, that would work too and would look much nicer.
>
> I think we should change the reset program check handler, so that it
searches
> the exception tables.

I think that this is really overkill, since having program checks in the
reset case will be VERY rare and stsch will probably the only case.
I would do that only, if we have a component, which needs that.

And I assume, that this will never happen!

Michael

[S390] cio: fix stsch_reset.

[Martin Schwidefsky]

From: Michael Holzheu <holzheu@de.ibm.com>

[S390] cio: fix stsch_reset.

Copy inline assembly of stsch and add "memory" to clobber list in order
to prevent gcc from optimizing away the checking of the global variable
"pgm_check_occured".

Signed-off-by: Michael Holzheu <holzheu@de.ibm.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---

 drivers/s390/cio/cio.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff -urpN linux-2.6/drivers/s390/cio/cio.c linux-2.6-patched/drivers/s390/cio/cio.c
--- linux-2.6/drivers/s390/cio/cio.c	2006-12-28 00:33:22.000000000 +0100
+++ linux-2.6-patched/drivers/s390/cio/cio.c	2006-12-28 00:33:36.000000000 +0100
@@ -2,8 +2,7 @@
  *  drivers/s390/cio/cio.c
  *   S/390 common I/O routines -- low level i/o calls
  *
- *    Copyright (C) 1999-2002 IBM Deutschland Entwicklung GmbH,
- *			      IBM Corporation
+ *    Copyright (C) IBM Corp. 1999,2006
  *    Author(s): Ingo Adlung (adlung@de.ibm.com)
  *		 Cornelia Huck (cornelia.huck@de.ibm.com)
  *		 Arnd Bergmann (arndb@de.ibm.com)
@@ -881,10 +880,18 @@ static void cio_reset_pgm_check_handler(
 static int stsch_reset(struct subchannel_id schid, volatile struct schib *addr)
 {
 	int rc;
+	register struct subchannel_id reg1 asm ("1") = schid;
 
 	pgm_check_occured = 0;
 	s390_reset_pgm_handler = cio_reset_pgm_check_handler;
-	rc = stsch(schid, addr);
+
+	asm volatile(
+		"       stsch   0(%2)\n"
+		"       ipm     %0\n"
+		"       srl     %0,28"
+		: "=d" (rc)
+		: "d" (reg1), "a" (addr), "m" (*addr) : "memory", "cc");
+
 	s390_reset_pgm_handler = NULL;
 	if (pgm_check_occured)
 		return -EIO;