Re: [PATCH v2 22/36] atari_scsi: Fix atari_scsi deadlocks on Falcon

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

 



Finn,
Index: linux/drivers/scsi/atari_NCR5380.c
===================================================================
--- linux.orig/drivers/scsi/atari_NCR5380.c	2014-10-27 16:25:36.000000000
+1100
+++ linux/drivers/scsi/atari_NCR5380.c	2014-10-27 16:25:45.000000000
+1100
@@ -879,10 +879,10 @@ static void NCR5380_exit(struct Scsi_Hos
   *
   */
  -static int NCR5380_queue_command_lck(struct scsi_cmnd *cmd,
-                                     void (*done)(struct scsi_cmnd *))
+static int NCR5380_queue_command(struct Scsi_Host *instance,
+                                 struct scsi_cmnd *cmd)
  {
-	SETUP_HOSTDATA(cmd->device->host);
+	struct NCR5380_hostdata *hostdata = shost_priv(instance);
   struct scsi_cmnd *tmp;
   unsigned long flags;
Nitpick - why did this change set go into this particular patch? Because you
are converting from NCR5380_queue_command_lck to NCR5380_queue_command?

That's right.

OK, I just wanted to understand why it's being combined with the stram.c patch (and the directly resulting changes in the SCSI driver).

The change to NCR5380_queue_command() can only now be done, as a result of the deadlock fixes in this patch, so it follows logically. Makes perfectly good sense to me now, thanks.


  @@ -893,7 +893,7 @@ static int NCR5380_queue_command_lck(str
    printk(KERN_NOTICE "scsi%d: WRITE attempted with NO_WRITE debugging flag
    set\n",
           H_NO(cmd));
  		cmd->result = (DID_ERROR << 16);
-		done(cmd);
+		cmd->scsi_done(cmd);
   	return 0;
  	}
  #endif /* (NDEBUG & NDEBUG_NO_WRITE) */
@@ -904,8 +904,6 @@ static int NCR5380_queue_command_lck(str
    */
SET_NEXT(cmd, NULL);
-	cmd->scsi_done = done;
-
   cmd->result = 0;
/*
Ditto for these two.

Again, it follows from the differences in the formal parameters between NCR5380_queue_command_lck() and NCR5380_queue_command().

Yep, I got that bit. Thanks for the explanation, and apologies for the noise.

@@ -915,7 +913,6 @@ static int NCR5380_queue_command_lck(str
    * 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.
    * Lock before actually inserting due to fairness reasons explained in
@@ -928,11 +925,13 @@ static int NCR5380_queue_command_lck(str
    * because also a timer int can trigger an abort or reset, which would
    * alter queues and touch the lock.
    */
-	if (!IS_A_TT()) {
-		/* perhaps stop command timer here */
-		falcon_get_lock();
-		/* perhaps restart command timer here */
-	}
+	/* perhaps stop command timer here */
+	if (!falcon_get_lock())
+		return SCSI_MLQUEUE_HOST_BUSY;
+	/* perhaps restart command timer here */
+
The comments about stopping and restarting the command timer can be removed. In 2.4 kernels, the driver would tweak the timers and wait on the lock unconditionally, Can't ne done anymore, for so many reasons.

Well, you sent an acked-by for this patch, so I'm a bit confused. Do you want me to re-spin it?

Not at all - this was just something I wanted to raise for future patches, before it slips my mind, again. Sorry I did not make that clear at all.

I expect we will have an opportunity to address this when we embark on a wider cleanup of comments throughout the driver - most of these comments were for the 0.9 kernel series and might still be somewhat germane up to 2.4. Might be totally misleading now.

Not a priority at all - I know you have bigger fish to fry. I should not have brought it up in this context.

No problems at all with this patch (or indeed with the rest of the series).

Cheers,

   Michael

--
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




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux