[PATCH 01/02] sata_mv: tidy up qc->tf usage in qc_prep() functions

[PATCH 01/02] sata_mv: tidy up qc->tf usage in qc_prep() functions

[Mark Lord]

Tidy up qc->tf accesses in the mv_qc_prep() functions.

Signed-off-by: Mark Lord <mlord@pobox.com>
---

For upstream-linus, please.
In preparation for the multi_count errata patch which follows.

--- old/drivers/ata/sata_mv.c.upstream	2009-04-13 09:46:51.000000000 -0400
+++ new/drivers/ata/sata_mv.c	2009-04-13 10:09:54.000000000 -0400
@@ -1898,17 +1898,17 @@
 	struct ata_port *ap = qc->ap;
 	struct mv_port_priv *pp = ap->private_data;
 	__le16 *cw;
-	struct ata_taskfile *tf;
+	struct ata_taskfile *tf = &qc->tf;
 	u16 flags = 0;
 	unsigned in_index;
 
-	if ((qc->tf.protocol != ATA_PROT_DMA) &&
-	    (qc->tf.protocol != ATA_PROT_NCQ))
+	if ((tf->protocol != ATA_PROT_DMA) &&
+	    (tf->protocol != ATA_PROT_NCQ))
 		return;
 
 	/* Fill in command request block
 	 */
-	if (!(qc->tf.flags & ATA_TFLAG_WRITE))
+	if (!(tf->flags & ATA_TFLAG_WRITE))
 		flags |= CRQB_FLAG_READ;
 	WARN_ON(MV_MAX_Q_DEPTH <= qc->tag);
 	flags |= qc->tag << CRQB_TAG_SHIFT;
@@ -1924,7 +1924,6 @@
 	pp->crqb[in_index].ctrl_flags = cpu_to_le16(flags);
 
 	cw = &pp->crqb[in_index].ata_cmd[0];
-	tf = &qc->tf;
 
 	/* Sadly, the CRQB cannot accomodate all registers--there are
 	 * only 11 bytes...so we must pick and choose required
@@ -1990,16 +1989,16 @@
 	struct ata_port *ap = qc->ap;
 	struct mv_port_priv *pp = ap->private_data;
 	struct mv_crqb_iie *crqb;
-	struct ata_taskfile *tf;
+	struct ata_taskfile *tf = &qc->tf;
 	unsigned in_index;
 	u32 flags = 0;
 
-	if ((qc->tf.protocol != ATA_PROT_DMA) &&
-	    (qc->tf.protocol != ATA_PROT_NCQ))
+	if ((tf->protocol != ATA_PROT_DMA) &&
+	    (tf->protocol != ATA_PROT_NCQ))
 		return;
 
 	/* Fill in Gen IIE command request block */
-	if (!(qc->tf.flags & ATA_TFLAG_WRITE))
+	if (!(tf->flags & ATA_TFLAG_WRITE))
 		flags |= CRQB_FLAG_READ;
 
 	WARN_ON(MV_MAX_Q_DEPTH <= qc->tag);
@@ -2015,7 +2014,6 @@
 	crqb->addr_hi = cpu_to_le32((pp->sg_tbl_dma[qc->tag] >> 16) >> 16);
 	crqb->flags = cpu_to_le32(flags);
 
-	tf = &qc->tf;
 	crqb->ata_cmd[0] = cpu_to_le32(
 			(tf->command << 16) |
 			(tf->feature << 24)

[PATCH 02/02] sata_mv: workaround for multi_count errata sata24

[Mark Lord]

Workaround for errata SATA#24 in sata_mv.
This errata affects WRITE_MULTI* commands when
the device multi_count produces a DRQ block size >= 4Kbytes.

We work around it here by converting such operations
into ordinary PIO_WRITEs instead.

Note that this might result in a PIO FUA write unavoidably being converted
into a non-FUA write.  In practice, any system using FUA is also going to be
using DMA rather than PIO, so this shouldn't affect anyone in the real world.

Signed-off-by: Mark Lord <mlord@pobox.com>
---
For upstream-linus, please.

--- old/drivers/ata/sata_mv.c.upstream	2009-04-13 09:46:51.000000000 -0400
+++ new/drivers/ata/sata_mv.c	2009-04-13 10:11:26.000000000 -0400
@@ -1881,6 +1881,39 @@
 	return status;
 }
 
+static void mv_rw_multi_errata_sata24(struct ata_queued_cmd *qc)
+{
+	struct ata_taskfile *tf = &qc->tf;
+	/*
+	 * Workaround for 88SX60x1 FEr SATA#24.
+	 *
+	 * Chip may corrupt WRITEs if multi_count >= 4kB.
+	 * Note that READs are unaffected.
+	 *
+	 * It's not clear if this errata really means "4K bytes",
+	 * or if it always happens for multi_count > 7
+	 * regardless of device sector_size.
+	 *
+	 * So, for safety, any write with multi_count > 7
+	 * gets converted here into a regular PIO write instead:
+	 */
+	if ((tf->flags & ATA_TFLAG_WRITE) && is_multi_taskfile(tf)) {
+		if (qc->dev->multi_count > 7) {
+			switch (tf->command) {
+			case ATA_CMD_WRITE_MULTI:
+				tf->command = ATA_CMD_PIO_WRITE;
+				break;
+			case ATA_CMD_WRITE_MULTI_FUA_EXT:
+				tf->flags &= ~ATA_TFLAG_FUA; /* ugh */
+				/* fall through */
+			case ATA_CMD_WRITE_MULTI_EXT:
+				tf->command = ATA_CMD_PIO_WRITE_EXT;
+				break;
+			}
+		}
+	}
+}
+
 /**
  *      mv_qc_prep - Host specific command preparation.
  *      @qc: queued command to prepare
@@ -1902,9 +1935,16 @@
 	u16 flags = 0;
 	unsigned in_index;
 
-	if ((tf->protocol != ATA_PROT_DMA) &&
-	    (tf->protocol != ATA_PROT_NCQ))
+	switch (tf->protocol) {
+	case ATA_PROT_DMA:
+	case ATA_PROT_NCQ:
+		break;	/* continue below */
+	case ATA_PROT_PIO:
+		mv_rw_multi_errata_sata24(qc);
+		return;
+	default:
 		return;
+	}
 
 	/* Fill in command request block
 	 */

Re: [PATCH 02/02] sata_mv: workaround for multi_count errata sata24

[Mark Lord]

Mark Lord wrote:
> Workaround for errata SATA#24 in sata_mv.
> This errata affects WRITE_MULTI* commands when
> the device multi_count produces a DRQ block size >= 4Kbytes.
> 
> We work around it here by converting such operations
> into ordinary PIO_WRITEs instead.
> 
> Note that this might result in a PIO FUA write unavoidably being converted
> into a non-FUA write.  In practice, any system using FUA is also going to be
> using DMA rather than PIO, so this shouldn't affect anyone in the real world.
..

Testing this was a real bear.

The drives I have here are among those which cannot do R/W MULTI
prior to having a SET_MULTIPLE command issued to them by libata.

Which libata currently does not do.

This means that libata is unable to cope with non-DMA devices
that report a non-zero multi_count value at startup.

I simulated this scenario in sata_mv by changing the .udma_mask field
to zero for the card under test.  System sat there retrying the
partition table read over and over and over and over..  :)

Someday I'll fix that, if you guys don't beat me to it.
I suppose we just need to issue the SET_MULTIPLE command from
the drive revalidation code.  

Cheers

Re: [PATCH 02/02] sata_mv: workaround for multi_count errata sata24

[Alan Cox]

On Mon, 13 Apr 2009 11:29:34 -0400
Mark Lord <liml@rtr.ca> wrote:

> Workaround for errata SATA#24 in sata_mv.
> This errata affects WRITE_MULTI* commands when
> the device multi_count produces a DRQ block size >= 4Kbytes.
> 
> We work around it here by converting such operations
> into ordinary PIO_WRITEs instead.
> 
> Note that this might result in a PIO FUA write unavoidably being converted
> into a non-FUA write.  In practice, any system using FUA is also going to be
> using DMA rather than PIO, so this shouldn't affect anyone in the real world.

You can just screen the FUA bit from the identify data when you do the
drive setup to avoid that bit.

Alan

Re: [PATCH 02/02] sata_mv: workaround for multi_count errata sata24

[Mark Lord]

Alan Cox wrote:
> On Mon, 13 Apr 2009 11:29:34 -0400
> Mark Lord <liml@rtr.ca> wrote:
> 
>> Workaround for errata SATA#24 in sata_mv.
>> This errata affects WRITE_MULTI* commands when
>> the device multi_count produces a DRQ block size >= 4Kbytes.
>>
>> We work around it here by converting such operations
>> into ordinary PIO_WRITEs instead.
>>
>> Note that this might result in a PIO FUA write unavoidably being converted
>> into a non-FUA write.  In practice, any system using FUA is also going to be
>> using DMA rather than PIO, so this shouldn't affect anyone in the real world.
> 
> You can just screen the FUA bit from the identify data when you do the
> drive setup to avoid that bit.
..

I think doing it at identify time would prevent DMA FUA commands,
which are fine in sata_mv.

So the only place to really hit it exactly, is at command construction
or issue time.

cheers

Re: [PATCH 02/02] sata_mv: workaround for multi_count errata sata24

[Mark Lord]

Mark Lord wrote:
> Alan Cox wrote:
>> On Mon, 13 Apr 2009 11:29:34 -0400
>> Mark Lord <liml@rtr.ca> wrote:
>>
>>> Workaround for errata SATA#24 in sata_mv.
>>> This errata affects WRITE_MULTI* commands when
>>> the device multi_count produces a DRQ block size >= 4Kbytes.
>>>
>>> We work around it here by converting such operations
>>> into ordinary PIO_WRITEs instead.
>>>
>>> Note that this might result in a PIO FUA write unavoidably being 
>>> converted
>>> into a non-FUA write.  In practice, any system using FUA is also 
>>> going to be
>>> using DMA rather than PIO, so this shouldn't affect anyone in the 
>>> real world.
>>
>> You can just screen the FUA bit from the identify data when you do the
>> drive setup to avoid that bit.
> ..
> 
> I think doing it at identify time would prevent DMA FUA commands,
> which are fine in sata_mv.
> 
> So the only place to really hit it exactly, is at command construction
> or issue time.
..

Note that libata already will convert would-be FUA commands
into non-FUA PIO commands if (dev->multi_count == 0).

So this isn't really anything new.
Nor can it be avoided.
And certainly not anything to fuss over.

Re: [PATCH 02/02] sata_mv: workaround for multi_count errata sata24

[Jeff Garzik]

Mark Lord wrote:
> Someday I'll fix that, if you guys don't beat me to it.
> I suppose we just need to issue the SET_MULTIPLE command from
> the drive revalidation code. 

IMO this would be a good idea to do, across many ATA devices.

IIRC there is at least one other chip that snoops the set-multiple 
command, and sets internal chip parameters.

Thus, like SET FEATURES - XFER MODE, it seems wise to unconditionally 
issue SET MULTIPLE for ATA devices, under the logic that it 
more-thoroughly initializes the controller and device, ensuring we are 
at a known state at all times.

Wanna do a patch?  :)  Although not the greatest code location, I'm 
guessing ata_dev_read_id() would probably be the place...  Ideally we 
would have a post-IDENTIFY reset sequence function.

	Jeff

Re: [PATCH 02/02] sata_mv: workaround for multi_count errata sata24

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> Someday I'll fix that, if you guys don't beat me to it.
>> I suppose we just need to issue the SET_MULTIPLE command from
>> the drive revalidation code. 
> 
> IMO this would be a good idea to do, across many ATA devices.
> 
> IIRC there is at least one other chip that snoops the set-multiple 
> command, and sets internal chip parameters.
> 
> Thus, like SET FEATURES - XFER MODE, it seems wise to unconditionally 
> issue SET MULTIPLE for ATA devices, under the logic that it 
> more-thoroughly initializes the controller and device, ensuring we are 
> at a known state at all times.
> 
> Wanna do a patch?  :)  Although not the greatest code location, I'm 
> guessing ata_dev_read_id() would probably be the place...  Ideally we 
> would have a post-IDENTIFY reset sequence function.
..

I agree with all of the above.

Except I have a certain other, eagerly anticipated,
patch to work on right now.  :)

Perhaps afterwards, though I'm due to disappear into the embedded
world again for a while, a couple of weeks from now.

If anyone else feels the urge, please don't wait for me!

Cheers

libata git repo guide (was Re: [PATCH 01/02] sata_mv: tidy up qc->tf usage in qc_prep() functions)

[Jeff Garzik]

Mark Lord wrote:
> For upstream-linus, please.


Any volunteers wanna format and post this to the wiki? 
http://ata.wiki.kernel.org/

For what it's worth, upstream-linus is for Linus only, not for anyone else.

Relevant branches of libata-dev.git:

ALL		All libata code I consider acceptable for public
		testing.  Intended for Andrew Morton's -mm tree,
		but I am always looking for people to help test!

NEXT		libata code intended for current kernel + 1
		This is a subset of ALL, and is intended for
		the linux-next tree, and even more rigorous
		public testing.

upstream-fixes	This branch exists when the merge window is NOT open.
		Fixes queued for Linus go here, during -rc.  Generally
		I push to Linus pretty rapidly, though, so programmers
		are encouraged to send patches diff'd against Linus
		upstream during -rc.

upstream	libata code intended for current kernel + 1.  When the
		merge window is closed, patches are queued here.  When
		the merge window is open, fixes for Linus are also
		queued here.

		Generally upstream==NEXT, but this is not guaranteed.

		i.e. sometimes I will put a fix on a separate git
		branch, and then merge that into the mix, such that

			NEXT == upstream + fix-that-needs-testing

And then there are individual branches that pop in and out of existence, 
as I do my own work.  For example, currently active branches are

fixes-eh-freeze	  Freeze ordering fix recently discussed; needs some
		codepath review before I am sure it is safe. (NEXT)

libahci		libahci patchset recently posted (NEXT, ALL)

sectsize	Variable sector size patch.  Will not go into
		#upstream branch until I satisfy some request of
		willy's, which slips my mind at the moment (ALL)

sx4		sata_sx4 new EH conversion (ALL)

And last, but not least,

master		Vanilla Linux kernel, with no changes from me.

		Exists to indicate the root of all above branches,
		so that "git log master..libahci" or
		"git diff master..upstream" works as expected.


NOTE:  All of the branches except 'master' may be rebased.

Re: [PATCH 02/02] sata_mv: workaround for multi_count errata sata24

[Jeff Garzik]

Mark Lord wrote:
> Workaround for errata SATA#24 in sata_mv.
> This errata affects WRITE_MULTI* commands when
> the device multi_count produces a DRQ block size >= 4Kbytes.
> 
> We work around it here by converting such operations
> into ordinary PIO_WRITEs instead.
> 
> Note that this might result in a PIO FUA write unavoidably being converted
> into a non-FUA write.  In practice, any system using FUA is also going 
> to be
> using DMA rather than PIO, so this shouldn't affect anyone in the real 
> world.

I'm applying this, but I think a follow-up patch would be nice:  it 
seems like a one-time printk, indicating the FUA conversion is active, 
could be a help in case someone does care.

	Jeff

Re: [PATCH 02/02] sata_mv: workaround for multi_count errata sata24

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>> Workaround for errata SATA#24 in sata_mv.
>> This errata affects WRITE_MULTI* commands when
>> the device multi_count produces a DRQ block size >= 4Kbytes.
>>
>> We work around it here by converting such operations
>> into ordinary PIO_WRITEs instead.
>>
>> Note that this might result in a PIO FUA write unavoidably being 
>> converted
>> into a non-FUA write.  In practice, any system using FUA is also going 
>> to be
>> using DMA rather than PIO, so this shouldn't affect anyone in the real 
>> world.
> 
> I'm applying this, but I think a follow-up patch would be nice:  it 
> seems like a one-time printk, indicating the FUA conversion is active, 
> could be a help in case someone does care.
..

Seems like .text bloat to me.

But if you want it, I'll cook up patches for both ata_rwcmd_protocol()
in libata-core.c as well as sata_mv.c later on.

Cheers