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

[Tejun Heo <tj@kernel.org>]

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