Ping...
On Fri, 31 Oct 2014, Finn Thain wrote:
On Thu, 30 Oct 2014, Hannes Reinecke wrote:
On 10/27/2014 06:26 AM, Finn Thain wrote:
Simplify falcon_release_lock_if_possible() by making callers
responsible for disabling local IRQ's, which they must do anyway to
correctly synchronize the ST DMA "lock" with core driver data
structures. Move this synchronization logic to the core driver with
which it is tightly coupled.
Other LLD's like sun3_scsi and mac_scsi that can make use of this core
driver can just stub out the NCR5380_acquire_dma_irq() and
NCR5380_release_dma_irq() calls so the compiler will eliminate the ST
DMA code.
Remove a redundant local_irq_save/restore pair (irq's are disabled for
interrupt handlers these days). Revise the locking for
atari_scsi_bus_reset(): use local_irq_save/restore() instead of
atari_turnoff/turnon_irq(). There is no guarantee that atari_scsi
still holds the ST DMA lock during EH, so atari_turnoff/turnon_irq()
could end up dropping an IDE or floppy interrupt.
Signed-off-by: Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx>
---
drivers/scsi/atari_NCR5380.c | 72 ++++++++++++++++++++++++++-----------------
drivers/scsi/atari_scsi.c | 47 ++++++++++------------------
2 files changed, 62 insertions(+), 57 deletions(-)
Index: linux/drivers/scsi/atari_NCR5380.c
===================================================================
--- linux.orig/drivers/scsi/atari_NCR5380.c 2014-10-27 16:25:54.000000000 +1100
+++ linux/drivers/scsi/atari_NCR5380.c 2014-10-27 16:25:55.000000000 +1100
@@ -926,7 +926,7 @@ static int NCR5380_queue_command(struct
* alter queues and touch the lock.
*/
/* perhaps stop command timer here */
- if (!falcon_get_lock())
+ if (!NCR5380_acquire_dma_irq(instance))
return SCSI_MLQUEUE_HOST_BUSY;
/* perhaps restart command timer here */
@@ -962,6 +962,18 @@ static int NCR5380_queue_command(struct
return 0;
}
+static inline void maybe_release_dma_irq(struct Scsi_Host *instance)
+{
+ struct NCR5380_hostdata *hostdata = shost_priv(instance);
+
+ /* Caller does the locking needed to set & test these data atomically */
+ if (!hostdata->disconnected_queue &&
+ !hostdata->issue_queue &&
+ !hostdata->connected &&
+ !hostdata->retain_dma_intr)
+ NCR5380_release_dma_irq(instance);
+}
+
/*
* Function : NCR5380_main (void)
*
@@ -1084,9 +1096,11 @@ static void NCR5380_main(struct work_str
cmd_get_tag(tmp, tmp->cmnd[0] != REQUEST_SENSE);
#endif
if (!NCR5380_select(instance, tmp)) {
+ local_irq_disable();
hostdata->retain_dma_intr--;
/* release if target did not response! */
- falcon_release_lock_if_possible(hostdata);
+ maybe_release_dma_irq(instance);
+ local_irq_restore(flags);
break;
} else {
local_irq_disable();
@@ -2085,11 +2099,12 @@ static void NCR5380_information_transfer
case COMMAND_COMPLETE:
/* Accept message by clearing ACK */
NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
- /* ++guenther: possible race with Falcon locking */
- hostdata->retain_dma_intr++;
- hostdata->connected = NULL;
dprintk(NDEBUG_QUEUES, "scsi%d: command for target %d, lun %llu "
"completed\n", HOSTNO, cmd->device->id, cmd->device->lun);
+
+ local_irq_save(flags);
+ hostdata->retain_dma_intr++;
+ hostdata->connected = NULL;
#ifdef SUPPORT_TAGS
cmd_free_tag(cmd);
if (status_byte(cmd->SCp.Status) == QUEUE_FULL) {
@@ -2148,17 +2163,17 @@ static void NCR5380_information_transfer
dprintk(NDEBUG_AUTOSENSE, "scsi%d: performing request sense\n", HOSTNO);
- local_irq_save(flags);
LIST(cmd,hostdata->issue_queue);
SET_NEXT(cmd, hostdata->issue_queue);
hostdata->issue_queue = (struct scsi_cmnd *) cmd;
- local_irq_restore(flags);
dprintk(NDEBUG_QUEUES, "scsi%d: REQUEST SENSE added to head of "
"issue queue\n", H_NO(cmd));
} else {
cmd->scsi_done(cmd);
}
+ local_irq_restore(flags);
+
NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
/*
* Restore phase bits to 0 so an interrupted selection,
Probably a stupid question, but you're disabling local interrupts.
scsi_done() OTOH is triggering a soft irq.
Isn't that how all bottom halves work?
Does this work?
Yes, I've tested that particular code. I'm pretty sure Michael has too.
A quick search turned up another example of this in
zfcp_fsf_fcp_cmnd_handler().
Shouldn't we enable interrupts before calling scsi_done() ?
I don't see anything in the scsi_done() implementation in scsi_lib.c that
would be problematic. The description says "This function is interrupt
context safe". The same would have to apply to scsi_eh_done().
--
--
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
