Re: [PATCH] m68k: Atari SCSI - ST-DMA locking and error handling fixes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

On Sun, Apr 26, 2009 at 09:24, Michael Schmitz
<schmitz@xxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:

some fixes to the Atari NCR5380 SCSI driver: if the ST-DMA lock cannot be taken
in the queue function, indicate host busy to the block layer. Fix return codes
of abort and reset functions, and add scsi_unregister to module exit.
Error handling is still messed up, most likely due to missing ST-DMA lock in
abort and reset. This is still WIP, but might benefit users with a Falcon less
prone to SCSI lockups, i.e. more stable SCSI clock signal.


Any news on this one ("still WIP")?


Signed-off-by: Michael Schmitz <schmitz@xxxxxxxxxx>

ÂMichael

---
Âdrivers/scsi/atari_NCR5380.c | Â 78 +++++++++++++++++++++++++++--------------
Âdrivers/scsi/atari_scsi.c  Â|  47 +++++++++++++++++++++----
Â2 files changed, 91 insertions(+), 34 deletions(-)

diff --git a/drivers/scsi/atari_NCR5380.c b/drivers/scsi/atari_NCR5380.c
index 0471f88..3ba46d5 100644
--- a/drivers/scsi/atari_NCR5380.c
+++ b/drivers/scsi/atari_NCR5380.c
@@ -966,13 +966,6 @@ static int NCR5380_queue_command(Scsi_Cmnd *cmd, void (*done)(Scsi_Cmnd *))

   Âcmd->result = 0;

- Â Â Â /*
- Â Â Â Â* Insert the cmd into the issue queue. Note that REQUEST SENSE
- Â Â Â Â* commands are added to the head of the queue since any command will
- Â Â Â Â* clear the contingent allegiance condition that exists and the
- Â Â Â Â* sense data is only guaranteed to be valid while the condition exists.
- Â Â Â Â*/
-
   Âlocal_irq_save(flags);
   Â/* ++guenther: now that the issue queue is being set up, we can lock ST-DMA.
    * Otherwise a running NCR5380_main may steal the lock.
@@ -987,10 +980,32 @@ static int NCR5380_queue_command(Scsi_Cmnd *cmd, void (*done)(Scsi_Cmnd *))
    * alter queues and touch the lock.
    */
   Âif (!IS_A_TT()) {
+ Â Â Â Â Â Â Â int rv;
+ Â Â Â Â Â Â Â /* MSch: since we may now get called from softirq context here,
+ Â Â Â Â Â Â Â Â* we cannot always sleep while waiting for the lock.
+ Â Â Â Â Â Â Â Â* Use status from falcon_get_lock() to detect if the lock was
+ Â Â Â Â Â Â Â Â* successfully taken, and return appropriate status to midlevel
+ Â Â Â Â Â Â Â Â* otherwise.
+ Â Â Â Â Â Â Â Â* We used to pause the midlevel SCSI timer here; races between
+ Â Â Â Â Â Â Â Â* command timeout and lowlevel error handling should not hurt
+ Â Â Â Â Â Â Â Â* because the command isn't in any of the queues yet.
+ Â Â Â Â Â Â Â Â*/
       Â/* perhaps stop command timer here */
- Â Â Â Â Â Â Â falcon_get_lock();
+ Â Â Â Â Â Â Â rv = falcon_get_lock();
       Â/* perhaps restart command timer here */
+ Â Â Â Â Â Â Â if (rv) {
+ Â Â Â Â Â Â Â Â Â Â Â local_irq_restore(flags);
+ Â Â Â Â Â Â Â Â Â Â Â return SCSI_MLQUEUE_HOST_BUSY;
+ Â Â Â Â Â Â Â }
   Â}
+
+ Â Â Â /*
+ Â Â Â Â* Insert the cmd into the issue queue. Note that REQUEST SENSE
+ Â Â Â Â* commands are added to the head of the queue since any command will
+ Â Â Â Â* clear the contingent allegiance condition that exists and the
+ Â Â Â Â* sense data is only guaranteed to be valid while the condition exists.
+ Â Â Â Â*/
+
   Âif (!(hostdata->issue_queue) || (cmd->cmnd[0] == REQUEST_SENSE)) {
       ÂLIST(cmd, hostdata->issue_queue);
       ÂSET_NEXT(cmd, hostdata->issue_queue);
@@ -1014,10 +1029,12 @@ static int NCR5380_queue_command(Scsi_Cmnd *cmd, void (*done)(Scsi_Cmnd *))
    * If we're not in an interrupt, we can call NCR5380_main()
    * unconditionally, because it cannot be already running.
    */
- Â Â Â if (in_interrupt() || ((flags >> 8) & 7) >= 6)
+ Â Â Â /* As of 2.6.19, the coroutine has to be put on the task queue instead
+ Â Â Â Â* of being called directly. It might still be called directly if not
+ Â Â Â Â* in softirq, though. Need to test this.
+ Â Â Â Â*/
       Âqueue_main();
- Â Â Â else
- Â Â Â Â Â Â Â NCR5380_main(NULL);
+
   Âreturn 0;
Â}

@@ -1463,6 +1480,7 @@ static int NCR5380_select(struct Scsi_Host *instance, Scsi_Cmnd *cmd, int tag)
   ÂARB_PRINTK("scsi%d: arbitration complete\n", HOSTNO);

   Âif (hostdata->connected) {
+
       ÂNCR5380_write(MODE_REG, MR_BASE);
       Âreturn -1;
   Â}
@@ -2065,7 +2083,6 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
               Â/*
                * The preferred transfer method is going to be
                * PSEUDO-DMA for systems that are strictly PIO,
- Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â* since we can let the hardware do the handshaking.
                *
                * For this to work, we need to know the transfersize
                * ahead of time, since the pseudo-DMA code will sit
@@ -2631,7 +2648,7 @@ static void NCR5380_reselect(struct Scsi_Host *instance)
Â* Â Â host byte of the result field to, if zero DID_ABORTED is
Â* Â Â used.
Â*
- * Returns : 0 - success, -1 on failure.
+ * Returns : SUCCESS - success, FAILED on failure.
Â*
Â* XXX - there is no way to abort the command that is currently
Â* Â Â Âconnected, you have to wait for it to complete. ÂIf this is
@@ -2701,11 +2718,12 @@ 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); */
+ Â Â Â Â Â Â Â Â Â Â Â /* not sure */
+ Â Â Â Â Â Â Â Â Â Â Â local_irq_restore(flags);
           Âprintk("scsi%d: abort of connected command failed!\n", HOSTNO);
- Â Â Â Â Â Â Â Â Â Â Â return SCSI_ABORT_ERROR;
+ Â Â Â Â Â Â Â Â Â Â Â return FAILED;
       Â}
   Â}
Â#endif
@@ -2729,7 +2747,7 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
            * yet... */
           Âtmp->scsi_done(tmp);
           Âfalcon_release_lock_if_possible(hostdata);
- Â Â Â Â Â Â Â Â Â Â Â return SCSI_ABORT_SUCCESS;
+ Â Â Â Â Â Â Â Â Â Â Â return SUCCESS;
       Â}
   Â}

@@ -2747,7 +2765,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;
   Â}

   Â/*
@@ -2782,7 +2800,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);

@@ -2809,7 +2827,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;
               Â}
           Â}
       Â}
@@ -2835,7 +2853,9 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
    */
   Âfalcon_release_lock_if_possible(hostdata);

- Â Â Â return SCSI_ABORT_NOT_RUNNING;
+ Â Â Â /* NCR5380 has FAILED here */
+ Â Â Â /* return SUCCESS; */
+ Â Â Â return FAILED;
Â}


@@ -2844,16 +2864,18 @@ int NCR5380_abort(Scsi_Cmnd *cmd)
Â*
Â* Purpose : reset the SCSI bus.
Â*
- * Returns : SCSI_RESET_WAKEUP
+ * Returns : SUCCESS
Â*
Â*/

+#undef FALCON_RESET_KILL_QUEUES
+
Âstatic int NCR5380_bus_reset(Scsi_Cmnd *cmd)
Â{
   ÂSETUP_HOSTDATA(cmd->device->host);
   Âint i;
   Âunsigned long flags;
-#if 1
+#if defined(FALCON_RESET_KILL_QUEUES)
   ÂScsi_Cmnd *connected, *disconnected_queue;
Â#endif

@@ -2878,7 +2900,8 @@ 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 */
+#if defined(FALCON_RESET_KILL_QUEUES)
+ Â Â Â /* 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 */
@@ -2934,7 +2957,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 */
@@ -2982,6 +3005,7 @@ 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;
-#endif /* 1 */
+ Â Â Â /* The new error handler code implicitly does this for us anyway */
+ Â Â Â return SUCCESS;
+#endif /* defined(FALCON_RESET_KILL_QUEUES) */
Â}
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index ad7a23a..9ebfbc6 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -196,7 +196,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
@@ -498,6 +498,16 @@ static int falcon_dont_release = 0;
Â* again (but others waiting longer more probably will win).
Â*/

+/*
+ * MSch 20061228: in 2.6.x, the fairness wait appears to open a race with
+ * concurrent use of the lock by the IDE driver. Once the lock is taken by
+ * IDE, the SCSI queuecmd function can no longer schedule because it is run
+ * from softirq context a lot.
+ * Disable the fairness wait until I have had time to sort out the lock issues.
+ */
+#undef FALCON_FAIRNESS_WAIT
+#define FALCON_NEVER_SLEEP 1
+
Âstatic void falcon_release_lock_if_possible(struct NCR5380_hostdata *hostdata)
Â{
   Âunsigned long flags;
@@ -519,7 +529,9 @@ static void falcon_release_lock_if_possible(struct NCR5380_hostdata *hostdata)
       Â}
       Âfalcon_got_lock = 0;
       Âstdma_release();
+#if defined(FALCON_FAIRNESS_WAIT)
       Âwake_up(&falcon_fairness_wait);
+#endif
   Â}

   Âlocal_irq_restore(flags);
@@ -540,21 +552,37 @@ static void falcon_release_lock_if_possible(struct NCR5380_hostdata *hostdata)
Â* Complicated, complicated.... Sigh...
Â*/

-static void falcon_get_lock(void)
+/* MSch 20061229: in order to avoid sleeping when the lock is not available,
+ * return the current lock state here. atari_queue_command() will then return
+ * with error, causing the midlevel code to pause request submission.
+ */
+static int falcon_get_lock(void)
Â{
   Âunsigned long flags;

   Âif (IS_A_TT())
- Â Â Â Â Â Â Â return;
+ Â Â Â Â Â Â Â return 0;

   Âlocal_irq_save(flags);

- Â Â Â while (!in_irq() && falcon_got_lock && stdma_others_waiting())
+#if defined(FALCON_FAIRNESS_WAIT)
+ Â Â Â /* MSch: we may not sleep even while in softirq ! */
+ Â Â Â while (!in_interrupt() && falcon_got_lock && stdma_others_waiting())
       Âsleep_on(&falcon_fairness_wait);
-
+#endif
   Âwhile (!falcon_got_lock) {
       Âif (in_irq())
           Âpanic("Falcon SCSI hasn't ST-DMA lock in interrupt");
+ Â Â Â Â Â Â Â /* we may not sleep in soft interrupts neither, so bail out */
+#if defined(FALCON_NEVER_SLEEP)
+ Â Â Â Â Â Â Â if (stdma_islocked()) {
+#else
+ Â Â Â Â Â Â Â if (in_softirq() && stdma_islocked()) {
+#endif
+ Â Â Â Â Â Â Â Â Â Â Â // printk(KERN_INFO "Falcon SCSI does not hold ST-DMA lock in softirq!\n" );
+ Â Â Â Â Â Â Â Â Â Â Â local_irq_restore(flags);
+ Â Â Â Â Â Â Â Â Â Â Â return 1;
+ Â Â Â Â Â Â Â }
       Âif (!falcon_trying_lock) {
           Âfalcon_trying_lock = 1;
           Âstdma_lock(scsi_falcon_intr, NULL);
@@ -569,6 +597,8 @@ static void falcon_get_lock(void)
   Âlocal_irq_restore(flags);
   Âif (!falcon_got_lock)
       Âpanic("Falcon SCSI: someone stole the lock :-(\n");
+
+ Â Â Â return 0;
Â}


@@ -589,7 +619,7 @@ int atari_queue_command(Scsi_Cmnd *cmd, void (*done)(Scsi_Cmnd *))
Â#endif


-int __init atari_scsi_detect(struct scsi_host_template *host)
+int atari_scsi_detect(struct scsi_host_template *host)
Â{
   Âstatic int called = 0;
   Âstruct Scsi_Host *instance;
@@ -652,6 +682,8 @@ int __init atari_scsi_detect(struct scsi_host_template *host)
       Â}
       Âatari_dma_phys_buffer = virt_to_phys(atari_dma_buffer);
       Âatari_dma_orig_addr = 0;
+ Â Â Â Â Â Â Â printk(KERN_ERR "atari_scsi_detect: ST-RAM "
+ Â Â Â Â Â Â Â Â Â Â Â "double buffer at %p\n", atari_dma_phys_buffer);
   Â}
Â#endif
   Âinstance = scsi_register(host, sizeof(struct NCR5380_hostdata));
@@ -745,6 +777,7 @@ int atari_scsi_release(struct Scsi_Host *sh)
Â{
   Âif (IS_A_TT())
       Âfree_irq(IRQ_TT_MFP_SCSI, sh);
+ Â Â Â scsi_unregister(atari_scsi_host);
   Âif (atari_dma_buffer)
       Âatari_stram_free(atari_dma_buffer);
   Âreturn 1;
@@ -828,7 +861,7 @@ 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.5.6.5






-- 
Gr{oetje,eeting}s,

            Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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
--
To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[Index of Archives]     [Video for Linux]     [Yosemite News]     [Linux S/390]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux