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

This patch fixes the dma state machine lockup due to the processing
of IDENTIFY DEVICE PIO mode command. The X-Gene AHCI controller
has an errata in which it cannot 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. This patch also removes the dma restart workaround
from the read_id function as the read_id function is only called by
libata layer for ATA_INTERNAL commands. But for somecases eg:
PORT MULTIPLIER and udev, the framework will enumerate using SCSI
commands and it will not call read_id function.

Signed-off-by: Loc Ho <lho@apm.com>
Signed-off-by: Suman Tripathi <stripathi@apm.com>
---
 drivers/ata/ahci.h       |  1 +
 drivers/ata/ahci_xgene.c | 70 +++++++++++++++++++++++++++++++++++++++---------
 drivers/ata/libahci.c    |  3 ++-
 3 files changed, 60 insertions(+), 14 deletions(-)

--
1.8.2.1

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Jun 19, 2014 at 12:20:54PM +0530, Suman Tripathi wrote:
> +	/*
> +	 * Restart the dma engine if the last cmd issued
> +	 * is IDENTIFY DEVICE command
> +	 */
> +	if (unlikely(ctx->last_cmd[ap->port_no] == ATA_CMD_ID_ATA))
> +		ahci_restart_engine(ap);

Is it really only for IDENTIFY?  Are other PIO commands okay?  The
previous version applied it to all PIO commands, right?

Two more things.

On Thu, Jun 19, 2014 at 12:20:54PM +0530, Suman Tripathi wrote:
>  /**
> + * xgene_ahci_qc_issue - Issue commands to the device
> + * @qc: Command to issue
> + *
> + * Due to H/W errata, for the IENTIFY DEVICE command
> + * 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.
> + * By restarting the dma engine to move it removes
> + * the controller out of lock up state.

Please re-flow it to 75th or so column.

> + */
> +static unsigned int xgene_ahci_qc_issue(struct ata_queued_cmd *qc)
> +{
> +	struct ata_port *ap = qc->ap;
> +	void __iomem *port_mmio = ahci_port_base(ap);
> +	struct ahci_port_priv *pp = ap->private_data;
> +	struct ahci_host_priv *hpriv = ap->host->private_data;
> +	struct xgene_ahci_context *ctx = hpriv->plat_data;
> +
> +	/* Keep track of the currently active link.  It will be used
> +	 * in completion path to determine whether NCQ phase is in
> +	 * progress.
> +	 */
> +	pp->active_link = qc->dev->link;
> +
> +	/*
> +	 * Restart the dma engine if the last cmd issued
> +	 * is IDENTIFY DEVICE command
> +	 */
> +	if (unlikely(ctx->last_cmd[ap->port_no] == ATA_CMD_ID_ATA))
> +		ahci_restart_engine(ap);
> +
> +	if (qc->tf.protocol == ATA_PROT_NCQ)
> +		writel(1 << qc->tag, port_mmio + PORT_SCR_ACT);
> +
> +	if (pp->fbs_enabled && pp->fbs_last_dev != qc->dev->link->pmp) {
> +		u32 fbs = readl(port_mmio + PORT_FBS);
> +		fbs &= ~(PORT_FBS_DEV_MASK | PORT_FBS_DEC);
> +		fbs |= qc->dev->link->pmp << PORT_FBS_DEV_OFFSET;
> +		writel(fbs, port_mmio + PORT_FBS);
> +		pp->fbs_last_dev = qc->dev->link->pmp;
> +	}
> +
> +	writel(1 << qc->tag, port_mmio + PORT_CMD_ISSUE);
> +
> +	/* Save the last command issued */
> +	ctx->last_cmd[ap->port_no] = qc->tf.command;
> +
> +	ahci_sw_activity(qc->dev->link);
> +
> +	return 0;
> +}

Why copy the whole function body?  Can't you do the following?

static unsigned int xgene_ahci_qc_issue(struct ata_queued_cmd *qc)
{
	unsigned int ret;

	if (unlikely(last_cmd == IDENTIFY))
		ahci_restart_engine(ap);

	ret = ahci_qc_issue(qc);

	last_cmd = qc->tf.command;

	return ret;
}

Thanks.

On Thu, Jun 19, 2014 at 07:44:28PM +0530, Suman Tripathi wrote:
> Hi Tejun,
> 
> On Thu, Jun 19, 2014 at 12:20:54PM +0530, Suman Tripathi wrote:
> > +     /*
> > +      * Restart the dma engine if the last cmd issued
> > +      * is IDENTIFY DEVICE command
> > +      */
> > +     if (unlikely(ctx->last_cmd[ap->port_no] == ATA_CMD_ID_ATA))
> > +             ahci_restart_engine(ap);
> 
> Is it really only for IDENTIFY?  Are other PIO commands okay?
> [Suman] : We root cause it , It is the IDENTIFY DEVICE command . Other are
> ok
> Theprevious version applied it to all PIO commands, right?
> [suman] : The v2 contains only the IDENTIFY DEVICE. The v1 is contains for
> all PIO commands and that didn't work because the ERRATA mentions that it
> happens for the IDENTIFY DEVICE command.

So, it's just ATA_CMD_ID_ATA and ATA_CMD_ID_ATAPI is okay?  That's
kinda weird.

Hello,

On Thu, Jun 19, 2014 at 07:57:33PM +0530, Suman Tripathi wrote:
> [suman] : Are you ok if I make ahci_qc_issue in the ahci.h as not static
> and make it as EXPORT_SYMBOL_GPL ? Currenty ahci_qc_issue is static.
> If you take the current case , I only had to make  ahci_sw_activity as non
> static and use it. So both the intention is same.

Yeah, sure.  We end up having to export a function anyway.  Better to
avoid duplicating the function body.

> So, it's just ATA_CMD_ID_ATA and ATA_CMD_ID_ATAPI is okay?  That's
> kinda weird.
> 
> [suman] : Not tested for ATA_CMD_ID_ATAPI. It's just for ATA_CMD_ID_ATA for
> now.

Can you give it a test?  The two commands are really similar, so it'd
be surprising if the controller only chokes on one of them.  Another
thing worthwhile testing is "libata.force=noncq,pio0" so that you can
be sure other pio commands are okay.

Thanks.