[RFC PATCH] sata_via: Apply WD workaround only when needed

[RFC PATCH] sata_via: Apply WD workaround only when needed

[Ondrej Zary]

Currently, workaround for broken WD drives is applied always, slowing
down all drives. And it has a bug - it's not applied after resume.

Apply the workaround only if the error really appears
(SErr == 0x1000500). This allows unaffected drives to run at full speed
(provided that no affected drive is connected to the controller).
It also fixes the suspend/resume problem.

The downside is that first error always appears in the log.

Unaffected drive (Hitachi HDS721680PLA380):
Before:
$ hdparm -t --direct /dev/sdb
/dev/sdb:
 Timing O_DIRECT disk reads: 160 MB in  3.01 seconds =  53.16 MB/sec

After:
$ hdparm -t --direct /dev/sdb
/dev/sdb:
 Timing O_DIRECT disk reads: 200 MB in  3.01 seconds =  66.47 MB/sec

Affected drive (WDC WD5003ABYX-18WERA0):
Before:
$ hdparm -t --direct /dev/sda

/dev/sda:
 Timing O_DIRECT disk reads: 180 MB in  3.02 seconds =  59.51 MB/sec

After:
$ hdparm -t --direct /dev/sdb
/dev/sdb:
 Timing O_DIRECT disk reads: 156 MB in  3.03 seconds =  51.48 MB/sec
$ hdparm -t --direct /dev/sdb
/dev/sdb:
 Timing O_DIRECT disk reads: 180 MB in  3.02 seconds =  59.64 MB/sec

The first hdparm is slower because of the error:
[   80.964060] ata5.00: exception Emask 0x12 SAct 0x0 SErr 0x1000500 action 0x6
[   80.964095] ata5.00: BMDMA stat 0x5
[   80.964108] ata5: SError: { UnrecovData Proto TrStaTrns }
[   80.964125] ata5.00: failed command: READ DMA EXT
[   80.964143] ata5.00: cmd 25/00:90:00:00:00/00:04:00:00:00/e0 tag 0 dma 598016 in
         res 51/84:af:df:00:00/84:03:00:00:00/e0 Emask 0x12 (ATA bus error)
[   80.964173] ata5.00: status: { DRDY ERR }
[   80.964185] ata5.00: error: { ICRC ABRT }
[   80.964209] ata5: hard resetting link
[   81.284056] ata5: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
[   81.300531] ata5.00: configured for UDMA/133
[   81.300569] ata5: Incompatible drive (WD?): enabling workaround
[   81.300598] ata5: EH complete

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/ata/sata_via.c |   72 +++++++++++++++++++++++++++++-------------------
 1 file changed, 44 insertions(+), 28 deletions(-)

diff --git a/drivers/ata/sata_via.c b/drivers/ata/sata_via.c
index 17d31fc..65c9ab9 100644
--- a/drivers/ata/sata_via.c
+++ b/drivers/ata/sata_via.c
@@ -85,6 +85,7 @@ static void vt6420_bmdma_start(struct ata_queued_cmd *qc);
 static int vt6421_pata_cable_detect(struct ata_port *ap);
 static void vt6421_set_pio_mode(struct ata_port *ap, struct ata_device *adev);
 static void vt6421_set_dma_mode(struct ata_port *ap, struct ata_device *adev);
+static void svia_error_handler(struct ata_port *ap);
 
 static const struct pci_device_id svia_pci_tbl[] = {
 	{ PCI_VDEVICE(VIA, 0x5337), vt6420 },
@@ -124,6 +125,7 @@ static struct ata_port_operations vt6420_sata_ops = {
 	.freeze			= svia_noop_freeze,
 	.prereset		= vt6420_prereset,
 	.bmdma_start		= vt6420_bmdma_start,
+	.error_handler		= svia_error_handler,
 };
 
 static struct ata_port_operations vt6421_pata_ops = {
@@ -137,6 +139,7 @@ static struct ata_port_operations vt6421_sata_ops = {
 	.inherits		= &svia_base_ops,
 	.scr_read		= svia_scr_read,
 	.scr_write		= svia_scr_write,
+	.error_handler		= svia_error_handler,
 };
 
 static struct ata_port_operations vt8251_ops = {
@@ -536,6 +539,47 @@ static int vt8251_prepare_host(struct pci_dev *pdev, struct ata_host **r_host)
 	return 0;
 }
 
+static void svia_error_handler(struct ata_port *ap)
+{
+	u8 tmp8;
+	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+	struct ata_eh_context *ehc = &ap->link.eh_context;
+
+	ata_sff_error_handler(ap);
+	/*
+	 * vt6420/1 has problems talking to some drives.  The following
+	 * is the fix from Joseph Chan <JosephChan@via.com.tw>.
+	 *
+	 * When host issues HOLD, device may send up to 20DW of data
+	 * before acknowledging it with HOLDA and the host should be
+	 * able to buffer them in FIFO.  Unfortunately, some WD drives
+	 * send up to 40DW before acknowledging HOLD and, in the
+	 * default configuration, this ends up overflowing vt6421's
+	 * FIFO, making the controller abort the transaction with
+	 * R_ERR.
+	 *
+	 * Rx52[2] is the internal 128DW FIFO Flow control watermark
+	 * adjusting mechanism enable bit and the default value 0
+	 * means host will issue HOLD to device when the left FIFO
+	 * size goes below 32DW.  Setting it to 1 makes the watermark
+	 * 64DW.
+	 *
+	 * https://bugzilla.kernel.org/show_bug.cgi?id=15173
+	 * http://article.gmane.org/gmane.linux.ide/46352
+	 * http://thread.gmane.org/gmane.linux.kernel/1062139
+	 *
+	 * As the fix slows down data transfer, apply it only if the error
+	 * actually appears (symptom: SErr == 0x1000500)
+	 */
+	if (ehc->i.serror == 0x1000500) {
+		pci_read_config_byte(pdev, 0x52, &tmp8);
+		if (!(tmp8 & BIT(2))) {
+			ata_port_warn(ap, "Incompatible drive (WD?): enabling workaround");
+			pci_write_config_byte(pdev, 0x52, tmp8 | BIT(2));
+		}
+	}
+}
+
 static void svia_configure(struct pci_dev *pdev, int board_id)
 {
 	u8 tmp8;
@@ -571,34 +615,6 @@ static void svia_configure(struct pci_dev *pdev, int board_id)
 		tmp8 |= NATIVE_MODE_ALL;
 		pci_write_config_byte(pdev, SATA_NATIVE_MODE, tmp8);
 	}
-
-	/*
-	 * vt6420/1 has problems talking to some drives.  The following
-	 * is the fix from Joseph Chan <JosephChan@via.com.tw>.
-	 *
-	 * When host issues HOLD, device may send up to 20DW of data
-	 * before acknowledging it with HOLDA and the host should be
-	 * able to buffer them in FIFO.  Unfortunately, some WD drives
-	 * send up to 40DW before acknowledging HOLD and, in the
-	 * default configuration, this ends up overflowing vt6421's
-	 * FIFO, making the controller abort the transaction with
-	 * R_ERR.
-	 *
-	 * Rx52[2] is the internal 128DW FIFO Flow control watermark
-	 * adjusting mechanism enable bit and the default value 0
-	 * means host will issue HOLD to device when the left FIFO
-	 * size goes below 32DW.  Setting it to 1 makes the watermark
-	 * 64DW.
-	 *
-	 * https://bugzilla.kernel.org/show_bug.cgi?id=15173
-	 * http://article.gmane.org/gmane.linux.ide/46352
-	 * http://thread.gmane.org/gmane.linux.kernel/1062139
-	 */
-	if (board_id == vt6420 || board_id == vt6421) {
-		pci_read_config_byte(pdev, 0x52, &tmp8);
-		tmp8 |= 1 << 2;
-		pci_write_config_byte(pdev, 0x52, tmp8);
-	}
 }
 
 static int svia_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
-- 
Ondrej Zary

Re: [RFC PATCH] sata_via: Apply WD workaround only when needed

[Tejun Heo]

Hello, Ondrej.

On Mon, Feb 15, 2016 at 10:01:47PM +0100, Ondrej Zary wrote:
> The first hdparm is slower because of the error:
> [   80.964060] ata5.00: exception Emask 0x12 SAct 0x0 SErr 0x1000500 action 0x6
> [   80.964095] ata5.00: BMDMA stat 0x5
> [   80.964108] ata5: SError: { UnrecovData Proto TrStaTrns }
> [   80.964125] ata5.00: failed command: READ DMA EXT
> [   80.964143] ata5.00: cmd 25/00:90:00:00:00/00:04:00:00:00/e0 tag 0 dma 598016 in
>          res 51/84:af:df:00:00/84:03:00:00:00/e0 Emask 0x12 (ATA bus error)
> [   80.964173] ata5.00: status: { DRDY ERR }
> [   80.964185] ata5.00: error: { ICRC ABRT }
> [   80.964209] ata5: hard resetting link
> [   81.284056] ata5: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
> [   81.300531] ata5.00: configured for UDMA/133
> [   81.300569] ata5: Incompatible drive (WD?): enabling workaround
> [   81.300598] ata5: EH complete

Hmm... I like the workaround but wish the kernel weren't generating
the above output.  How about doing something like the following?

svia_error_handler()
{
	if (workaround hasn't been applied yet &&
	    the conditions match for the workaround) {
		apply the workaround;
		mark as such;
		print informational message;
		set ATA_EHI_QUIET;
	}

	ata_sff_error_handler(ap);
}

-- 
tejun

Re: [RFC PATCH] sata_via: Apply WD workaround only when needed

[Ondrej Zary]

On Tuesday 16 February 2016 22:42:53 Tejun Heo wrote:
> Hello, Ondrej.
> 
> On Mon, Feb 15, 2016 at 10:01:47PM +0100, Ondrej Zary wrote:
> > The first hdparm is slower because of the error:
> > [   80.964060] ata5.00: exception Emask 0x12 SAct 0x0 SErr 0x1000500 action 0x6
> > [   80.964095] ata5.00: BMDMA stat 0x5
> > [   80.964108] ata5: SError: { UnrecovData Proto TrStaTrns }
> > [   80.964125] ata5.00: failed command: READ DMA EXT
> > [   80.964143] ata5.00: cmd 25/00:90:00:00:00/00:04:00:00:00/e0 tag 0 dma 598016 in
> >          res 51/84:af:df:00:00/84:03:00:00:00/e0 Emask 0x12 (ATA bus error)
> > [   80.964173] ata5.00: status: { DRDY ERR }
> > [   80.964185] ata5.00: error: { ICRC ABRT }
> > [   80.964209] ata5: hard resetting link
> > [   81.284056] ata5: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
> > [   81.300531] ata5.00: configured for UDMA/133
> > [   81.300569] ata5: Incompatible drive (WD?): enabling workaround
> > [   81.300598] ata5: EH complete
> 
> Hmm... I like the workaround but wish the kernel weren't generating
> the above output.  How about doing something like the following?

I wish that too but wasn't sure it was possible. Thanks for idea.

> svia_error_handler()
> {
> 	if (workaround hasn't been applied yet &&
> 	    the conditions match for the workaround) {
> 		apply the workaround;
> 		mark as such;
> 		print informational message;
> 		set ATA_EHI_QUIET;
> 	}
> 
> 	ata_sff_error_handler(ap);
> }
> 

Guess that I need to read SErr manually?

-- 
Ondrej Zary

Re: [RFC PATCH] sata_via: Apply WD workaround only when needed

[Tejun Heo]

Hello,

On Tue, Feb 16, 2016 at 11:57:52PM +0100, Ondrej Zary wrote:
> Guess that I need to read SErr manually?

Yeah, read whatever is necessary.  It's restricted to the controller
anyway.

Thanks.

-- 
tejun