[patch 29/30] sata_nv: don't read shadow registers when in ADMA mode

[patch 29/30] sata_nv: don't read shadow registers when in ADMA mode

[akpm]

From: Robert Hancock <hancockr@shaw.ca>

Reading from the ATA shadow registers while we are in ADMA mode may cause
undefined behavior.  Don't read the ATA status register when completing
commands for this reason, it shouldn't be needed as the controller will
notify us if the command failed.  Also, don't allow commands with result
taskfile requested to execute in ADMA mode, since that requires accessing
the shadow registers.  We also still need to override tf_read since libata
will read the result taskfile on a command failure, and we need to go into
port register mode before allowing this.

Signed-off-by: Robert Hancock <hancockr@shaw.ca>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/ata/sata_nv.c |   33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff -puN drivers/ata/sata_nv.c~sata_nv-dont-read-shadow-registers-when-in-adma-mode drivers/ata/sata_nv.c
--- a/drivers/ata/sata_nv.c~sata_nv-dont-read-shadow-registers-when-in-adma-mode
+++ a/drivers/ata/sata_nv.c
@@ -260,6 +260,7 @@ static int nv_adma_port_resume(struct at
 static void nv_adma_error_handler(struct ata_port *ap);
 static void nv_adma_host_stop(struct ata_host *host);
 static void nv_adma_post_internal_cmd(struct ata_queued_cmd *qc);
+static void nv_adma_tf_read(struct ata_port *ap, struct ata_taskfile *tf);
 
 enum nv_host_type
 {
@@ -435,7 +436,7 @@ static const struct ata_port_operations 
 static const struct ata_port_operations nv_adma_ops = {
 	.port_disable		= ata_port_disable,
 	.tf_load		= ata_tf_load,
-	.tf_read		= ata_tf_read,
+	.tf_read		= nv_adma_tf_read,
 	.check_atapi_dma	= nv_adma_check_atapi_dma,
 	.exec_command		= ata_exec_command,
 	.check_status		= ata_check_status,
@@ -667,6 +668,18 @@ static int nv_adma_check_atapi_dma(struc
 	return !(pp->flags & NV_ADMA_ATAPI_SETUP_COMPLETE);
 }
 
+static void nv_adma_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
+{
+	/* Since commands where a result TF is requested are not
+	   executed in ADMA mode, the only time this function will be called
+	   in ADMA mode will be if a command fails. In this case we
+	   don't care about going into register mode with ADMA commands
+	   pending, as the commands will all shortly be aborted anyway. */
+	nv_adma_register_mode(ap);
+
+	ata_tf_read(ap, tf);
+}
+
 static unsigned int nv_adma_tf_to_cpb(struct ata_taskfile *tf, __le16 *cpb)
 {
 	unsigned int idx = 0;
@@ -738,19 +751,11 @@ static int nv_adma_check_cpb(struct ata_
 		return 1;
 	}
 
-	if (flags & NV_CPB_RESP_DONE) {
+	if (likely(flags & NV_CPB_RESP_DONE)) {
 		struct ata_queued_cmd *qc = ata_qc_from_tag(ap, cpb_num);
 		VPRINTK("CPB flags done, flags=0x%x\n", flags);
 		if (likely(qc)) {
-			/* Grab the ATA port status for non-NCQ commands.
-			   For NCQ commands the current status may have nothing to do with
-			   the command just completed. */
-			if (qc->tf.protocol != ATA_PROT_NCQ) {
-				u8 ata_status = readb(pp->ctl_block + (ATA_REG_STATUS * 4));
-				qc->err_mask |= ac_err_mask(ata_status);
-			}
-			DPRINTK("Completing qc from tag %d with err_mask %u\n",cpb_num,
-				qc->err_mask);
+			DPRINTK("Completing qc from tag %d\n",cpb_num);
 			ata_qc_complete(qc);
 		} else {
 			struct ata_eh_info *ehi = &ap->eh_info;
@@ -1161,9 +1166,11 @@ static int nv_adma_use_reg_mode(struct a
 	struct nv_adma_port_priv *pp = qc->ap->private_data;
 
 	/* ADMA engine can only be used for non-ATAPI DMA commands,
-	   or interrupt-driven no-data commands. */
+	   or interrupt-driven no-data commands, where a result taskfile
+	   is not required. */
 	if((pp->flags & NV_ADMA_ATAPI_SETUP_COMPLETE) ||
-	   (qc->tf.flags & ATA_TFLAG_POLLING))
+	   (qc->tf.flags & ATA_TFLAG_POLLING) ||
+	   (qc->flags & ATA_QCFLAG_RESULT_TF))
 		return 1;
 
 	if((qc->flags & ATA_QCFLAG_DMAMAP) ||
_

Re: [patch 29/30] sata_nv: don't read shadow registers when in ADMA mode

[Jeff Garzik]

akpm@linux-foundation.org wrote:
> From: Robert Hancock <hancockr@shaw.ca>
> 
> Reading from the ATA shadow registers while we are in ADMA mode may cause
> undefined behavior.  Don't read the ATA status register when completing
> commands for this reason, it shouldn't be needed as the controller will
> notify us if the command failed.  Also, don't allow commands with result
> taskfile requested to execute in ADMA mode, since that requires accessing
> the shadow registers.  We also still need to override tf_read since libata
> will read the result taskfile on a command failure, and we need to go into
> port register mode before allowing this.
> 
> Signed-off-by: Robert Hancock <hancockr@shaw.ca>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  drivers/ata/sata_nv.c |   33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)

Robert, do you think this should be pushed into #upstream-fixes (2.6.21-rc)?

I lean towards "yes", since it is a needed-by-hardware fix, but I also 
am interested in testing feedback since it is so late in the 2.6.21-rc game.

	Jeff

Re: [patch 29/30] sata_nv: don't read shadow registers when in ADMA mode

[Robert Hancock]

Jeff Garzik wrote:
> akpm@linux-foundation.org wrote:
>> From: Robert Hancock <hancockr@shaw.ca>
>>
>> Reading from the ATA shadow registers while we are in ADMA mode may cause
>> undefined behavior.  Don't read the ATA status register when completing
>> commands for this reason, it shouldn't be needed as the controller will
>> notify us if the command failed.  Also, don't allow commands with result
>> taskfile requested to execute in ADMA mode, since that requires accessing
>> the shadow registers.  We also still need to override tf_read since 
>> libata
>> will read the result taskfile on a command failure, and we need to go 
>> into
>> port register mode before allowing this.
>>
>> Signed-off-by: Robert Hancock <hancockr@shaw.ca>
>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>> ---
>>
>>  drivers/ata/sata_nv.c |   33 ++++++++++++++++++++-------------
>>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> Robert, do you think this should be pushed into #upstream-fixes 
> (2.6.21-rc)?
> 
> I lean towards "yes", since it is a needed-by-hardware fix, but I also 
> am interested in testing feedback since it is so late in the 2.6.21-rc 
> game.

I would lean toward that as well, but it would be good to get some 
testing from some sata_nv ADMA users to make sure it doesn't do anything 
funny for them..

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: [patch 29/30] sata_nv: don't read shadow registers when in ADMA mode

[Alistair John Strachan]

On Friday 09 March 2007 14:27, you wrote:
> Jeff Garzik wrote:
> > akpm@linux-foundation.org wrote:
> >> From: Robert Hancock <hancockr@shaw.ca>
> >>
> >> Reading from the ATA shadow registers while we are in ADMA mode may
> >> cause undefined behavior.  Don't read the ATA status register when
> >> completing commands for this reason, it shouldn't be needed as the
> >> controller will notify us if the command failed.  Also, don't allow
> >> commands with result taskfile requested to execute in ADMA mode, since
> >> that requires accessing the shadow registers.  We also still need to
> >> override tf_read since libata
> >> will read the result taskfile on a command failure, and we need to go
> >> into
> >> port register mode before allowing this.
> >>
> >> Signed-off-by: Robert Hancock <hancockr@shaw.ca>
> >> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> >> ---
> >>
> >>  drivers/ata/sata_nv.c |   33 ++++++++++++++++++++-------------
> >>  1 file changed, 20 insertions(+), 13 deletions(-)
> >
> > Robert, do you think this should be pushed into #upstream-fixes
> > (2.6.21-rc)?
> >
> > I lean towards "yes", since it is a needed-by-hardware fix, but I also
> > am interested in testing feedback since it is so late in the 2.6.21-rc
> > game.
>
> I would lean toward that as well, but it would be good to get some
> testing from some sata_nv ADMA users to make sure it doesn't do anything
> funny for them..

Since I've been a bit of a problem case this time, I'd be happy to test it.

Can I assume that I can apply the patch you sent to Jeff "[PATCH] sata_nv: 
revert use of notifiers for now", and apply this one, to -rc3, and then be 
able to usefully test?

-- 
Cheers,
Alistair.

Final year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

Re: [patch 29/30] sata_nv: don't read shadow registers when in ADMA mode

[Robert Hancock]

Alistair John Strachan wrote:
>>> I lean towards "yes", since it is a needed-by-hardware fix, but I also
>>> am interested in testing feedback since it is so late in the 2.6.21-rc
>>> game.
>> I would lean toward that as well, but it would be good to get some
>> testing from some sata_nv ADMA users to make sure it doesn't do anything
>> funny for them..
> 
> Since I've been a bit of a problem case this time, I'd be happy to test it.
> 
> Can I assume that I can apply the patch you sent to Jeff "[PATCH] sata_nv: 
> revert use of notifiers for now", and apply this one, to -rc3, and then be 
> able to usefully test?

Yes, you should be able to.

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/