Skip to contentSkip to conversation

a better Silicon Image SATA mod15 write fix?

PATCH In review Jeff Garzik 2 participants 6 messages Mar 15, 2004

Subsystems (2)
SCSI libata
lore
Share & tools
lore

Jeff Garzik

author
#0 lore raw

PATCH 1/1 a better Silicon Image SATA mod15 write fix?

Anyone with a Silicon Image controller interested in testing this patch on a non-production box?

It adds support for a new, Silicon Image-specific DMA table which should not only reduce CPU utilization a bit, but hopefully also magically solve the "mod15write bug" that affected Silicon Image chipsets _when in combination with_ certain SATA drives.

Feedback from people with and without the affected SATA drives ("mod15write" bug) is welcome.

This patch should apply against the latest 2.6.4-bkN snapshot version of sata_sil.c, but for ease I have attached the full 2.6 version of the driver as well (compressed, it's just over 4K).

Jeff

===== drivers/scsi/sata_sil.c 1.14 vs edited =====

drivers/scsi/sata_sil.c +28 -27
@@ -52,12 +52,12 @@
 
 	SIL_IDE0_TF		= 0x80,
 	SIL_IDE0_CTL		= 0x8A,
-	SIL_IDE0_BMDMA		= 0x00,
+	SIL_IDE0_BMDMA		= 0x10,
 	SIL_IDE0_SCR		= 0x100,
 
 	SIL_IDE1_TF		= 0xC0,
 	SIL_IDE1_CTL		= 0xCA,
-	SIL_IDE1_BMDMA		= 0x08,
+	SIL_IDE1_BMDMA		= 0x18,
 	SIL_IDE1_SCR		= 0x180,
 
 	SIL_IDE2_TF		= 0x280,

@@ -71,7 +71,6 @@
 	SIL_IDE3_BMDMA		= 0x208,
 	SIL_IDE3_SCR		= 0x380,
 
-	SIL_QUIRK_MOD15WRITE	= (1 << 0),
 	SIL_QUIRK_UDMA5MAX	= (1 << 1),
 };
 

@@ -79,6 +78,7 @@
 static void sil_dev_config(struct ata_port *ap, struct ata_device *dev);
 static u32 sil_scr_read (struct ata_port *ap, unsigned int sc_reg);
 static void sil_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
+static void sil_fill_sg(struct ata_queued_cmd *qc);
 
 static struct pci_device_id sil_pci_tbl[] = {
 	{ 0x1095, 0x3112, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },

@@ -94,17 +94,6 @@
 	const char * product;
 	unsigned int quirk;
 } sil_blacklist [] = {
-	{ "ST320012AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST330013AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST340017AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST360015AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST380023AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST3120023AS",	SIL_QUIRK_MOD15WRITE },
-	{ "ST340014ASL",	SIL_QUIRK_MOD15WRITE },
-	{ "ST360014ASL",	SIL_QUIRK_MOD15WRITE },
-	{ "ST380011ASL",	SIL_QUIRK_MOD15WRITE },
-	{ "ST3120022ASL",	SIL_QUIRK_MOD15WRITE },
-	{ "ST3160021ASL",	SIL_QUIRK_MOD15WRITE },
 	{ "Maxtor 4D060H3",	SIL_QUIRK_UDMA5MAX },
 	{ }
 };

@@ -123,13 +112,13 @@
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
-	.sg_tablesize		= LIBATA_MAX_PRD,
+	.sg_tablesize		= 256,
 	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
-	.use_clustering		= ATA_SHT_USE_CLUSTERING,
+	.use_clustering		= 1,
 	.proc_name		= DRV_NAME,
-	.dma_boundary		= ATA_DMA_BOUNDARY,
+	.dma_boundary		= 0xffffffff,
 	.slave_configure	= ata_scsi_slave_config,
 	.bios_param		= ata_std_bios_param,
 };

@@ -144,7 +133,7 @@
 	.phy_reset		= sata_phy_reset,
 	.phy_config		= pata_phy_config,	/* not a typo */
 	.bmdma_start            = ata_bmdma_start_mmio,
-	.fill_sg		= ata_fill_sg,
+	.fill_sg		= sil_fill_sg,
 	.eng_timeout		= ata_eng_timeout,
 	.irq_handler		= ata_interrupt,
 	.scr_read		= sil_scr_read,

@@ -212,6 +201,27 @@
 		writel(val, mmio);
 }
 
+static void sil_fill_sg(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	unsigned int idx, n_elem = qc->n_elem;
+	struct scatterlist *sg = qc->sg;
+
+	for (idx = 0; idx < n_elem; idx++, sg++) {
+		u32 addr;
+		u32 size;
+
+		addr = (u32) sg_dma_address(sg);
+		size = (u32) sg_dma_len(sg);
+		size &= ~ATA_PRD_EOT;
+		if (idx == (n_elem - 1))
+			size |= ATA_PRD_EOT;
+
+		ap->prd[idx].addr = cpu_to_le32(addr);
+		ap->prd[idx].flags_len = cpu_to_le32(size);
+	}
+}
+
 /**
  *	sil_dev_config - Apply device/host-specific errata fixups
  *	@ap: Port containing device to be examined

@@ -258,15 +268,6 @@
 			break;
 		}
 	
-	/* limit requests to 15 sectors */
-	if (quirks & SIL_QUIRK_MOD15WRITE) {
-		printk(KERN_INFO "ata%u(%u): applying Seagate errata fix\n",
-		       ap->id, dev->devno);
-		ap->host->max_sectors = 15;
-		ap->host->hostt->max_sectors = 15;
-		return;
-	}
-
 	/* limit to udma5 */
 	if (quirks & SIL_QUIRK_UDMA5MAX) {
 		printk(KERN_INFO "ata%u(%u): applying Maxtor errata fix %s\n",
author
#1 lore raw

And here already is an updated patch :)

===== drivers/scsi/sata_sil.c 1.14 vs edited =====

drivers/scsi/sata_sil.c +38 -4
@@ -79,6 +79,7 @@
 static void sil_dev_config(struct ata_port *ap, struct ata_device *dev);
 static u32 sil_scr_read (struct ata_port *ap, unsigned int sc_reg);
 static void sil_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
+static void sil_fill_sg(struct ata_queued_cmd *qc);
 
 static struct pci_device_id sil_pci_tbl[] = {
 	{ 0x1095, 0x3112, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },

@@ -123,13 +124,13 @@
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
-	.sg_tablesize		= LIBATA_MAX_PRD,
+	.sg_tablesize		= 120,
 	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
-	.use_clustering		= ATA_SHT_USE_CLUSTERING,
+	.use_clustering		= 1,
 	.proc_name		= DRV_NAME,
-	.dma_boundary		= ATA_DMA_BOUNDARY,
+	.dma_boundary		= 0x1fff,
 	.slave_configure	= ata_scsi_slave_config,
 	.bios_param		= ata_std_bios_param,
 };

@@ -144,7 +145,7 @@
 	.phy_reset		= sata_phy_reset,
 	.phy_config		= pata_phy_config,	/* not a typo */
 	.bmdma_start            = ata_bmdma_start_mmio,
-	.fill_sg		= ata_fill_sg,
+	.fill_sg		= sil_fill_sg,
 	.eng_timeout		= ata_eng_timeout,
 	.irq_handler		= ata_interrupt,
 	.scr_read		= sil_scr_read,

@@ -210,6 +211,39 @@
 	void *mmio = (void *) sil_scr_addr(ap, sc_reg);
 	if (mmio)
 		writel(val, mmio);
+}
+
+static void sil_fill_sg(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	u32 addr, len;
+	unsigned int idx;
+
+	ata_fill_sg(qc);
+
+	if (unlikely(qc->n_elem < 1))
+		return;
+
+	/* hardware S/G list may be longer than number of 
+	 * PCI-mapped S/G entries (qc->n_elem), due to splitting
+	 * in ata_fill_sg().  Start at qc->n_elem, and skip to end
+	 * of list, if we're not already there.
+	 */
+	idx = qc->n_elem - 1;
+	while ((le32_to_cpu(ap->prd[idx].flags_len) & ATA_PRD_EOT) == 0)
+		idx++;
+
+	/* Errata workaround: if last segment is exactly 8K, split
+	 * into 7.5K and 512b pieces.
+	 */
+	len = (le32_to_cpu(ap->prd[idx].flags_len) & ~ATA_PRD_EOT) & 0xffff;
+	if (len == 8192) {
+		addr = le32_to_cpu(ap->prd[idx].addr);
+		ap->prd[idx].flags_len = cpu_to_le32(15 * 512);
+
+		ap->prd[idx].addr = cpu_to_le32(addr + (15 * 512));
+		ap->prd[idx].flags_len = cpu_to_le32(512 | ATA_PRD_EOT);
+	}
 }
 
 /**
author
#2 lore raw

PATCH 1/1 a better Silicon Image SATA mod15 write fix?

Sigh... it would help to disable the old errata fix.

Try 3.

===== drivers/scsi/sata_sil.c 1.14 vs edited =====

drivers/scsi/sata_sil.c +38 -25
@@ -71,7 +71,6 @@
 	SIL_IDE3_BMDMA		= 0x208,
 	SIL_IDE3_SCR		= 0x380,
 
-	SIL_QUIRK_MOD15WRITE	= (1 << 0),
 	SIL_QUIRK_UDMA5MAX	= (1 << 1),
 };
 

@@ -79,6 +78,7 @@
 static void sil_dev_config(struct ata_port *ap, struct ata_device *dev);
 static u32 sil_scr_read (struct ata_port *ap, unsigned int sc_reg);
 static void sil_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
+static void sil_fill_sg(struct ata_queued_cmd *qc);
 
 static struct pci_device_id sil_pci_tbl[] = {
 	{ 0x1095, 0x3112, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },

@@ -94,17 +94,6 @@
 	const char * product;
 	unsigned int quirk;
 } sil_blacklist [] = {
-	{ "ST320012AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST330013AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST340017AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST360015AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST380023AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST3120023AS",	SIL_QUIRK_MOD15WRITE },
-	{ "ST340014ASL",	SIL_QUIRK_MOD15WRITE },
-	{ "ST360014ASL",	SIL_QUIRK_MOD15WRITE },
-	{ "ST380011ASL",	SIL_QUIRK_MOD15WRITE },
-	{ "ST3120022ASL",	SIL_QUIRK_MOD15WRITE },
-	{ "ST3160021ASL",	SIL_QUIRK_MOD15WRITE },
 	{ "Maxtor 4D060H3",	SIL_QUIRK_UDMA5MAX },
 	{ }
 };

@@ -123,13 +112,13 @@
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
-	.sg_tablesize		= LIBATA_MAX_PRD,
+	.sg_tablesize		= 120,
 	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
-	.use_clustering		= ATA_SHT_USE_CLUSTERING,
+	.use_clustering		= 1,
 	.proc_name		= DRV_NAME,
-	.dma_boundary		= ATA_DMA_BOUNDARY,
+	.dma_boundary		= 0x1fff,
 	.slave_configure	= ata_scsi_slave_config,
 	.bios_param		= ata_std_bios_param,
 };

@@ -144,7 +133,7 @@
 	.phy_reset		= sata_phy_reset,
 	.phy_config		= pata_phy_config,	/* not a typo */
 	.bmdma_start            = ata_bmdma_start_mmio,
-	.fill_sg		= ata_fill_sg,
+	.fill_sg		= sil_fill_sg,
 	.eng_timeout		= ata_eng_timeout,
 	.irq_handler		= ata_interrupt,
 	.scr_read		= sil_scr_read,

@@ -212,6 +201,39 @@
 		writel(val, mmio);
 }
 
+static void sil_fill_sg(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	u32 addr, len;
+	unsigned int idx;
+
+	ata_fill_sg(qc);
+
+	if (unlikely(qc->n_elem < 1))
+		return;
+
+	/* hardware S/G list may be longer than number of 
+	 * PCI-mapped S/G entries (qc->n_elem), due to splitting
+	 * in ata_fill_sg().  Start at qc->n_elem, and skip to end
+	 * of list, if we're not already there.
+	 */
+	idx = qc->n_elem - 1;
+	while ((le32_to_cpu(ap->prd[idx].flags_len) & ATA_PRD_EOT) == 0)
+		idx++;
+
+	/* Errata workaround: if last segment is exactly 8K, split
+	 * into 7.5K and 512b pieces.
+	 */
+	len = (le32_to_cpu(ap->prd[idx].flags_len) & ~ATA_PRD_EOT) & 0xffff;
+	if (len == 8192) {
+		addr = le32_to_cpu(ap->prd[idx].addr);
+		ap->prd[idx].flags_len = cpu_to_le32(15 * 512);
+
+		ap->prd[idx].addr = cpu_to_le32(addr + (15 * 512));
+		ap->prd[idx].flags_len = cpu_to_le32(512 | ATA_PRD_EOT);
+	}
+}
+
 /**
  *	sil_dev_config - Apply device/host-specific errata fixups
  *	@ap: Port containing device to be examined

@@ -258,15 +280,6 @@
 			break;
 		}
 	
-	/* limit requests to 15 sectors */
-	if (quirks & SIL_QUIRK_MOD15WRITE) {
-		printk(KERN_INFO "ata%u(%u): applying Seagate errata fix\n",
-		       ap->id, dev->devno);
-		ap->host->max_sectors = 15;
-		ap->host->hostt->max_sectors = 15;
-		return;
-	}
-
 	/* limit to udma5 */
 	if (quirks & SIL_QUIRK_UDMA5MAX) {
 		printk(KERN_INFO "ata%u(%u): applying Maxtor errata fix %s\n",
#3 lore raw

Hello list,

This driver works :o)
running hdparm gives those results :
Timing buffered disk reads: 170 MB in 3.00 seconds = 56.60 MB/sec
(with 2.6.4-bk4 and sata_sil.c try 3)
Timing buffered disk reads: 170 MB in 3.01 seconds = 56.56 MB/sec
(with 2.6.4-mm2)

running boonie on the 2.6.4-mm2 version hangs the computer, with no
possibilities of doing anything (can't even use the magic SysRq keys), while on the 2.6.4-bk4 version using the patched version of sata_sil.c it doesn't exactly hang : it is still possible to switch from one console to another, but it's impossible to log into any of them (after typing in the password, you never get the prompt).
here is the command I used :
bonnie++ -u jdidron -d /tmp -x 10 -s 2g -n 32:30000:5:500 -m
kisskool > output

Mobo : A7N8X Deluxe
HD : 6Y120L0

Quoted 4 lines · Jeff Garzik

Sigh... it would help to disable the old errata fix.

Try 3.

author
#4 lore raw
Quoted 8 lines · Julien Didron

Hello list,

This driver works :o)
running hdparm gives those results :
Timing buffered disk reads: 170 MB in 3.00 seconds = 56.60 MB/sec
(with 2.6.4-bk4 and sata_sil.c try 3)
Timing buffered disk reads: 170 MB in 3.01 seconds = 56.56 MB/sec
(with 2.6.4-mm2)

Thanks much for testing. Try3 had a nasty bug, though.

Can you test Try4 as well?

Regards and thanks again,

Jeff

author
#5 lore raw

PATCH 1/1 a better Silicon Image SATA mod15 write fix?

In reply to Jeff Garzik

And here goes try4. Thanks for all the feedback so far...

===== drivers/scsi/sata_sil.c 1.14 vs edited =====

drivers/scsi/sata_sil.c +39 -25
@@ -71,7 +71,6 @@
 	SIL_IDE3_BMDMA		= 0x208,
 	SIL_IDE3_SCR		= 0x380,
 
-	SIL_QUIRK_MOD15WRITE	= (1 << 0),
 	SIL_QUIRK_UDMA5MAX	= (1 << 1),
 };
 

@@ -79,6 +78,7 @@
 static void sil_dev_config(struct ata_port *ap, struct ata_device *dev);
 static u32 sil_scr_read (struct ata_port *ap, unsigned int sc_reg);
 static void sil_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
+static void sil_fill_sg(struct ata_queued_cmd *qc);
 
 static struct pci_device_id sil_pci_tbl[] = {
 	{ 0x1095, 0x3112, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },

@@ -94,17 +94,6 @@
 	const char * product;
 	unsigned int quirk;
 } sil_blacklist [] = {
-	{ "ST320012AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST330013AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST340017AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST360015AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST380023AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST3120023AS",	SIL_QUIRK_MOD15WRITE },
-	{ "ST340014ASL",	SIL_QUIRK_MOD15WRITE },
-	{ "ST360014ASL",	SIL_QUIRK_MOD15WRITE },
-	{ "ST380011ASL",	SIL_QUIRK_MOD15WRITE },
-	{ "ST3120022ASL",	SIL_QUIRK_MOD15WRITE },
-	{ "ST3160021ASL",	SIL_QUIRK_MOD15WRITE },
 	{ "Maxtor 4D060H3",	SIL_QUIRK_UDMA5MAX },
 	{ }
 };

@@ -123,13 +112,13 @@
 	.eh_strategy_handler	= ata_scsi_error,
 	.can_queue		= ATA_DEF_QUEUE,
 	.this_id		= ATA_SHT_THIS_ID,
-	.sg_tablesize		= LIBATA_MAX_PRD,
+	.sg_tablesize		= 120,
 	.max_sectors		= ATA_MAX_SECTORS,
 	.cmd_per_lun		= ATA_SHT_CMD_PER_LUN,
 	.emulated		= ATA_SHT_EMULATED,
-	.use_clustering		= ATA_SHT_USE_CLUSTERING,
+	.use_clustering		= 1,
 	.proc_name		= DRV_NAME,
-	.dma_boundary		= ATA_DMA_BOUNDARY,
+	.dma_boundary		= 0x1fff,
 	.slave_configure	= ata_scsi_slave_config,
 	.bios_param		= ata_std_bios_param,
 };

@@ -144,7 +133,7 @@
 	.phy_reset		= sata_phy_reset,
 	.phy_config		= pata_phy_config,	/* not a typo */
 	.bmdma_start            = ata_bmdma_start_mmio,
-	.fill_sg		= ata_fill_sg,
+	.fill_sg		= sil_fill_sg,
 	.eng_timeout		= ata_eng_timeout,
 	.irq_handler		= ata_interrupt,
 	.scr_read		= sil_scr_read,

@@ -212,6 +201,40 @@
 		writel(val, mmio);
 }
 
+static void sil_fill_sg(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	u32 addr, len;
+	unsigned int idx;
+
+	ata_fill_sg(qc);
+
+	if (unlikely(qc->n_elem < 1))
+		return;
+
+	/* hardware S/G list may be longer (or shorter) than number of 
+	 * PCI-mapped S/G entries (qc->n_elem), due to splitting
+	 * in ata_fill_sg().  Start at zero, and skip to end
+	 * of list, if we're not already there.
+	 */
+	idx = 0;
+	while ((le32_to_cpu(ap->prd[idx].flags_len) & ATA_PRD_EOT) == 0)
+		idx++;
+
+	/* Errata workaround: if last segment is exactly 8K, split
+	 * into 7.5K and 512b pieces.
+	 */
+	len = le32_to_cpu(ap->prd[idx].flags_len) & 0xffff;
+	if (len == 8192) {
+		addr = le32_to_cpu(ap->prd[idx].addr);
+		ap->prd[idx].flags_len = cpu_to_le32(15 * 512);
+
+		idx++;
+		ap->prd[idx].addr = cpu_to_le32(addr + (15 * 512));
+		ap->prd[idx].flags_len = cpu_to_le32(512 | ATA_PRD_EOT);
+	}
+}
+
 /**
  *	sil_dev_config - Apply device/host-specific errata fixups
  *	@ap: Port containing device to be examined

@@ -258,15 +281,6 @@
 			break;
 		}
 	
-	/* limit requests to 15 sectors */
-	if (quirks & SIL_QUIRK_MOD15WRITE) {
-		printk(KERN_INFO "ata%u(%u): applying Seagate errata fix\n",
-		       ap->id, dev->devno);
-		ap->host->max_sectors = 15;
-		ap->host->hostt->max_sectors = 15;
-		return;
-	}
-
 	/* limit to udma5 */
 	if (quirks & SIL_QUIRK_UDMA5MAX) {
 		printk(KERN_INFO "ata%u(%u): applying Maxtor errata fix %s\n",