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-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
