[PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Mikulas Patocka]

Hi

This patch fixes a data corruption when SSD is connected to Ultra 5.

Mikulas

--

Serialize CMD643 and CMD646 to fix a hardware bug with SSD

CMD646 corrupts data on concurrent transfers on both channels when IDE SSD is
connected to one of the channels.

Setup that demonstrates this hardware bug: Ultra 5, onboard CMD646, rev 3.
/dev/hda is 8GB Seagate ST38410A in MWDMA2
/dev/hdd is 32GB SSD SiliconHardDisk in MWDMA2

- When reading /dev/hdd (for example with dd or fsck), reads from /dev/hda
  are corrupted, there are twiddled single bits 1->0 and some full 32-bit
  words corrupted, sometimes commands fail (which switches /dev/hda to
  PIO mode but the corruptions happen even in PIO).
- Reads from /dev/hdd don't seem to be corrupted (i.e. fsck passes fine).
- When I connected normal rotating harddisk to /dev/hdd, there was no
  corruption, so the corruption is something specific to SSD.
- I tried the same setup on a PCI card with CMD649 and saw no corruption.

This patch serializes the operation for CMD646 and 643 (I didn't test
CMD643 but it may have the same hw bug too because it's earlier design).
CMD649 is good. I don't know anything about CMD 648.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/ide/cmd64x.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-2.6.31-fast/drivers/ide/cmd64x.c
===================================================================
--- linux-2.6.31-fast.orig/drivers/ide/cmd64x.c	2009-10-21 06:08:42.000000000 +0200
+++ linux-2.6.31-fast/drivers/ide/cmd64x.c	2009-10-21 06:09:09.000000000 +0200
@@ -379,7 +379,8 @@ static const struct ide_port_info cmd64x
 		.enablebits	= {{0x00,0x00,0x00}, {0x51,0x08,0x08}},
 		.port_ops	= &cmd64x_port_ops,
 		.host_flags	= IDE_HFLAG_CLEAR_SIMPLEX |
-				  IDE_HFLAG_ABUSE_PREFETCH,
+				  IDE_HFLAG_ABUSE_PREFETCH |
+				  IDE_HFLAG_SERIALIZE,
 		.pio_mask	= ATA_PIO5,
 		.mwdma_mask	= ATA_MWDMA2,
 		.udma_mask	= 0x00, /* no udma */
@@ -389,7 +390,8 @@ static const struct ide_port_info cmd64x
 		.init_chipset	= init_chipset_cmd64x,
 		.enablebits	= {{0x51,0x04,0x04}, {0x51,0x08,0x08}},
 		.port_ops	= &cmd648_port_ops,
-		.host_flags	= IDE_HFLAG_ABUSE_PREFETCH,
+		.host_flags	= IDE_HFLAG_ABUSE_PREFETCH |
+				  IDE_HFLAG_SERIALIZE,
 		.pio_mask	= ATA_PIO5,
 		.mwdma_mask	= ATA_MWDMA2,
 		.udma_mask	= ATA_UDMA2,

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Mikael Pettersson]

Mikulas Patocka writes:
 > Hi
 > 
 > This patch fixes a data corruption when SSD is connected to Ultra 5.
 > 
 > Mikulas
 > 
 > --
 > 
 > Serialize CMD643 and CMD646 to fix a hardware bug with SSD
 > 
 > CMD646 corrupts data on concurrent transfers on both channels when IDE SSD is
 > connected to one of the channels.
 > 
 > Setup that demonstrates this hardware bug: Ultra 5, onboard CMD646, rev 3.
 > /dev/hda is 8GB Seagate ST38410A in MWDMA2
 > /dev/hdd is 32GB SSD SiliconHardDisk in MWDMA2
 > 
 > - When reading /dev/hdd (for example with dd or fsck), reads from /dev/hda
 >   are corrupted, there are twiddled single bits 1->0 and some full 32-bit
 >   words corrupted, sometimes commands fail (which switches /dev/hda to
 >   PIO mode but the corruptions happen even in PIO).
 > - Reads from /dev/hdd don't seem to be corrupted (i.e. fsck passes fine).
 > - When I connected normal rotating harddisk to /dev/hdd, there was no
 >   corruption, so the corruption is something specific to SSD.
 > - I tried the same setup on a PCI card with CMD649 and saw no corruption.
 > 
 > This patch serializes the operation for CMD646 and 643 (I didn't test
 > CMD643 but it may have the same hw bug too because it's earlier design).
 > CMD649 is good. I don't know anything about CMD 648.

Have you checked if the libata pata_cmd64x driver also has this
problem?  I know that it works fine on the Ultra 5 with just a
single pata drive, but the pata+ssd combo may not have been tested.

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Mikulas Patocka]

>  > This patch serializes the operation for CMD646 and 643 (I didn't test
>  > CMD643 but it may have the same hw bug too because it's earlier design).
>  > CMD649 is good. I don't know anything about CMD 648.
> 
> Have you checked if the libata pata_cmd64x driver also has this
> problem?  I know that it works fine on the Ultra 5 with just a
> single pata drive, but the pata+ssd combo may not have been tested.

I have tried libata driver now --- I see no data corruption but I've got 
this error under the same operation (both channels reading):

ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
ata1.00: BMDMA stat 0x24
ata1.00: cmd c8/00:10:10:c8:d9/00:00:00:00:00/e0 tag 0 dma 8192 in
         res 50/00:00:1f:c8:d9/00:00:00:00:00/e0 Emask 0x2 (HSM violation)
ata1.00: status: { DRDY }
pata_cmd64x: active 10 recovery 10 setup 3.
pata_cmd64x: active 10 recovery 10 setup 3.
ata1: soft resetting link
pata_cmd64x: active 3 recovery 1 setup 1.
pata_cmd64x: active 3 recovery 1 setup 1.
ata1.00: configured for MWDMA2
ata1: EH complete

(ata1 is the primary channel)
--- I'm wondering, what does it mean? status 0x50 is OK. DMA status 0x24 
is also OK. What was the problem there?

Mikulas

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Alan Cox]

> (ata1 is the primary channel)
> --- I'm wondering, what does it mean? status 0x50 is OK. DMA status 0x24 
> is also OK. What was the problem there?

Beats me still. Thanks to Daniela for the info about the chip contention
I've got some bits that can be tried, but I don't actually have a 646 to
check this.

It should do the neccessary serializing and IRQ bit checks to avoid
hitting the case described in the app note.

cmd64x: implement serialization as per notes

From: Alan Cox <alan@linux.intel.com>

Daniela Engert pointed out that there are some implementation notes for the
643 and 646 that deal with certain serialization rules. In theory we don't
need them because they apply when the motherboard decides not to retry PCI
requests for long enough and the chip is busy doing a DMA transfer on the
other channel.

The rule basically is "don't touch the taskfile of the other channel while
a DMA is in progress". To implement that we need to

- not issue a command on a channel when there is a DMA command queued
- not issue a DMA command on a channel when there are PIO commands queued
- use the alternative access to the interrupt source so that we do not
  touch altstatus or status on shared IRQ.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/ata/pata_cmd64x.c |  132 ++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 124 insertions(+), 8 deletions(-)


diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c
index f98dffe..64917ac 100644
--- a/drivers/ata/pata_cmd64x.c
+++ b/drivers/ata/pata_cmd64x.c
@@ -31,7 +31,7 @@
 #include <linux/libata.h>
 
 #define DRV_NAME "pata_cmd64x"
-#define DRV_VERSION "0.2.5"
+#define DRV_VERSION "0.3.1"
 
 /*
  * CMD64x specific registers definition.
@@ -75,6 +75,13 @@ enum {
 	DTPR1		= 0x7C
 };
 
+struct cmd_priv
+{
+	int dma_live;		/* Channel using DMA */
+	int irq_t[2];		/* Register to check for IRQ */
+	int irq_m[2];		/* Bit to check */
+};
+
 static int cmd648_cable_detect(struct ata_port *ap)
 {
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
@@ -254,17 +261,107 @@ static void cmd648_bmdma_stop(struct ata_queued_cmd *qc)
 }
 
 /**
- *	cmd646r1_dma_stop	-	DMA stop callback
+ *	cmd64x_bmdma_stop	-	DMA stop callback
  *	@qc: Command in progress
  *
- *	Stub for now while investigating the r1 quirk in the old driver.
+ *	Track the completion of live DMA commands and clear the dma_live
+ *	tracking flag as we do.
  */
 
-static void cmd646r1_bmdma_stop(struct ata_queued_cmd *qc)
+static void cmd64x_bmdma_stop(struct ata_queued_cmd *qc)
 {
+	struct ata_port *ap = qc->ap;
+	struct cmd_priv *priv = ap->host->private_data;
 	ata_bmdma_stop(qc);
+	WARN_ON(priv->dma_live != ap->port_no );
+	priv->dma_live = -1;
+}
+
+/**
+ *	cmd64x_qc_defer		-	Defer logic for chip limits
+ *	@qc: queued command
+ *
+ *	Decide whether we can issue the command. Called under the host lock.
+ */
+
+static int cmd64x_qc_defer(struct ata_queued_cmd *qc)
+{
+	struct ata_host *host = qc->ap->host;
+	struct cmd_priv *priv = host->private_data;
+	struct ata_port *alt = host->ports[1 ^ qc->ap->port_no];
+	int rc;
+
+	/* Apply the ATA rules first */
+	rc = ata_std_qc_defer(qc);
+	if (rc)
+		return rc;
+
+	/* If the other port is not live then issue the command */
+	if (alt == NULL || !alt->qc_active)
+		return 0;
+	/* If there is a live DMA command then wait */
+	if (priv->dma_live != -1)
+		return 	ATA_DEFER_PORT;
+	if (qc->tf.protocol == ATAPI_PROT_DMA ||
+				qc->tf.protocol == ATA_PROT_DMA) {
+		/* Cannot overlap our DMA command */
+		if (alt->qc_active)
+			return ATA_DEFER_PORT;
+		/* Claim the DMA */
+		priv->dma_live = qc->ap->port_no;
+	}
+	return 0;
 }
 
+/**
+ *	cmd64x_interrupt - ATA host interrupt handler
+ *	@irq: irq line (unused)
+ *	@dev_instance: pointer to our ata_host information structure
+ *
+ *	Our interrupt handler for PCI IDE devices.  Calls
+ *	ata_sff_host_intr() for each port that is flagging an IRQ. We cannot
+ *	use the defaults as we need to avoid touching status/altstatus during
+ *	a DMA.
+ *
+ *	LOCKING:
+ *	Obtains host lock during operation.
+ *
+ *	RETURNS:
+ *	IRQ_NONE or IRQ_HANDLED.
+ */
+irqreturn_t cmd64x_interrupt(int irq, void *dev_instance)
+{
+	struct ata_host *host = dev_instance;
+	struct pci_dev *pdev = to_pci_dev(host->dev);
+	struct cmd_priv *priv = host->private_data;
+	unsigned int i;
+	unsigned int handled = 0;
+	unsigned long flags;
+
+	/* TODO: make _irqsave conditional on x86 PCI IDE legacy mode */
+	spin_lock_irqsave(&host->lock, flags);
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap;
+		u8 reg;
+
+		pci_read_config_byte(pdev, priv->irq_t[i], &reg);
+		ap = host->ports[i];
+		if (ap && (reg & priv->irq_m[i]) &&
+		    !(ap->flags & ATA_FLAG_DISABLED)) {
+			struct ata_queued_cmd *qc;
+
+			qc = ata_qc_from_tag(ap, ap->link.active_tag);
+			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
+			    (qc->flags & ATA_QCFLAG_ACTIVE))
+				handled |= ata_sff_host_intr(ap, qc);
+		}
+	}
+
+	spin_unlock_irqrestore(&host->lock, flags);
+
+	return IRQ_RETVAL(handled);
+}
 static struct scsi_host_template cmd64x_sht = {
 	ATA_BMDMA_SHT(DRV_NAME),
 };
@@ -273,6 +370,8 @@ static const struct ata_port_operations cmd64x_base_ops = {
 	.inherits	= &ata_bmdma_port_ops,
 	.set_piomode	= cmd64x_set_piomode,
 	.set_dmamode	= cmd64x_set_dmamode,
+	.bmdma_stop	= cmd64x_bmdma_stop,
+	.qc_defer	= cmd64x_qc_defer,
 };
 
 static struct ata_port_operations cmd64x_port_ops = {
@@ -282,7 +381,6 @@ static struct ata_port_operations cmd64x_port_ops = {
 
 static struct ata_port_operations cmd646r1_port_ops = {
 	.inherits	= &cmd64x_base_ops,
-	.bmdma_stop	= cmd646r1_bmdma_stop,
 	.cable_detect	= ata_cable_40wire,
 };
 
@@ -290,6 +388,7 @@ static struct ata_port_operations cmd648_port_ops = {
 	.inherits	= &cmd64x_base_ops,
 	.bmdma_stop	= cmd648_bmdma_stop,
 	.cable_detect	= cmd648_cable_detect,
+	.qc_defer	= ata_std_qc_defer
 };
 
 static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
@@ -340,6 +439,8 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	const struct ata_port_info *ppi[] = { &cmd_info[id->driver_data], NULL };
 	u8 mrdmode;
 	int rc;
+	struct ata_host *host;
+	struct cmd_priv *cpriv;
 
 	rc = pcim_enable_device(pdev);
 	if (rc)
@@ -348,6 +449,17 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	pci_read_config_dword(pdev, PCI_CLASS_REVISION, &class_rev);
 	class_rev &= 0xFF;
 
+	cpriv = devm_kzalloc(&pdev->dev, sizeof(*cpriv), GFP_KERNEL);
+	if (cpriv == NULL)
+		return -ENOMEM;
+	cpriv->dma_live = -1;
+
+	/* Table for IRQ checking */
+	cpriv->irq_t[0] = CFR;
+	cpriv->irq_m[0] = 1 << 2;
+	cpriv->irq_t[1] = ARTTIM23;
+	cpriv->irq_m[1] = 1 << 4;
+
 	if (id->driver_data == 0)	/* 643 */
 		ata_pci_bmdma_clear_simplex(pdev);
 
@@ -360,20 +472,24 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 			ppi[0] = &cmd_info[3];
 	}
 
+
 	pci_write_config_byte(pdev, PCI_LATENCY_TIMER, 64);
 	pci_read_config_byte(pdev, MRDMODE, &mrdmode);
 	mrdmode &= ~ 0x30;	/* IRQ set up */
 	mrdmode |= 0x02;	/* Memory read line enable */
 	pci_write_config_byte(pdev, MRDMODE, mrdmode);
 
-	/* Force PIO 0 here.. */
-
 	/* PPC specific fixup copied from old driver */
 #ifdef CONFIG_PPC
 	pci_write_config_byte(pdev, UDIDETCR0, 0xF0);
 #endif
+	rc = ata_pci_sff_prepare_host(pdev, ppi, &host);
+	if (rc)
+		return rc;
+	host->private_data = cpriv;
 
-	return ata_pci_sff_init_one(pdev, ppi, &cmd64x_sht, NULL);
+	pci_set_master(pdev);
+	return ata_pci_sff_activate_host(host, cmd64x_interrupt, &cmd64x_sht);
 }
 
 #ifdef CONFIG_PM

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Mikulas Patocka]

With every transfer, the pastchcauses this:

------------[ cut here ]------------
WARNING: at drivers/ata/pata_cmd64x.c:276 cmd64x_bmdma_stop+0x48/0x60()
Modules linked in: nbd sunhme openpromfs sermouse unix
Call Trace:
 [00000000005cab68] cmd64x_bmdma_stop+0x48/0x60
 [00000000005c99b8] ata_sff_host_intr+0x158/0x1e0
 [00000000005cb2f8] cmd64x_interrupt+0x178/0x1a0
 [000000000048354c] handle_IRQ_event+0x6c/0x140
 [0000000000485380] handle_fasteoi_irq+0x80/0x140
 [000000000042a660] handler_irq+0xc0/0x100
 [00000000004208b4] tl0_irq5+0x14/0x20
 [00000000005ec08c] skb_copy_datagram_iovec+0x1ec/0x220
 [0000000010000800] unix_dgram_recvmsg+0xc0/0x300 [unix]
 [00000000005e0ca8] sock_recvmsg+0x88/0xc0
 [00000000005e1f50] SyS_recvfrom+0x70/0xe0
 [0000000000406114] linux_sparc_syscall32+0x34/0x40
---[ end trace 62aae040ef69a03d ]---

Mikulas

On Tue, 27 Oct 2009, Alan Cox wrote:

> > (ata1 is the primary channel)
> > --- I'm wondering, what does it mean? status 0x50 is OK. DMA status 0x24 
> > is also OK. What was the problem there?
> 
> Beats me still. Thanks to Daniela for the info about the chip contention
> I've got some bits that can be tried, but I don't actually have a 646 to
> check this.
> 
> It should do the neccessary serializing and IRQ bit checks to avoid
> hitting the case described in the app note.
> 
> cmd64x: implement serialization as per notes
> 
> From: Alan Cox <alan@linux.intel.com>
> 
> Daniela Engert pointed out that there are some implementation notes for the
> 643 and 646 that deal with certain serialization rules. In theory we don't
> need them because they apply when the motherboard decides not to retry PCI
> requests for long enough and the chip is busy doing a DMA transfer on the
> other channel.
> 
> The rule basically is "don't touch the taskfile of the other channel while
> a DMA is in progress". To implement that we need to
> 
> - not issue a command on a channel when there is a DMA command queued
> - not issue a DMA command on a channel when there are PIO commands queued
> - use the alternative access to the interrupt source so that we do not
>   touch altstatus or status on shared IRQ.
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> ---
> 
>  drivers/ata/pata_cmd64x.c |  132 ++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 124 insertions(+), 8 deletions(-)
> 
> 
> diff --git a/drivers/ata/pata_cmd64x.c b/drivers/ata/pata_cmd64x.c
> index f98dffe..64917ac 100644
> --- a/drivers/ata/pata_cmd64x.c
> +++ b/drivers/ata/pata_cmd64x.c
> @@ -31,7 +31,7 @@
>  #include <linux/libata.h>
>  
>  #define DRV_NAME "pata_cmd64x"
> -#define DRV_VERSION "0.2.5"
> +#define DRV_VERSION "0.3.1"
>  
>  /*
>   * CMD64x specific registers definition.
> @@ -75,6 +75,13 @@ enum {
>  	DTPR1		= 0x7C
>  };
>  
> +struct cmd_priv
> +{
> +	int dma_live;		/* Channel using DMA */
> +	int irq_t[2];		/* Register to check for IRQ */
> +	int irq_m[2];		/* Bit to check */
> +};
> +
>  static int cmd648_cable_detect(struct ata_port *ap)
>  {
>  	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
> @@ -254,17 +261,107 @@ static void cmd648_bmdma_stop(struct ata_queued_cmd *qc)
>  }
>  
>  /**
> - *	cmd646r1_dma_stop	-	DMA stop callback
> + *	cmd64x_bmdma_stop	-	DMA stop callback
>   *	@qc: Command in progress
>   *
> - *	Stub for now while investigating the r1 quirk in the old driver.
> + *	Track the completion of live DMA commands and clear the dma_live
> + *	tracking flag as we do.
>   */
>  
> -static void cmd646r1_bmdma_stop(struct ata_queued_cmd *qc)
> +static void cmd64x_bmdma_stop(struct ata_queued_cmd *qc)
>  {
> +	struct ata_port *ap = qc->ap;
> +	struct cmd_priv *priv = ap->host->private_data;
>  	ata_bmdma_stop(qc);
> +	WARN_ON(priv->dma_live != ap->port_no );
> +	priv->dma_live = -1;
> +}
> +
> +/**
> + *	cmd64x_qc_defer		-	Defer logic for chip limits
> + *	@qc: queued command
> + *
> + *	Decide whether we can issue the command. Called under the host lock.
> + */
> +
> +static int cmd64x_qc_defer(struct ata_queued_cmd *qc)
> +{
> +	struct ata_host *host = qc->ap->host;
> +	struct cmd_priv *priv = host->private_data;
> +	struct ata_port *alt = host->ports[1 ^ qc->ap->port_no];
> +	int rc;
> +
> +	/* Apply the ATA rules first */
> +	rc = ata_std_qc_defer(qc);
> +	if (rc)
> +		return rc;
> +
> +	/* If the other port is not live then issue the command */
> +	if (alt == NULL || !alt->qc_active)
> +		return 0;
> +	/* If there is a live DMA command then wait */
> +	if (priv->dma_live != -1)
> +		return 	ATA_DEFER_PORT;
> +	if (qc->tf.protocol == ATAPI_PROT_DMA ||
> +				qc->tf.protocol == ATA_PROT_DMA) {
> +		/* Cannot overlap our DMA command */
> +		if (alt->qc_active)
> +			return ATA_DEFER_PORT;
> +		/* Claim the DMA */
> +		priv->dma_live = qc->ap->port_no;
> +	}
> +	return 0;
>  }
>  
> +/**
> + *	cmd64x_interrupt - ATA host interrupt handler
> + *	@irq: irq line (unused)
> + *	@dev_instance: pointer to our ata_host information structure
> + *
> + *	Our interrupt handler for PCI IDE devices.  Calls
> + *	ata_sff_host_intr() for each port that is flagging an IRQ. We cannot
> + *	use the defaults as we need to avoid touching status/altstatus during
> + *	a DMA.
> + *
> + *	LOCKING:
> + *	Obtains host lock during operation.
> + *
> + *	RETURNS:
> + *	IRQ_NONE or IRQ_HANDLED.
> + */
> +irqreturn_t cmd64x_interrupt(int irq, void *dev_instance)
> +{
> +	struct ata_host *host = dev_instance;
> +	struct pci_dev *pdev = to_pci_dev(host->dev);
> +	struct cmd_priv *priv = host->private_data;
> +	unsigned int i;
> +	unsigned int handled = 0;
> +	unsigned long flags;
> +
> +	/* TODO: make _irqsave conditional on x86 PCI IDE legacy mode */
> +	spin_lock_irqsave(&host->lock, flags);
> +
> +	for (i = 0; i < host->n_ports; i++) {
> +		struct ata_port *ap;
> +		u8 reg;
> +
> +		pci_read_config_byte(pdev, priv->irq_t[i], &reg);
> +		ap = host->ports[i];
> +		if (ap && (reg & priv->irq_m[i]) &&
> +		    !(ap->flags & ATA_FLAG_DISABLED)) {
> +			struct ata_queued_cmd *qc;
> +
> +			qc = ata_qc_from_tag(ap, ap->link.active_tag);
> +			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
> +			    (qc->flags & ATA_QCFLAG_ACTIVE))
> +				handled |= ata_sff_host_intr(ap, qc);
> +		}
> +	}
> +
> +	spin_unlock_irqrestore(&host->lock, flags);
> +
> +	return IRQ_RETVAL(handled);
> +}
>  static struct scsi_host_template cmd64x_sht = {
>  	ATA_BMDMA_SHT(DRV_NAME),
>  };
> @@ -273,6 +370,8 @@ static const struct ata_port_operations cmd64x_base_ops = {
>  	.inherits	= &ata_bmdma_port_ops,
>  	.set_piomode	= cmd64x_set_piomode,
>  	.set_dmamode	= cmd64x_set_dmamode,
> +	.bmdma_stop	= cmd64x_bmdma_stop,
> +	.qc_defer	= cmd64x_qc_defer,
>  };
>  
>  static struct ata_port_operations cmd64x_port_ops = {
> @@ -282,7 +381,6 @@ static struct ata_port_operations cmd64x_port_ops = {
>  
>  static struct ata_port_operations cmd646r1_port_ops = {
>  	.inherits	= &cmd64x_base_ops,
> -	.bmdma_stop	= cmd646r1_bmdma_stop,
>  	.cable_detect	= ata_cable_40wire,
>  };
>  
> @@ -290,6 +388,7 @@ static struct ata_port_operations cmd648_port_ops = {
>  	.inherits	= &cmd64x_base_ops,
>  	.bmdma_stop	= cmd648_bmdma_stop,
>  	.cable_detect	= cmd648_cable_detect,
> +	.qc_defer	= ata_std_qc_defer
>  };
>  
>  static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
> @@ -340,6 +439,8 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
>  	const struct ata_port_info *ppi[] = { &cmd_info[id->driver_data], NULL };
>  	u8 mrdmode;
>  	int rc;
> +	struct ata_host *host;
> +	struct cmd_priv *cpriv;
>  
>  	rc = pcim_enable_device(pdev);
>  	if (rc)
> @@ -348,6 +449,17 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
>  	pci_read_config_dword(pdev, PCI_CLASS_REVISION, &class_rev);
>  	class_rev &= 0xFF;
>  
> +	cpriv = devm_kzalloc(&pdev->dev, sizeof(*cpriv), GFP_KERNEL);
> +	if (cpriv == NULL)
> +		return -ENOMEM;
> +	cpriv->dma_live = -1;
> +
> +	/* Table for IRQ checking */
> +	cpriv->irq_t[0] = CFR;
> +	cpriv->irq_m[0] = 1 << 2;
> +	cpriv->irq_t[1] = ARTTIM23;
> +	cpriv->irq_m[1] = 1 << 4;
> +
>  	if (id->driver_data == 0)	/* 643 */
>  		ata_pci_bmdma_clear_simplex(pdev);
>  
> @@ -360,20 +472,24 @@ static int cmd64x_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
>  			ppi[0] = &cmd_info[3];
>  	}
>  
> +
>  	pci_write_config_byte(pdev, PCI_LATENCY_TIMER, 64);
>  	pci_read_config_byte(pdev, MRDMODE, &mrdmode);
>  	mrdmode &= ~ 0x30;	/* IRQ set up */
>  	mrdmode |= 0x02;	/* Memory read line enable */
>  	pci_write_config_byte(pdev, MRDMODE, mrdmode);
>  
> -	/* Force PIO 0 here.. */
> -
>  	/* PPC specific fixup copied from old driver */
>  #ifdef CONFIG_PPC
>  	pci_write_config_byte(pdev, UDIDETCR0, 0xF0);
>  #endif
> +	rc = ata_pci_sff_prepare_host(pdev, ppi, &host);
> +	if (rc)
> +		return rc;
> +	host->private_data = cpriv;
>  
> -	return ata_pci_sff_init_one(pdev, ppi, &cmd64x_sht, NULL);
> +	pci_set_master(pdev);
> +	return ata_pci_sff_activate_host(host, cmd64x_interrupt, &cmd64x_sht);
>  }
>  
>  #ifdef CONFIG_PM
> 

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Bartlomiej Zolnierkiewicz]

On Wednesday 21 October 2009 20:55:28 Mikulas Patocka wrote:
> Hi
> 
> This patch fixes a data corruption when SSD is connected to Ultra 5.
> 
> Mikulas
> 
> --
> 
> Serialize CMD643 and CMD646 to fix a hardware bug with SSD
> 
> CMD646 corrupts data on concurrent transfers on both channels when IDE SSD is
> connected to one of the channels.
> 
> Setup that demonstrates this hardware bug: Ultra 5, onboard CMD646, rev 3.
> /dev/hda is 8GB Seagate ST38410A in MWDMA2
> /dev/hdd is 32GB SSD SiliconHardDisk in MWDMA2
> 
> - When reading /dev/hdd (for example with dd or fsck), reads from /dev/hda
>   are corrupted, there are twiddled single bits 1->0 and some full 32-bit
>   words corrupted, sometimes commands fail (which switches /dev/hda to
>   PIO mode but the corruptions happen even in PIO).
> - Reads from /dev/hdd don't seem to be corrupted (i.e. fsck passes fine).
> - When I connected normal rotating harddisk to /dev/hdd, there was no
>   corruption, so the corruption is something specific to SSD.
> - I tried the same setup on a PCI card with CMD649 and saw no corruption.
> 
> This patch serializes the operation for CMD646 and 643 (I didn't test
> CMD643 but it may have the same hw bug too because it's earlier design).
> CMD649 is good. I don't know anything about CMD 648.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

Seems like SSD (simply by being faster) triggers some race condition that
hardware has tolerated in the past and since we used to always serialize
operation for CMD646 before:

commit e01698aed04811b9a9c4f8d54b73cb182757063d           
Author: David S. Miller <davem@davemloft.net>             
Date:   Sun Jun 21 22:48:03 2009 -0700

    ide cmd64x: Remove serialize setting.

it went undetected until now..

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Wed, 21 Oct 2009 21:39:24 +0200

> Seems like SSD (simply by being faster) triggers some race condition that
> hardware has tolerated in the past and since we used to always serialize
> operation for CMD646 before:

Yes, and technically we only did the synchronization for one
of the two chips mpatocka is adding the serialize setting to.

> commit e01698aed04811b9a9c4f8d54b73cb182757063d           
> Author: David S. Miller <davem@davemloft.net>             
> Date:   Sun Jun 21 22:48:03 2009 -0700
> 
>     ide cmd64x: Remove serialize setting.
> 
> it went undetected until now..

Right, and see also:

commit 6b5cde3629701258004b94cde75dd1089b556b02
Author: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date:   Mon Dec 29 20:27:32 2008 +0100

    cmd64x: set IDE_HFLAG_SERIALIZE explictly for CMD646

Which is how we got there.

The most conservative thing to do is to set the flag as
is done by mpatocka's patch but I'd like Frans to regression
test that on his ultra5.

Frans can you do that?

Thanks.

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Bartlomiej Zolnierkiewicz]

On Thursday 22 October 2009 02:41:55 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Wed, 21 Oct 2009 21:39:24 +0200
> 
> > Seems like SSD (simply by being faster) triggers some race condition that
> > hardware has tolerated in the past and since we used to always serialize
> > operation for CMD646 before:
> 
> Yes, and technically we only did the synchronization for one
> of the two chips mpatocka is adding the serialize setting to.
> 
> > commit e01698aed04811b9a9c4f8d54b73cb182757063d           
> > Author: David S. Miller <davem@davemloft.net>             
> > Date:   Sun Jun 21 22:48:03 2009 -0700
> > 
> >     ide cmd64x: Remove serialize setting.
> > 
> > it went undetected until now..
> 
> Right, and see also:
> 
> commit 6b5cde3629701258004b94cde75dd1089b556b02
> Author: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date:   Mon Dec 29 20:27:32 2008 +0100
> 
>     cmd64x: set IDE_HFLAG_SERIALIZE explictly for CMD646
> 
> Which is how we got there.

We are through this the second time and you're still not willing
neither to listen nor to read the code.  We always did serialization
for CMD646, we just used  hwif->chipset == ide_cmd646 (without using
IDE_HFLAG_SERIALIZE flag):

1061                 if (h && h->hwgroup) {  /* scan only initialized ports */
1062                         if (hwif->irq == h->irq) {
1063                                 hwif->sharing_irq = h->sharing_irq = 1;
1064                                 if (hwif->chipset != ide_pci ||
1065                                     h->chipset != ide_pci) {
1066                                         save_match(hwif, h, &match);
1067                                 }
1068                         }

so the code was using the same serialized hwgroup for CMD646 (which always
uses shared PCI IRQ AFAIK) because of hwif->chipset == ide_cmd646.  My patch
only made this explicit in preparation for other changes (one of such other
changes resulted later in uncovering unexpected IRQ problem on Ultra 5).

> The most conservative thing to do is to set the flag as
> is done by mpatocka's patch but I'd like Frans to regression
> test that on his ultra5.

Agreed, though I wonder whether we should also provide module parameter to
disable serializing on those chipsets for people not using SSDs...

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Thu, 22 Oct 2009 11:44:04 +0200

> We are through this the second time and you're still not willing
> neither to listen nor to read the code.

I can understand why you might attribute malice and ignorance to my
actions by default, but it doesn't apply here.

> We always did serialization for CMD646, we just used hwif->chipset
> == ide_cmd646 (without using IDE_HFLAG_SERIALIZE flag):

I fully acknowledge that we always serialized, sorry for not
being explicit.

I was just showing where the serialize IRQ flag got added
(and again, it was a correct change).

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Bartlomiej Zolnierkiewicz]

On Thursday 22 October 2009 13:00:49 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Thu, 22 Oct 2009 11:44:04 +0200
> 
> > We always did serialization for CMD646, we just used hwif->chipset
> > == ide_cmd646 (without using IDE_HFLAG_SERIALIZE flag):
> 
> I fully acknowledge that we always serialized, sorry for not
> being explicit.
> 
> I was just showing where the serialize IRQ flag got added
> (and again, it was a correct change).

What for?  The only patch changing behavior was yours and it seems
your luck is much worse than mine when it comes to unexpected side
effects.. ;)

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Thu, 22 Oct 2009 13:15:59 +0200

> On Thursday 22 October 2009 13:00:49 David Miller wrote:
>> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>> Date: Thu, 22 Oct 2009 11:44:04 +0200
>> 
>> > We always did serialization for CMD646, we just used hwif->chipset
>> > == ide_cmd646 (without using IDE_HFLAG_SERIALIZE flag):
>> 
>> I fully acknowledge that we always serialized, sorry for not
>> being explicit.
>> 
>> I was just showing where the serialize IRQ flag got added
>> (and again, it was a correct change).
> 
> What for?  The only patch changing behavior was yours and it seems
> your luck is much worse than mine when it comes to unexpected side
> effects.. ;)

Like I said, you can attribute my actions to malice or ignorance
if you want, but it simply isn't there.

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Mikulas Patocka]

> We are through this the second time and you're still not willing
> neither to listen nor to read the code.  We always did serialization
> for CMD646, we just used  hwif->chipset == ide_cmd646 (without using
> IDE_HFLAG_SERIALIZE flag):

> Agreed, though I wonder whether we should also provide module parameter to
> disable serializing on those chipsets for people not using SSDs...

Don't do it --- disks have cache and reading from the cache is as fast as 
reading from SSD (or even faster), so if there is some speed-race in the 
chip, there is a possibility that the data corruption happens with disks 
too --- just with lower probability.

If we don't know why the chip corrupts data, we must treat it as always 
corrupting data.

Mikulas

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[David Miller]

From: Mikulas Patocka <mpatocka@redhat.com>
Date: Fri, 23 Oct 2009 10:29:16 -0400 (EDT)

> If we don't know why the chip corrupts data, we must treat it as always 
> corrupting data.

Completely agreed.

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Bartlomiej Zolnierkiewicz]

On Friday 23 October 2009 16:29:16 Mikulas Patocka wrote:
> > We are through this the second time and you're still not willing
> > neither to listen nor to read the code.  We always did serialization
> > for CMD646, we just used  hwif->chipset == ide_cmd646 (without using
> > IDE_HFLAG_SERIALIZE flag):
> 
> > Agreed, though I wonder whether we should also provide module parameter to
> > disable serializing on those chipsets for people not using SSDs...
> 
> Don't do it --- disks have cache and reading from the cache is as fast as 
> reading from SSD (or even faster), so if there is some speed-race in the 
> chip, there is a possibility that the data corruption happens with disks 
> too --- just with lower probability.
> 
> If we don't know why the chip corrupts data, we must treat it as always 
> corrupting data.

I actually suspect that it is device/chipset specific interaction and not
generic problem so adding a fallback option (w/ BIG FAT WARNING) seem to
make sense..  Especially since we have never serialized on CMD643 and your
patch will be adding performance regression without even verifying that
the issue also affects this chipset..

-- 
Bartlomiej Zolnierkiewicz

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Mikulas Patocka]

On Fri, 23 Oct 2009, Bartlomiej Zolnierkiewicz wrote:

> On Friday 23 October 2009 16:29:16 Mikulas Patocka wrote:
> > > We are through this the second time and you're still not willing
> > > neither to listen nor to read the code.  We always did serialization
> > > for CMD646, we just used  hwif->chipset == ide_cmd646 (without using
> > > IDE_HFLAG_SERIALIZE flag):
> > 
> > > Agreed, though I wonder whether we should also provide module parameter to
> > > disable serializing on those chipsets for people not using SSDs...
> > 
> > Don't do it --- disks have cache and reading from the cache is as fast as 
> > reading from SSD (or even faster), so if there is some speed-race in the 
> > chip, there is a possibility that the data corruption happens with disks 
> > too --- just with lower probability.
> > 
> > If we don't know why the chip corrupts data, we must treat it as always 
> > corrupting data.
> 
> I actually suspect that it is device/chipset specific interaction and not
> generic problem so adding a fallback option (w/ BIG FAT WARNING) seem to
> make sense..

So, why was it serialized before? I assume that either someone noticed the 
corruption or someone read some datasheet noting the corruption or someone 
reverse engineered some other driver and saw the serialization.

> Especially since we have never serialized on CMD643 and your patch will 
> be adding performance regression without even verifying that the issue 
> also affects this chipset..

Do you have this chip? If you were IDE maintainer, did you collect cards 
with IDE chips?

Mikulas

> -- 
> Bartlomiej Zolnierkiewicz
> 

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Bartlomiej Zolnierkiewicz]

On Friday 23 October 2009 16:55:59 Mikulas Patocka wrote:
> 
> On Fri, 23 Oct 2009, Bartlomiej Zolnierkiewicz wrote:
> 
> > On Friday 23 October 2009 16:29:16 Mikulas Patocka wrote:
> > > > We are through this the second time and you're still not willing
> > > > neither to listen nor to read the code.  We always did serialization
> > > > for CMD646, we just used  hwif->chipset == ide_cmd646 (without using
> > > > IDE_HFLAG_SERIALIZE flag):
> > > 
> > > > Agreed, though I wonder whether we should also provide module parameter to
> > > > disable serializing on those chipsets for people not using SSDs...
> > > 
> > > Don't do it --- disks have cache and reading from the cache is as fast as 
> > > reading from SSD (or even faster), so if there is some speed-race in the 
> > > chip, there is a possibility that the data corruption happens with disks 
> > > too --- just with lower probability.
> > > 
> > > If we don't know why the chip corrupts data, we must treat it as always 
> > > corrupting data.
> > 
> > I actually suspect that it is device/chipset specific interaction and not
> > generic problem so adding a fallback option (w/ BIG FAT WARNING) seem to
> > make sense..
> 
> So, why was it serialized before? I assume that either someone noticed the 
> corruption or someone read some datasheet noting the corruption or someone 
> reverse engineered some other driver and saw the serialization.

Serialization is usually needed in case of chipset not handling concurrent
data transfers on both ports.  Unfortunately I don't know details for CMD646.

> > Especially since we have never serialized on CMD643 and your patch will 
> > be adding performance regression without even verifying that the issue 
> > also affects this chipset..
> 
> Do you have this chip? If you were IDE maintainer, did you collect cards 
> with IDE chips?

I recall having CMD648 and CMD649 cards somewhere, however not earlier
chipsets.

-- 
Bartlomiej Zolnierkiewicz

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Daniela Engert]

Bartlomiej Zolnierkiewicz schrieb:
> On Friday 23 October 2009 16:55:59 Mikulas Patocka wrote:
>    
>> On Fri, 23 Oct 2009, Bartlomiej Zolnierkiewicz wrote:
>>
>>      
>>> On Friday 23 October 2009 16:29:16 Mikulas Patocka wrote:
>>>        
>>>>> We are through this the second time and you're still not willing
>>>>> neither to listen nor to read the code.  We always did serialization
>>>>> for CMD646, we just used  hwif->chipset == ide_cmd646 (without using
>>>>> IDE_HFLAG_SERIALIZE flag):
>>>>>            
>>>>          
>>>>> Agreed, though I wonder whether we should also provide module parameter to
>>>>> disable serializing on those chipsets for people not using SSDs...
>>>>>            
>>>> Don't do it --- disks have cache and reading from the cache is as fast as
>>>> reading from SSD (or even faster), so if there is some speed-race in the
>>>> chip, there is a possibility that the data corruption happens with disks
>>>> too --- just with lower probability.
>>>>
>>>> If we don't know why the chip corrupts data, we must treat it as always
>>>> corrupting data.
>>>>          
>>> I actually suspect that it is device/chipset specific interaction and not
>>> generic problem so adding a fallback option (w/ BIG FAT WARNING) seem to
>>> make sense..
>>>        
>> So, why was it serialized before? I assume that either someone noticed the
>> corruption or someone read some datasheet noting the corruption or someone
>> reverse engineered some other driver and saw the serialization.
>>      
> Serialization is usually needed in case of chipset not handling concurrent
> data transfers on both ports.  Unfortunately I don't know details for CMD646.
>    
Guys, this is old news. CMD643 and CMD646 have a shared data path from 
the PCI interface to the ATA channel ports. In a document issued by CMD, 
they explain the requirement for serialization. This issue is rectified 
with CMD648 and later chips.
>    
>>> Especially since we have never serialized on CMD643 and your patch will
>>> be adding performance regression without even verifying that the issue
>>> also affects this chipset..
>>>        
>> Do you have this chip? If you were IDE maintainer, did you collect cards
>> with IDE chips?
>>      
> I recall having CMD648 and CMD649 cards somewhere, however not earlier
> chipsets.
>
>    

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Alan Cox]

> So, why was it serialized before? I assume that either someone noticed the 
> corruption or someone read some datasheet noting the corruption or someone 
> reverse engineered some other driver and saw the serialization.

The data sheet has some things to say for the 643 abd 646

In PIO mode it says you must check

CFR bit 2   (indicates primary interrupt)
ARTIM23 bit 4  (secondary interrupt)

Both bits can be written 1 to clear the interrupt. The doc doesn't say
anything about not touching status but it also uses the word "must" about
the other bits so make what you will of it.

In DMA mode I would read the BMDMA status in preference to status but the
doc doesn't specifically say to do so and simply say it works to the
spec. it isn't clear if ARTIM23 and CFR work for reporting/clearing a DMA
interrupt. You'd have to try it.

ARTIM23 may well need some locking care


The 646U2 adds the MRDMODE register so you can check the bits more sanely

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

>>So, why was it serialized before? I assume that either someone noticed the 
>>corruption or someone read some datasheet noting the corruption or someone 
>>reverse engineered some other driver and saw the serialization.

> The data sheet has some things to say for the 643 abd 646

> In PIO mode it says you must check

> CFR bit 2   (indicates primary interrupt)
> ARTIM23 bit 4  (secondary interrupt)

> Both bits can be written 1 to clear the interrupt.

    Not in the original PCI0646 datasheet -- it says that reading the 
register clears the interrupt bit. PCI0646U datashett however started 
talking about writing one to clear the bit.

> The doc doesn't say
> anything about not touching status but it also uses the word "must" about
> the other bits so make what you will of it.

    I think they use must in the sense that if the driver *really* needs to 
know from which channel the interrupt has come. Since most of the chip don't 
provide that capability, the real world Linux drivers, both old and new, had 
to get along without such knowledge. The IDE driver does read and clear the 
interrupt bits now though, in both PIO abnd DMA modes.

> In DMA mode I would read the BMDMA status in preference to status but the
> doc doesn't specifically say to do so and simply say it works to the
> spec. it isn't clear if ARTIM23 and CFR work for reporting/clearing a DMA
> interrupt. You'd have to try it.

    Earlier the IDE driver did chec CFR/ARTTIM23 bits before the BMIDE 
status interrupt bit in dma_test_irq() method and that used to work.  Now it 
also does so, via the new test_irq() method.

> ARTIM23 may well need some locking care

    Locking WRT what? It's not shared between channels...

> The 646U2 adds the MRDMODE register so you can check the bits more sanely

    Yeah. The libata driver however doesn't make use of that register, 
unlike  cmd64x. It doesn't even touch the interrupt bits on PCI0646, only 
manipulation the "old" bits on PCI-64[89].
    It seems that I need to sync up these two, pata_cmd64x.c seems to be 
lagging behind the changes in cmd64x.c. Well, not only this one, at least 
pata_hpt* drivers are lagging behind too...

WBR, Sergei

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Alan Cox]

>     Yeah. The libata driver however doesn't make use of that register, 
> unlike  cmd64x. It doesn't even touch the interrupt bits on PCI0646, only 
> manipulation the "old" bits on PCI-64[89].

Yes I purposefully left out all the complexity because when I tested the
cards I had it simply wasn't needed. I'm also not sure we should merge
anything from the old to the new one until we know why the old one
corrupts and the new one merely gets upset. Accidentally porting over a
corruptor would be very bad indeed.

>     It seems that I need to sync up these two, pata_cmd64x.c seems to be 
> lagging behind the changes in cmd64x.c. Well, not only this one, at least 
> pata_hpt* drivers are lagging behind too...

That would be useful - although several of the differences are because
the pata_hpt driver took from the vendor supplied reference code not the
old IDE code. It also seems more stable for having done that.

Alan

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Bartlomiej Zolnierkiewicz]

On Friday 23 October 2009 20:22:50 Alan Cox wrote:
> >     Yeah. The libata driver however doesn't make use of that register, 
> > unlike  cmd64x. It doesn't even touch the interrupt bits on PCI0646, only 
> > manipulation the "old" bits on PCI-64[89].
> 
> Yes I purposefully left out all the complexity because when I tested the
> cards I had it simply wasn't needed. I'm also not sure we should merge
> anything from the old to the new one until we know why the old one
> corrupts and the new one merely gets upset. Accidentally porting over a
> corruptor would be very bad indeed.

Oh, we know that.

"Old" one corrupts because somebody thought it would be a smart move to remove
serialized flag instead of debugging certain problem properly..

"New" one gets serialized at the command issue level (like all other new shiny
libata PATA stuff) and this may as well explain the difference..

> >     It seems that I need to sync up these two, pata_cmd64x.c seems to be 
> > lagging behind the changes in cmd64x.c. Well, not only this one, at least 
> > pata_hpt* drivers are lagging behind too...

Just from the memory:

pata_sis, pata_atiixp, pata_it8213, pata_cs5536, pata_amd..

So, gentlemen, when are you planning to remove IDE (not from your playground
Linux distribution but from kernel.org)?

I've heard that it will happen "soon" 4 years ago... :)

> That would be useful - although several of the differences are because
> the pata_hpt driver took from the vendor supplied reference code not the
> old IDE code. It also seems more stable for having done that.

Yeah, right..

This reminds me of that hpt366 blacklist that somebody forgot to port over
initially and which resulted in real world data corruptions..

-- 
Bartlomiej Zolnierkiewicz

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Fri, 23 Oct 2009 20:52:47 +0200

> "Old" one corrupts because somebody thought it would be a smart move
> to remove serialized flag instead of debugging certain problem
> properly..

Bart, I fear you may live your entire life always with some
axe to grind with someone.

It makes it impossible to work with you effectively.

I've tried to start taking the personal attacks out of my
interactions with you, but you seem to be completely unable
to drop it.

If you're constantly bitter about this or that, nobody will
want to work with you.

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Bartlomiej Zolnierkiewicz]

On Saturday 24 October 2009 05:24:47 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Fri, 23 Oct 2009 20:52:47 +0200
> 
> > "Old" one corrupts because somebody thought it would be a smart move
> > to remove serialized flag instead of debugging certain problem
> > properly..
> 
> Bart, I fear you may live your entire life always with some
> axe to grind with someone.
> 
> It makes it impossible to work with you effectively.

You mean that I don't enjoy the idea of "just a managers" trying to
"manage" my free time?  Well, I don't.

I also don't buy the definition of "working with" presented by you.

> I've tried to start taking the personal attacks out of my
> interactions with you, but you seem to be completely unable
> to drop it.
> 
> If you're constantly bitter about this or that, nobody will
> want to work with you.

I'm not bitter, I just stick to the facts.

My mails may sound rude sometimes but they _always_ have the backing
in facts (and if I'm wrong I have no problem with saying "sorry, I was
wrong").

If you want to see how (unprovoked) personal attacks look like please
go re-read your own mails from few months ago...

It is all here:

	http://patchwork.ozlabs.org/patch/28945/

Please start with:

| Unlike the commit log message states, I suspect this change
| "introduces" incorrect handling of unexpected IRQs rather than
| "fixing".  I suspect the problem arises when the controller

and

| That's why all the IDE drivers are constantly breaking on sparc.

parts.

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Sat, 24 Oct 2009 14:38:00 +0200

> My mails may sound rude sometimes but they _always_ have the backing
> in facts (and if I'm wrong I have no problem with saying "sorry, I was
> wrong").

Being rude is not justified by being right.

> If you want to see how (unprovoked) personal attacks look like please
> go re-read your own mails from few months ago...

Yes, I am a complete asshole sometimes, and I need to improve
upon that.

Are you possesing such a level of willingness to change too?

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Bartlomiej Zolnierkiewicz]

On Saturday 24 October 2009 14:58:21 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> > If you want to see how (unprovoked) personal attacks look like please
> > go re-read your own mails from few months ago...
> 
> Yes, I am a complete asshole sometimes, and I need to improve
> upon that.

I also try to keep working on this but reality keeps standing in my way. ;)

> Are you possesing such a level of willingness to change too?

Lets just put all past misunderstandings behind and start over..

-- 
Bartlomiej Zolnierkiewicz

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Sat, 24 Oct 2009 15:13:12 +0200

> On Saturday 24 October 2009 14:58:21 David Miller wrote:
>> Are you possesing such a level of willingness to change too?
> 
> Lets just put all past misunderstandings behind and start over..

Sounds like a good idea to me :-)

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Mikulas Patocka]

On Fri, 23 Oct 2009, Alan Cox wrote:

> >     Yeah. The libata driver however doesn't make use of that register, 
> > unlike  cmd64x. It doesn't even touch the interrupt bits on PCI0646, only 
> > manipulation the "old" bits on PCI-64[89].
> 
> Yes I purposefully left out all the complexity because when I tested the
> cards I had it simply wasn't needed. I'm also not sure we should merge
> anything from the old to the new one until we know why the old one
> corrupts and the new one merely gets upset. Accidentally porting over a
> corruptor would be very bad indeed.

Well, if Daniela showed that cmd64[36] docs say it needs serialization, 
then just add it there. Don't rely on the fact that the corruption didn't 
happen in my case.

Mikulas

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Alan Cox]

On Mon, 26 Oct 2009 07:36:48 -0400 (EDT)
Mikulas Patocka <mpatocka@redhat.com> wrote:

> On Fri, 23 Oct 2009, Alan Cox wrote:
> 
> > >     Yeah. The libata driver however doesn't make use of that register, 
> > > unlike  cmd64x. It doesn't even touch the interrupt bits on PCI0646, only 
> > > manipulation the "old" bits on PCI-64[89].
> > 
> > Yes I purposefully left out all the complexity because when I tested the
> > cards I had it simply wasn't needed. I'm also not sure we should merge
> > anything from the old to the new one until we know why the old one
> > corrupts and the new one merely gets upset. Accidentally porting over a
> > corruptor would be very bad indeed.
> 
> Well, if Daniela showed that cmd64[36] docs say it needs serialization, 
> then just add it there. Don't rely on the fact that the corruption didn't 
> happen in my case.

Daniela sent me the relevant document - it doesn't exactly match what is
being described here as it relates to motherboards improperly timing out
PCI access requests.

Basically the fix is "don't touch the task file on one channel while DMA
is live on the other". That means that for libata serialize is not
sufficient on a shared IRQ set up as we'll touch ALTSTATUS. Plus it is
overkill as you can have parallel issue PIO commands.

For libata the best way to fix it seems to be to avoid parallel issuing
commands with an ATA_PROT_DMA/ATAPI_PROT_DMA command outstanding, and to
check the IRQ bits. Patch to follow when I get a moment.

[PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Mikulas Patocka]

Hi

Here is another patch for SSD for VIA UDMA33. Alan, please backport it 
into libata.

Mikulas

---

Don't use UDMA on VIA UDMA33 controller with Transcend SSD

The computer locks up if Transcend SSD runs in any of UDMA modes.
It doesn't lockup with different brand SSD, so this is specific to Transcend.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/ide/via82cxxx.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Index: linux-2.6.31.5-fast/drivers/ide/via82cxxx.c
===================================================================
--- linux-2.6.31.5-fast.orig/drivers/ide/via82cxxx.c	2009-05-29 22:36:58.000000000 +0200
+++ linux-2.6.31.5-fast/drivers/ide/via82cxxx.c	2009-11-04 22:32:55.000000000 +0100
@@ -195,6 +195,21 @@ static void via_set_pio_mode(ide_drive_t
 	via_set_drive(drive, XFER_PIO_0 + pio);
 }
 
+static u8 via_udma_filter(ide_drive_t *drive)
+{
+	char *m = (char *)&drive->id[ATA_ID_PROD];
+
+	/*
+	 * Restrict UDMA for Transcend flash cards.
+	 * On VIA 33, UDMA locks up. On VIA 133, it works. I can't test other
+	 * controllers.
+	 */
+	if (!memcmp(m, "TS", 2) && drive->hwif->ultra_mask == ATA_UDMA2)
+		return 0;
+
+	return drive->hwif->ultra_mask;
+}
+
 static struct via_isa_bridge *via_config_find(struct pci_dev **isa)
 {
 	struct via_isa_bridge *via_config;
@@ -372,6 +387,7 @@ static const struct ide_port_ops via_por
 	.set_pio_mode		= via_set_pio_mode,
 	.set_dma_mode		= via_set_drive,
 	.cable_detect		= via82cxxx_cable_detect,
+	.udma_filter		= via_udma_filter,
 };
 
 static const struct ide_port_info via82cxxx_chipset __devinitdata = {

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Alan Cox]

On Wed, 4 Nov 2009 20:25:21 -0500 (EST)
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Hi
> 
> Here is another patch for SSD for VIA UDMA33. Alan, please backport it 
> into libata.

I might consider adding the TS device to the blacklist on libata IFF
- I have the full ID string so I can match device accurately not blanket
  cripple hardware
- I had a much more detailed bug report - eg exact dmesg data up to the
  lock
- I know which exact controller card(s) it occurs on (rather than
  blanket "via")
- If I knew it occurred on libata
- If I knew it wasn't an IDE and/or shared IDE/libata bug or didn't
  appear to be one.

Crippling every other transcend device user on the planet for your
specific report (that nobody else has so far reported to me) would almost
certainly be a regression on a massive scale.

Does your TS device work with other controllers - do you just have a
faulty unit or marginal cabling ?

For any real failure case like this I'd expect a pile of entries in
bugzillas from multiple people. Now it could simply be yours is the first
in which case I'll look at it a blacklist when I see a few more.

Alan

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Mikulas Patocka]

On Thu, 5 Nov 2009, Alan Cox wrote:

> On Wed, 4 Nov 2009 20:25:21 -0500 (EST)
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > Hi
> > 
> > Here is another patch for SSD for VIA UDMA33. Alan, please backport it 
> > into libata.
> 
> I might consider adding the TS device to the blacklist on libata IFF
> - I have the full ID string so I can match device accurately not blanket
>   cripple hardware
> - I had a much more detailed bug report - eg exact dmesg data up to the
>   lock
> - I know which exact controller card(s) it occurs on (rather than
>   blanket "via")
> - If I knew it occurred on libata
> - If I knew it wasn't an IDE and/or shared IDE/libata bug or didn't
>   appear to be one.
> 
> Crippling every other transcend device user on the planet for your
> specific report (that nobody else has so far reported to me) would almost
> certainly be a regression on a massive scale.
> 
> Does your TS device work with other controllers - do you just have a
> faulty unit or marginal cabling ?
> 
> For any real failure case like this I'd expect a pile of entries in
> bugzillas from multiple people. Now it could simply be yours is the first
> in which case I'll look at it a blacklist when I see a few more.
> 
> Alan

The device ID string is: TS64GSSD25-M, serial 002328450083, revision V0826

The south bridge is VIA VT82C586B

The lockup happens on partition scan, sometimes the scan passes and it 
locks up when loading init. It doesn't get further.

The lockup is not dependent on IDE driver. I experienced it with Linux 2.6 
IDE, Linux 2.6 LIBATA, Daniela's OS/2 DANIS506 driver (Daniela, if you 
maintain a blacklist, you can add it there too) - switching to MWDMA 
helped. Linux 2.2 locked up too (it doesn't set transfer mode and relies 
on BIOS) and disabling UDMA in BIOS cured the problem.

I tried to switch to UDMA 0 and UDMA 1, but the lockups didn't go away, 
they just became less frequent. With MWDMA 2 it works reliably.

I tested it in a motherboard with VIA133 IDE and it works there. I think 
it also worked with that VT6421 card, but I can't test it now because I 
moved root to that Transcend SSD and it wouldn't boot of that.

This is the list of PCI devices for the detailed information (note that 
onboard IDE locks-up, not that additional VT6421 controller on a PCI 
card).

Mikulas

BUS 0, DEV 0, FN 0
1106 0597 0000 0000 06 00 00 03
        Bridge - Host bridge
        VIA Technologies, Inc. - VT82C597 [Apollo VP3]
BUS 0, DEV 1, FN 0
1106 8598 0000 0000 06 04 00 00
        Bridge - PCI bridge - Normal decode
        VIA Technologies, Inc. - VT82C598/694x [Apollo MVP3/Pro133x AGP]
BUS 0, DEV 7, FN 0
1106 0586 0000 0000 06 01 00 41
        Bridge - ISA bridge
        VIA Technologies, Inc. - VT82C586/A/B PCI-to-ISA [Apollo VP]
BUS 0, DEV 7, FN 1 (CLAIMED)
1106 0571 0000 0000 01 01 8A 06
        Mass storage controller - IDE interface
        VIA Technologies, Inc. - VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC 
Bus Master IDE
BUS 0, DEV 7, FN 2 (CLAIMED)
1106 3038 0925 1234 0C 03 00 02
        Serial bus controller - USB Controller - UHCI
        VIA Technologies, Inc. - VT82xxxxx UHCI USB 1.1 Controller - USB 
Controller
BUS 0, DEV 7, FN 3
1106 3040 0000 0000 06 04 00 10
        Bridge - PCI bridge - Normal decode
        VIA Technologies, Inc. - VT82C586B ACPI
BUS 0, DEV 8, FN 0 (CLAIMED)
1101 1060 1101 1060 01 00 00 01
        Mass storage controller - SCSI storage controller
        Initio Corporation - INI-A100U2W
BUS 0, DEV 9, FN 0 (CLAIMED)
1106 3038 1106 3038 0C 03 00 62
        Serial bus controller - USB Controller - UHCI
        VIA Technologies, Inc. - VT82xxxxx UHCI USB 1.1 Controller
BUS 0, DEV 9, FN 1 (CLAIMED)
1106 3038 1106 3038 0C 03 00 62
        Serial bus controller - USB Controller - UHCI
        VIA Technologies, Inc. - VT82xxxxx UHCI USB 1.1 Controller
BUS 0, DEV 9, FN 2
1106 3104 1106 3104 0C 03 20 65
        Serial bus controller - USB Controller - EHCI
        VIA Technologies, Inc. - USB 2.0
BUS 0, DEV 9, FN 3 (CLAIMED)
1106 3249 1106 3249 01 04 00 50
        Mass storage controller - RAID bus controller
        VIA Technologies, Inc. - VT6421 IDE RAID Controller
BUS 0, DEV 10, FN 0 (CLAIMED)
8086 1076 8086 1176 02 00 00 00
        Network controller - Ethernet controller
        Intel Corporation - 82541GI Gigabit Ethernet Controller - PRO/1000 
MT Desktop Adapter
BUS 1, DEV 0, FN 0 (CLAIMED)
5333 8A13 5333 8A13 03 00 00 02
        Display controller - VGA compatible controller - VGA
        S3 Inc. - 86c368 [Trio 3D/2X] - Trio3D/2X

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Alan Cox]

> The device ID string is: TS64GSSD25-M, serial 002328450083, revision V0826
> The south bridge is VIA VT82C586B
> 
> The lockup happens on partition scan, sometimes the scan passes and it 
> locks up when loading init. It doesn't get further.

The Linux driver doesn't set all the time setup strictly but it does set
it to be a safer maximum value. That might be worth checking but it would
still be a device naughty if so.

> The lockup is not dependent on IDE driver. I experienced it with Linux 2.6 
> IDE, Linux 2.6 LIBATA, Daniela's OS/2 DANIS506 driver (Daniela, if you 
> maintain a blacklist, you can add it there too) - switching to MWDMA 
> helped. Linux 2.2 locked up too (it doesn't set transfer mode and relies 
> on BIOS) and disabling UDMA in BIOS cured the problem.

> I tested it in a motherboard with VIA133 IDE and it works there. I think 
> it also worked with that VT6421 card, but I can't test it now because I 
> moved root to that Transcend SSD and it wouldn't boot of that.

Thanks - so basically its a specific card/device combination (or a dud
device you own), that makes it easier as any blacklisting can be very
much narrower.

> 1106 8598 0000 0000 06 04 00 00
>         Bridge - PCI bridge - Normal decode
>         VIA Technologies, Inc. - VT82C598/694x [Apollo MVP3/Pro133x AGP]

Ok so its also a dinosaur which might explain the lack of other reports.


Thanks

Alan

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Mikulas Patocka]

On Thu, 5 Nov 2009, Alan Cox wrote:

> > The device ID string is: TS64GSSD25-M, serial 002328450083, revision V0826
> > The south bridge is VIA VT82C586B
> > 
> > The lockup happens on partition scan, sometimes the scan passes and it 
> > locks up when loading init. It doesn't get further.
> 
> The Linux driver doesn't set all the time setup strictly but it does set
> it to be a safer maximum value. That might be worth checking but it would
> still be a device naughty if so.

I tried UDMA 0 and 1 and no help. I didn't try to fiddle the timing 
register manually. You mean, try set the disk to UDMA 2 and set the 
register to a lower timing? Or the other way? I can try ...

Mikulas

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[David Miller]

From: Mikulas Patocka <mpatocka@redhat.com>
Date: Wed, 4 Nov 2009 20:25:21 -0500 (EST)

> Don't use UDMA on VIA UDMA33 controller with Transcend SSD
> 
> The computer locks up if Transcend SSD runs in any of UDMA modes.
> It doesn't lockup with different brand SSD, so this is specific to Transcend.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

Mikulas, I'm happy to apply this if you match on the full
ID string, not just "TS".

Please update your patch and I'll push it upstream.

Thank you.

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Mikulas Patocka]

On Tue, 17 Nov 2009, David Miller wrote:

> From: Mikulas Patocka <mpatocka@redhat.com>
> Date: Wed, 4 Nov 2009 20:25:21 -0500 (EST)
> 
> > Don't use UDMA on VIA UDMA33 controller with Transcend SSD
> > 
> > The computer locks up if Transcend SSD runs in any of UDMA modes.
> > It doesn't lockup with different brand SSD, so this is specific to Transcend.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> Mikulas, I'm happy to apply this if you match on the full
> ID string, not just "TS".
> 
> Please update your patch and I'll push it upstream.
> 
> Thank you.

Transcend makes various versions of their SSDs and all begin with TS. You 
can assume that Transcend SSDs with different capacity or format won't 
work too because they likely use the same controller. The naming is this:

TS64GSSD25-M
TS: Transcend
64G: capacity
SSD25: 2.5" format
-M: MLC

So the problem is that if you match against the full string, you are going 
to miss the other Transcend devices and the patch becomes quite useless.

If you want to harden it against false negatives, you can grep the string 
for "SSD", as in the patch below, but there is not anything better to do 
--- if you include "64G" in the string, you fail on non-64G devices, if 
you include "SSD25", you fail on 1.8" devices, if you include "-M", you 
fail on SLC.

Mikulas

---

Don't use UDMA on VIA UDMA33 controller with Transcend SSD

The computer locks up after if Transcend SSD runs in any of UDMA modes.
It doesn't lockup with different brand SSD, so this is specific to Transcend.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/ide/via82cxxx.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Index: linux-2.6.31.6-fast/drivers/ide/via82cxxx.c
===================================================================
--- linux-2.6.31.6-fast.orig/drivers/ide/via82cxxx.c	2009-11-16 13:08:04.000000000 +0100
+++ linux-2.6.31.6-fast/drivers/ide/via82cxxx.c	2009-11-18 17:57:22.000000000 +0100
@@ -195,6 +195,22 @@ static void via_set_pio_mode(ide_drive_t
 	via_set_drive(drive, XFER_PIO_0 + pio);
 }
 
+static u8 via_udma_filter(ide_drive_t *drive)
+{
+	char *m = (char *)&drive->id[ATA_ID_PROD];
+
+	/*
+	 * Restrict UDMA for Transcend flash cards.
+	 * On VIA 33, UDMA locks up. On VIA 133, it works. I can't test other
+	 * controllers.
+	 */
+	if (!memcmp(m, "TS", 2) && strstr(m, "SSD") &&
+	    drive->hwif->ultra_mask == ATA_UDMA2)
+		return 0;
+
+	return drive->hwif->ultra_mask;
+}
+
 static struct via_isa_bridge *via_config_find(struct pci_dev **isa)
 {
 	struct via_isa_bridge *via_config;
@@ -372,6 +388,7 @@ static const struct ide_port_ops via_por
 	.set_pio_mode		= via_set_pio_mode,
 	.set_dma_mode		= via_set_drive,
 	.cable_detect		= via82cxxx_cable_detect,
+	.udma_filter		= via_udma_filter,
 };
 
 static const struct ide_port_info via82cxxx_chipset __devinitdata = {

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Alan Cox]

> Transcend makes various versions of their SSDs and all begin with TS. You 
> can assume that Transcend SSDs with different capacity or format won't 
> work too because they likely use the same controller. The naming is this:

You don't want to assume that because then you will never find out if
there is no problem.

> So the problem is that if you match against the full string, you are going 
> to miss the other Transcend devices and the patch becomes quite useless.

Quite the reverse. It's not implausible that only one or two devices are
affected, but match TS SSD is going to match zillions of devices of many
generations for years to come. Thats not good.

Experience is also that very few of the existing blacklist entries we
have grow to be large scale wildcards.

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[David Miller]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Wed, 18 Nov 2009 17:22:58 +0000

>> Transcend makes various versions of their SSDs and all begin with TS. You 
>> can assume that Transcend SSDs with different capacity or format won't 
>> work too because they likely use the same controller. The naming is this:
> 
> You don't want to assume that because then you will never find out if
> there is no problem.

Agreed.

And if Alan's patch for the ATA layer is going to use an exact string
match, which it is, and we should do the same in the IDE layer to be
consistent.

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Mikulas Patocka]

On Wed, 18 Nov 2009, David Miller wrote:

> From: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Date: Wed, 18 Nov 2009 17:22:58 +0000
> 
> >> Transcend makes various versions of their SSDs and all begin with TS. You 
> >> can assume that Transcend SSDs with different capacity or format won't 
> >> work too because they likely use the same controller. The naming is this:
> > 
> > You don't want to assume that because then you will never find out if
> > there is no problem.
> 
> Agreed.
> 
> And if Alan's patch for the ATA layer is going to use an exact string
> match, which it is, and we should do the same in the IDE layer to be
> consistent.

And what will you do with 16G or 32G versions of the same device? You 
really think they use different controllers and are not affected?

Mikulas

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[David Miller]

From: Mikulas Patocka <mpatocka@redhat.com>
Date: Wed, 18 Nov 2009 12:46:37 -0500 (EST)

> And what will you do with 16G or 32G versions of the same device? You 
> really think they use different controllers and are not affected?

Firmware changes often.

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Mikulas Patocka]

On Wed, 18 Nov 2009, David Miller wrote:

> From: Mikulas Patocka <mpatocka@redhat.com>
> Date: Wed, 18 Nov 2009 12:46:37 -0500 (EST)
> 
> > And what will you do with 16G or 32G versions of the same device? You 
> > really think they use different controllers and are not affected?
> 
> Firmware changes often.

So use the firmware revision field to match it.

Mikulas

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Mikulas Patocka]

> > Transcend makes various versions of their SSDs and all begin with TS. You 
> > can assume that Transcend SSDs with different capacity or format won't 
> > work too because they likely use the same controller. The naming is this:
> 
> You don't want to assume that because then you will never find out if
> there is no problem.

I can be certain that no company would be stupid enough to design a 
separate controller for each capacity and form factor of their devices. 
They use one or a very few chips.

Furthemore, even if they upgrade the chip and produce 64G 2.5" MLC device 
with the new working chip, you can't tell it from the ID string at all.

> > So the problem is that if you match against the full string, you are going 
> > to miss the other Transcend devices and the patch becomes quite useless.
> 
> Quite the reverse. It's not implausible that only one or two devices are
> affected, but match TS SSD is going to match zillions of devices of many
> generations for years to come. Thats not good.

Why no good? People just get lower performance (on an old motherboard, 
where no one expects high performance anyway).

On the other hand, if you miss a device from the blacklist, people get 
crashes.

Crashes are worse than lower performance.

> Experience is also that very few of the existing blacklist entries we
> have grow to be large scale wildcards.

Mikulas

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Alan Cox]

> I can be certain that no company would be stupid enough to design a 
> separate controller for each capacity and form factor of their devices. 
> They use one or a very few chips.

But they do fix the firmware, and you see that with hard disks as well.

> Why no good? People just get lower performance (on an old motherboard, 
> where no one expects high performance anyway).

Which they don't notice or report
> 
> On the other hand, if you miss a device from the blacklist, people get 
> crashes.

Which they do notice or report (and in the meantime can boot with
libata.dma=0 or one of the MWDMA settings).

> Crashes are worse than lower performance.

Not always. Inflicting poor performance (and also *much* lower data
integrity) on people isn't a good idea in this case IMHO. That's based
upon experience with disks and what gets fixed.

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Mikulas Patocka]

On Wed, 18 Nov 2009, Alan Cox wrote:

> > I can be certain that no company would be stupid enough to design a 
> > separate controller for each capacity and form factor of their devices. 
> > They use one or a very few chips.
> 
> But they do fix the firmware, and you see that with hard disks as well.

So match it against a firmware revision (V0826 in my case, so blacklist 
all lower).

Anyway, I think it is bug on the motherboard because similar lockups can 
be seen with one WD 40G disk operating at UDMA0 or UDMA1 (UDMA2 is OK). 
And if this is old buggy motherboard, Transcend will fix nothing.

> > Why no good? People just get lower performance (on an old motherboard, 
> > where no one expects high performance anyway).
> 
> Which they don't notice or report

If they don't notice it, it is not a problem :)

Mikulas

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Bartlomiej Zolnierkiewicz]

Mikulas Patocka wrote:

> On Tue, 17 Nov 2009, David Miller wrote:
> 
> > From: Mikulas Patocka <mpatocka@redhat.com>
> > Date: Wed, 4 Nov 2009 20:25:21 -0500 (EST)
> > 
> > > Don't use UDMA on VIA UDMA33 controller with Transcend SSD
> > > 
> > > The computer locks up if Transcend SSD runs in any of UDMA modes.
> > > It doesn't lockup with different brand SSD, so this is specific to Transcend.
> > > 
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > Mikulas, I'm happy to apply this if you match on the full
> > ID string, not just "TS".
> > 
> > Please update your patch and I'll push it upstream.
> > 
> > Thank you.
> 
> Transcend makes various versions of their SSDs and all begin with TS. You 
> can assume that Transcend SSDs with different capacity or format won't 
> work too because they likely use the same controller. The naming is this:
> 
> TS64GSSD25-M
> TS: Transcend
> 64G: capacity
> SSD25: 2.5" format
> -M: MLC
> 
> So the problem is that if you match against the full string, you are going 
> to miss the other Transcend devices and the patch becomes quite useless.
> 
> If you want to harden it against false negatives, you can grep the string 
> for "SSD", as in the patch below, but there is not anything better to do 
> --- if you include "64G" in the string, you fail on non-64G devices, if 
> you include "SSD25", you fail on 1.8" devices, if you include "-M", you 
> fail on SLC.
> 
> Mikulas
> 
> ---
> 
> Don't use UDMA on VIA UDMA33 controller with Transcend SSD
> 
> The computer locks up after if Transcend SSD runs in any of UDMA modes.
> It doesn't lockup with different brand SSD, so this is specific to Transcend.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

Reviewed-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

Still valid for 3.1.  Dave, ping?

> ---
>  drivers/ide/via82cxxx.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> Index: linux-2.6.31.6-fast/drivers/ide/via82cxxx.c
> ===================================================================
> --- linux-2.6.31.6-fast.orig/drivers/ide/via82cxxx.c	2009-11-16 13:08:04.000000000 +0100
> +++ linux-2.6.31.6-fast/drivers/ide/via82cxxx.c	2009-11-18 17:57:22.000000000 +0100
> @@ -195,6 +195,22 @@ static void via_set_pio_mode(ide_drive_t
>  	via_set_drive(drive, XFER_PIO_0 + pio);
>  }
>  
> +static u8 via_udma_filter(ide_drive_t *drive)
> +{
> +	char *m = (char *)&drive->id[ATA_ID_PROD];
> +
> +	/*
> +	 * Restrict UDMA for Transcend flash cards.
> +	 * On VIA 33, UDMA locks up. On VIA 133, it works. I can't test other
> +	 * controllers.
> +	 */
> +	if (!memcmp(m, "TS", 2) && strstr(m, "SSD") &&
> +	    drive->hwif->ultra_mask == ATA_UDMA2)
> +		return 0;
> +
> +	return drive->hwif->ultra_mask;
> +}
> +
>  static struct via_isa_bridge *via_config_find(struct pci_dev **isa)
>  {
>  	struct via_isa_bridge *via_config;
> @@ -372,6 +388,7 @@ static const struct ide_port_ops via_por
>  	.set_pio_mode		= via_set_pio_mode,
>  	.set_dma_mode		= via_set_drive,
>  	.cable_detect		= via82cxxx_cable_detect,
> +	.udma_filter		= via_udma_filter,
>  };
>  
>  static const struct ide_port_info via82cxxx_chipset __devinitdata = {

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[David Miller]

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Tue, 11 Oct 2011 19:12:01 +0200

> Still valid for 3.1.  Dave, ping?

Feedback given and changes requested/required, patch was not updated
and resubmitted by the author:

http://patchwork.ozlabs.org/patch/38764/

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Alan Cox]

On Tue, 11 Oct 2011 15:05:04 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Tue, 11 Oct 2011 19:12:01 +0200
> 
> > Still valid for 3.1.  Dave, ping?
> 
> Feedback given and changes requested/required, patch was not updated
> and resubmitted by the author:
> 
> http://patchwork.ozlabs.org/patch/38764/

The last state on this I saw it seemd that exactly one person had seen it
on exactly one machine ? Has that changed ?

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Bartlomiej Zolnierkiewicz]

Alan Cox wrote:

> On Tue, 11 Oct 2011 15:05:04 -0400 (EDT)
> David Miller <davem@davemloft.net> wrote:
> 
> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Date: Tue, 11 Oct 2011 19:12:01 +0200
> > 
> > > Still valid for 3.1.  Dave, ping?
> > 
> > Feedback given and changes requested/required, patch was not updated
> > and resubmitted by the author:
> > 
> > http://patchwork.ozlabs.org/patch/38764/
> 
> The last state on this I saw it seemd that exactly one person had seen it
> on exactly one machine ? Has that changed ?

No, you're right here.

Here is updated patch:

From: Mikulas Patocka <mpatocka@redhat.com>
Subject: [PATCH] via82cxxx: don't use UDMA on VIA UDMA33 controller with Transcend SSD

Don't use UDMA on VIA UDMA33 controller with Transcend SSD.

The computer locks up if Transcend SSD runs in any of UDMA modes.
It doesn't lockup with different brand SSD, so this is specific to Transcend.

bzolnier:
- limit it to VT82C586A/B + TS64GSSD25-M (per commit 10734fc ("pata_via:
  Blacklist some combinations of Transcend Flash and via")) for now
- add warning message

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: David Miller <davem@davemloft.net>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/via82cxxx.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Index: b/drivers/ide/via82cxxx.c
===================================================================
--- a/drivers/ide/via82cxxx.c
+++ b/drivers/ide/via82cxxx.c
@@ -220,6 +220,24 @@ static void via_set_pio_mode(ide_hwif_t
 	via_set_drive(hwif, drive);
 }
 
+static u8 via_udma_filter(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	struct pci_dev *dev = to_pci_dev(hwif->dev);
+	struct ide_host *host = pci_get_drvdata(dev);
+	struct via82cxxx_dev *vdev = host->host_priv;
+	char *m = (char *)&drive->id[ATA_ID_PROD];
+
+	if (vdev->via_config->id == PCI_DEVICE_ID_VIA_82C586_0 &&
+	    strcmp(m, "TS64GSSD25-M") == 0) {
+		printk(KERN_WARNING "%s: disabling UDMA mode due to reported "
+			"lockups with this device.\n", drive->name);
+		return 0;
+	}
+
+	return hwif->ultra_mask;
+}
+
 static struct via_isa_bridge *via_config_find(struct pci_dev **isa)
 {
 	struct via_isa_bridge *via_config;
@@ -401,6 +419,7 @@ static const struct ide_port_ops via_por
 	.set_pio_mode		= via_set_pio_mode,
 	.set_dma_mode		= via_set_drive,
 	.cable_detect		= via82cxxx_cable_detect,
+	.udma_filter		= via_udma_filter,
 };
 
 static const struct ide_port_info via82cxxx_chipset __devinitdata = {

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Alan Cox]

> > The last state on this I saw it seemd that exactly one person had seen it
> > on exactly one machine ? Has that changed ?
> 
> No, you're right here.

So as far as is known its a weird glitch on one persons box with one
device with one firmware that's not been replicated ?

That's not a patch candidate really is it.

Alan

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Bartlomiej Zolnierkiewicz]

Alan Cox wrote:

> > > The last state on this I saw it seemd that exactly one person had seen it
> > > on exactly one machine ? Has that changed ?
> > 
> > No, you're right here.
> 
> So as far as is known its a weird glitch on one persons box with one
> device with one firmware that's not been replicated ?
> 
> That's not a patch candidate really is it.

Should we revert following commit then?

commit 10734fc8d5fbf89e88519d72e58cce83be21941a
Author: Alan Cox <alan@linux.intel.com>
Date:   Mon Nov 30 13:22:43 2009 +0000

    pata_via: Blacklist some combinations of Transcend Flash and via
    
    Reported by Mikulas Patocka.
    
    VIA VT82C586B + Transcend TS64GSSD25-M v0826 does not work in UDMA mode
    
    Signed-off-by: Alan Cox <alan@linux.intel.com>
    Signed-off-by: Jeff Garzik <jgarzik@redhat.com>

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Bartlomiej Zolnierkiewicz]

On Thursday 05 November 2009 02:25:21 am Mikulas Patocka wrote:
> Hi
> 
> Here is another patch for SSD for VIA UDMA33. Alan, please backport it 
> into libata.
> 
> Mikulas
> 
> ---
> 
> Don't use UDMA on VIA UDMA33 controller with Transcend SSD
> 
> The computer locks up if Transcend SSD runs in any of UDMA modes.
> It doesn't lockup with different brand SSD, so this is specific to Transcend.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

FWIW the following modified version of your patch is now in my atang tree:

From: Mikulas Patocka <mpatocka@redhat.com>
Subject: [PATCH] via82cxxx: don't use UDMA on VIA UDMA33 controller with Transcend SSD

Don't use UDMA on VIA UDMA33 controller with Transcend SSD.

The computer locks up if Transcend SSD runs in any of UDMA modes.
It doesn't lockup with different brand SSD, so this is specific to Transcend.

bzolnier:
- limit it to VT82C586A/B + TS64GSSD25-M (per commit 10734fc) for now
- add warning message

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: David Miller <davem@davemloft.net>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/via82cxxx.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Index: b/drivers/ide/via82cxxx.c
===================================================================
--- a/drivers/ide/via82cxxx.c
+++ b/drivers/ide/via82cxxx.c
@@ -195,6 +195,24 @@ static void via_set_pio_mode(ide_drive_t
 	via_set_drive(drive, XFER_PIO_0 + pio);
 }
 
+static u8 via_udma_filter(ide_drive_t *drive)
+{
+	ide_hwif_t *hwif = drive->hwif;
+	struct pci_dev *dev = to_pci_dev(hwif->dev);
+	struct ide_host *host = pci_get_drvdata(dev);
+	struct via82cxxx_dev *vdev = host->host_priv;
+	char *m = (char *)&drive->id[ATA_ID_PROD];
+
+	if (vdev->via_config->id == PCI_DEVICE_ID_VIA_82C586_0 &&
+	    strcmp(m, "TS64GSSD25-M") == 0) {
+		printk(KERN_WARNING "%s: disabling UDMA mode due to reported "
+			"lockups with this device.\n", drive->name);
+		return 0;
+	}
+
+	return hwif->ultra_mask;
+}
+
 static struct via_isa_bridge *via_config_find(struct pci_dev **isa)
 {
 	struct via_isa_bridge *via_config;
@@ -372,6 +390,7 @@ static const struct ide_port_ops via_por
 	.set_pio_mode		= via_set_pio_mode,
 	.set_dma_mode		= via_set_drive,
 	.cable_detect		= via82cxxx_cable_detect,
+	.udma_filter		= via_udma_filter,
 };
 
 static const struct ide_port_info via82cxxx_chipset __devinitdata = {

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Alan Cox]

> The computer locks up if Transcend SSD runs in any of UDMA modes.
> It doesn't lockup with different brand SSD, so this is specific to Transcend.
> 
> bzolnier:
> - limit it to VT82C586A/B + TS64GSSD25-M (per commit 10734fc) for now
> - add warning message

Looks good to me.


> +		printk(KERN_WARNING "%s: disabling UDMA mode due to reported "
> +			"lockups with this device.\n", drive->name);

That sounds odd - I think I'd have put ": not using UDMA mode as lockups
have been reported with this device" or similar.

Ackity-ack

Re: [PATCH] Don't use UDMA on VIA UDMA33 controller with Transcend SSD

[Bartlomiej Zolnierkiewicz]

On Thursday 14 January 2010 08:24:10 pm Alan Cox wrote:
> > The computer locks up if Transcend SSD runs in any of UDMA modes.
> > It doesn't lockup with different brand SSD, so this is specific to Transcend.
> > 
> > bzolnier:
> > - limit it to VT82C586A/B + TS64GSSD25-M (per commit 10734fc) for now
> > - add warning message
> 
> Looks good to me.

Thanks.

> > +		printk(KERN_WARNING "%s: disabling UDMA mode due to reported "
> > +			"lockups with this device.\n", drive->name);
> 
> That sounds odd - I think I'd have put ": not using UDMA mode as lockups
> have been reported with this device" or similar.

For compatibility reasons the warning message has been kept identical to
the one used by pata_via driver (please see commit 10734fc for details 8)..

--
Bartlomiej Zolnierkiewicz

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Alan Cox]

> Don't do it --- disks have cache and reading from the cache is as fast as 
> reading from SSD (or even faster), so if there is some speed-race in the 
> chip, there is a possibility that the data corruption happens with disks 
> too --- just with lower probability.
> 
> If we don't know why the chip corrupts data, we must treat it as always 
> corrupting data.

In which case we should delete the driver because serializing it is
probably not sufficient. There is a proper way to deal with IDE problems
and randomly turning things on and off isn't the solution from experience.

So
- It would be useful to get the data sheet that Daniela refers to
- If there is some kind of data path issue then serializing probably isn't
  enough on its own as has been said

and we need to understand why the libata case doesn't show corruption but
clearly shows it is unhappy. Libata doesn't serialize the device and
doesn't do fancy IRQ checking either.

Just serializing is likely to make it worse - it may well become a rare
deeply obscure corruption rather than a nice visible one like we have now
- and that is far far nastier. Most likely libata also needs work.

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Alan Cox]

I doubt there is any hardware bug with the SSD. Some CMD64x hardware
certainly has errata and as the data sheets were published your starting
point would be the data sheets for the various chip versions. Does the
sparc use the same off the shelf part as the PC people or does it have
different glue or chip versions (eg as a macro cell in something else ?)

Alan

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[David Miller]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Thu, 22 Oct 2009 14:56:13 +0100

> I doubt there is any hardware bug with the SSD. Some CMD64x hardware
> certainly has errata and as the data sheets were published your starting
> point would be the data sheets for the various chip versions. Does the
> sparc use the same off the shelf part as the PC people or does it have
> different glue or chip versions (eg as a macro cell in something else ?)

The sparc machines use the standard cmd64x chips without any special
glue logic.

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Mikulas Patocka]

> Right, and see also:
> 
> commit 6b5cde3629701258004b94cde75dd1089b556b02
> Author: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date:   Mon Dec 29 20:27:32 2008 +0100
> 
>     cmd64x: set IDE_HFLAG_SERIALIZE explictly for CMD646
> 
> Which is how we got there.
> 
> The most conservative thing to do is to set the flag as
> is done by mpatocka's patch but I'd like Frans to regression
> test that on his ultra5.
> 
> Frans can you do that?
> 
> Thanks.

I read the thread about wild interrupts that Frans was observing and that 
led to disabling the serialization.

The thing is tricky --- if we read status register on interrupt, we break 
the serialization principle and introduce potential data corruption.

If we don't read the status register, the wild interrupt kills the whole 
interrupt line.

I think the interrupt handler should read the BM-status register on both 
channels (it reflects the interrupt state even in PIO mode) to test if 
there is an unexpected interrupt on the inactive channel --- this should 
be much more safe than reading the status register. If there is an 
interrupt, then
--- read the status of the inactive channel? (potential data corruption, 
but it is reported to happen only on boot).
--- Or can the interrupt be acknowledged only in BM-status without 
touching the device? I believe yes, it shoud shut the PCI interrupt but it 
would leave the IDE interrupt line on (should be cleared on next command).

Mikulas

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Sergei Shtylyov]

Hello.

Mikulas Patocka wrote:

>> Right, and see also:
>>
>> commit 6b5cde3629701258004b94cde75dd1089b556b02
>> Author: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>> Date:   Mon Dec 29 20:27:32 2008 +0100
>>
>>     cmd64x: set IDE_HFLAG_SERIALIZE explictly for CMD646
>>
>> Which is how we got there.
>>
>> The most conservative thing to do is to set the flag as
>> is done by mpatocka's patch but I'd like Frans to regression
>> test that on his ultra5.
>>
>> Frans can you do that?
>>
>> Thanks.
>>     
>
> I read the thread about wild interrupts that Frans was observing and that 
> led to disabling the serialization.
>
> The thing is tricky --- if we read status register on interrupt, we break 
> the serialization principle and introduce potential data corruption.
>
> If we don't read the status register, the wild interrupt kills the whole 
> interrupt line.
>
> I think the interrupt handler should read the BM-status register on both 
> channels (it reflects the interrupt state even in PIO mode) to test if 
>   

   Are you sure? Anyway, there's no need as we're reading the interrupt 
bits CFR/ARTTIM23 registers first (at least in the IDE code). Look at 
the cmd*_test_irq() methods and ide_intr().

> there is an unexpected interrupt on the inactive channel --- this should 
> be much more safe than reading the status register. If there is an 
> interrupt, then
> --- read the status of the inactive channel? (potential data corruption, 
> but it is reported to happen only on boot).
> --- Or can the interrupt be acknowledged only in BM-status without 
> touching the device? I believe yes,

   And I believe no. BMIDE status bit doesn't acknoledge (clear, to be 
precise) the IDE interrupts, only the status register read does.


> it shoud shut the PCI interrupt but it 
> would leave the IDE interrupt line on (should be cleared on next command).
>   

    I think the negated wired-OR of both INTRQ signals serves as an 
-INTA source, not the BMIDE status bits. At least in the general case, 
where the BMIDE status doesn't reflect PIO mode interrupts.

> Mikulas

WBR, Sergei

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Mikulas Patocka]

>   Are you sure? Anyway, there's no need as we're reading the interrupt bits
> CFR/ARTTIM23 registers first (at least in the IDE code). Look at the
> cmd*_test_irq() methods and ide_intr().

Maybe the BMIDE status bit is just the same as CFR/ARTTIM23 interrupt bit.

> > there is an unexpected interrupt on the inactive channel --- this should be
> > much more safe than reading the status register. If there is an interrupt,
> > then
> > --- read the status of the inactive channel? (potential data corruption, but
> > it is reported to happen only on boot).
> > --- Or can the interrupt be acknowledged only in BM-status without touching
> > the device? I believe yes,
> 
>   And I believe no. BMIDE status bit doesn't acknoledge (clear, to be precise)
> the IDE interrupts, only the status register read does.

There are two things: IDE interrupt line set by the device (BMIDE status 
doesn't do anything with it) and chipset's INT[A-D] interrupt line --- and 
BMIDE status should clear it, at least for some chipsets.

Some chipset documentation (not for CMD64x) thatI have says that BMIDE 
irq status is set on any interrupt regardless if it's DMA or NONDMA.

On ICH SATA (in legacy non-AHCI mode), it is even required to acknowledge 
PIO interrupts with BMIDE status, otherwise the interrupt stays pending.

> > it shoud shut the PCI interrupt but it would leave the IDE interrupt line on
> > (should be cleared on next command).
> >   
> 
>    I think the negated wired-OR of both INTRQ signals serves as an -INTA
> source, not the BMIDE status bits. At least in the general case, where the
> BMIDE status doesn't reflect PIO mode interrupts.

It is not as simple, INTA and BMIDE status must be postponed until the 
chip flushes its buffers and writes the DMA last byte to the memory.

I agree with you that for some chipsets BMIDE doesn't have to be signalled 
in PIO mode --- but remember that here we are talking about dealing with 
broken devices that set the interrupt line spuriously and about 
serializing chipsets --- not about all chipsets and all devices.

So the best that can be done for such broken devices is to try to shut the 
interrupt in BMIDE register (or PCI registers in CMD64x). There is nothing 
better to do. If you have serializing chipset that doesn't let you shut 
interrupt and the inactive device fires spuriously --- there is absolutely 
nothing that can be done about it.

Mikulas

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Sergei Shtylyov]

Hello.

Mikulas Patocka wrote:

>>  Are you sure? Anyway, there's no need as we're reading the interrupt bits
>>CFR/ARTTIM23 registers first (at least in the IDE code). Look at the
>>cmd*_test_irq() methods and ide_intr().

> Maybe the BMIDE status bit is just the same as CFR/ARTTIM23 interrupt bit.

    Maybe -- if it indeed gets set in PIO mode as well. In this case though, 
there's quite little sense in having that bit mirrored (even twice with the 
newer controllers).

>>>there is an unexpected interrupt on the inactive channel --- this should be
>>>much more safe than reading the status register. If there is an interrupt,
>>>then
>>>--- read the status of the inactive channel? (potential data corruption, but
>>>it is reported to happen only on boot).
>>>--- Or can the interrupt be acknowledged only in BM-status without touching
>>>the device? I believe yes,

>>  And I believe no. BMIDE status bit doesn't acknoledge (clear, to be precise)
>>the IDE interrupts, only the status register read does.

> There are two things: IDE interrupt line set by the device (BMIDE status 
> doesn't do anything with it) and chipset's INT[A-D] interrupt line --- and 
> BMIDE status should clear it, at least for some chipsets.

> Some chipset documentation (not for CMD64x) thatI have says that BMIDE 
> irq status is set on any interrupt regardless if it's DMA or NONDMA.

    That is rather untypical behavior although some chipsets like Intel ICH 
are known to do it.

> On ICH SATA (in legacy non-AHCI mode), it is even required to acknowledge 
> PIO interrupts with BMIDE status, otherwise the interrupt stays pending.

>>>it shoud shut the PCI interrupt but it would leave the IDE interrupt line on
>>>(should be cleared on next command).

>>   I think the negated wired-OR of both INTRQ signals serves as an -INTA
>>source, not the BMIDE status bits. At least in the general case, where the
>>BMIDE status doesn't reflect PIO mode interrupts.

> It is not as simple, INTA and BMIDE status must be postponed until the 
> chip flushes its buffers and writes the DMA last byte to the memory.

    I know. The delay logic only acts in the DMA case. And it doesn't have 
to delay the interrupt itself, only the BMIDE status read with bit 2 set -- 
which is achievable by retrying the I/O transaction on PCI until the DMA 
actually completes.

> I agree with you that for some chipsets BMIDE doesn't have to be signalled 
> in PIO mode --- but remember that here we are talking about dealing with 
> broken devices that set the interrupt line spuriously and about 
> serializing chipsets --- not about all chipsets and all devices.

> So the best that can be done for such broken devices is to try to shut the 
> interrupt in BMIDE register (or PCI registers in CMD64x). There is nothing 
> better to do.

    And we're doing it, now for the PIO case also.

> If you have serializing chipset that doesn't let you shut 
> interrupt and the inactive device fires spuriously --- there is absolutely 
> nothing that can be done about it.

    Yes, seems so from ide_intr()...

> Mikulas

WBR, Sergei

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Frans Pop]

On Thursday 22 October 2009, David Miller wrote:
> The most conservative thing to do is to set the flag as
> is done by mpatocka's patch but I'd like Frans to regression
> test that on his ultra5.
>
> Frans can you do that?

Sure, if you send me the patch (or a link to it). May take a few days.

Also, is it still needed given the whole discussion that happened after 
this mail?

Cheers,
FJP

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[David Miller]

From: Frans Pop <elendil@planet.nl>
Date: Sat, 24 Oct 2009 13:28:39 +0200

> On Thursday 22 October 2009, David Miller wrote:
>> The most conservative thing to do is to set the flag as
>> is done by mpatocka's patch but I'd like Frans to regression
>> test that on his ultra5.
>>
>> Frans can you do that?
> 
> Sure, if you send me the patch (or a link to it). May take a few days.

It's here:

http://patchwork.ozlabs.org/patch/36615/

Also, all pending IDE patches can always be found at:

http://patchwork.ozlabs.org/project/linux-ide/list/

> Also, is it still needed given the whole discussion that happened after 
> this mail?

Yes, I still fully intend to apply mpatocka's patch, as-is.

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[Frans Pop]

On Saturday 24 October 2009, David Miller wrote:
> > On Thursday 22 October 2009, David Miller wrote:
> >> The most conservative thing to do is to set the flag as
> >> is done by mpatocka's patch but I'd like Frans to regression
> >> test that on his ultra5.
> >>
> >> Frans can you do that?
> >
> > Sure, if you send me the patch (or a link to it). May take a few days.
>
> It's here:
> http://patchwork.ozlabs.org/patch/36615/

Looks to work OK on my system. dmesg shows ide0 and ide1 are now 
serialized.

Cheers,
FJP

Re: [PATCH] Serialize CMD643 and CMD646 to fix a hardware bug with SSD

[David Miller]

From: Frans Pop <elendil@planet.nl>
Date: Sun, 25 Oct 2009 03:48:32 +0100

> On Saturday 24 October 2009, David Miller wrote:
>> > On Thursday 22 October 2009, David Miller wrote:
>> >> The most conservative thing to do is to set the flag as
>> >> is done by mpatocka's patch but I'd like Frans to regression
>> >> test that on his ultra5.
>> >>
>> >> Frans can you do that?
>> >
>> > Sure, if you send me the patch (or a link to it). May take a few days.
>>
>> It's here:
>> http://patchwork.ozlabs.org/patch/36615/
> 
> Looks to work OK on my system. dmesg shows ide0 and ide1 are now 
> serialized.

Thanks a lot for testing Frans, I've applied Mikulas's patch
to ide-2.6