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

[Tejun Heo <tj@kernel.org>]

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.

-- 
tejun