Re: [PATCH] libata: implement ata_qc_exec_internal()

[Albert Lee <albertcc@tw.ibm.com>]

Tejun Heo wrote:
> Each user libata internal commands initializes a qc, issues it, waits
> for its completion and checks for errors.  This patch factors internal
> command execution sequence into ata_qc_exec_internal().  The function
> also removes race condition between irq and timeout handling.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index f1047a0..40c488e 100644
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -1047,28 +1047,73 @@ static unsigned int ata_pio_modes(const 
>  	return modes;
>  }
>  
> -static int ata_qc_wait_err(struct ata_queued_cmd *qc,
> -			   struct completion *wait)
> +/**
> + *	ata_qc_exec_internal - execute libata internal command
> + *	@qc: Command to execute
> + *
> + *	Executes libata internal command with timeout.  Timeout and
> + *	error conditions are reported via return value.  No recovery
> + *	action is taken after a command times out.  It's caller's duty
> + *	to clean up after timeout.
> + *
> + *	Also, note that error condition is checked after the qc is
> + *	completed, meaning that if another command is issued before
> + *	checking the condition, we get the wrong values.  As internal
> + *	cmds are used only for initialization and recovery, this
> + *	shouldn't cause any problem.
> + *
> + *	LOCKING:
> + *	None.  Should be called with kernel context, might sleep.
> + */
> +
> +static unsigned ata_qc_exec_internal(struct ata_queued_cmd *qc)
>  {
> -	int rc = 0;
> +	struct ata_port *ap = qc->ap;
> +	DECLARE_COMPLETION(wait);
> +	unsigned long tmout_left;
> +	unsigned int err_mask;
> +	int rc;
>  
> -	if (wait_for_completion_timeout(wait, 30 * HZ) < 1) {
> -		/* timeout handling */
> -		unsigned int err_mask = ac_err_mask(ata_chk_status(qc->ap));
> +	if (ata_busy_sleep(ap, ATA_TMOUT_INTERNAL_QUICK, ATA_TMOUT_INTERNAL))
> +		return -ETIMEDOUT;
>  
> -		if (!err_mask) {
> +	qc->complete_fn = ata_qc_complete_noop;
> +	qc->waiting = &wait;
> +
> +	spin_lock_irq(&ap->host_set->lock);
> +	rc = ata_qc_issue(qc);
> +	spin_unlock_irq(&ap->host_set->lock);
> +
> +	if (rc)
> +		return AC_ERR_OTHER;
> +
> +	tmout_left = wait_for_completion_timeout(&wait, ATA_TMOUT_INTERNAL);
> +	err_mask = ac_err_mask(ata_chk_status(qc->ap));

Could we get the err_mask from the qc->complete_fn?
(For some case where the software finds something wrong, but the device ata_chk_status(qc->ap) is zero.)

Albert

> +
> +	if (!tmout_left) {
> +		/* timeout handling */
> +		if (!err_mask)
>  			printk(KERN_WARNING "ata%u: slow completion (cmd %x)\n",
>  			       qc->ap->id, qc->tf.command);
> -		} else {
> +		else
>  			printk(KERN_WARNING "ata%u: qc timeout (cmd %x)\n",
>  			       qc->ap->id, qc->tf.command);
> -			rc = -EIO;
> -		}
>  
> -		ata_qc_complete(qc, err_mask);
> +		spin_lock_irq(&ap->host_set->lock);
> +
> +		/* We're racing with irq here.  If we lose, the
> +		 * following test prevents us from completing the qc
> +		 * again.  If completion irq occurs after here but
> +		 * before the caller cleans up, it will result in a
> +		 * spurious interrupt.  We can live with that.
> +		 */
> +		if (qc->flags & ATA_QCFLAG_ACTIVE)
> +			ata_qc_complete(qc, err_mask);
> +
> +		spin_unlock_irq(&ap->host_set->lock);
>  	}
>  
> -	return rc;
> +	return err_mask;
>  }
>