[PATCH v2 0/2]ata: Fix the dma state machine lockup for APM X-Gene SoC

[PATCH v2 0/2]ata: Fix the dma state machine lockup for APM X-Gene SoC

[Suman Tripathi]

Suman Tripathi (2):
  libahci: Implement the function ahci_restart_engine to restart the
    port dma engine.
  ata: Fix the dma state machine lockup for the IDENTIFY DEVICE PIO mode
    command.

 drivers/ata/ahci.h       |  4 ++++
 drivers/ata/ahci_xgene.c | 17 +++--------------
 drivers/ata/libahci.c    | 44 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 14 deletions(-)

--
1.8.2.1

[PATCH v2 1/2] libahci: Implement the function ahci_restart_engine to restart the port dma engine.

[Suman Tripathi]

This patch implements the function ahci_restart_engine function to restart the port dma engine.
---
 drivers/ata/ahci.h    |  1 +
 drivers/ata/libahci.c | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index af63c75..3c1760e 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -372,6 +372,7 @@ int ahci_do_softreset(struct ata_link *link, unsigned int *class,

 int ahci_stop_engine(struct ata_port *ap);
 void ahci_start_engine(struct ata_port *ap);
+int ahci_restart_engine(struct ata_port *ap);
 int ahci_check_ready(struct ata_link *link);
 int ahci_kick_engine(struct ata_port *ap);
 int ahci_port_resume(struct ata_port *ap);
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index b986145..584da77 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -742,6 +742,16 @@ static int ahci_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 	return 0;
 }

+int ahci_restart_engine(struct ata_port *ap)
+{
+	ahci_stop_engine(ap);
+	ahci_start_fis_rx(ap);
+	ahci_start_engine(ap);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ahci_restart_engine);
+
 #ifdef CONFIG_PM
 static void ahci_power_down(struct ata_port *ap)
 {
--
1.8.2.1

[PATCH v2 2/2] ata: Fix the dma state machine lockup for the IDENTIFY DEVICE PIO mode command.

[Suman Tripathi]

This patch fixes the dma state machine lockup due to the IDENTIFY DEVICE PIO mode command. The controller is unable to clear the BSY bit after receiving the PIO setup FIS and results the dma state machine to go into the CMFatalErrorUpdate state resulting in the dma state machine lockup.
---
 drivers/ata/ahci.h       |  3 +++
 drivers/ata/ahci_xgene.c | 17 +++--------------
 drivers/ata/libahci.c    | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 3c1760e..a4dd567 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -237,6 +237,9 @@ enum {
 						        error-handling stage) */
 	AHCI_HFLAG_MULTI_MSI		= (1 << 16), /* multiple PCI MSIs */
 	AHCI_HFLAG_NO_DEVSLP		= (1 << 17), /* no device sleep */
+	AHCI_HFLAG_BROKEN_PIO_CMD	= (1 << 18), /* Some PIO cmd's
+							resulting in HBA
+							dma state lockup */

 	/* ap->flags bits */

diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
index 77c89bf..741d5c7 100644
--- a/drivers/ata/ahci_xgene.c
+++ b/drivers/ata/ahci_xgene.c
@@ -104,14 +104,12 @@ static int xgene_ahci_init_memram(struct xgene_ahci_context *ctx)
  * @id: data buffer
  *
  * This custom read ID function is required due to the fact that the HW
- * does not support DEVSLP and the controller state machine may get stuck
- * after processing the ID query command.
+ * does not support DEVSLP.
  */
 static unsigned int xgene_ahci_read_id(struct ata_device *dev,
 				       struct ata_taskfile *tf, u16 *id)
 {
 	u32 err_mask;
-	void __iomem *port_mmio = ahci_port_base(dev->link->ap);

 	err_mask = ata_do_dev_read_id(dev, tf, id);
 	if (err_mask)
@@ -133,16 +131,6 @@ static unsigned int xgene_ahci_read_id(struct ata_device *dev,
 	 */
 	id[ATA_ID_FEATURE_SUPP] &= ~(1 << 8);

-	/*
-	 * Due to HW errata, restart the port if no other command active.
-	 * Otherwise the controller may get stuck.
-	 */
-	if (!readl(port_mmio + PORT_CMD_ISSUE)) {
-		writel(PORT_CMD_FIS_RX, port_mmio + PORT_CMD);
-		readl(port_mmio + PORT_CMD);	/* Force a barrier */
-		writel(PORT_CMD_FIS_RX | PORT_CMD_START, port_mmio + PORT_CMD);
-		readl(port_mmio + PORT_CMD);	/* Force a barrier */
-	}
 	return 0;
 }

@@ -303,7 +291,8 @@ static struct ata_port_operations xgene_ahci_ops = {
 };

 static const struct ata_port_info xgene_ahci_port_info = {
-	AHCI_HFLAGS(AHCI_HFLAG_NO_PMP | AHCI_HFLAG_YES_NCQ),
+	AHCI_HFLAGS(AHCI_HFLAG_NO_PMP | AHCI_HFLAG_YES_NCQ |
+		    AHCI_HFLAG_BROKEN_PIO_CMD),
 	.flags = AHCI_FLAG_COMMON | ATA_FLAG_NCQ,
 	.pio_mask = ATA_PIO4,
 	.udma_mask = ATA_UDMA6,
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 584da77..2f0daa3 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1592,6 +1592,32 @@ static void ahci_fbs_dec_intr(struct ata_port *ap)
 		dev_err(ap->host->dev, "failed to clear device error\n");
 }

+int ahci_complete_pio_cmd(struct ata_port *ap, u32 cmd_done)
+{
+	struct ata_queued_cmd *qc;
+
+	while (cmd_done) {
+		unsigned int tag = __ffs(cmd_done);
+
+		qc = ata_qc_from_tag(ap, tag);
+		if (qc) {
+			/*
+			 * For the IDENTIFY DEVICE PIO command,
+			 * some controller's unable to clear the BSY bit after
+			 * receiving the PIO Setup FIS from device resulting
+			 * the DMA state to go into CMFatalErrorUpdate state.
+			 * So need to restart the dma engine to get the
+			 * controller out of this state.
+			 */
+			if (qc->tf.command == ATA_CMD_ID_ATA)
+				ahci_restart_engine(ap);
+		}
+		cmd_done &= ~(1 << tag);
+	}
+
+	return 0;
+}
+
 static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
 {
 	struct ahci_host_priv *hpriv = ap->host->private_data;
@@ -1778,6 +1804,14 @@ static void ahci_handle_port_interrupt(struct ata_port *ap,
 	}


+	/* Due to HW errata, some controllers need special handling
+	 * of the completion of some PIO commands.
+	 */
+	if (hpriv->flags & AHCI_HFLAG_BROKEN_PIO_CMD) {
+		u32 cmd_done = ap->qc_active ^ qc_active;
+		ahci_complete_pio_cmd(ap, cmd_done);
+	}
+
 	rc = ata_qc_complete_multiple(ap, qc_active);

 	/* while resetting, invalid completions are expected */
--
1.8.2.1

Re: [PATCH v2 0/2]ata: Fix the dma state machine lockup for APM X-Gene SoC

[Ming Lei]

On Mon, Jun 16, 2014 at 5:35 PM, Suman Tripathi <stripathi@apm.com> wrote:
> Suman Tripathi (2):
>   libahci: Implement the function ahci_restart_engine to restart the
>     port dma engine.
>   ata: Fix the dma state machine lockup for the IDENTIFY DEVICE PIO mode
>     command.
>
>  drivers/ata/ahci.h       |  4 ++++
>  drivers/ata/ahci_xgene.c | 17 +++--------------
>  drivers/ata/libahci.c    | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 51 insertions(+), 14 deletions(-)

V2 looks good and makes my mutang board bootable, thanks.

Reported-and-tested-by: Ming Lei <ming.lei@canonical.com>


Thanks,
--
Ming Lei

Re: [PATCH v2 1/2] libahci: Implement the function ahci_restart_engine to restart the port dma engine.

[Tejun Heo]

Hello,

On Mon, Jun 16, 2014 at 03:05:35PM +0530, Suman Tripathi wrote:
> This patch implements the function ahci_restart_engine function to restart the port dma engine.

Please fit the text under 80 column.

> ---

I can't apply w/o your SOB.

> +int ahci_restart_engine(struct ata_port *ap)
> +{
> +	ahci_stop_engine(ap);
> +	ahci_start_fis_rx(ap);
> +	ahci_start_engine(ap);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(ahci_restart_engine);

Why is this exported?  The next patch doesn't seem to use the function
outside libahci.c proper.

Thanks.

-- 
tejun

Re: [PATCH v2 2/2] ata: Fix the dma state machine lockup for the IDENTIFY DEVICE PIO mode command.

[Tejun Heo]

On Mon, Jun 16, 2014 at 03:05:36PM +0530, Suman Tripathi wrote:
> This patch fixes the dma state machine lockup due to the IDENTIFY DEVICE PIO mode command. The controller is unable to clear the BSY bit after receiving the PIO setup FIS and results the dma state machine to go into the CMFatalErrorUpdate state resulting in the dma state machine lockup.

Ditto on description formatting and SOB.  Also, please describe why
this patch is removing the restart from read_id.

> +int ahci_complete_pio_cmd(struct ata_port *ap, u32 cmd_done)
> +{
> +	struct ata_queued_cmd *qc;
> +
> +	while (cmd_done) {
> +		unsigned int tag = __ffs(cmd_done);
> +
> +		qc = ata_qc_from_tag(ap, tag);
> +		if (qc) {
> +			/*
> +			 * For the IDENTIFY DEVICE PIO command,
> +			 * some controller's unable to clear the BSY bit after
> +			 * receiving the PIO Setup FIS from device resulting
> +			 * the DMA state to go into CMFatalErrorUpdate state.
> +			 * So need to restart the dma engine to get the
> +			 * controller out of this state.
> +			 */
> +			if (qc->tf.command == ATA_CMD_ID_ATA)
> +				ahci_restart_engine(ap);
> +		}
> +		cmd_done &= ~(1 << tag);
> +	}
> +
> +	return 0;
> +}
> +
>  static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
>  {
>  	struct ahci_host_priv *hpriv = ap->host->private_data;
> @@ -1778,6 +1804,14 @@ static void ahci_handle_port_interrupt(struct ata_port *ap,
>  	}
> 
> 
> +	/* Due to HW errata, some controllers need special handling
> +	 * of the completion of some PIO commands.
> +	 */
> +	if (hpriv->flags & AHCI_HFLAG_BROKEN_PIO_CMD) {
> +		u32 cmd_done = ap->qc_active ^ qc_active;
> +		ahci_complete_pio_cmd(ap, cmd_done);
> +	}

Heh, I feel this is a bit too specific to bury in libahci.c and don't
really like adding this sort workaround in the hot path for everybody.
Then again, ahci interrupt handler isn't exactly structured in a way
which is easy to wrap and create a custom one.

Does this have to happen from interrupt path tho?  Can't you just
record the last protocol executed and restart the engine on the next
qc_issue?

Thanks.

-- 
tejun