[PATCH linux-2.6.13-rc3] Mod15Write quirk against v2.6.13

[PATCH linux-2.6.13-rc3] Mod15Write quirk against v2.6.13

[Tejun Heo]

On Wed, Jul 20, 2005 at 02:13:55PM -0700, Michael Madore wrote:
> Hi,
> 
> I have been successfully using your mod15write quirk patch with 2.6.12-
> rc3.  I recently applied the patch (with a minor reject) to 2.6.12.3.
> Reading seems to work fine, but writing to the disk results in the
> following output:
> 
> irq 169: nobody cared!
> 
> Call Trace: <IRQ> <ffffffff80161e15>{__report_bad_irq+53}
> <ffffffff80161eec>{note_interrupt+92}
>        <ffffffff80161730>{__do_IRQ+256} <ffffffff801116b8>{do_IRQ+72}
>        <ffffffff8010f057>{ret_from_intr+0}  <EOI>
> <ffffffff8010d390>{default_idle+0}
>        <ffffffff8010d3b2>{default_idle+34} <ffffffff8010d407>{cpu_idle
> +71}
>        <ffffffff80515094>{start_secondary+564} 
> handlers:
> [<ffffffff88035490>] (ata_interrupt+0x0/0x180 [libata])
> [<ffffffff88151440>] (snd_intel8x0_interrupt+0x0/0x240 [snd_intel8x0])
> Disabling IRQ #169
> 
> Shortly thereafter, there is a kernel oops and the machine has to be
> rebooted.  Do you have an updated driver for 2.6.12?  I assume something
> has changed in libata.
> 

 Hello, Michael, again.
 Hello, Jeff, Albert & ATA guys.

 This is reply message to Michael's private mail reporting patch apply
failure and (after hand-fixing it) malfunction.  I hope Michael
wouldn't mind adding recipients to this reply.

 sata_sil Mod15Write workaround was broken by the following commit by
Albert Lee.

Commit: 21b1ed74ee3667dcabcba92e486988ea9119a085
[PATCH] libata: Prevent the interrupt handler from completing a command twice

 This commit clears ATA_QCFLAG_ACTIVE in ata_qc_complete() and doesn't
handle IRQ if ATA_QCFLAG_ACTIVE is cleared on entry to interrupt
routine.  As m15w workaround executes single command multiple times,
the flag is cleared after the first chunk completion and the following
interrupt gets ignored resulting in "nobody cared" interrupt error.

 The following changes are made in m15w workaround to fix this.

 * Moved clearing of ATA_QCFLAG_ACTIVE before invoking ->complete_fn,
   so that ->complete_fn can mangle with the flag.  This doesn't affect
   any users.
 * Added setting ATA_QCFLAG_ACTIVE in m15w chunk completion function.

 One thing that bothers me is how Albert's commit and the original
ata_host_intr tell IRQ subsystem that an interrupt isn't ours when we
know that we have generated a spurious interrupt.  IMHO, we always
should enter ata_host_intr and always tell IRQ subsystem that it's our
interrupt if bmdma_status tells us so, regardless of ata status value.
The current code is likely to cause "nobody cared" error which can be
avoided.

 Also, Jeff, I know you're very busy, but what do you think about
taking m15w workaround into ata tree?  It's been around for a while
now and I haven't received any complaints (except for this one) yet.
The workaround is ugly but it surely helps and I'm willing to maintain
it.

 This patch is against v2.6.13-rc3 but also applies to v2.6.12 (with a
fuss).

 Thanks.


Signed-off-by: Tejun Heo <htejun@gmail.com>


diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3087,8 +3087,8 @@ void ata_qc_complete(struct ata_queued_c
 		ata_sg_clean(qc);
 
 	/* call completion callback */
-	rc = qc->complete_fn(qc, drv_stat);
 	qc->flags &= ~ATA_QCFLAG_ACTIVE;
+	rc = qc->complete_fn(qc, drv_stat);
 
 	/* if callback indicates not to complete command (non-zero),
 	 * return immediately
diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -71,9 +71,12 @@ enum {
 
 static int sil_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
 static void sil_dev_config(struct ata_port *ap, struct ata_device *dev);
+static void sil_qc_prep (struct ata_queued_cmd *qc);
+static void sil_eng_timeout (struct ata_port *ap);
 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_post_set_mode (struct ata_port *ap);
+static void sil_host_stop (struct ata_host_set *host_set);
 
 static struct pci_device_id sil_pci_tbl[] = {
 	{ 0x1095, 0x3112, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
@@ -152,16 +155,16 @@ static struct ata_port_operations sil_op
 	.bmdma_start            = ata_bmdma_start,
 	.bmdma_stop		= ata_bmdma_stop,
 	.bmdma_status		= ata_bmdma_status,
-	.qc_prep		= ata_qc_prep,
+	.qc_prep		= sil_qc_prep,
 	.qc_issue		= ata_qc_issue_prot,
-	.eng_timeout		= ata_eng_timeout,
+	.eng_timeout		= sil_eng_timeout,
 	.irq_handler		= ata_interrupt,
 	.irq_clear		= ata_bmdma_irq_clear,
 	.scr_read		= sil_scr_read,
 	.scr_write		= sil_scr_write,
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_host_stop,
+	.host_stop		= sil_host_stop,
 };
 
 static struct ata_port_info sil_port_info[] = {
@@ -204,6 +207,53 @@ static const struct {
 	/* ... port 3 */
 };
 
+/*
+ * Context to loop over write requests > 15 sectors for Mod15Write bug.
+ *
+ * The following libata layer fields are saved at the beginning and
+ * mangled as necessary.
+ *
+ * qc->sg		: To fool ata_fill_sg().
+ * qc->n_elem		: ditto.
+ * qc->flags		: Except for the last iteration, ATA_QCFLAG_DMAMAP
+ *			  should be off on entering ata_interrupt() such
+ *			  that ata_qc_complete() doesn't call ata_sg_clean()
+ *			  before sil_m15w_chunk_complete(), but the flags
+ *			  should be set for ata_qc_prep() to work.  This flag
+ *			  handling is the hackiest part of this workaround.
+ * qc->complete_fn	: Overrided to sil_m15w_chunk_complete().
+ *
+ * The following cxt fields are used to iterate over write requests.
+ *
+ * next_block		: The starting block of the next chunk.
+ * next_sg		: The first sg entry of the next chunk.
+ * left			: Total bytes left.
+ * cur_sg_ofs		: Number of processed bytes in the first sg entry
+ *			  of this chunk.
+ * next_sg_ofs		: Number of bytes to be processed in the last sg
+ *			  entry of this chunk.
+ * next_sg_len		: Number of bytes to be processed in the first sg
+ *			  entry of the next chunk.
+ */
+#define M15W_DEBUG
+struct sil_m15w_cxt {
+	u64			next_block;
+	struct scatterlist *	next_sg;
+	unsigned int		left;
+	unsigned int		cur_sg_ofs;
+	unsigned int		next_sg_ofs;
+	unsigned int		next_sg_len;
+	int			timedout;
+
+	struct scatterlist *	orig_sg;
+	unsigned int		orig_nelem;
+	unsigned long		orig_flags;
+	ata_qc_cb_t		orig_complete_fn;
+#ifdef M15W_DEBUG
+	struct scatterlist	sg_copy[LIBATA_MAX_PRD];
+#endif
+};
+
 MODULE_AUTHOR("Jeff Garzik");
 MODULE_DESCRIPTION("low-level driver for Silicon Image SATA controller");
 MODULE_LICENSE("GPL");
@@ -244,6 +294,226 @@ static void sil_post_set_mode (struct at
 	readl(addr);	/* flush */
 }
 
+static inline u64 sil_m15w_read_tf_block (struct ata_taskfile *tf)
+{
+	u64 block = 0;
+
+	block |= (u64)tf->lbal;
+	block |= (u64)tf->lbam << 8;
+	block |= (u64)tf->lbah << 16;
+
+	if (tf->flags & ATA_TFLAG_LBA48) {
+		block |= (u64)tf->hob_lbal << 24;
+		block |= (u64)tf->hob_lbam << 32;
+		block |= (u64)tf->hob_lbah << 40;
+	} else
+		block |= (u64)(tf->device & 0xf) << 24;
+
+	return block;
+}
+
+static inline void sil_m15w_rewrite_tf (struct ata_taskfile *tf,
+					u64 block, u16 nsect)
+{
+	tf->nsect = nsect & 0xff;
+	tf->lbal = block & 0xff;
+	tf->lbam = (block >> 8) & 0xff;
+	tf->lbah = (block >> 16) & 0xff;
+
+	if (tf->flags & ATA_TFLAG_LBA48) {
+		tf->hob_nsect = (nsect >> 8) & 0xff;
+		tf->hob_lbal = (block >> 24) & 0xff;
+		tf->hob_lbam = (block >> 32) & 0xff;
+		tf->hob_lbah = (block >> 40) & 0xff;
+	} else {
+		tf->device &= ~0xf;
+		tf->device |= (block >> 24) & 0xf;
+	}
+}
+
+static void sil_m15w_next(struct ata_queued_cmd *qc)
+{
+	struct sil_m15w_cxt *cxt = qc->private_data;
+	struct scatterlist *sg;
+	unsigned int todo, res, nelem;
+
+	if (qc->sg != cxt->next_sg) {
+		sg_dma_address(qc->sg) -= cxt->cur_sg_ofs;
+		sg_dma_len(qc->sg) += cxt->cur_sg_ofs;
+		cxt->cur_sg_ofs = 0;
+	}
+	cxt->cur_sg_ofs += cxt->next_sg_ofs;
+
+	qc->sg = sg = cxt->next_sg;
+	sg_dma_address(sg) += cxt->next_sg_ofs;
+	sg_dma_len(sg) = cxt->next_sg_len;
+
+	res = todo = min_t(unsigned int, cxt->left, 15 << 9);
+
+	nelem = 0;
+	while (sg_dma_len(sg) <= res) {
+		res -= sg_dma_len(sg);
+		sg++;
+		nelem++;
+	}
+
+	if (todo < cxt->left) {
+		cxt->next_sg = sg;
+		cxt->next_sg_ofs = res;
+		cxt->next_sg_len = sg_dma_len(sg) - res;
+		if (res) {
+			nelem++;
+			sg_dma_len(sg) = res;
+		}
+	} else {
+		cxt->next_sg = NULL;
+		cxt->next_sg_ofs = 0;
+		cxt->next_sg_len = 0;
+	}
+
+	DPRINTK("block=%llu nelem=%u todo=%u left=%u\n",
+		cxt->next_block, nelem, todo, cxt->left);
+
+	qc->n_elem = nelem;
+	sil_m15w_rewrite_tf(&qc->tf, cxt->next_block, todo >> 9);
+	cxt->left -= todo;
+	cxt->next_block += todo >> 9;
+}
+
+static inline void sil_m15w_restore_qc (struct ata_queued_cmd *qc)
+{
+	struct sil_m15w_cxt *cxt = qc->private_data;
+
+	DPRINTK("ENTER\n");
+
+	sg_dma_address(qc->sg) -= cxt->cur_sg_ofs;
+	sg_dma_len(qc->sg) += cxt->cur_sg_ofs;
+	if (cxt->next_sg_ofs)
+		sg_dma_len(cxt->next_sg) += cxt->next_sg_len;
+	qc->sg = cxt->orig_sg;
+	qc->n_elem = cxt->orig_nelem;
+	qc->flags |= cxt->orig_flags;
+	qc->complete_fn = cxt->orig_complete_fn;
+#ifdef M15W_DEBUG
+	{
+		int i, j;
+		for (i = 0; i < qc->n_elem; i++)
+			if (memcmp(&cxt->sg_copy[i], &qc->sg[i],
+				   sizeof(qc->sg[0])))
+				break;
+		if (i < qc->n_elem) {
+			printk(KERN_ERR "sil_m15w: sg mismatch\n");
+			printk(KERN_ERR "orig: ");
+			for (j = 0; j < qc->n_elem; j++)
+				printk("%s%08x:%04u ",
+				       i == j ? "*" : "",
+				       (u32)sg_dma_address(&cxt->sg_copy[j]),
+				       sg_dma_len(&cxt->sg_copy[j]));
+			printk("\n");
+			printk(KERN_ERR "used: ");
+			for (j = 0; j < qc->n_elem; j++)
+				printk("%s%08x:%04u ",
+				       i == j ? "*" : "",
+				       (u32)sg_dma_address(&qc->sg[j]),
+				       sg_dma_len(&qc->sg[j]));
+			printk("\n");
+		}
+	}
+#endif
+}
+
+static int sil_m15w_chunk_complete (struct ata_queued_cmd *qc, u8 drv_stat)
+{
+	struct sil_m15w_cxt *cxt = qc->private_data;
+
+	DPRINTK("ENTER\n");
+
+	/* This command is still active, turn ACTIVE back on */
+	qc->flags |= ATA_QCFLAG_ACTIVE;
+
+	if (unlikely(cxt->timedout))
+		drv_stat |= ATA_BUSY;	/* Any better error status? */
+
+	/* Complete the command immediately on error */
+	if (unlikely(drv_stat & (ATA_ERR | ATA_BUSY | ATA_DRQ))) {
+		sil_m15w_restore_qc(qc);
+		ata_qc_complete(qc, drv_stat);
+		return 1;
+	}
+
+	sil_m15w_next(qc);
+
+	qc->flags |= cxt->orig_flags;
+	ata_qc_prep(qc);
+	qc->flags &= ~ATA_QCFLAG_DMAMAP;
+
+	/* On last iteration, restore fields such that normal
+	 * completion path is run */
+	if (!cxt->left)
+		sil_m15w_restore_qc(qc);
+	sil_ops.qc_issue(qc);
+	return 1;
+}
+
+static void sil_qc_prep (struct ata_queued_cmd *qc)
+{
+	struct sil_m15w_cxt *cxt = qc->private_data;
+
+	if (unlikely(cxt && qc->tf.flags & ATA_TFLAG_WRITE && qc->nsect > 15)) {
+		BUG_ON(cxt->left);
+		if (qc->tf.protocol == ATA_PROT_DMA) {
+			/* Okay, begin mod15write workaround */
+			cxt->next_block = sil_m15w_read_tf_block(&qc->tf);
+			cxt->next_sg = qc->sg;
+			cxt->left = qc->nsect << 9;
+			cxt->cur_sg_ofs = 0;
+			cxt->next_sg_ofs = 0;
+			cxt->next_sg_len = sg_dma_len(qc->sg);
+			cxt->timedout = 0;
+
+			/* Save fields we're gonna mess with.  Read comments
+			 * above struct sil_m15w_cxt for more info. */
+			cxt->orig_sg = qc->sg;
+			cxt->orig_nelem = qc->n_elem;
+			cxt->orig_flags = qc->flags & ATA_QCFLAG_DMAMAP;
+			cxt->orig_complete_fn = qc->complete_fn;
+			qc->complete_fn = sil_m15w_chunk_complete;
+#ifdef M15W_DEBUG
+			{
+				int i;
+				for (i = 0; i < qc->n_elem; i++)
+					cxt->sg_copy[i] = qc->sg[i];
+			}
+#endif
+			DPRINTK("MOD15WRITE, block=%llu nsect=%u\n",
+				cxt->next_block, qc->nsect);
+			sil_m15w_next(qc);
+
+			ata_qc_prep(qc);
+			qc->flags &= ~ATA_QCFLAG_DMAMAP;
+			return;
+		} else
+			printk(KERN_WARNING "ata%u(%u): write request > 15 "
+			       "issued using non-DMA protocol.  Drive may "
+			       "lock up.\n", qc->ap->id, qc->dev->devno);
+	}
+
+	ata_qc_prep(qc);
+}
+
+static void sil_eng_timeout (struct ata_port *ap)
+{
+	struct ata_queued_cmd *qc = ata_qc_from_tag(ap, ap->active_tag);
+
+	if (qc && qc->private_data) {
+		struct sil_m15w_cxt *cxt = qc->private_data;
+		if (cxt->left)
+			cxt->timedout = 1;
+	}
+
+	ata_eng_timeout(ap);
+}
+
 static inline unsigned long sil_scr_addr(struct ata_port *ap, unsigned int sc_reg)
 {
 	unsigned long offset = ap->ioaddr.scr_addr;
@@ -278,6 +548,14 @@ static void sil_scr_write (struct ata_po
 		writel(val, mmio);
 }
 
+static void sil_host_stop (struct ata_host_set *host_set)
+{
+	/* Free mod15write context array. */
+	kfree(host_set->private_data);
+
+	ata_host_stop(host_set);
+}
+
 /**
  *	sil_dev_config - Apply device/host-specific errata fixups
  *	@ap: Port containing device to be examined
@@ -288,17 +566,12 @@ static void sil_scr_write (struct ata_po
  *	We apply two errata fixups which are specific to Silicon Image,
  *	a Seagate and a Maxtor fixup.
  *
- *	For certain Seagate devices, we must limit the maximum sectors
- *	to under 8K.
+ *	For certain Seagate devices, we cannot issue write requests
+ *	larger than 15 sectors.
  *
  *	For certain Maxtor devices, we must not program the drive
  *	beyond udma5.
  *
- *	Both fixups are unfairly pessimistic.  As soon as I get more
- *	information on these errata, I will create a more exhaustive
- *	list, and apply the fixups to only the specific
- *	devices/hosts/firmwares that need it.
- *
  *	20040111 - Seagate drives affected by the Mod15Write bug are blacklisted
  *	The Maxtor quirk is in the blacklist, but I'm keeping the original
  *	pessimistic fix for the following reasons...
@@ -306,6 +579,15 @@ static void sil_scr_write (struct ata_po
  *	Windows	driver, maybe only one is affected.  More info would be greatly
  *	appreciated.
  *	- But then again UDMA5 is hardly anything to complain about
+ *
+ *	20050316 Tejun Heo - Proper Mod15Write workaround implemented.
+ *	sata_sil doesn't report affected Seagate drives as having max
+ *	sectors of 15 anymore, but handle write requests larger than
+ *	15 sectors by looping over it inside this driver proper.  This
+ *	is messy but it's better to isolate this kind of peculiar bug
+ *	handling inside individual drivers than tainting libata layer.
+ *	This workaround results in unhampered read performance and
+ *	much better write performance.
  */
 static void sil_dev_config(struct ata_port *ap, struct ata_device *dev)
 {
@@ -313,6 +595,7 @@ static void sil_dev_config(struct ata_po
 	unsigned char model_num[40];
 	const char *s;
 	unsigned int len;
+	int i;
 
 	ata_dev_id_string(dev->id, model_num, ATA_ID_PROD_OFS,
 			  sizeof(model_num));
@@ -330,15 +613,23 @@ static void sil_dev_config(struct ata_po
 			break;
 		}
 	
-	/* limit requests to 15 sectors */
+	/* Activate mod15write quirk workaround */
 	if (quirks & SIL_QUIRK_MOD15WRITE) {
+		struct sil_m15w_cxt *cxt;
+
 		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;
-		dev->flags |= ATA_DFLAG_LOCK_SECTORS;
+
+		cxt = ap->host_set->private_data;
+		cxt += ap->port_no * ATA_MAX_QUEUE;
+		for (i = 0; i < ATA_MAX_QUEUE; i++)
+			ap->qcmd[i].private_data = cxt++;
+
 		return;
 	}
+	/* Clear qcmd->private_data if mod15write quirk isn't present */
+	for (i = 0; i < ATA_MAX_QUEUE; i++)
+		ap->qcmd[i].private_data = NULL;
 
 	/* limit to udma5 */
 	if (quirks & SIL_QUIRK_UDMA5MAX) {
@@ -352,7 +643,8 @@ static void sil_dev_config(struct ata_po
 static int sil_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	static int printed_version;
-	struct ata_probe_ent *probe_ent = NULL;
+	struct ata_probe_ent *probe_ent;
+	struct sil_m15w_cxt *m15w_cxt;
 	unsigned long base;
 	void *mmio_base;
 	int rc;
@@ -385,11 +677,17 @@ static int sil_init_one (struct pci_dev 
 	if (rc)
 		goto err_out_regions;
 
-	probe_ent = kmalloc(sizeof(*probe_ent), GFP_KERNEL);
-	if (probe_ent == NULL) {
-		rc = -ENOMEM;
+	rc = -ENOMEM;
+
+	tmp = sizeof(m15w_cxt[0]) * ATA_MAX_PORTS * ATA_MAX_QUEUE;
+	m15w_cxt = kmalloc(tmp, GFP_KERNEL);
+	if (m15w_cxt == NULL)
 		goto err_out_regions;
-	}
+	memset(m15w_cxt, 0, tmp);
+
+	probe_ent = kmalloc(sizeof(*probe_ent), GFP_KERNEL);
+	if (probe_ent == NULL)
+		goto err_out_free_m15w;
 
 	memset(probe_ent, 0, sizeof(*probe_ent));
 	INIT_LIST_HEAD(&probe_ent->node);
@@ -403,6 +701,7 @@ static int sil_init_one (struct pci_dev 
        	probe_ent->irq = pdev->irq;
        	probe_ent->irq_flags = SA_SHIRQ;
 	probe_ent->host_flags = sil_port_info[ent->driver_data].host_flags;
+	probe_ent->private_data = m15w_cxt;
 
 	mmio_base = ioremap(pci_resource_start(pdev, 5),
 		            pci_resource_len(pdev, 5));
@@ -479,6 +778,8 @@ static int sil_init_one (struct pci_dev 
 
 err_out_free_ent:
 	kfree(probe_ent);
+err_out_free_m15w:
+	kfree(m15w_cxt);
 err_out_regions:
 	pci_release_regions(pdev);
 err_out:

Re: [PATCH linux-2.6.13-rc3] Mod15Write quirk against v2.6.13

[Jeff Garzik]

Tejun Heo wrote:
> Also, Jeff, I know you're very busy, but what do you think about
> taking m15w workaround into ata tree?  It's been around for a while
> now and I haven't received any complaints (except for this one) yet.
> The workaround is ugly but it surely helps and I'm willing to maintain
> it.

I think your mod15write solution is too messy to deal with long-term. 
Maintenance burden is much lower on us without it.  It's not too 
difficult to simply avoid certain combinations of hardware.

Note!  As Carlos @ Silicon Image points out, the blacklist should only 
apply to SiI 3112, not 3512/3114/etc.  That's an open FIXME that would 
benefit users.


>  One thing that bothers me is how Albert's commit and the original
> ata_host_intr tell IRQ subsystem that an interrupt isn't ours when we
> know that we have generated a spurious interrupt.  IMHO, we always
> should enter ata_host_intr and always tell IRQ subsystem that it's our
> interrupt if bmdma_status tells us so, regardless of ata status value.
> The current code is likely to cause "nobody cared" error which can be
> avoided.

If there is a mismatch between BMDMA's IRQ bit and ATA device indicating 
an interrupt, that's a host state machine[1] violation that should be 
addressed elsewhere.

I wouldn't mind using BMDMA IRQ bit as an indicator of the ATA intrq 
status, though.

(for others) As linked on

	http://www.t13.org/project/d1510r1-Host-Adapter.pdf
		via
	http://linux.yyz.us/sata/devel.html

you can find documentation on the BMDMA IRQ bit.

The main problem with using BMDMA IRQ bit is that it is likely never set 
unless the commands are READ/WRITE DMA commands, which means we must 
have separate host state machine tracking for PIO and non-data commands, 
increasing complexity.

	Jeff



[1] "host state machine"  These are illustrated by the state machine 
diagrams in ATA/ATAPI-7 Volume 3, under the chapter headings "Bus idle 
protocol", "Non-data command protocol", "DMA command protocol", "Ultra 
DMA data-{in,out} command protocol".  STUDY THESE.

Re: [PATCH linux-2.6.13-rc3] Mod15Write quirk against v2.6.13

[Jeff Garzik]

Tejun Heo wrote:
>  sata_sil Mod15Write workaround was broken by the following commit by
> Albert Lee.
> 
> Commit: 21b1ed74ee3667dcabcba92e486988ea9119a085
> [PATCH] libata: Prevent the interrupt handler from completing a command twice
> 
>  This commit clears ATA_QCFLAG_ACTIVE in ata_qc_complete() and doesn't
> handle IRQ if ATA_QCFLAG_ACTIVE is cleared on entry to interrupt
> routine.  As m15w workaround executes single command multiple times,
> the flag is cleared after the first chunk completion and the following
> interrupt gets ignored resulting in "nobody cared" interrupt error.
> 
>  The following changes are made in m15w workaround to fix this.
> 
>  * Moved clearing of ATA_QCFLAG_ACTIVE before invoking ->complete_fn,
>    so that ->complete_fn can mangle with the flag.  This doesn't affect
>    any users.
>  * Added setting ATA_QCFLAG_ACTIVE in m15w chunk completion function.

> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -3087,8 +3087,8 @@ void ata_qc_complete(struct ata_queued_c
>  		ata_sg_clean(qc);
>  
>  	/* call completion callback */
> -	rc = qc->complete_fn(qc, drv_stat);
>  	qc->flags &= ~ATA_QCFLAG_ACTIVE;
> +	rc = qc->complete_fn(qc, drv_stat);
>  
>  	/* if callback indicates not to complete command (non-zero),
>  	 * return immediately


I'm leaning towards applying latest Albert's ATA_QCFLAG_ACTIVE fix, 
which does the same thing your patch did.

For various complex tasks in the SCSI translation layer (SAT), we may 
wish to issue multiple ATA commands, before signalling completion.  As 
your mod15write patch is an example of this, it helps point out what 
parts of libata need work in order to accomplish this.

Any comments before I apply Albert's patch?

	Jeff

Re: [PATCH linux-2.6.13-rc3] Mod15Write quirk against v2.6.13

[Tejun Heo]

  Hi, Jeff.

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>>  sata_sil Mod15Write workaround was broken by the following commit by
>> Albert Lee.
>>
>> Commit: 21b1ed74ee3667dcabcba92e486988ea9119a085
>> [PATCH] libata: Prevent the interrupt handler from completing a 
>> command twice
>>
>>  This commit clears ATA_QCFLAG_ACTIVE in ata_qc_complete() and doesn't
>> handle IRQ if ATA_QCFLAG_ACTIVE is cleared on entry to interrupt
>> routine.  As m15w workaround executes single command multiple times,
>> the flag is cleared after the first chunk completion and the following
>> interrupt gets ignored resulting in "nobody cared" interrupt error.
>>
>>  The following changes are made in m15w workaround to fix this.
>>
>>  * Moved clearing of ATA_QCFLAG_ACTIVE before invoking ->complete_fn,
>>    so that ->complete_fn can mangle with the flag.  This doesn't affect
>>    any users.
>>  * Added setting ATA_QCFLAG_ACTIVE in m15w chunk completion function.
> 
> 
>> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
>> --- a/drivers/scsi/libata-core.c
>> +++ b/drivers/scsi/libata-core.c
>> @@ -3087,8 +3087,8 @@ void ata_qc_complete(struct ata_queued_c
>>          ata_sg_clean(qc);
>>  
>>      /* call completion callback */
>> -    rc = qc->complete_fn(qc, drv_stat);
>>      qc->flags &= ~ATA_QCFLAG_ACTIVE;
>> +    rc = qc->complete_fn(qc, drv_stat);
>>  
>>      /* if callback indicates not to complete command (non-zero),
>>       * return immediately
> 
> 
> 
> I'm leaning towards applying latest Albert's ATA_QCFLAG_ACTIVE fix, 
> which does the same thing your patch did.
> 
> For various complex tasks in the SCSI translation layer (SAT), we may 
> wish to issue multiple ATA commands, before signalling completion.  As 
> your mod15write patch is an example of this, it helps point out what 
> parts of libata need work in order to accomplish this.
> 
> Any comments before I apply Albert's patch?
> 

  I think the following patch I've posted deal with the same problem.

http://marc.theaimsgroup.com/?l=linux-ide&m=112454734102242&w=2

  Which, I think, is a better way to fix it.  It's dependant on the 
previous patchset you've just NAK'ed, so it cannot be applied but please 
take a look.  The base of the problem is that we run both 
ata_qc_complete and EH concurrently.

-- 
tejun

Re: [PATCH linux-2.6.13-rc3] Mod15Write quirk against v2.6.13

[Jeff Garzik]

Tejun Heo wrote:
> 
>  Hi, Jeff.
> 
> Jeff Garzik wrote:
> 
>> Tejun Heo wrote:
>>
>>>  sata_sil Mod15Write workaround was broken by the following commit by
>>> Albert Lee.
>>>
>>> Commit: 21b1ed74ee3667dcabcba92e486988ea9119a085
>>> [PATCH] libata: Prevent the interrupt handler from completing a 
>>> command twice
>>>
>>>  This commit clears ATA_QCFLAG_ACTIVE in ata_qc_complete() and doesn't
>>> handle IRQ if ATA_QCFLAG_ACTIVE is cleared on entry to interrupt
>>> routine.  As m15w workaround executes single command multiple times,
>>> the flag is cleared after the first chunk completion and the following
>>> interrupt gets ignored resulting in "nobody cared" interrupt error.
>>>
>>>  The following changes are made in m15w workaround to fix this.
>>>
>>>  * Moved clearing of ATA_QCFLAG_ACTIVE before invoking ->complete_fn,
>>>    so that ->complete_fn can mangle with the flag.  This doesn't affect
>>>    any users.
>>>  * Added setting ATA_QCFLAG_ACTIVE in m15w chunk completion function.
>>
>>
>>
>>> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
>>> --- a/drivers/scsi/libata-core.c
>>> +++ b/drivers/scsi/libata-core.c
>>> @@ -3087,8 +3087,8 @@ void ata_qc_complete(struct ata_queued_c
>>>          ata_sg_clean(qc);
>>>  
>>>      /* call completion callback */
>>> -    rc = qc->complete_fn(qc, drv_stat);
>>>      qc->flags &= ~ATA_QCFLAG_ACTIVE;
>>> +    rc = qc->complete_fn(qc, drv_stat);
>>>  
>>>      /* if callback indicates not to complete command (non-zero),
>>>       * return immediately
>>
>>
>>
>>
>> I'm leaning towards applying latest Albert's ATA_QCFLAG_ACTIVE fix, 
>> which does the same thing your patch did.
>>
>> For various complex tasks in the SCSI translation layer (SAT), we may 
>> wish to issue multiple ATA commands, before signalling completion.  As 
>> your mod15write patch is an example of this, it helps point out what 
>> parts of libata need work in order to accomplish this.
>>
>> Any comments before I apply Albert's patch?
>>
> 
>  I think the following patch I've posted deal with the same problem.
> 
> http://marc.theaimsgroup.com/?l=linux-ide&m=112454734102242&w=2


In the SCSI translation layer (SAT), some simulation may require issuing 
multiple ATA commands, before indicating a completion to the upper 
layers.  That capability is something I definitely want libata to 
support.  Your mod15write patch is an excellent example of this, even 
though I NAK'd it due to long term sata_sil maintenance burden.

That's why I think move ATA_QCFLAG_ACTIVE -anyway- may be a good idea.

	Jeff

Re: [PATCH linux-2.6.13-rc3] Mod15Write quirk against v2.6.13

[Tejun Heo]

  Hello, Jeff.

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> Also, Jeff, I know you're very busy, but what do you think about
>> taking m15w workaround into ata tree?  It's been around for a while
>> now and I haven't received any complaints (except for this one) yet.
>> The workaround is ugly but it surely helps and I'm willing to maintain
>> it.
> 
> 
> I think your mod15write solution is too messy to deal with long-term. 
> Maintenance burden is much lower on us without it.  It's not too 
> difficult to simply avoid certain combinations of hardware.
> 

  No problem, I'll maintain it out of tree.

> Note!  As Carlos @ Silicon Image points out, the blacklist should only 
> apply to SiI 3112, not 3512/3114/etc.  That's an open FIXME that would 
> benefit users.
> 

  Cool, I'll submit a patch right away.

> 
>>  One thing that bothers me is how Albert's commit and the original
>> ata_host_intr tell IRQ subsystem that an interrupt isn't ours when we
>> know that we have generated a spurious interrupt.  IMHO, we always
>> should enter ata_host_intr and always tell IRQ subsystem that it's our
>> interrupt if bmdma_status tells us so, regardless of ata status value.
>> The current code is likely to cause "nobody cared" error which can be
>> avoided.
> 
> 
> If there is a mismatch between BMDMA's IRQ bit and ATA device indicating 
> an interrupt, that's a host state machine[1] violation that should be 
> addressed elsewhere.
> 
> I wouldn't mind using BMDMA IRQ bit as an indicator of the ATA intrq 
> status, though.
> 
> (for others) As linked on
> 
>     http://www.t13.org/project/d1510r1-Host-Adapter.pdf
>         via
>     http://linux.yyz.us/sata/devel.html
> 
> you can find documentation on the BMDMA IRQ bit.
> 
> The main problem with using BMDMA IRQ bit is that it is likely never set 
> unless the commands are READ/WRITE DMA commands, which means we must 
> have separate host state machine tracking for PIO and non-data commands, 
> increasing complexity.
> 
>     Jeff
> 
> 
> 
> [1] "host state machine"  These are illustrated by the state machine 
> diagrams in ATA/ATAPI-7 Volume 3, under the chapter headings "Bus idle 
> protocol", "Non-data command protocol", "DMA command protocol", "Ultra 
> DMA data-{in,out} command protocol".  STUDY THESE.

  Yeap, I'll.

-- 
tejun

Re: [PATCH linux-2.6.13-rc3] Mod15Write quirk against v2.6.13

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> Also, Jeff, I know you're very busy, but what do you think about
>>> taking m15w workaround into ata tree?  It's been around for a while
>>> now and I haven't received any complaints (except for this one) yet.
>>> The workaround is ugly but it surely helps and I'm willing to maintain
>>> it.

>> I think your mod15write solution is too messy to deal with long-term. 
>> Maintenance burden is much lower on us without it.  It's not too 
>> difficult to simply avoid certain combinations of hardware.

>  No problem, I'll maintain it out of tree.

You're welcome to maintain it as an independent branch of 
libata-dev.git, if you would prefer.


>> Note!  As Carlos @ Silicon Image points out, the blacklist should only 
>> apply to SiI 3112, not 3512/3114/etc.  That's an open FIXME that would 
>> benefit users.

>  Cool, I'll submit a patch right away.

Thanks,

	Jeff

Re: [PATCH linux-2.6.13-rc3] Mod15Write quirk against v2.6.13

[Tejun Heo]

  Hello, Jeff.

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> Jeff Garzik wrote:
>>
>>> Tejun Heo wrote:
>>>
>>>> Also, Jeff, I know you're very busy, but what do you think about
>>>> taking m15w workaround into ata tree?  It's been around for a while
>>>> now and I haven't received any complaints (except for this one) yet.
>>>> The workaround is ugly but it surely helps and I'm willing to maintain
>>>> it.
> 
> 
>>> I think your mod15write solution is too messy to deal with long-term. 
>>> Maintenance burden is much lower on us without it.  It's not too 
>>> difficult to simply avoid certain combinations of hardware.
> 
> 
>>  No problem, I'll maintain it out of tree.
> 
> 
> You're welcome to maintain it as an independent branch of 
> libata-dev.git, if you would prefer.
> 

  I think I'll just keep it out of tree.  I don't think many libata 
developers would be interested in m15w workaround.

> 
>>> Note!  As Carlos @ Silicon Image points out, the blacklist should 
>>> only apply to SiI 3112, not 3512/3114/etc.  That's an open FIXME that 
>>> would benefit users.
> 
> 
>>  Cool, I'll submit a patch right away.
> 

  Can you please tell me which are affected and which are not?

	{ 0x1095, 0x3112, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
	{ 0x1095, 0x0240, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
	{ 0x1095, 0x3512, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
	{ 0x1095, 0x3114, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3114 },
	{ 0x1002, 0x436e, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
	{ 0x1002, 0x4379, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
	{ 0x1002, 0x437a, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },

  Thanks.

-- 
tejun

Re: [PATCH linux-2.6.13-rc3] Mod15Write quirk against v2.6.13

[Bartlomiej Zolnierkiewicz]

On 8/21/05, Tejun Heo <htejun@gmail.com> wrote:
> 
>   Hello, Jeff.
> 
> Jeff Garzik wrote:
> > Tejun Heo wrote:
> >
> >> Jeff Garzik wrote:
> >>
> >>> Tejun Heo wrote:
> >>>
> >>>> Also, Jeff, I know you're very busy, but what do you think about
> >>>> taking m15w workaround into ata tree?  It's been around for a while
> >>>> now and I haven't received any complaints (except for this one) yet.
> >>>> The workaround is ugly but it surely helps and I'm willing to maintain
> >>>> it.
> >
> >
> >>> I think your mod15write solution is too messy to deal with long-term.
> >>> Maintenance burden is much lower on us without it.  It's not too
> >>> difficult to simply avoid certain combinations of hardware.
> >
> >
> >>  No problem, I'll maintain it out of tree.
> >
> >
> > You're welcome to maintain it as an independent branch of
> > libata-dev.git, if you would prefer.
> >
> 
>   I think I'll just keep it out of tree.  I don't think many libata
> developers would be interested in m15w workaround.
> 
> >
> >>> Note!  As Carlos @ Silicon Image points out, the blacklist should
> >>> only apply to SiI 3112, not 3512/3114/etc.  That's an open FIXME that
> >>> would benefit users.

Weird, Windows drivers (at least *.INF files) seem to apply workarounds
to both 3112 and 3512 while 3114 is OK.

> >
> >
> >>  Cool, I'll submit a patch right away.
> >
> 
>   Can you please tell me which are affected and which are not?

What about fixing 3114 for a start? :-)

>         { 0x1095, 0x3112, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
>         { 0x1095, 0x0240, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
>         { 0x1095, 0x3512, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
>         { 0x1095, 0x3114, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3114 },
>         { 0x1002, 0x436e, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
>         { 0x1002, 0x4379, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
>         { 0x1002, 0x437a, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
> 
>   Thanks.

Bartlomiej

[PATCH libata:upstream] sil: apply M15W quirk selectively

[Tejun Heo]

 Hello, Jeff & Bartlomiej & all.

> What about fixing 3114 for a start? :-)

 Yeap, why not?  Here it is.
--

 This patch adds chipset mask to sil_blacklist such that quirks can be
applied selectively depending on chipset.  And add sil_3112_m15w
chipset which is identical to sil_3112 except that mod15write quirk is
applied.  As we don't know which PCI IDs are affected yet, I've
changed all 3112's to sil_3112_m15w.  Later when we know which PCI IDs
are affected, we can just change them to sil_3112 later.


Signed-off-by: Tejun Heo <htejun@gmail.com>


diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -41,8 +41,10 @@
 #define DRV_VERSION	"0.9"
 
 enum {
-	sil_3112		= 0,
-	sil_3114		= 1,
+	sil_3112_m15w		= (1 << 0),
+	sil_3112		= (1 << 1),
+	sil_3114		= (1 << 2),
+	sil_all			= 0xffffffff,
 
 	SIL_FIFO_R0		= 0x40,
 	SIL_FIFO_W0		= 0x41,
@@ -76,38 +78,39 @@ static void sil_scr_write (struct ata_po
 static void sil_post_set_mode (struct ata_port *ap);
 
 static struct pci_device_id sil_pci_tbl[] = {
-	{ 0x1095, 0x3112, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
-	{ 0x1095, 0x0240, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
-	{ 0x1095, 0x3512, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
+	{ 0x1095, 0x3112, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112_m15w },
+	{ 0x1095, 0x0240, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112_m15w },
+	{ 0x1095, 0x3512, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112_m15w },
 	{ 0x1095, 0x3114, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3114 },
-	{ 0x1002, 0x436e, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
-	{ 0x1002, 0x4379, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
-	{ 0x1002, 0x437a, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
+	{ 0x1002, 0x436e, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112_m15w },
+	{ 0x1002, 0x4379, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112_m15w },
+	{ 0x1002, 0x437a, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112_m15w },
 	{ }	/* terminate list */
 };
 
 
 /* TODO firmware versions should be added - eric */
-static const struct sil_drivelist {
-	const char * product;
+static const struct sil_blacklist {
+	int chipmask;
+	const char * drive;
 	unsigned int quirk;
 } sil_blacklist [] = {
-	{ "ST320012AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST330013AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST340017AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST360015AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST380013AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST380023AS",		SIL_QUIRK_MOD15WRITE },
-	{ "ST3120023AS",	SIL_QUIRK_MOD15WRITE },
-	{ "ST3160023AS",	SIL_QUIRK_MOD15WRITE },
-	{ "ST3120026AS",	SIL_QUIRK_MOD15WRITE },
-	{ "ST3200822AS",	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 },
+	{ sil_3112_m15w, "ST320012AS",		SIL_QUIRK_MOD15WRITE },
+	{ sil_3112_m15w, "ST330013AS",		SIL_QUIRK_MOD15WRITE },
+	{ sil_3112_m15w, "ST340017AS",		SIL_QUIRK_MOD15WRITE },
+	{ sil_3112_m15w, "ST360015AS",		SIL_QUIRK_MOD15WRITE },
+	{ sil_3112_m15w, "ST380013AS",		SIL_QUIRK_MOD15WRITE },
+	{ sil_3112_m15w, "ST380023AS",		SIL_QUIRK_MOD15WRITE },
+	{ sil_3112_m15w, "ST3120023AS",		SIL_QUIRK_MOD15WRITE },
+	{ sil_3112_m15w, "ST3160023AS",		SIL_QUIRK_MOD15WRITE },
+	{ sil_3112_m15w, "ST3120026AS",		SIL_QUIRK_MOD15WRITE },
+	{ sil_3112_m15w, "ST3200822AS",		SIL_QUIRK_MOD15WRITE },
+	{ sil_3112_m15w, "ST340014ASL",		SIL_QUIRK_MOD15WRITE },
+	{ sil_3112_m15w, "ST360014ASL",		SIL_QUIRK_MOD15WRITE },
+	{ sil_3112_m15w, "ST380011ASL",		SIL_QUIRK_MOD15WRITE },
+	{ sil_3112_m15w, "ST3120022ASL",	SIL_QUIRK_MOD15WRITE },
+	{ sil_3112_m15w, "ST3160021ASL",	SIL_QUIRK_MOD15WRITE },
+	{ sil_all,	 "Maxtor 4D060H3",	SIL_QUIRK_UDMA5MAX },
 	{ }
 };
 
@@ -165,7 +168,16 @@ static struct ata_port_operations sil_op
 };
 
 static struct ata_port_info sil_port_info[] = {
-	/* sil_3112 */
+	/* sil_3112_m15w, keep it sync'd w/ sil_3112 */
+	{
+		.sht		= &sil_sht,
+		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+				  ATA_FLAG_SRST | ATA_FLAG_MMIO,
+		.pio_mask	= 0x1f,			/* pio0-4 */
+		.mwdma_mask	= 0x07,			/* mwdma0-2 */
+		.udma_mask	= 0x3f,			/* udma0-5 */
+		.port_ops	= &sil_ops,
+	}, /* sil_3112 */
 	{
 		.sht		= &sil_sht,
 		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
@@ -309,6 +321,7 @@ static void sil_scr_write (struct ata_po
  */
 static void sil_dev_config(struct ata_port *ap, struct ata_device *dev)
 {
+	int chip = (int)ap->host_set->private_data;
 	unsigned int n, quirks = 0;
 	unsigned char model_num[40];
 	const char *s;
@@ -323,9 +336,10 @@ static void sil_dev_config(struct ata_po
 	while ((len > 0) && (s[len - 1] == ' '))
 		len--;
 
-	for (n = 0; sil_blacklist[n].product; n++)
-		if (!memcmp(sil_blacklist[n].product, s,
-			    strlen(sil_blacklist[n].product))) {
+	for (n = 0; sil_blacklist[n].drive; n++)
+		if ((sil_blacklist[n].chipmask & chip) &&
+		    !memcmp(sil_blacklist[n].drive, s,
+			    strlen(sil_blacklist[n].drive))) {
 			quirks = sil_blacklist[n].quirk;
 			break;
 		}
@@ -403,6 +417,7 @@ static int sil_init_one (struct pci_dev 
        	probe_ent->irq = pdev->irq;
        	probe_ent->irq_flags = SA_SHIRQ;
 	probe_ent->host_flags = sil_port_info[ent->driver_data].host_flags;
+	probe_ent->private_data = (void *)ent->driver_data;
 
 	mmio_base = ioremap(pci_resource_start(pdev, 5),
 		            pci_resource_len(pdev, 5));

Re: [PATCH libata:upstream] sil: apply M15W quirk selectively

[Jeff Garzik]

Tejun Heo wrote:
>  Hello, Jeff & Bartlomiej & all.
> 
> 
>>What about fixing 3114 for a start? :-)
> 
> 
>  Yeap, why not?  Here it is.
> --
> 
>  This patch adds chipset mask to sil_blacklist such that quirks can be
> applied selectively depending on chipset.  And add sil_3112_m15w
> chipset which is identical to sil_3112 except that mod15write quirk is
> applied.  As we don't know which PCI IDs are affected yet, I've
> changed all 3112's to sil_3112_m15w.  Later when we know which PCI IDs
> are affected, we can just change them to sil_3112 later.
> 
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

Thanks for tackling this.  I would approach it in a different manner:

1) add to the enum at the top,
+	SIL_FLAG_M15W		= (1 << 30),

2) do
         sil_3112                = 0,
-       sil_3114                = 1,
+	sil_3112_m15w		= 1,
+	sil_3114		= 2,


3) in sil_pci_tbl[], change all the sil_3112 entries to sil_3112_m15w, 
EXCEPT for PCI ID 0x1095, 0x3512, which should remain sil_3112.



4) in sil_port_info[], duplicate sil_3112 entry to new entry 
sil_3112_m15w.  make sure order in sil_port_info[] array matches the 
values in step #2.


5) add SIL_FLAG_M15W to the list of host_flags in the new sil_3112_m15w 
entry created in step #4.


6) in sil_dev_config(), change
        if (quirks & SIL_QUIRK_MOD15WRITE) {
test to also test (ap->flags & SIL_FLAG_M15W)


Check over these steps to see if I missing anything...  at the very 
least, I have illustrated the approach that should be taken.

	Jeff

Re: [PATCH libata:upstream] sil: apply M15W quirk selectively

[Tejun Heo]

 Hi, Jeff.

 My posted patch is broken anyway.  I was indexing sil_port_info w/
bit masks.  :-(

On Mon, Aug 22, 2005 at 03:36:57PM -0400, Jeff Garzik wrote:
> Tejun Heo wrote:
> > Hello, Jeff & Bartlomiej & all.
> >
> >
> >>What about fixing 3114 for a start? :-)
> >
> >
> > Yeap, why not?  Here it is.
> >--
> >
> > This patch adds chipset mask to sil_blacklist such that quirks can be
> >applied selectively depending on chipset.  And add sil_3112_m15w
> >chipset which is identical to sil_3112 except that mod15write quirk is
> >applied.  As we don't know which PCI IDs are affected yet, I've
> >changed all 3112's to sil_3112_m15w.  Later when we know which PCI IDs
> >are affected, we can just change them to sil_3112 later.
> >
> >
> >Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> Thanks for tackling this.  I would approach it in a different manner:
> 
> 1) add to the enum at the top,
> +	SIL_FLAG_M15W		= (1 << 30),
> 
> 2) do
>         sil_3112                = 0,
> -       sil_3114                = 1,
> +	sil_3112_m15w		= 1,
> +	sil_3114		= 2,
> 
> 
> 3) in sil_pci_tbl[], change all the sil_3112 entries to sil_3112_m15w, 
> EXCEPT for PCI ID 0x1095, 0x3512, which should remain sil_3112.
> 
> 
> 
> 4) in sil_port_info[], duplicate sil_3112 entry to new entry 
> sil_3112_m15w.  make sure order in sil_port_info[] array matches the 
> values in step #2.
> 
> 
> 5) add SIL_FLAG_M15W to the list of host_flags in the new sil_3112_m15w 
> entry created in step #4.

 Can we use host_flags for that?  It seems that it's for ATA_FLAG_*
flags, and we currently don't explicitly reserve any bits from
ATA_FLAG_*.

> 
> 
> 6) in sil_dev_config(), change
>        if (quirks & SIL_QUIRK_MOD15WRITE) {
> test to also test (ap->flags & SIL_FLAG_M15W)
> 
> 
> Check over these steps to see if I missing anything...  at the very 
> least, I have illustrated the approach that should be taken.
> 
> 	Jeff
> 

 Thanks.

-- 
tejun

Re: [PATCH libata:upstream] sil: apply M15W quirk selectively

[Jeff Garzik]

Tejun Heo wrote:
> On Mon, Aug 22, 2005 at 03:36:57PM -0400, Jeff Garzik wrote:
>>Tejun Heo wrote:

>>5) add SIL_FLAG_M15W to the list of host_flags in the new sil_3112_m15w 
>>entry created in step #4.

>  Can we use host_flags for that?  It seems that it's for ATA_FLAG_*
> flags, and we currently don't explicitly reserve any bits from
> ATA_FLAG_*.

The drivers are free to use the upper bits of ap->flags for their own 
purposes.  ata_piix.c is an example of this, with PIIX_FLAG_xxx.

We can worry about bitspace collision when libata general flags and 
driver-specific flags get so numerous its a problem.

	Jeff

Re: [PATCH libata:upstream] sil: apply M15W quirk selectively (take 2)

[Tejun Heo]

 As SII reports that only original 3112's are affected by M15W quirk,
This patch adds SIL_FLAG_MOD15WRITE to selectively apply M15W quirk
depending on chipsets.  As of yet, we don't know exactly which PCI IDs
are for original 3112, so M15W quirk is applied to all except for 3512
and 3124.  Once more info is avaliable, we can change some of these
sil_3112_m15w's to sil_3112.

Signed-off-by: Tejun Heo <htejun@gmail.com>

diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -41,8 +41,11 @@
 #define DRV_VERSION	"0.9"
 
 enum {
+	SIL_FLAG_MOD15WRITE	= (1 << 30),
+
 	sil_3112		= 0,
-	sil_3114		= 1,
+	sil_3112_m15w		= 1,
+	sil_3114		= 2,
 
 	SIL_FIFO_R0		= 0x40,
 	SIL_FIFO_W0		= 0x41,
@@ -76,13 +79,13 @@ static void sil_scr_write (struct ata_po
 static void sil_post_set_mode (struct ata_port *ap);
 
 static struct pci_device_id sil_pci_tbl[] = {
-	{ 0x1095, 0x3112, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
-	{ 0x1095, 0x0240, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
+	{ 0x1095, 0x3112, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112_m15w },
+	{ 0x1095, 0x0240, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112_m15w },
 	{ 0x1095, 0x3512, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
 	{ 0x1095, 0x3114, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3114 },
-	{ 0x1002, 0x436e, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
-	{ 0x1002, 0x4379, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
-	{ 0x1002, 0x437a, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112 },
+	{ 0x1002, 0x436e, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112_m15w },
+	{ 0x1002, 0x4379, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112_m15w },
+	{ 0x1002, 0x437a, PCI_ANY_ID, PCI_ANY_ID, 0, 0, sil_3112_m15w },
 	{ }	/* terminate list */
 };
 
@@ -174,6 +177,16 @@ static struct ata_port_info sil_port_inf
 		.mwdma_mask	= 0x07,			/* mwdma0-2 */
 		.udma_mask	= 0x3f,			/* udma0-5 */
 		.port_ops	= &sil_ops,
+	}, /* sil_3112_15w - keep it sync'd w/ sil_3112 */
+	{
+		.sht		= &sil_sht,
+		.host_flags	= ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
+				  ATA_FLAG_SRST | ATA_FLAG_MMIO |
+				  SIL_FLAG_MOD15WRITE,
+		.pio_mask	= 0x1f,			/* pio0-4 */
+		.mwdma_mask	= 0x07,			/* mwdma0-2 */
+		.udma_mask	= 0x3f,			/* udma0-5 */
+		.port_ops	= &sil_ops,
 	}, /* sil_3114 */
 	{
 		.sht		= &sil_sht,
@@ -331,7 +344,7 @@ static void sil_dev_config(struct ata_po
 		}
 
 	/* limit requests to 15 sectors */
-	if (quirks & SIL_QUIRK_MOD15WRITE) {
+	if (ap->flags & SIL_FLAG_MOD15WRITE && quirks & SIL_QUIRK_MOD15WRITE) {
 		printk(KERN_INFO "ata%u(%u): applying Seagate errata fix\n",
 		       ap->id, dev->devno);
 		ap->host->max_sectors = 15;

Re: [PATCH libata:upstream] sil: apply M15W quirk selectively (take 2)

[Jeff Garzik]

applied