Fwd: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

Fwd: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

[Geert Uytterhoeven]

---------- Forwarded message ----------
From: Arnd Bergmann <arnd@arndb.de>
Date: Thu, Jan 2, 2014 at 1:07 PM
Subject: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race
To: linux-kernel@vger.kernel.org
Cc: Arnd Bergmann <arnd@arndb.de>, Geert Uytterhoeven
<geert@linux-m68k.org>, "James E.J. Bottomley"
<JBottomley@parallels.com>, linux-scsi@vger.kernel.org


sleep_on is known broken and going away. The atari_scsi driver is one of
two remaining users in the falcon_get_lock() function, which is a rather
crazy piece of code. This does not attempt to fix the driver's locking
scheme in general, but at least prevents falcon_get_lock from going to
sleep when no other thread holds the same lock or tries to get it,
and we no longer schedule with irqs disabled.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: James E.J. Bottomley <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/atari_scsi.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index a3e6c8a..b55a58a 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -90,6 +90,7 @@
 #include <linux/init.h>
 #include <linux/nvram.h>
 #include <linux/bitops.h>
+#include <linux/wait.h>

 #include <asm/setup.h>
 #include <asm/atarihw.h>
@@ -549,8 +550,10 @@ static void falcon_get_lock(void)

        local_irq_save(flags);

-       while (!in_irq() && falcon_got_lock && stdma_others_waiting())
-               sleep_on(&falcon_fairness_wait);
+       wait_event_cmd(falcon_fairness_wait,
+                      !in_irq() && falcon_got_lock && stdma_others_waiting(),
+                      local_irq_restore(flags),
+                      local_irq_save(flags));

        while (!falcon_got_lock) {
                if (in_irq())
@@ -562,7 +565,10 @@ static void falcon_get_lock(void)
                        falcon_trying_lock = 0;
                        wake_up(&falcon_try_wait);
                } else {
-                       sleep_on(&falcon_try_wait);
+                       wait_event_cmd(falcon_try_wait,
+                                      !falcon_got_lock && !falcon_trying_lock,
+                                      local_irq_restore(flags),
+                                      local_irq_save(flags));
                }
        }

--
1.8.3.2

Fwd: [PATCH, RFC 01/30] ataflop: fix sleep_on races

[Geert Uytterhoeven]

---------- Forwarded message ----------
From: Arnd Bergmann <arnd@arndb.de>
Date: Thu, Jan 2, 2014 at 1:07 PM
Subject: [PATCH, RFC 01/30] ataflop: fix sleep_on races
To: linux-kernel@vger.kernel.org
Cc: Arnd Bergmann <arnd@arndb.de>, Jens Axboe <axboe@kernel.dk>, Geert
Uytterhoeven <geert@linux-m68k.org>


sleep_on() is inherently racy, and has been deprecated for a long time.
This fixes two instances in the atari floppy driver:

* fdc_wait/fdc_busy becomes an open-coded mutex. We cannot use the
  regular mutex since it gets released in interrupt context. The
  open-coded version using wait_event() and cmpxchg() is equivalent
  to the existing code but does the checks atomically, and we can
  now safely check the condition with irqs enabled.

* format_wait becomes a completion, which is the natural structure
  here. The format ioctl waits for the background task to either
  complete or abort.

This does not attempt to fix the preexisting bug of calling schedule
with local interrupts disabled.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/block/ataflop.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index 0e30c6e..96b629e 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -68,6 +68,8 @@
 #include <linux/init.h>
 #include <linux/blkdev.h>
 #include <linux/mutex.h>
+#include <linux/completion.h>
+#include <linux/wait.h>

 #include <asm/atafd.h>
 #include <asm/atafdreg.h>
@@ -301,7 +303,7 @@ module_param_array(UserSteprate, int, NULL, 0);
 /* Synchronization of FDC access. */
 static volatile int fdc_busy = 0;
 static DECLARE_WAIT_QUEUE_HEAD(fdc_wait);
-static DECLARE_WAIT_QUEUE_HEAD(format_wait);
+static DECLARE_COMPLETION(format_wait);

 static unsigned long changed_floppies = 0xff, fake_change = 0;
 #define        CHECK_CHANGE_DELAY      HZ/2
@@ -608,7 +610,7 @@ static void fd_error( void )
        if (IsFormatting) {
                IsFormatting = 0;
                FormatError = 1;
-               wake_up( &format_wait );
+               complete(&format_wait);
                return;
        }

@@ -650,9 +652,8 @@ static int do_format(int drive, int type, struct
atari_format_descr *desc)
        DPRINT(("do_format( dr=%d tr=%d he=%d offs=%d )\n",
                drive, desc->track, desc->head, desc->sect_offset ));

+       wait_event(fdc_wait, cmpxchg(&fdc_busy, 0, 1) == 0);
        local_irq_save(flags);
-       while( fdc_busy ) sleep_on( &fdc_wait );
-       fdc_busy = 1;
        stdma_lock(floppy_irq, NULL);
        atari_turnon_irq( IRQ_MFP_FDC ); /* should be already, just to
be sure */
        local_irq_restore(flags);
@@ -706,7 +707,7 @@ static int do_format(int drive, int type, struct
atari_format_descr *desc)
        ReqSide  = desc->head;
        do_fd_action( drive );

-       sleep_on( &format_wait );
+       wait_for_completion(&format_wait);

        redo_fd_request();
        return( FormatError ? -EIO : 0 );
@@ -1229,7 +1230,7 @@ static void fd_writetrack_done( int status )
                goto err_end;
        }

-       wake_up( &format_wait );
+       complete(&format_wait);
        return;

   err_end:
@@ -1497,8 +1498,7 @@ repeat:
 void do_fd_request(struct request_queue * q)
 {
        DPRINT(("do_fd_request for pid %d\n",current->pid));
-       while( fdc_busy ) sleep_on( &fdc_wait );
-       fdc_busy = 1;
+       wait_event(fdc_wait, cmpxchg(&fdc_busy, 0, 1) == 0);
        stdma_lock(floppy_irq, NULL);

        atari_disable_irq( IRQ_MFP_FDC );
--
1.8.3.2

Fwd: [PATCH, RFC 14/30] tty/amiserial: avoid interruptible_sleep_on

[Geert Uytterhoeven]

---------- Forwarded message ----------
From: Arnd Bergmann <arnd@arndb.de>
Date: Thu, Jan 2, 2014 at 1:07 PM
Subject: [PATCH, RFC 14/30] tty/amiserial: avoid interruptible_sleep_on
To: linux-kernel@vger.kernel.org
Cc: Arnd Bergmann <arnd@arndb.de>, Geert Uytterhoeven
<geert@linux-m68k.org>, Greg Kroah-Hartman
<gregkh@linuxfoundation.org>


interruptible_sleep_on is generally problematic and we want to get
rid of it. In case of TIOCMIWAIT, that race is actually in user
space and does not get fixed since we can only detect changes after
entering the ioctl handler, but it removes one more caller.

This instance can not be trivially replaced with wait_event, so
I chose to open-code the wait loop using prepare_to_wait/finish_wait.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/tty/amiserial.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index 71630a2..979e7c3 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -1248,6 +1248,8 @@ static int rs_ioctl(struct tty_struct *tty,
        struct async_icount cprev, cnow;        /* kernel counter temps */
        void __user *argp = (void __user *)arg;
        unsigned long flags;
+       DEFINE_WAIT(wait);
+       int ret;

        if (serial_paranoia_check(info, tty->name, "rs_ioctl"))
                return -ENODEV;
@@ -1288,25 +1290,33 @@ static int rs_ioctl(struct tty_struct *tty,
                        cprev = info->icount;
                        local_irq_restore(flags);
                        while (1) {
-
interruptible_sleep_on(&info->tport.delta_msr_wait);
-                               /* see if a signal did it */
-                               if (signal_pending(current))
-                                       return -ERESTARTSYS;
+                               prepare_to_wait(&info->tport.delta_msr_wait,
+                                               &wait, TASK_INTERRUPTIBLE);
                                local_irq_save(flags);
                                cnow = info->icount; /* atomic copy */
                                local_irq_restore(flags);
                                if (cnow.rng == cprev.rng && cnow.dsr
== cprev.dsr &&
-                                   cnow.dcd == cprev.dcd && cnow.cts
== cprev.cts)
-                                       return -EIO; /* no change => error */
+                                   cnow.dcd == cprev.dcd && cnow.cts
== cprev.cts) {
+                                       ret = -EIO; /* no change => error */
+                                       break;
+                               }
                                if ( ((arg & TIOCM_RNG) && (cnow.rng
!= cprev.rng)) ||
                                     ((arg & TIOCM_DSR) && (cnow.dsr
!= cprev.dsr)) ||
                                     ((arg & TIOCM_CD)  && (cnow.dcd
!= cprev.dcd)) ||
                                     ((arg & TIOCM_CTS) && (cnow.cts
!= cprev.cts)) ) {
-                                       return 0;
+                                       ret = 0;
+                                       break;
+                               }
+                               schedule();
+                               /* see if a signal did it */
+                               if (signal_pending(current)) {
+                                       ret = -ERESTARTSYS;
+                                       break;
                                }
                                cprev = cnow;
                        }
-                       /* NOTREACHED */
+                       finish_wait(&info->tport.delta_msr_wait, &wait);
+                       return ret;

                case TIOCSERGWILD:
                case TIOCSERSWILD:
--
1.8.3.2

Re: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

[Michael Schmitz]

Geert,

thanks for passing that on - I had in fact disabled that feature of the 
driver for a good part of the 3.x series of kernels while attempting to 
figure out what else is wrong with the locking scheme.

I'll take a fresh stab at this.

Cheers,

	Michael


Am 03.01.2014 um 01:26 schrieb Geert Uytterhoeven:

> ---------- Forwarded message ----------
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Thu, Jan 2, 2014 at 1:07 PM
> Subject: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race
> To: linux-kernel@vger.kernel.org
> Cc: Arnd Bergmann <arnd@arndb.de>, Geert Uytterhoeven
> <geert@linux-m68k.org>, "James E.J. Bottomley"
> <JBottomley@parallels.com>, linux-scsi@vger.kernel.org
>
>
> sleep_on is known broken and going away. The atari_scsi driver is one 
> of
> two remaining users in the falcon_get_lock() function, which is a 
> rather
> crazy piece of code. This does not attempt to fix the driver's locking
> scheme in general, but at least prevents falcon_get_lock from going to
> sleep when no other thread holds the same lock or tries to get it,
> and we no longer schedule with irqs disabled.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: James E.J. Bottomley <JBottomley@parallels.com>
> Cc: linux-scsi@vger.kernel.org
> ---
>  drivers/scsi/atari_scsi.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
> index a3e6c8a..b55a58a 100644
> --- a/drivers/scsi/atari_scsi.c
> +++ b/drivers/scsi/atari_scsi.c
> @@ -90,6 +90,7 @@
>  #include <linux/init.h>
>  #include <linux/nvram.h>
>  #include <linux/bitops.h>
> +#include <linux/wait.h>
>
>  #include <asm/setup.h>
>  #include <asm/atarihw.h>
> @@ -549,8 +550,10 @@ static void falcon_get_lock(void)
>
>         local_irq_save(flags);
>
> -       while (!in_irq() && falcon_got_lock && stdma_others_waiting())
> -               sleep_on(&falcon_fairness_wait);
> +       wait_event_cmd(falcon_fairness_wait,
> +                      !in_irq() && falcon_got_lock && 
> stdma_others_waiting(),
> +                      local_irq_restore(flags),
> +                      local_irq_save(flags));
>
>         while (!falcon_got_lock) {
>                 if (in_irq())
> @@ -562,7 +565,10 @@ static void falcon_get_lock(void)
>                         falcon_trying_lock = 0;
>                         wake_up(&falcon_try_wait);
>                 } else {
> -                       sleep_on(&falcon_try_wait);
> +                       wait_event_cmd(falcon_try_wait,
> +                                      !falcon_got_lock && 
> !falcon_trying_lock,
> +                                      local_irq_restore(flags),
> +                                      local_irq_save(flags));
>                 }
>         }
>
> --
> 1.8.3.2
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" 
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH, RFC 01/30] ataflop: fix sleep_on races

[Michael Schmitz]

Geert,

will look at this one as well, thanks.

Cheers,

	Michael

Am 03.01.2014 um 01:27 schrieb Geert Uytterhoeven:

> ---------- Forwarded message ----------
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Thu, Jan 2, 2014 at 1:07 PM
> Subject: [PATCH, RFC 01/30] ataflop: fix sleep_on races
> To: linux-kernel@vger.kernel.org
> Cc: Arnd Bergmann <arnd@arndb.de>, Jens Axboe <axboe@kernel.dk>, Geert
> Uytterhoeven <geert@linux-m68k.org>
>
>
> sleep_on() is inherently racy, and has been deprecated for a long time.
> This fixes two instances in the atari floppy driver:
>
> * fdc_wait/fdc_busy becomes an open-coded mutex. We cannot use the
>   regular mutex since it gets released in interrupt context. The
>   open-coded version using wait_event() and cmpxchg() is equivalent
>   to the existing code but does the checks atomically, and we can
>   now safely check the condition with irqs enabled.
>
> * format_wait becomes a completion, which is the natural structure
>   here. The format ioctl waits for the background task to either
>   complete or abort.
>
> This does not attempt to fix the preexisting bug of calling schedule
> with local interrupts disabled.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>  drivers/block/ataflop.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
> index 0e30c6e..96b629e 100644
> --- a/drivers/block/ataflop.c
> +++ b/drivers/block/ataflop.c
> @@ -68,6 +68,8 @@
>  #include <linux/init.h>
>  #include <linux/blkdev.h>
>  #include <linux/mutex.h>
> +#include <linux/completion.h>
> +#include <linux/wait.h>
>
>  #include <asm/atafd.h>
>  #include <asm/atafdreg.h>
> @@ -301,7 +303,7 @@ module_param_array(UserSteprate, int, NULL, 0);
>  /* Synchronization of FDC access. */
>  static volatile int fdc_busy = 0;
>  static DECLARE_WAIT_QUEUE_HEAD(fdc_wait);
> -static DECLARE_WAIT_QUEUE_HEAD(format_wait);
> +static DECLARE_COMPLETION(format_wait);
>
>  static unsigned long changed_floppies = 0xff, fake_change = 0;
>  #define        CHECK_CHANGE_DELAY      HZ/2
> @@ -608,7 +610,7 @@ static void fd_error( void )
>         if (IsFormatting) {
>                 IsFormatting = 0;
>                 FormatError = 1;
> -               wake_up( &format_wait );
> +               complete(&format_wait);
>                 return;
>         }
>
> @@ -650,9 +652,8 @@ static int do_format(int drive, int type, struct
> atari_format_descr *desc)
>         DPRINT(("do_format( dr=%d tr=%d he=%d offs=%d )\n",
>                 drive, desc->track, desc->head, desc->sect_offset ));
>
> +       wait_event(fdc_wait, cmpxchg(&fdc_busy, 0, 1) == 0);
>         local_irq_save(flags);
> -       while( fdc_busy ) sleep_on( &fdc_wait );
> -       fdc_busy = 1;
>         stdma_lock(floppy_irq, NULL);
>         atari_turnon_irq( IRQ_MFP_FDC ); /* should be already, just to
> be sure */
>         local_irq_restore(flags);
> @@ -706,7 +707,7 @@ static int do_format(int drive, int type, struct
> atari_format_descr *desc)
>         ReqSide  = desc->head;
>         do_fd_action( drive );
>
> -       sleep_on( &format_wait );
> +       wait_for_completion(&format_wait);
>
>         redo_fd_request();
>         return( FormatError ? -EIO : 0 );
> @@ -1229,7 +1230,7 @@ static void fd_writetrack_done( int status )
>                 goto err_end;
>         }
>
> -       wake_up( &format_wait );
> +       complete(&format_wait);
>         return;
>
>    err_end:
> @@ -1497,8 +1498,7 @@ repeat:
>  void do_fd_request(struct request_queue * q)
>  {
>         DPRINT(("do_fd_request for pid %d\n",current->pid));
> -       while( fdc_busy ) sleep_on( &fdc_wait );
> -       fdc_busy = 1;
> +       wait_event(fdc_wait, cmpxchg(&fdc_busy, 0, 1) == 0);
>         stdma_lock(floppy_irq, NULL);
>
>         atari_disable_irq( IRQ_MFP_FDC );
> --
> 1.8.3.2
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" 
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

[Michael Schmitz]

Arnd,

your patch breaks the Atari NCR5380 SCSI driver (easily verified using 
ARAnyM). Last console output:

> scsi0: options CAN_QUEUE=8 CMD_PER_LUN=1 SCAT-GAT=0 TAGGED-QUEUING=no 
> HOSTID=7 generic options AUTOSENSE REAL DMA SCSI-2 TAGGED QUEUING 
> generic release=7
> scsi0 : Atari native SCSI
> blk_queue_max_segments: set to minimum 1
> scsi0: aborting command
> scsi 0:0:0:0: CDB:
> Inquiry: 12 00 00 00 24 00
>
> NCR5380 core release=7.
> NCR5380: coroutine isn't running.
> scsi0: no currently connected command
> scsi0: issue_queue
> scsi0: disconnected_queue
> random: nonblocking pool is initialized
>
> scsi0: !!BINGO!! Falcon has no lock in NCR5380_abort
> scsi0: warning : SCSI command probably completed successfully before 
> abortion

No targets are connected, so the driver is expected to time out and 
abort each INQUIRY command. This works fine with the unmodified version 
of the driver (even though its locking scheme is well beyond crazy - I 
grant you that much). Note that the unmodified version also works fine 
on my actual Falcon hardware under moderate load (meaning to say, no 
hardware instabilities are triggered that would cause the SCSI chip to 
lock up and require a bus reset).

I'll debug this a bit to see where it gets stuck. Thanks for looking 
into this ugly mess of code!

Cheers,

    Michael



Michael Schmitz wrote:
> Geert,
>
> thanks for passing that on - I had in fact disabled that feature of 
> the driver for a good part of the 3.x series of kernels while 
> attempting to figure out what else is wrong with the locking scheme.
>
> I'll take a fresh stab at this.
>
> Cheers,
>
>     Michael
>
>
> Am 03.01.2014 um 01:26 schrieb Geert Uytterhoeven:
>
>> ---------- Forwarded message ----------
>> From: Arnd Bergmann <arnd@arndb.de>
>> Date: Thu, Jan 2, 2014 at 1:07 PM
>> Subject: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race
>> To: linux-kernel@vger.kernel.org
>> Cc: Arnd Bergmann <arnd@arndb.de>, Geert Uytterhoeven
>> <geert@linux-m68k.org>, "James E.J. Bottomley"
>> <JBottomley@parallels.com>, linux-scsi@vger.kernel.org
>>
>>
>> sleep_on is known broken and going away. The atari_scsi driver is one of
>> two remaining users in the falcon_get_lock() function, which is a rather
>> crazy piece of code. This does not attempt to fix the driver's locking
>> scheme in general, but at least prevents falcon_get_lock from going to
>> sleep when no other thread holds the same lock or tries to get it,
>> and we no longer schedule with irqs disabled.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>> Cc: James E.J. Bottomley <JBottomley@parallels.com>
>> Cc: linux-scsi@vger.kernel.org
>> ---
>>  drivers/scsi/atari_scsi.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
>> index a3e6c8a..b55a58a 100644
>> --- a/drivers/scsi/atari_scsi.c
>> +++ b/drivers/scsi/atari_scsi.c
>> @@ -90,6 +90,7 @@
>>  #include <linux/init.h>
>>  #include <linux/nvram.h>
>>  #include <linux/bitops.h>
>> +#include <linux/wait.h>
>>
>>  #include <asm/setup.h>
>>  #include <asm/atarihw.h>
>> @@ -549,8 +550,10 @@ static void falcon_get_lock(void)
>>
>>         local_irq_save(flags);
>>
>> -       while (!in_irq() && falcon_got_lock && stdma_others_waiting())
>> -               sleep_on(&falcon_fairness_wait);
>> +       wait_event_cmd(falcon_fairness_wait,
>> +                      !in_irq() && falcon_got_lock && 
>> stdma_others_waiting(),
>> +                      local_irq_restore(flags),
>> +                      local_irq_save(flags));
>>
>>         while (!falcon_got_lock) {
>>                 if (in_irq())
>> @@ -562,7 +565,10 @@ static void falcon_get_lock(void)
>>                         falcon_trying_lock = 0;
>>                         wake_up(&falcon_try_wait);
>>                 } else {
>> -                       sleep_on(&falcon_try_wait);
>> +                       wait_event_cmd(falcon_try_wait,
>> +                                      !falcon_got_lock && 
>> !falcon_trying_lock,
>> +                                      local_irq_restore(flags),
>> +                                      local_irq_save(flags));
>>                 }
>>         }
>>
>> -- 
>> 1.8.3.2
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

[Arnd Bergmann]

On Sunday 12 January 2014, Michael Schmitz wrote:
> Arnd,
> 
> your patch breaks the Atari NCR5380 SCSI driver (easily verified using 
> ARAnyM). Last console output:
> 

Thanks so much for testing and sorry for your troubles. It seems I
got the wrong polarity on at least one of the conditions when converting
from a while()-style loop to an until()-style wait_event loop.
I did double-check all the patches before, but this one must have
slipped through because the use is so obscure.

	Arnd

Re: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

[Michael Schmitz]

Arnd,

>> your patch breaks the Atari NCR5380 SCSI driver (easily verified using
>> ARAnyM). Last console output:
>>
>
> Thanks so much for testing and sorry for your troubles. It seems I

Not to worry - without the occasional incentive to work on it, the 
driver would just keep on bitrotting more. Not that I ever officially 
took it on ...

> got the wrong polarity on at least one of the conditions when 
> converting
> from a while()-style loop to an until()-style wait_event loop.

Yep, that's my hunch too. From what I've seen so far it blocks at the 
falcon_fairness_wait stage, but more tests are needed.

> I did double-check all the patches before, but this one must have
> slipped through because the use is so obscure.

Just say it - it is a nightmare. The code dates back to the times of 
the Big Kernel Lock, and interrupt priorities on Atari made it 
necessary to enable interrupts while in the SCSI inthandler at that 
time (after masking off the SCSI interrupt).

Sharing the interrupt with DMA completion, floppy and IDE without a way 
to properly check which one raised it just does not help.

If you want me to try something else, let me know.

Cheers,

	Michael

Re: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

[schmitz]

Arnd,
>
> I'll debug this a bit to see where it gets stuck. Thanks for looking 
> into this ugly mess of code!
>
> Cheers,
>
>    Michael
>
>
>
> Michael Schmitz wrote:
>> Geert,
>>
>> thanks for passing that on - I had in fact disabled that feature of 
>> the driver for a good part of the 3.x series of kernels while 
>> attempting to figure out what else is wrong with the locking scheme.
>>
>> I'll take a fresh stab at this.
>>
>> Cheers,
>>
>>     Michael
>>
>>
>> Am 03.01.2014 um 01:26 schrieb Geert Uytterhoeven:
>>
>>> ---------- Forwarded message ----------
>>> From: Arnd Bergmann <arnd@arndb.de>
>>> Date: Thu, Jan 2, 2014 at 1:07 PM
>>> Subject: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race
>>> To: linux-kernel@vger.kernel.org
>>> Cc: Arnd Bergmann <arnd@arndb.de>, Geert Uytterhoeven
>>> <geert@linux-m68k.org>, "James E.J. Bottomley"
>>> <JBottomley@parallels.com>, linux-scsi@vger.kernel.org
>>>
>>>
>>> sleep_on is known broken and going away. The atari_scsi driver is 
>>> one of
>>> two remaining users in the falcon_get_lock() function, which is a 
>>> rather
>>> crazy piece of code. This does not attempt to fix the driver's locking
>>> scheme in general, but at least prevents falcon_get_lock from going to
>>> sleep when no other thread holds the same lock or tries to get it,
>>> and we no longer schedule with irqs disabled.
>>>
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>>> Cc: James E.J. Bottomley <JBottomley@parallels.com>
>>> Cc: linux-scsi@vger.kernel.org
>>> ---
>>>  drivers/scsi/atari_scsi.c | 12 +++++++++---
>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
>>> index a3e6c8a..b55a58a 100644
>>> --- a/drivers/scsi/atari_scsi.c
>>> +++ b/drivers/scsi/atari_scsi.c
>>> @@ -90,6 +90,7 @@
>>>  #include <linux/init.h>
>>>  #include <linux/nvram.h>
>>>  #include <linux/bitops.h>
>>> +#include <linux/wait.h>
>>>
>>>  #include <asm/setup.h>
>>>  #include <asm/atarihw.h>
>>> @@ -549,8 +550,10 @@ static void falcon_get_lock(void)
>>>
>>>         local_irq_save(flags);
>>>
>>> -       while (!in_irq() && falcon_got_lock && stdma_others_waiting())
>>> -               sleep_on(&falcon_fairness_wait);
>>> +       wait_event_cmd(falcon_fairness_wait,
>>> +                      !in_irq() && falcon_got_lock && 
>>> stdma_others_waiting(),

Replace by:
                      in_irq() || !falcon_got_lock || 
!stdma_others_waiting(),
 
Much better now. Needs stress testing on real hardware, but that'll have 
to wait until the weekend I guess.

Cheers,

    Michael

Re: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

[Michael Schmitz]

Arnd,

this one looks like it should be changed as well:
>>> @@ -562,7 +565,10 @@ static void falcon_get_lock(void)
>>>                         falcon_trying_lock = 0;
>>>                         wake_up(&falcon_try_wait);
>>>                 } else {
>>> -                       sleep_on(&falcon_try_wait);
>>> +                       wait_event_cmd(falcon_try_wait,
>>> +                                      !falcon_got_lock && 
>>> !falcon_trying_lock,

I think using falcon_got_lock && !falcon_trying_lock here reflects what 
we try to achieve - the if branch above this will set that exact
condition, then wake the wait queue. Any other calls to falcon_get_lock 
that have been waiting on the lock should then continue on
as the lock is now held by the driver. Thoughts?

I've not see that case used in a cursory test on hardware, more stress 
testing is needed (logging lock history to a kernel buffer instead
of clogging up the console).

Cheers,

     Michael

>>> + local_irq_restore(flags),
>>> +                                      local_irq_save(flags));
>>>                 }
>>>         }
>>>
>>> -- 
>>> 1.8.3.2
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe 
>>> linux-m68k" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>

Re: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

[Michael Schmitz]

Arnd, Geert,


On Tue, Jan 14, 2014 at 9:29 PM, Michael Schmitz <schmitzmic@gmail.com> wrote:
> Arnd,
>
> this one looks like it should be changed as well:
>
>>>> @@ -562,7 +565,10 @@ static void falcon_get_lock(void)
>>>>                         falcon_trying_lock = 0;
>>>>                         wake_up(&falcon_try_wait);
>>>>                 } else {
>>>> -                       sleep_on(&falcon_try_wait);
>>>> +                       wait_event_cmd(falcon_try_wait,
>>>> +                                      !falcon_got_lock &&
>>>> !falcon_trying_lock,
>
>
> I think using falcon_got_lock && !falcon_trying_lock here reflects what we
> try to achieve - the if branch above this will set that exact
> condition, then wake the wait queue. Any other calls to falcon_get_lock that
> have been waiting on the lock should then continue on
> as the lock is now held by the driver. Thoughts?

I can confirm that the condition (falcon_got_lock &&
!falcon_trying_lock) does work here - no problems for basic
operations. Arnd, do you want to handle this, or should I send the
updated patch via Geert?

A bit of stress testing using concurrent SCSI and IDE I/O has revealed
the driver still deadlocks when SCSI commands are queued from softirq
context and the ST-DMA lock is held by the IDE or floppy driver
(presumably; haven't verified that). I will try a few likely
workarounds that had worked in the past, but the real question is
this:

Does stdma_lock need the uninterruptible wait? The comment there says
it is needed to protect locked buffers, but this was all set up back
in the dark ages. IDE takes the host lock in addition to the ST-DMA
lock, SCSI still locks out interrupts where critical (I need to change
that) so it appears all should be safe? Comments?

Cheers,

  Michael

Re: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

[Michael Schmitz]

Geert,

I will submit patches to the Atari SCSI driver based on Arnd's patch 
later this week. Has Arnd's initial patch made it into mainline yet?

While working on the driver, I noticed my Falcon ran into a memory 
squeeze once a day, resulting in oom-killing processes and rendering the 
system unusable. Most likely culprit to trigger this is the daily 
updatedb run. This sort of trouble started pretty much with my work on 
the SCSI driver, based on commit 
aa5311c454ed0ff959adca29c65be2157f52a84c (3.13-rc7). Do you know of any 
memory leak affecting m68k, introduced between last November and 3.13-rc7?

Cheers,

    Michael
>> Arnd,
>>
>> your patch breaks the Atari NCR5380 SCSI driver (easily verified using 
>> ARAnyM). Last console output:
>>
>>     
>
> Thanks so much for testing and sorry for your troubles. It seems I
> got the wrong polarity on at least one of the conditions when converting
> from a while()-style loop to an until()-style wait_event loop.
> I did double-check all the patches before, but this one must have
> slipped through because the use is so obscure.
>
> 	Arnd
>
>   

[PATCH 0/3] m68k/atari - Atari NCR5380 SCSI driver fixes

[Michael Schmitz]

Geert,

this patch series brings the Atari NCR5380 driver up to date with current 3.x
(or even 2.6) series SCSI midlevel and error handling changes. 

The first patch fixes completion condition errors contained in Arnd Bergmann's
patch ([PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race). The locking
scheme still isn't 100% race free that way, but much improved, and I've seen
the driver successfully handle error recovery from aborting a disconnected
command via bus reset with all three patches applied. Never managed to do that
before. Not sure whether this one should be taken through your tree or handled
by Arnd rather. 

The other two are a rehash of old patches I had prepared somewhere along the
2.6 series - somehow or other these were lost when one of my old git trees
went pear shaped. Abort and reset handler return codes have changed, and the
SCSI midlevel now queues commands (in particular error handling ones) from
softirq context - the Falcon locking scheme was ill equipped to deal with that.

Another issue that surfaced while debugging SCSI deadlocks was the odd fact
that two interrupts get registered for the ST-DMA/floppy/SCSI/IDE interrupt:
the stdma_int multiplexer, and the IDE interrupt handler itself. I'll send a
separate patch for that. 

Cheers,

	Michael


[PATCH 1/3] m68k/atari - atari_scsi: fix wait_event completion conditions
[PATCH 2/3] m68k/atari - atari_scsi: change abort/reset return codes
[PATCH 3/3] m68k/atari - atari_scsi: punt if deadlocked on stdma_lock

[PATCH 1/3] m68k/atari - atari_scsi: fix wait_event completion conditions

[Michael Schmitz]

Fix patch by ArndB changing falcon_get_lock to use wait_event.
Some of the completion conditions had been missed when converting
from while() {} to do {} until() logic.

Signed-off-by: Michael Schmitz <schmitz@debian.org>
---
 drivers/scsi/atari_scsi.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 1986ecb..cc1b013 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -550,10 +550,10 @@ static void falcon_get_lock(void)
 
 	local_irq_save(flags);
 
-       wait_event_cmd(falcon_fairness_wait,
-                      !in_irq() && falcon_got_lock && stdma_others_waiting(),
-                      local_irq_restore(flags),
-                      local_irq_save(flags));
+	wait_event_cmd(falcon_fairness_wait,
+		in_irq() || !falcon_got_lock || !stdma_others_waiting(),
+		local_irq_restore(flags),
+		local_irq_save(flags));
 
 	while (!falcon_got_lock) {
 		if (in_irq())
@@ -566,9 +566,9 @@ static void falcon_get_lock(void)
 			wake_up(&falcon_try_wait);
 		} else {
                        wait_event_cmd(falcon_try_wait,
-                                      !falcon_got_lock && !falcon_trying_lock,
-                                      local_irq_restore(flags),
-                                      local_irq_save(flags));
+				falcon_got_lock && !falcon_trying_lock,
+				local_irq_restore(flags),
+				local_irq_save(flags));
 		}
 	}
 
-- 
1.7.0.4

[PATCH 2/3] m68k/atari - atari_scsi: change abort/reset return codes

[Michael Schmitz]

The abort/reset lowlevel return codes had changed with the new
error SCSI handling - update Atari NCR5380 driver to reflect this.

Change reset handling to clear queues only, do not attempt to
call done() on each command aborted by the reset. The EH code
should do that for us.
Queues _must_ be cleared, otherwise atari_scsi_bus_reset will not
release the ST-DMA lock, deadlocking further error recovery.

Signed-off-by: Michael Schmitz <schmitz@debian.org>
---
 drivers/scsi/atari_NCR5380.c |   29 ++++++++++++++++++-----------
 drivers/scsi/atari_scsi.c    |    2 +-
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/atari_NCR5380.c b/drivers/scsi/atari_NCR5380.c
index 0f3cdbc..465e63d 100644
--- a/drivers/scsi/atari_NCR5380.c
+++ b/drivers/scsi/atari_NCR5380.c
@@ -2683,11 +2683,11 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 			local_irq_restore(flags);
 			cmd->scsi_done(cmd);
 			falcon_release_lock_if_possible(hostdata);
-			return SCSI_ABORT_SUCCESS;
+			return SUCCESS;
 		} else {
 /*			local_irq_restore(flags); */
 			printk("scsi%d: abort of connected command failed!\n", HOSTNO);
-			return SCSI_ABORT_ERROR;
+			return FAILED;
 		}
 	}
 #endif
@@ -2711,7 +2711,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 			 * yet... */
 			tmp->scsi_done(tmp);
 			falcon_release_lock_if_possible(hostdata);
-			return SCSI_ABORT_SUCCESS;
+			return SUCCESS;
 		}
 	}
 
@@ -2729,7 +2729,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 	if (hostdata->connected) {
 		local_irq_restore(flags);
 		ABRT_PRINTK("scsi%d: abort failed, command connected.\n", HOSTNO);
-		return SCSI_ABORT_SNOOZE;
+		return FAILED;
 	}
 
 	/*
@@ -2764,7 +2764,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 			ABRT_PRINTK("scsi%d: aborting disconnected command.\n", HOSTNO);
 
 			if (NCR5380_select(instance, cmd, (int)cmd->tag))
-				return SCSI_ABORT_BUSY;
+				return FAILED;
 
 			ABRT_PRINTK("scsi%d: nexus reestablished.\n", HOSTNO);
 
@@ -2791,7 +2791,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 					local_irq_restore(flags);
 					tmp->scsi_done(tmp);
 					falcon_release_lock_if_possible(hostdata);
-					return SCSI_ABORT_SUCCESS;
+					return SUCCESS;
 				}
 			}
 		}
@@ -2816,7 +2816,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 	 */
 	falcon_release_lock_if_possible(hostdata);
 
-	return SCSI_ABORT_NOT_RUNNING;
+	return FAILED;
 }
 
 
@@ -2834,7 +2834,7 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 	SETUP_HOSTDATA(cmd->device->host);
 	int i;
 	unsigned long flags;
-#if 1
+#if defined(RESET_RUN_DONE)
 	Scsi_Cmnd *connected, *disconnected_queue;
 #endif
 
@@ -2859,7 +2859,14 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 	 * through anymore ... */
 	(void)NCR5380_read(RESET_PARITY_INTERRUPT_REG);
 
-#if 1	/* XXX Should now be done by midlevel code, but it's broken XXX */
+	/* MSch 20140115 - looking at the generic NCR5380 driver, all of this
+	 * should go.
+	 * Catch-22: if we don't clear all queues, the SCSI driver lock will
+	 * not be reset by atari_scsi_reset()!
+	 */
+
+#if defined(RESET_RUN_DONE)
+	/* XXX Should now be done by midlevel code, but it's broken XXX */
 	/* XXX see below                                            XXX */
 
 	/* MSch: old-style reset: actually abort all command processing here */
@@ -2915,7 +2922,7 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 	 * the midlevel code that the reset was SUCCESSFUL, and there is no
 	 * need to 'wake up' the commands by a request_sense
 	 */
-	return SCSI_RESET_SUCCESS | SCSI_RESET_BUS_RESET;
+	return SUCCESS;
 #else /* 1 */
 
 	/* MSch: new-style reset handling: let the mid-level do what it can */
@@ -2963,6 +2970,6 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 	local_irq_restore(flags);
 
 	/* we did no complete reset of all commands, so a wakeup is required */
-	return SCSI_RESET_WAKEUP | SCSI_RESET_BUS_RESET;
+	return SUCCESS;
 #endif /* 1 */
 }
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index cc1b013..5e19509 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -827,7 +827,7 @@ static int atari_scsi_bus_reset(Scsi_Cmnd *cmd)
 	} else {
 		atari_turnon_irq(IRQ_MFP_FSCSI);
 	}
-	if ((rv & SCSI_RESET_ACTION) == SCSI_RESET_SUCCESS)
+	if (rv == SUCCESS)
 		falcon_release_lock_if_possible(hostdata);
 
 	return rv;
-- 
1.7.0.4

[PATCH 3/3] m68k/atari - atari_scsi lock fixes: punt if deadlocked

[Michael Schmitz]

In case a SCSI command is queued from softirq context, and another
driver currently holds the ST-DMA lock, tell the SCSI midlevel to
hold off queueing commands for now. Something will hopefully resume
play later.

Signed-off-by: Michael Schmitz <schmitz@debian.org>
---
 drivers/scsi/atari_NCR5380.c |   12 +++++++++---
 drivers/scsi/atari_scsi.c    |   26 +++++++++++++++++++++-----
 2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/atari_NCR5380.c b/drivers/scsi/atari_NCR5380.c
index 465e63d..90a90e8 100644
--- a/drivers/scsi/atari_NCR5380.c
+++ b/drivers/scsi/atari_NCR5380.c
@@ -967,9 +967,15 @@ static int NCR5380_queue_command_lck(Scsi_Cmnd *cmd, void (*done)(Scsi_Cmnd *))
 	 * alter queues and touch the lock.
 	 */
 	if (!IS_A_TT()) {
-		/* perhaps stop command timer here */
-		falcon_get_lock();
-		/* perhaps restart command timer here */
+		/* MSch 20140119: check whether obtaining the ST-DMA lock did
+		 * succeed.
+		 * If the lock could not be acquired without risking to
+		 * deadlock, i.e. from softirq context with ST-DMA currently
+		 * otherwise locked, defer queueing further commands.
+		 */
+		if (falcon_get_lock()) {
+			return SCSI_MLQUEUE_HOST_BUSY;
+		}
 	}
 	if (!(hostdata->issue_queue) || (cmd->cmnd[0] == REQUEST_SENSE)) {
 		LIST(cmd, hostdata->issue_queue);
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 5e19509..2b62ce2 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -197,7 +197,7 @@ static unsigned long atari_dma_xfer_len(unsigned long wanted_len,
 static irqreturn_t scsi_tt_intr(int irq, void *dummy);
 static irqreturn_t scsi_falcon_intr(int irq, void *dummy);
 static void falcon_release_lock_if_possible(struct NCR5380_hostdata *hostdata);
-static void falcon_get_lock(void);
+static int falcon_get_lock(void);
 #ifdef CONFIG_ATARI_SCSI_RESET_BOOT
 static void atari_scsi_reset_boot(void);
 #endif
@@ -541,24 +541,39 @@ static void falcon_release_lock_if_possible(struct NCR5380_hostdata *hostdata)
  * Complicated, complicated.... Sigh...
  */
 
-static void falcon_get_lock(void)
+static int falcon_get_lock(void)
 {
 	unsigned long flags;
 
 	if (IS_A_TT())
-		return;
+		return 0;
 
 	local_irq_save(flags);
 
 	wait_event_cmd(falcon_fairness_wait,
-		in_irq() || !falcon_got_lock || !stdma_others_waiting(),
+		in_interrupt() || !falcon_got_lock || !stdma_others_waiting(),
 		local_irq_restore(flags),
 		local_irq_save(flags));
 
 	while (!falcon_got_lock) {
 		if (in_irq())
-			panic("Falcon SCSI hasn't ST-DMA lock in interrupt");
+			panic("Falcon SCSI hasn't got ST-DMA lock in irq");
 		if (!falcon_trying_lock) {
+			if (in_interrupt()) {
+				if (stdma_islocked()) {
+					/* MSch 20140119: problem -
+					 * cannot schedule in interrupt!
+					 * Unless stdma_lock can be modified
+					 * to interruptible wait, this will
+					 * be needed to avoid deadlocking here!
+					 * Return error to indicate command
+					 * queueing would deadlock, and allow
+					 * queue_command() to stall queueing.
+					 */
+					local_irq_restore(flags);
+					return 1;
+				}
+			}
 			falcon_trying_lock = 1;
 			stdma_lock(scsi_falcon_intr, NULL);
 			falcon_got_lock = 1;
@@ -575,6 +590,7 @@ static void falcon_get_lock(void)
 	local_irq_restore(flags);
 	if (!falcon_got_lock)
 		panic("Falcon SCSI: someone stole the lock :-(\n");
+	return 0;
 }
 
 
-- 
1.7.0.4

Re: [PATCH 0/3] m68k/atari - Atari NCR5380 SCSI driver fixes

[Geert Uytterhoeven]

Hi Michael,

On Tue, Jan 28, 2014 at 8:52 AM, Michael Schmitz <schmitzmic@gmail.com> wrote:
> this patch series brings the Atari NCR5380 driver up to date with current 3.x
> (or even 2.6) series SCSI midlevel and error handling changes.
>
> The first patch fixes completion condition errors contained in Arnd Bergmann's
> patch ([PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race). The locking
> scheme still isn't 100% race free that way, but much improved, and I've seen
> the driver successfully handle error recovery from aborting a disconnected
> command via bus reset with all three patches applied. Never managed to do that
> before. Not sure whether this one should be taken through your tree or handled
> by Arnd rather.
>
> The other two are a rehash of old patches I had prepared somewhere along the
> 2.6 series - somehow or other these were lost when one of my old git trees
> went pear shaped. Abort and reset handler return codes have changed, and the
> SCSI midlevel now queues commands (in particular error handling ones) from
> softirq context - the Falcon locking scheme was ill equipped to deal with that.

Can you please resend the series including
  1. the SCSI maintainer,
  2. the SCSI mailing list,
  3. Arnd Bergmann?

scripts/get_maintainer.pl is your friend.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 0/3] m68k/atari - Atari NCR5380 SCSI driver fixes (resent)

[Michael Schmitz]

Geert,

this patch series brings the Atari NCR5380 driver up to date with current 3.x
(or perhaps even 2.6) series SCSI midlevel and error handling changes.

The first patch fixes completion condition errors contained in Arnd Bergmann's
patch ([PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race). The locking
scheme still isn't 100% race free that way, but much improved, and I've seen
the driver successfully handle error recovery from aborting a disconnected
command via bus reset with all three patches applied (never managed to do that
since 2.4 days). 
Not sure whether this one should be taken through your tree or handled by Arnd
rather.

The other two are a rehash of old patches I had prepared somewhere along the
2.6 series - somehow or other these were lost when one of my old git trees
went pear shaped. 

Abort and reset handlers nowadays return SUCCESS or FAILED only, and the SCSI
midlevel now queues commands (in particular error handling ones) from soft
interrupt context - the Falcon locking scheme still is ill equipped to handle
that. Return codes have been changed and the reset handler in particular will
leave running command completion handlers to the midlevel now. 

If a SCSI command is queued while IDE or floppy have reserved the shared DMA
and interrupt, queue_command() may not wait for lock release and instead will
return SCSI_MLQUEUE_HOST_BUSY to indicate that. 

CC: to linux-scsi and the SCSI maintainer added as per your request. 

Cheers,

	Michael

[PATCH 1/3] m68k/atari - atari_scsi: fix wait_event completion conditions

[Michael Schmitz]

Fix patch by ArndB changing falcon_get_lock to use wait_event.
Some of the completion conditions had been missed when converting
from while() {} to do {} until() logic.

Signed-off-by: Michael Schmitz <schmitz@debian.org>
---
 drivers/scsi/atari_scsi.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 1986ecb..cc1b013 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -550,10 +550,10 @@ static void falcon_get_lock(void)
 
 	local_irq_save(flags);
 
-       wait_event_cmd(falcon_fairness_wait,
-                      !in_irq() && falcon_got_lock && stdma_others_waiting(),
-                      local_irq_restore(flags),
-                      local_irq_save(flags));
+	wait_event_cmd(falcon_fairness_wait,
+		in_irq() || !falcon_got_lock || !stdma_others_waiting(),
+		local_irq_restore(flags),
+		local_irq_save(flags));
 
 	while (!falcon_got_lock) {
 		if (in_irq())
@@ -566,9 +566,9 @@ static void falcon_get_lock(void)
 			wake_up(&falcon_try_wait);
 		} else {
                        wait_event_cmd(falcon_try_wait,
-                                      !falcon_got_lock && !falcon_trying_lock,
-                                      local_irq_restore(flags),
-                                      local_irq_save(flags));
+				falcon_got_lock && !falcon_trying_lock,
+				local_irq_restore(flags),
+				local_irq_save(flags));
 		}
 	}
 
-- 
1.7.0.4

[PATCH 2/3] m68k/atari - atari_scsi: change abort/reset return codes

[Michael Schmitz]

The abort/reset lowlevel return codes had changed with the new
error SCSI handling - update Atari NCR5380 driver to reflect this.

Change reset handling to clear queues only, do not attempt to
call done() on each command aborted by the reset. The EH code
should do that for us.
Queues _must_ be cleared, otherwise atari_scsi_bus_reset will not
release the ST-DMA lock, deadlocking further error recovery.

Signed-off-by: Michael Schmitz <schmitz@debian.org>
---
 drivers/scsi/atari_NCR5380.c |   29 ++++++++++++++++++-----------
 drivers/scsi/atari_scsi.c    |    2 +-
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/atari_NCR5380.c b/drivers/scsi/atari_NCR5380.c
index 0f3cdbc..465e63d 100644
--- a/drivers/scsi/atari_NCR5380.c
+++ b/drivers/scsi/atari_NCR5380.c
@@ -2683,11 +2683,11 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 			local_irq_restore(flags);
 			cmd->scsi_done(cmd);
 			falcon_release_lock_if_possible(hostdata);
-			return SCSI_ABORT_SUCCESS;
+			return SUCCESS;
 		} else {
 /*			local_irq_restore(flags); */
 			printk("scsi%d: abort of connected command failed!\n", HOSTNO);
-			return SCSI_ABORT_ERROR;
+			return FAILED;
 		}
 	}
 #endif
@@ -2711,7 +2711,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 			 * yet... */
 			tmp->scsi_done(tmp);
 			falcon_release_lock_if_possible(hostdata);
-			return SCSI_ABORT_SUCCESS;
+			return SUCCESS;
 		}
 	}
 
@@ -2729,7 +2729,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 	if (hostdata->connected) {
 		local_irq_restore(flags);
 		ABRT_PRINTK("scsi%d: abort failed, command connected.\n", HOSTNO);
-		return SCSI_ABORT_SNOOZE;
+		return FAILED;
 	}
 
 	/*
@@ -2764,7 +2764,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 			ABRT_PRINTK("scsi%d: aborting disconnected command.\n", HOSTNO);
 
 			if (NCR5380_select(instance, cmd, (int)cmd->tag))
-				return SCSI_ABORT_BUSY;
+				return FAILED;
 
 			ABRT_PRINTK("scsi%d: nexus reestablished.\n", HOSTNO);
 
@@ -2791,7 +2791,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 					local_irq_restore(flags);
 					tmp->scsi_done(tmp);
 					falcon_release_lock_if_possible(hostdata);
-					return SCSI_ABORT_SUCCESS;
+					return SUCCESS;
 				}
 			}
 		}
@@ -2816,7 +2816,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
 	 */
 	falcon_release_lock_if_possible(hostdata);
 
-	return SCSI_ABORT_NOT_RUNNING;
+	return FAILED;
 }
 
 
@@ -2834,7 +2834,7 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 	SETUP_HOSTDATA(cmd->device->host);
 	int i;
 	unsigned long flags;
-#if 1
+#if defined(RESET_RUN_DONE)
 	Scsi_Cmnd *connected, *disconnected_queue;
 #endif
 
@@ -2859,7 +2859,14 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 	 * through anymore ... */
 	(void)NCR5380_read(RESET_PARITY_INTERRUPT_REG);
 
-#if 1	/* XXX Should now be done by midlevel code, but it's broken XXX */
+	/* MSch 20140115 - looking at the generic NCR5380 driver, all of this
+	 * should go.
+	 * Catch-22: if we don't clear all queues, the SCSI driver lock will
+	 * not be reset by atari_scsi_reset()!
+	 */
+
+#if defined(RESET_RUN_DONE)
+	/* XXX Should now be done by midlevel code, but it's broken XXX */
 	/* XXX see below                                            XXX */
 
 	/* MSch: old-style reset: actually abort all command processing here */
@@ -2915,7 +2922,7 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 	 * the midlevel code that the reset was SUCCESSFUL, and there is no
 	 * need to 'wake up' the commands by a request_sense
 	 */
-	return SCSI_RESET_SUCCESS | SCSI_RESET_BUS_RESET;
+	return SUCCESS;
 #else /* 1 */
 
 	/* MSch: new-style reset handling: let the mid-level do what it can */
@@ -2963,6 +2970,6 @@ static int NCR5380_bus_reset(Scsi_Cmnd *cmd)
 	local_irq_restore(flags);
 
 	/* we did no complete reset of all commands, so a wakeup is required */
-	return SCSI_RESET_WAKEUP | SCSI_RESET_BUS_RESET;
+	return SUCCESS;
 #endif /* 1 */
 }
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index cc1b013..5e19509 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -827,7 +827,7 @@ static int atari_scsi_bus_reset(Scsi_Cmnd *cmd)
 	} else {
 		atari_turnon_irq(IRQ_MFP_FSCSI);
 	}
-	if ((rv & SCSI_RESET_ACTION) == SCSI_RESET_SUCCESS)
+	if (rv == SUCCESS)
 		falcon_release_lock_if_possible(hostdata);
 
 	return rv;
-- 
1.7.0.4

[PATCH 3/3] m68k/atari - atari_scsi lock fixes: punt if deadlocked

[Michael Schmitz]

In case a SCSI command is queued from softirq context, and another
driver currently holds the ST-DMA lock, tell the SCSI midlevel to
hold off queueing commands for now. Something will hopefully resume
play later.

Signed-off-by: Michael Schmitz <schmitz@debian.org>
---
 drivers/scsi/atari_NCR5380.c |   12 +++++++++---
 drivers/scsi/atari_scsi.c    |   26 +++++++++++++++++++++-----
 2 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/atari_NCR5380.c b/drivers/scsi/atari_NCR5380.c
index 465e63d..90a90e8 100644
--- a/drivers/scsi/atari_NCR5380.c
+++ b/drivers/scsi/atari_NCR5380.c
@@ -967,9 +967,15 @@ static int NCR5380_queue_command_lck(Scsi_Cmnd *cmd, void (*done)(Scsi_Cmnd *))
 	 * alter queues and touch the lock.
 	 */
 	if (!IS_A_TT()) {
-		/* perhaps stop command timer here */
-		falcon_get_lock();
-		/* perhaps restart command timer here */
+		/* MSch 20140119: check whether obtaining the ST-DMA lock did
+		 * succeed.
+		 * If the lock could not be acquired without risking to
+		 * deadlock, i.e. from softirq context with ST-DMA currently
+		 * otherwise locked, defer queueing further commands.
+		 */
+		if (falcon_get_lock()) {
+			return SCSI_MLQUEUE_HOST_BUSY;
+		}
 	}
 	if (!(hostdata->issue_queue) || (cmd->cmnd[0] == REQUEST_SENSE)) {
 		LIST(cmd, hostdata->issue_queue);
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 5e19509..2b62ce2 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -197,7 +197,7 @@ static unsigned long atari_dma_xfer_len(unsigned long wanted_len,
 static irqreturn_t scsi_tt_intr(int irq, void *dummy);
 static irqreturn_t scsi_falcon_intr(int irq, void *dummy);
 static void falcon_release_lock_if_possible(struct NCR5380_hostdata *hostdata);
-static void falcon_get_lock(void);
+static int falcon_get_lock(void);
 #ifdef CONFIG_ATARI_SCSI_RESET_BOOT
 static void atari_scsi_reset_boot(void);
 #endif
@@ -541,24 +541,39 @@ static void falcon_release_lock_if_possible(struct NCR5380_hostdata *hostdata)
  * Complicated, complicated.... Sigh...
  */
 
-static void falcon_get_lock(void)
+static int falcon_get_lock(void)
 {
 	unsigned long flags;
 
 	if (IS_A_TT())
-		return;
+		return 0;
 
 	local_irq_save(flags);
 
 	wait_event_cmd(falcon_fairness_wait,
-		in_irq() || !falcon_got_lock || !stdma_others_waiting(),
+		in_interrupt() || !falcon_got_lock || !stdma_others_waiting(),
 		local_irq_restore(flags),
 		local_irq_save(flags));
 
 	while (!falcon_got_lock) {
 		if (in_irq())
-			panic("Falcon SCSI hasn't ST-DMA lock in interrupt");
+			panic("Falcon SCSI hasn't got ST-DMA lock in irq");
 		if (!falcon_trying_lock) {
+			if (in_interrupt()) {
+				if (stdma_islocked()) {
+					/* MSch 20140119: problem -
+					 * cannot schedule in interrupt!
+					 * Unless stdma_lock can be modified
+					 * to interruptible wait, this will
+					 * be needed to avoid deadlocking here!
+					 * Return error to indicate command
+					 * queueing would deadlock, and allow
+					 * queue_command() to stall queueing.
+					 */
+					local_irq_restore(flags);
+					return 1;
+				}
+			}
 			falcon_trying_lock = 1;
 			stdma_lock(scsi_falcon_intr, NULL);
 			falcon_got_lock = 1;
@@ -575,6 +590,7 @@ static void falcon_get_lock(void)
 	local_irq_restore(flags);
 	if (!falcon_got_lock)
 		panic("Falcon SCSI: someone stole the lock :-(\n");
+	return 0;
 }
 
 
-- 
1.7.0.4

Re: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

[Arnd Bergmann]

On Monday 27 January 2014, Michael Schmitz wrote:
> I will submit patches to the Atari SCSI driver based on Arnd's patch 
> later this week. Has Arnd's initial patch made it into mainline yet?

I've been a little occupied with other work recently, so I have never
gotten around to post a non-RFC version of the sleep_on patches.
Some have been picked up by various maintainers anyway, but this
one has not.

Thanks so much for taking care of this one!

	Arnd

Re: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

[schmitz]

Hi Arnd,
> On Monday 27 January 2014, Michael Schmitz wrote:
>   
>> I will submit patches to the Atari SCSI driver based on Arnd's patch 
>> later this week. Has Arnd's initial patch made it into mainline yet?
>>     
>
> I've been a little occupied with other work recently, so I have never
> gotten around to post a non-RFC version of the sleep_on patches.
> Some have been picked up by various maintainers anyway, but this
> one has not.
>   

OK, I will resend a merged patch for you or Geert to submit further up 
the food chain.

> Thanks so much for taking care of this one!
>   

No matter - thanks for taking the time to look into arcane stuff like 
Atari SCSI drivers!

Cheers,

    Michael

> 	Arnd
>   

Re: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

[Geert Uytterhoeven]

Hi Michael,

On Mon, Jan 27, 2014 at 9:28 AM, Michael Schmitz <schmitzmic@gmail.com> wrote:
> While working on the driver, I noticed my Falcon ran into a memory squeeze
> once a day, resulting in oom-killing processes and rendering the system
> unusable. Most likely culprit to trigger this is the daily updatedb run.
> This sort of trouble started pretty much with my work on the SCSI driver,
> based on commit aa5311c454ed0ff959adca29c65be2157f52a84c (3.13-rc7). Do you
> know of any memory leak affecting m68k, introduced between last November and
> 3.13-rc7?

How much RAM and swap do you have?

Modern kernels just require more memory...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

[schmitz]

Hi Geert,

>
> While working on the driver, I noticed my Falcon ran into a memory 
> squeeze once a day, resulting in oom-killing processes and rendering 
> the system unusable. Most likely culprit to trigger this is the daily 
> updatedb run. This sort of trouble started pretty much with my work on 
> the SCSI driver, based on commit 
> aa5311c454ed0ff959adca29c65be2157f52a84c (3.13-rc7). Do you know of 
> any memory leak affecting m68k, introduced between last November and 
> 3.13-rc7?

Looks like Arnd's patch is not to blame, as I get a memory leak without 
it as well.

Straight after boot:
schmitz@hobbes:~$ cat /proc/interrupts
13:     316007     atari  timer
15:       2151     atari  ST-DMA floppy,ACSI,IDE,Falcon-SCSI, ide0
schmitz@hobbes:~$ free
             total       used       free     shared    buffers     cached
Mem:        526996      41928     485068          0       1944       9548
-/+ buffers/cache:      30436     496560

One day with no significant activity:
 13:    8895122     atari  timer
 15:      17483     atari  ST-DMA floppy,ACSI,IDE,Falcon-SCSI, ide0
Mem:        526996      83616     443380          0      17324      12732
-/+ buffers/cache:      53560     473436

Two days ...
 13:   17385461     atari  timer
 15:      31264     atari  ST-DMA floppy,ACSI,IDE,Falcon-SCSI, ide0
Mem:        526996      99528     427468          0      25136      12780
-/+ buffers/cache:      61612     465384

I'll try and go backwards to see when this all started. I switched 
toolchains in December, but that should not cause such behaviour, I 
suppose?

Cheers,

    Michael

>
> Cheers,
>
>    Michael
>>> Arnd,
>>>
>>> your patch breaks the Atari NCR5380 SCSI driver (easily verified 
>>> using ARAnyM). Last console output:
>>>
>>>     
>>
>> Thanks so much for testing and sorry for your troubles. It seems I
>> got the wrong polarity on at least one of the conditions when converting
>> from a while()-style loop to an until()-style wait_event loop.
>> I did double-check all the patches before, but this one must have
>> slipped through because the use is so obscure.
>>
>>     Arnd
>>
>>   
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

[schmitz]

Hi Geert,
>> While working on the driver, I noticed my Falcon ran into a memory squeeze
>> once a day, resulting in oom-killing processes and rendering the system
>> unusable. Most likely culprit to trigger this is the daily updatedb run.
>> This sort of trouble started pretty much with my work on the SCSI driver,
>> based on commit aa5311c454ed0ff959adca29c65be2157f52a84c (3.13-rc7). Do you
>> know of any memory leak affecting m68k, introduced between last November and
>> 3.13-rc7?
>>     
>
> How much RAM and swap do you have?
>   

14 MB ST-RAM, 512 MB FastRAM, 2GB swap.

> Modern kernels just require more memory...
>   
Surely not that much more, I hope?

Cheers,

    Michael


>   

Re: [PATCH, RFC 02/30] scsi: atari_scsi: fix sleep_on race

[Geert Uytterhoeven]

On Thu, Jan 30, 2014 at 9:08 AM, schmitz
<schmitz@biophys.uni-duesseldorf.de> wrote:
>>> While working on the driver, I noticed my Falcon ran into a memory
>>> squeeze
>>> once a day, resulting in oom-killing processes and rendering the system
>>> unusable. Most likely culprit to trigger this is the daily updatedb run.
>>> This sort of trouble started pretty much with my work on the SCSI driver,
>>> based on commit aa5311c454ed0ff959adca29c65be2157f52a84c (3.13-rc7). Do
>>> you
>>> know of any memory leak affecting m68k, introduced between last November
>>> and
>>> 3.13-rc7?
>>
>> How much RAM and swap do you have?
>
> 14 MB ST-RAM, 512 MB FastRAM, 2GB swap.
>
>> Modern kernels just require more memory...
>
> Surely not that much more, I hope?

512 MB should be plenty ;-)
My poor m68k box has 500 MB less...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds