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

[Mikulas Patocka <mpatocka@redhat.com>]

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
>