[PATCH/RFT] libata "DMA timeout" fix

[PATCH/RFT] libata "DMA timeout" fix

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]


The desired effect of a DMA timeout should be to throw an I/O error, but 
that doesn't appear to be happening.

Those seeing DMA timeout messages, please test this patch.

Kernel hacker note:  James B recommended that I implement my own 
scsi_done() function, which duplicates the real scsi_done() but omits 
the scsi_delete_timer() call.  This is probably the best long term fix, 
but doing so involves exporting several currently-private bits of SCSI 
mid-layer, which I would rather not do.  Probably best to create a 
__scsi_done() inside the SCSI mid-layer, and call that.

	Jeff, the only user of ->eh_strategy_handler() in any kernel




[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 624 bytes --]

===== drivers/scsi/libata-core.c 1.19 vs edited =====
--- 1.19/drivers/scsi/libata-core.c	Wed Feb 25 22:41:13 2004
+++ edited/drivers/scsi/libata-core.c	Sat Feb 28 14:03:18 2004
@@ -2130,6 +2130,14 @@
 				cmd->result = SAM_STAT_CHECK_CONDITION;
 			else
 				ata_to_sense_error(qc);
+
+			/* hack alert! we need this to get past the
+			 * first check in scsi_done().  libata is the
+			 * -only- user of ->eh_strategy_handler() in
+			 * any kernel tree, which exposes some incorrect
+			 * assumptions in the SCSI layer.
+			 */
+			scsi_add_timer(cmd, 2000 * HZ, NULL);
 		} else {
 			cmd->result = SAM_STAT_GOOD;
 		}

Re: [PATCH/RFT] libata "DMA timeout" fix

[James Bottomley]

On Sat, 2004-02-28 at 13:10, Jeff Garzik wrote:
> ===== drivers/scsi/libata-core.c 1.19 vs edited =====
> --- 1.19/drivers/scsi/libata-core.c	Wed Feb 25 22:41:13 2004
> +++ edited/drivers/scsi/libata-core.c	Sat Feb 28 14:03:18 2004
> @@ -2130,6 +2130,14 @@
>  				cmd->result = SAM_STAT_CHECK_CONDITION;
>  			else
>  				ata_to_sense_error(qc);
> +
> +			/* hack alert! we need this to get past the
> +			 * first check in scsi_done().  libata is the
> +			 * -only- user of ->eh_strategy_handler() in
> +			 * any kernel tree, which exposes some incorrect
> +			 * assumptions in the SCSI layer.
> +			 */
> +			scsi_add_timer(cmd, 2000 * HZ, NULL);
>  		} else {
>  			cmd->result = SAM_STAT_GOOD;
>  		}

You can't do this.  Supposing there command's delayed, the timer fires
and then the command returns with a sense error?  The done will go
through automatically completing the command, but your strategy handler
will still think it has a failed command to handle.

The correct fix is this, I think (uncompiled, but you get the idea):

===== libata-core.c 1.19 vs edited =====
--- 1.19/drivers/scsi/libata-core.c	Wed Feb 25 21:41:13 2004
+++ edited/libata-core.c	Sat Feb 28 14:46:17 2004
@@ -1972,6 +1972,11 @@
 	/* FIXME */
 }
 
+static void ata_eng_timeout_done(struct scsi_cmnd *cmnd)
+{
+	scsi_finish_command(cmnd);
+}
+
 /**
  *	ata_eng_timeout - Handle timeout of queued command
  *	@ap: Port on which timed-out command is active
@@ -2005,6 +2010,7 @@
 		goto out;
 	}
 
+	qc->scsidone = ata_eng_timeout_done;
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA_READ:
 	case ATA_PROT_DMA_WRITE:

James

Re: [PATCH/RFT] libata "DMA timeout" fix

[Jeff Garzik]

James Bottomley wrote:
> On Sat, 2004-02-28 at 13:10, Jeff Garzik wrote:
> 
>>===== drivers/scsi/libata-core.c 1.19 vs edited =====
>>--- 1.19/drivers/scsi/libata-core.c	Wed Feb 25 22:41:13 2004
>>+++ edited/drivers/scsi/libata-core.c	Sat Feb 28 14:03:18 2004
>>@@ -2130,6 +2130,14 @@
>> 				cmd->result = SAM_STAT_CHECK_CONDITION;
>> 			else
>> 				ata_to_sense_error(qc);
>>+
>>+			/* hack alert! we need this to get past the
>>+			 * first check in scsi_done().  libata is the
>>+			 * -only- user of ->eh_strategy_handler() in
>>+			 * any kernel tree, which exposes some incorrect
>>+			 * assumptions in the SCSI layer.
>>+			 */
>>+			scsi_add_timer(cmd, 2000 * HZ, NULL);
>> 		} else {
>> 			cmd->result = SAM_STAT_GOOD;
>> 		}
> 
> 
> You can't do this.  Supposing there command's delayed, the timer fires
> and then the command returns with a sense error?  The done will go
> through automatically completing the command, but your strategy handler
> will still think it has a failed command to handle.

hmmm, yeah that will be a problem iff we are not already in the strategy 
handler.


> The correct fix is this, I think (uncompiled, but you get the idea):

Yeah, that's much better.  That function is not exported though ;-)

	Jeff

Re: [PATCH/RFT] libata "DMA timeout" fix

[James Bottomley]

On Sat, 2004-02-28 at 15:23, Jeff Garzik wrote:
> Yeah, that's much better.  That function is not exported though ;-)

I can fix that.  It really is a necessary function for drivers doing
their own strategy handler ... of which yours seems to be the only one.

James