[PATCH] libata: implement ata_qc_exec_internal()

[PATCH] libata: implement ata_qc_exec_internal()

[Tejun Heo]

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));
+
+	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;
 }
 
 /**
@@ -1100,9 +1145,8 @@ static void ata_dev_identify(struct ata_
 	u16 tmp;
 	unsigned long xfer_modes;
 	unsigned int using_edd;
-	DECLARE_COMPLETION(wait);
 	struct ata_queued_cmd *qc;
-	unsigned long flags;
+	unsigned int err_mask;
 	int rc;
 
 	if (!ata_dev_present(dev)) {
@@ -1140,23 +1184,18 @@ retry:
 		DPRINTK("do ATAPI identify\n");
 	}
 
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
+	err_mask = ata_qc_exec_internal(qc);
 
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	if (err_mask) {
+		unsigned long flags;
 
-	if (rc)
-		goto err_out;
-	else
-		ata_qc_wait_err(qc, &wait);
+		if (err_mask & ~AC_ERR_DEV)
+			goto err_out;
 
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	ap->ops->tf_read(ap, &qc->tf);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+		spin_lock_irqsave(&ap->host_set->lock, flags);
+		ap->ops->tf_read(ap, &qc->tf);
+		spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
-	if (qc->tf.command & ATA_ERR) {
 		/*
 		 * arg!  EDD works for all test cases, but seems to return
 		 * the ATA signature for some ATAPI devices.  Until the
@@ -2288,10 +2327,7 @@ static int ata_choose_xfer_mode(const st
 
 static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev)
 {
-	DECLARE_COMPLETION(wait);
 	struct ata_queued_cmd *qc;
-	int rc;
-	unsigned long flags;
 
 	/* set up set-features taskfile */
 	DPRINTK("set features - xfer mode\n");
@@ -2305,17 +2341,11 @@ static void ata_dev_set_xfermode(struct 
 	qc->tf.protocol = ATA_PROT_NODATA;
 	qc->tf.nsect = dev->xfer_mode;
 
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
-
-	if (rc)
+	if (ata_qc_exec_internal(qc)) {
+		printk(KERN_ERR "ata%u: failed to set xfermode, disabled\n",
+		       ap->id);
 		ata_port_disable(ap);
-	else
-		ata_qc_wait_err(qc, &wait);
+	}
 
 	DPRINTK("EXIT\n");
 }
@@ -2330,10 +2360,7 @@ static void ata_dev_set_xfermode(struct 
 
 static void ata_dev_reread_id(struct ata_port *ap, struct ata_device *dev)
 {
-	DECLARE_COMPLETION(wait);
 	struct ata_queued_cmd *qc;
-	unsigned long flags;
-	int rc;
 
 	qc = ata_qc_new_init(ap, dev);
 	BUG_ON(qc == NULL);
@@ -2353,18 +2380,9 @@ static void ata_dev_reread_id(struct ata
 	qc->tf.protocol = ATA_PROT_PIO;
 	qc->nsect = 1;
 
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
-
-	if (rc)
+	if (ata_qc_exec_internal(qc))
 		goto err_out;
 
-	ata_qc_wait_err(qc, &wait);
-
 	swap_buf_le16(dev->id, ATA_ID_WORDS);
 
 	ata_dump_id(dev);
@@ -2373,6 +2391,7 @@ static void ata_dev_reread_id(struct ata
 
 	return;
 err_out:
+	printk(KERN_ERR "ata%u: failed to reread ID, disabled\n", ap->id);
 	ata_port_disable(ap);
 }
 
@@ -2386,10 +2405,7 @@ err_out:
 
 static void ata_dev_init_params(struct ata_port *ap, struct ata_device *dev)
 {
-	DECLARE_COMPLETION(wait);
 	struct ata_queued_cmd *qc;
-	int rc;
-	unsigned long flags;
 	u16 sectors = dev->id[6];
 	u16 heads   = dev->id[3];
 
@@ -2409,17 +2425,11 @@ static void ata_dev_init_params(struct a
 	qc->tf.nsect = sectors;
 	qc->tf.device |= (heads - 1) & 0x0f; /* max head = num. of heads - 1 */
 
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
-
-	if (rc)
+	if (ata_qc_exec_internal(qc)) {
+		printk(KERN_ERR "ata%u: failed to init parameters, disabled\n",
+		       ap->id);
 		ata_port_disable(ap);
-	else
-		ata_qc_wait_err(qc, &wait);
+	}
 
 	DPRINTK("EXIT\n");
 }
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 9aa10df..2fe64f6 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -136,6 +136,8 @@ enum {
 	ATA_TMOUT_BOOT_QUICK	= 7 * HZ,	/* hueristic */
 	ATA_TMOUT_DATAOUT	= 30 * HZ,
 	ATA_TMOUT_DATAOUT_QUICK	= 5 * HZ,
+	ATA_TMOUT_INTERNAL	= 30 * HZ,
+	ATA_TMOUT_INTERNAL_QUICK = 5 * HZ,
 
 	/* ATA bus states */
 	BUS_UNKNOWN		= 0,

Re: [PATCH] libata: implement ata_qc_exec_internal()

[Raz Ben-Jehuda(caro)]

does it solve the cpu usage ?

On 11/28/05, Tejun Heo <htejun@gmail.com> 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));
> +
> +       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;
>  }
>
>  /**
> @@ -1100,9 +1145,8 @@ static void ata_dev_identify(struct ata_
>         u16 tmp;
>         unsigned long xfer_modes;
>         unsigned int using_edd;
> -       DECLARE_COMPLETION(wait);
>         struct ata_queued_cmd *qc;
> -       unsigned long flags;
> +       unsigned int err_mask;
>         int rc;
>
>         if (!ata_dev_present(dev)) {
> @@ -1140,23 +1184,18 @@ retry:
>                 DPRINTK("do ATAPI identify\n");
>         }
>
> -       qc->waiting = &wait;
> -       qc->complete_fn = ata_qc_complete_noop;
> +       err_mask = ata_qc_exec_internal(qc);
>
> -       spin_lock_irqsave(&ap->host_set->lock, flags);
> -       rc = ata_qc_issue(qc);
> -       spin_unlock_irqrestore(&ap->host_set->lock, flags);
> +       if (err_mask) {
> +               unsigned long flags;
>
> -       if (rc)
> -               goto err_out;
> -       else
> -               ata_qc_wait_err(qc, &wait);
> +               if (err_mask & ~AC_ERR_DEV)
> +                       goto err_out;
>
> -       spin_lock_irqsave(&ap->host_set->lock, flags);
> -       ap->ops->tf_read(ap, &qc->tf);
> -       spin_unlock_irqrestore(&ap->host_set->lock, flags);
> +               spin_lock_irqsave(&ap->host_set->lock, flags);
> +               ap->ops->tf_read(ap, &qc->tf);
> +               spin_unlock_irqrestore(&ap->host_set->lock, flags);
>
> -       if (qc->tf.command & ATA_ERR) {
>                 /*
>                  * arg!  EDD works for all test cases, but seems to return
>                  * the ATA signature for some ATAPI devices.  Until the
> @@ -2288,10 +2327,7 @@ static int ata_choose_xfer_mode(const st
>
>  static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev)
>  {
> -       DECLARE_COMPLETION(wait);
>         struct ata_queued_cmd *qc;
> -       int rc;
> -       unsigned long flags;
>
>         /* set up set-features taskfile */
>         DPRINTK("set features - xfer mode\n");
> @@ -2305,17 +2341,11 @@ static void ata_dev_set_xfermode(struct
>         qc->tf.protocol = ATA_PROT_NODATA;
>         qc->tf.nsect = dev->xfer_mode;
>
> -       qc->waiting = &wait;
> -       qc->complete_fn = ata_qc_complete_noop;
> -
> -       spin_lock_irqsave(&ap->host_set->lock, flags);
> -       rc = ata_qc_issue(qc);
> -       spin_unlock_irqrestore(&ap->host_set->lock, flags);
> -
> -       if (rc)
> +       if (ata_qc_exec_internal(qc)) {
> +               printk(KERN_ERR "ata%u: failed to set xfermode, disabled\n",
> +                      ap->id);
>                 ata_port_disable(ap);
> -       else
> -               ata_qc_wait_err(qc, &wait);
> +       }
>
>         DPRINTK("EXIT\n");
>  }
> @@ -2330,10 +2360,7 @@ static void ata_dev_set_xfermode(struct
>
>  static void ata_dev_reread_id(struct ata_port *ap, struct ata_device *dev)
>  {
> -       DECLARE_COMPLETION(wait);
>         struct ata_queued_cmd *qc;
> -       unsigned long flags;
> -       int rc;
>
>         qc = ata_qc_new_init(ap, dev);
>         BUG_ON(qc == NULL);
> @@ -2353,18 +2380,9 @@ static void ata_dev_reread_id(struct ata
>         qc->tf.protocol = ATA_PROT_PIO;
>         qc->nsect = 1;
>
> -       qc->waiting = &wait;
> -       qc->complete_fn = ata_qc_complete_noop;
> -
> -       spin_lock_irqsave(&ap->host_set->lock, flags);
> -       rc = ata_qc_issue(qc);
> -       spin_unlock_irqrestore(&ap->host_set->lock, flags);
> -
> -       if (rc)
> +       if (ata_qc_exec_internal(qc))
>                 goto err_out;
>
> -       ata_qc_wait_err(qc, &wait);
> -
>         swap_buf_le16(dev->id, ATA_ID_WORDS);
>
>         ata_dump_id(dev);
> @@ -2373,6 +2391,7 @@ static void ata_dev_reread_id(struct ata
>
>         return;
>  err_out:
> +       printk(KERN_ERR "ata%u: failed to reread ID, disabled\n", ap->id);
>         ata_port_disable(ap);
>  }
>
> @@ -2386,10 +2405,7 @@ err_out:
>
>  static void ata_dev_init_params(struct ata_port *ap, struct ata_device *dev)
>  {
> -       DECLARE_COMPLETION(wait);
>         struct ata_queued_cmd *qc;
> -       int rc;
> -       unsigned long flags;
>         u16 sectors = dev->id[6];
>         u16 heads   = dev->id[3];
>
> @@ -2409,17 +2425,11 @@ static void ata_dev_init_params(struct a
>         qc->tf.nsect = sectors;
>         qc->tf.device |= (heads - 1) & 0x0f; /* max head = num. of heads - 1 */
>
> -       qc->waiting = &wait;
> -       qc->complete_fn = ata_qc_complete_noop;
> -
> -       spin_lock_irqsave(&ap->host_set->lock, flags);
> -       rc = ata_qc_issue(qc);
> -       spin_unlock_irqrestore(&ap->host_set->lock, flags);
> -
> -       if (rc)
> +       if (ata_qc_exec_internal(qc)) {
> +               printk(KERN_ERR "ata%u: failed to init parameters, disabled\n",
> +                      ap->id);
>                 ata_port_disable(ap);
> -       else
> -               ata_qc_wait_err(qc, &wait);
> +       }
>
>         DPRINTK("EXIT\n");
>  }
> diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 9aa10df..2fe64f6 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -136,6 +136,8 @@ enum {
>         ATA_TMOUT_BOOT_QUICK    = 7 * HZ,       /* hueristic */
>         ATA_TMOUT_DATAOUT       = 30 * HZ,
>         ATA_TMOUT_DATAOUT_QUICK = 5 * HZ,
> +       ATA_TMOUT_INTERNAL      = 30 * HZ,
> +       ATA_TMOUT_INTERNAL_QUICK = 5 * HZ,
>
>         /* ATA bus states */
>         BUS_UNKNOWN             = 0,
> -
> 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
>


--
Raz

Re: [PATCH] libata: implement ata_qc_exec_internal()

[Tejun Heo]

Raz Ben-Jehuda(caro) wrote:
> does it solve the cpu usage ?
> 
> On 11/28/05, Tejun Heo <htejun@gmail.com> 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>
>>

Hi, Raz.

It propabably has nothing to do with the cpu usage problem on piix you 
reported.  I think you need to post more data to get people on the list 
interested in your problem.  The followings may help.

1. Please report exactly which kernel you're using and more information 
on the hardware will also help.

2. Don't use raid or any fs on it.  Just perform tests on raw /dev/sdx 
devices.  Do something like 'time dd if=/dev/sdx of=/dev/zero bs=1M 
count=1024' and report how much user/system time it uses.

3. If above shows large system time consumption, build your kernel with 
oprofile.  Start recording non-idle cpu cycles (the default) including 
the kernel.  Repeat test in #2.  Dump oprofile result and post that here.

Hope it helps.

-- 
tejun

Re: [PATCH] libata: implement ata_qc_exec_internal()

[Tejun Heo]

Tejun Heo wrote:
> Raz Ben-Jehuda(caro) wrote:
> 
>> does it solve the cpu usage ?
>>
>> On 11/28/05, Tejun Heo <htejun@gmail.com> 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>
>>>
> 
> Hi, Raz.
> 
> It propabably has nothing to do with the cpu usage problem on piix you 
> reported.  I think you need to post more data to get people on the list 
> interested in your problem.  The followings may help.
> 
> 1. Please report exactly which kernel you're using and more information 
> on the hardware will also help.
> 
> 2. Don't use raid or any fs on it.  Just perform tests on raw /dev/sdx 
> devices.  Do something like 'time dd if=/dev/sdx of=/dev/zero bs=1M 
> count=1024' and report how much user/system time it uses.

Oops, count=1024 is way too small.  Do something like bs=1M count=1M or 
maybe count=10M.

> 
> 3. If above shows large system time consumption, build your kernel with 
> oprofile.  Start recording non-idle cpu cycles (the default) including 
> the kernel.  Repeat test in #2.  Dump oprofile result and post that here.
> 
> Hope it helps.
> 


-- 
tejun

Re: [PATCH] libata: implement ata_qc_exec_internal()

[Tejun Heo]

Tejun Heo wrote:
> Tejun Heo wrote:
> 
>> Raz Ben-Jehuda(caro) wrote:
>>
>>> does it solve the cpu usage ?
>>>
>>> On 11/28/05, Tejun Heo <htejun@gmail.com> 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>
>>>>
>>
>> Hi, Raz.
>>
>> It propabably has nothing to do with the cpu usage problem on piix you 
>> reported.  I think you need to post more data to get people on the 
>> list interested in your problem.  The followings may help.
>>
>> 1. Please report exactly which kernel you're using and more 
>> information on the hardware will also help.
>>
>> 2. Don't use raid or any fs on it.  Just perform tests on raw /dev/sdx 
>> devices.  Do something like 'time dd if=/dev/sdx of=/dev/zero bs=1M 
>> count=1024' and report how much user/system time it uses.
> 
> 
> Oops, count=1024 is way too small.  Do something like bs=1M count=1M or 
> maybe count=10M.

I hate myself.  I must be on crack or something now.  count=1024 should 
be fine (count=2k or 4k should be fine too).  Don't do count=1M or 10M.

Sorry guys.  :-)

-- 
tejun

Re: [PATCH] libata: implement ata_qc_exec_internal()

[Albert Lee]

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;
>  }
>  

[PATCH 1/2] libata: implement ata_exec_internal()

[Tejun Heo]

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().  This change
fixes the following bugs.

* qc not freed on issue failure
* ap->qactive clearing could race with the next internal command
* race between timeout handling and irq
* ignoring error condition not represented in tf->status

Also, qc & hardware are not accessed anymore once it's completed,
making internal commands more conformant with general semantics.  This
change also makes it easy to issue internal commands from multiple
threads if that becomes necessary.

Signed-off-by: Tejun Heo <htejun@gmail.com>

--

This is take #2 of ata_exec_internal().  qc allocation and tf
reading-back have also been moved into ata_exec_internal().  Now all
qc handling is done while properly holding host_set lock removing
above mentioned rare race conditon (the second item).

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2005-11-29 22:04:15.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2005-11-29 22:06:20.000000000 +0900
@@ -1047,28 +1047,119 @@ static unsigned int ata_pio_modes(const 
 	return modes;
 }
 
-static int ata_qc_wait_err(struct ata_queued_cmd *qc,
-			   struct completion *wait)
+struct ata_exec_internal_arg {
+	unsigned int err_mask;
+	struct ata_taskfile *tf;
+	struct completion *waiting;
+};
+
+int ata_qc_complete_internal(struct ata_queued_cmd *qc, unsigned int err_mask)
 {
-	int rc = 0;
+	struct ata_exec_internal_arg *arg = qc->complete_data;
+	struct completion *waiting = arg->waiting;
 
-	if (wait_for_completion_timeout(wait, 30 * HZ) < 1) {
-		/* timeout handling */
-		unsigned int err_mask = ac_err_mask(ata_chk_status(qc->ap));
-
-		if (!err_mask) {
-			printk(KERN_WARNING "ata%u: slow completion (cmd %x)\n",
-			       qc->ap->id, qc->tf.command);
-		} else {
-			printk(KERN_WARNING "ata%u: qc timeout (cmd %x)\n",
-			       qc->ap->id, qc->tf.command);
-			rc = -EIO;
-		}
+	if (!(err_mask & ~AC_ERR_DEV))
+		qc->ap->ops->tf_read(qc->ap, arg->tf);
+	arg->err_mask = err_mask;
+	arg->waiting = NULL;
+	complete(waiting);
+
+	return 0;
+}
+
+/**
+ *	ata_exec_internal - execute libata internal command
+ *	@ap: Port to which the command is sent
+ *	@dev: Device to which the command is sent
+ *	@tf: Taskfile registers for the command and the result
+ *	@dma_dir: Data tranfer direction of the command
+ *	@buf: Data buffer of the command
+ *	@buflen: Length of data buffer
+ *
+ *	Executes libata internal command with timeout.  @tf contains
+ *	command on entry and result on return.  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.
+ *
+ *	LOCKING:
+ *	None.  Should be called with kernel context, might sleep.
+ */
+
+static unsigned
+ata_exec_internal(struct ata_port *ap, struct ata_device *dev,
+		  struct ata_taskfile *tf,
+		  int dma_dir, void *buf, unsigned int buflen)
+{
+	u8 command = tf->command;
+	struct ata_queued_cmd *qc;
+	DECLARE_COMPLETION(wait);
+	unsigned long flags;
+	struct ata_exec_internal_arg arg;
+
+	if (ata_busy_sleep(ap, ATA_TMOUT_INTERNAL_QUICK, ATA_TMOUT_INTERNAL))
+		return AC_ERR_OTHER;
+
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+
+	qc = ata_qc_new_init(ap, dev);
+	BUG_ON(qc == NULL);
+
+	qc->tf = *tf;
+	qc->dma_dir = dma_dir;
+	if (dma_dir != DMA_NONE) {
+		ata_sg_init_one(qc, buf, buflen);
+		qc->nsect = buflen / ATA_SECT_SIZE;
+		printk("dma_dir=%d buflen=%u nsect=%d\n",
+		       dma_dir, buflen, qc->nsect);
+	}
+
+	arg.waiting = &wait;
+	arg.tf = tf;
+	qc->complete_data = &arg;
+	qc->complete_fn = ata_qc_complete_internal;
+
+	if (ata_qc_issue(qc))
+		goto issue_fail;
+
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+	if (!wait_for_completion_timeout(&wait, ATA_TMOUT_INTERNAL)) {
+		int timedout;
+
+		spin_lock_irqsave(&ap->host_set->lock, flags);
+
+		/* 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.
+		 */
+		timedout = arg.waiting != NULL;
+		if (timedout)
+			ata_qc_complete(qc, 0);
+
+		spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
-		ata_qc_complete(qc, err_mask);
+		if (timedout) {
+			arg.err_mask = ac_err_mask(tf->command);
+			if (!arg.err_mask)
+				printk(KERN_WARNING
+				       "ata%u: slow completion (cmd %x)\n",
+				       ap->id, command);
+			else
+				printk(KERN_WARNING
+				       "ata%u: qc timeout (cmd %x)\n",
+				       ap->id, command);
+		}
 	}
 
-	return rc;
+	return arg.err_mask;
+
+ issue_fail:
+	ata_qc_free(qc);
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	return AC_ERR_OTHER;
 }
 
 /**
@@ -1100,9 +1191,8 @@ static void ata_dev_identify(struct ata_
 	u16 tmp;
 	unsigned long xfer_modes;
 	unsigned int using_edd;
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	unsigned long flags;
+	struct ata_taskfile tf;
+	unsigned int err_mask;
 	int rc;
 
 	if (!ata_dev_present(dev)) {
@@ -1123,40 +1213,26 @@ static void ata_dev_identify(struct ata_
 
 	ata_dev_select(ap, device, 1, 1); /* select device 0/1 */
 
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
-
-	ata_sg_init_one(qc, dev->id, sizeof(dev->id));
-	qc->dma_dir = DMA_FROM_DEVICE;
-	qc->tf.protocol = ATA_PROT_PIO;
-	qc->nsect = 1;
-
 retry:
+	ata_tf_init(ap, &tf, device);
+
 	if (dev->class == ATA_DEV_ATA) {
-		qc->tf.command = ATA_CMD_ID_ATA;
+		tf.command = ATA_CMD_ID_ATA;
 		DPRINTK("do ATA identify\n");
 	} else {
-		qc->tf.command = ATA_CMD_ID_ATAPI;
+		tf.command = ATA_CMD_ID_ATAPI;
 		DPRINTK("do ATAPI identify\n");
 	}
 
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
+	tf.protocol = ATA_PROT_PIO;
 
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	err_mask = ata_exec_internal(ap, dev, &tf, DMA_FROM_DEVICE,
+				     dev->id, sizeof(dev->id));
 
-	if (rc)
-		goto err_out;
-	else
-		ata_qc_wait_err(qc, &wait);
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	ap->ops->tf_read(ap, &qc->tf);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	if (err_mask) {
+		if (err_mask & ~AC_ERR_DEV)
+			goto err_out;
 
-	if (qc->tf.command & ATA_ERR) {
 		/*
 		 * arg!  EDD works for all test cases, but seems to return
 		 * the ATA signature for some ATAPI devices.  Until the
@@ -1169,13 +1245,9 @@ retry:
 		 * to have this problem.
 		 */
 		if ((using_edd) && (dev->class == ATA_DEV_ATA)) {
-			u8 err = qc->tf.feature;
+			u8 err = tf.feature;
 			if (err & ATA_ABORTED) {
 				dev->class = ATA_DEV_ATAPI;
-				qc->cursg = 0;
-				qc->cursg_ofs = 0;
-				qc->cursect = 0;
-				qc->nsect = 1;
 				goto retry;
 			}
 		}
@@ -2288,34 +2360,23 @@ static int ata_choose_xfer_mode(const st
 
 static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev)
 {
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	int rc;
-	unsigned long flags;
+	struct ata_taskfile tf;
 
 	/* set up set-features taskfile */
 	DPRINTK("set features - xfer mode\n");
 
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
-
-	qc->tf.command = ATA_CMD_SET_FEATURES;
-	qc->tf.feature = SETFEATURES_XFER;
-	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-	qc->tf.protocol = ATA_PROT_NODATA;
-	qc->tf.nsect = dev->xfer_mode;
-
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	ata_tf_init(ap, &tf, dev->devno);
+	tf.command = ATA_CMD_SET_FEATURES;
+	tf.feature = SETFEATURES_XFER;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.nsect = dev->xfer_mode;
 
-	if (rc)
+	if (ata_exec_internal(ap, dev, &tf, DMA_NONE, NULL, 0)) {
+		printk(KERN_ERR "ata%u: failed to set xfermode, disabled\n",
+		       ap->id);
 		ata_port_disable(ap);
-	else
-		ata_qc_wait_err(qc, &wait);
+	}
 
 	DPRINTK("EXIT\n");
 }
@@ -2330,41 +2391,25 @@ static void ata_dev_set_xfermode(struct 
 
 static void ata_dev_reread_id(struct ata_port *ap, struct ata_device *dev)
 {
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	unsigned long flags;
-	int rc;
-
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
+	struct ata_taskfile tf;
 
-	ata_sg_init_one(qc, dev->id, sizeof(dev->id));
-	qc->dma_dir = DMA_FROM_DEVICE;
+	ata_tf_init(ap, &tf, dev->devno);
 
 	if (dev->class == ATA_DEV_ATA) {
-		qc->tf.command = ATA_CMD_ID_ATA;
+		tf.command = ATA_CMD_ID_ATA;
 		DPRINTK("do ATA identify\n");
 	} else {
-		qc->tf.command = ATA_CMD_ID_ATAPI;
+		tf.command = ATA_CMD_ID_ATAPI;
 		DPRINTK("do ATAPI identify\n");
 	}
 
-	qc->tf.flags |= ATA_TFLAG_DEVICE;
-	qc->tf.protocol = ATA_PROT_PIO;
-	qc->nsect = 1;
-
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
+	tf.flags |= ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_PIO;
 
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
-
-	if (rc)
+	if (ata_exec_internal(ap, dev, &tf, DMA_FROM_DEVICE,
+			      dev->id, sizeof(dev->id)))
 		goto err_out;
 
-	ata_qc_wait_err(qc, &wait);
-
 	swap_buf_le16(dev->id, ATA_ID_WORDS);
 
 	ata_dump_id(dev);
@@ -2373,6 +2418,7 @@ static void ata_dev_reread_id(struct ata
 
 	return;
 err_out:
+	printk(KERN_ERR "ata%u: failed to reread ID, disabled\n", ap->id);
 	ata_port_disable(ap);
 }
 
@@ -2386,10 +2432,7 @@ err_out:
 
 static void ata_dev_init_params(struct ata_port *ap, struct ata_device *dev)
 {
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	int rc;
-	unsigned long flags;
+	struct ata_taskfile tf;
 	u16 sectors = dev->id[6];
 	u16 heads   = dev->id[3];
 
@@ -2400,26 +2443,18 @@ static void ata_dev_init_params(struct a
 	/* set up init dev params taskfile */
 	DPRINTK("init dev params \n");
 
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
-
-	qc->tf.command = ATA_CMD_INIT_DEV_PARAMS;
-	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-	qc->tf.protocol = ATA_PROT_NODATA;
-	qc->tf.nsect = sectors;
-	qc->tf.device |= (heads - 1) & 0x0f; /* max head = num. of heads - 1 */
-
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	ata_tf_init(ap, &tf, dev->devno);
+	tf.command = ATA_CMD_INIT_DEV_PARAMS;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.nsect = sectors;
+	tf.device |= (heads - 1) & 0x0f; /* max head = num. of heads - 1 */
 
-	if (rc)
+	if (ata_exec_internal(ap, dev, &tf, DMA_NONE, NULL, 0)) {
+		printk(KERN_ERR "ata%u: failed to init parameters, disabled\n",
+		       ap->id);
 		ata_port_disable(ap);
-	else
-		ata_qc_wait_err(qc, &wait);
+	}
 
 	DPRINTK("EXIT\n");
 }
@@ -3641,11 +3676,6 @@ struct ata_queued_cmd *ata_qc_new_init(s
 	return qc;
 }
 
-int ata_qc_complete_noop(struct ata_queued_cmd *qc, unsigned int err_mask)
-{
-	return 0;
-}
-
 static void __ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
Index: work/drivers/scsi/libata.h
===================================================================
--- work.orig/drivers/scsi/libata.h	2005-11-29 22:04:15.000000000 +0900
+++ work/drivers/scsi/libata.h	2005-11-29 22:05:26.000000000 +0900
@@ -39,7 +39,6 @@ struct ata_scsi_args {
 
 /* libata-core.c */
 extern int atapi_enabled;
-extern int ata_qc_complete_noop(struct ata_queued_cmd *qc, unsigned int err_mask);
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
 				      struct ata_device *dev);
 extern void ata_rwcmd_protocol(struct ata_queued_cmd *qc);
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h	2005-11-29 22:04:15.000000000 +0900
+++ work/include/linux/libata.h	2005-11-29 22:06:20.000000000 +0900
@@ -136,6 +136,8 @@ enum {
 	ATA_TMOUT_BOOT_QUICK	= 7 * HZ,	/* hueristic */
 	ATA_TMOUT_DATAOUT	= 30 * HZ,
 	ATA_TMOUT_DATAOUT_QUICK	= 5 * HZ,
+	ATA_TMOUT_INTERNAL	= 30 * HZ,
+	ATA_TMOUT_INTERNAL_QUICK = 5 * HZ,
 
 	/* ATA bus states */
 	BUS_UNKNOWN		= 0,
@@ -284,6 +286,7 @@ struct ata_queued_cmd {
 	struct scatterlist	*__sg;
 
 	ata_qc_cb_t		complete_fn;
+	void			*complete_data;
 
 	struct completion	*waiting;
 

[PATCH 2/2] libata: remove qc->waiting

[Tejun Heo]

With internal commands converted to use ata_exec_internal(), there is
no user of qc->waiting left.  Kill it.

Signed-off-by: Tejun Heo <htejun@gmail.com>

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2005-11-29 22:06:20.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2005-11-29 22:06:38.000000000 +0900
@@ -3679,7 +3679,7 @@ struct ata_queued_cmd *ata_qc_new_init(s
 static void __ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
-	unsigned int tag, do_clear = 0;
+	unsigned int tag;
 
 	qc->flags = 0;
 	tag = qc->tag;
@@ -3687,17 +3687,8 @@ static void __ata_qc_complete(struct ata
 		if (tag == ap->active_tag)
 			ap->active_tag = ATA_TAG_POISON;
 		qc->tag = ATA_TAG_POISON;
-		do_clear = 1;
-	}
-
-	if (qc->waiting) {
-		struct completion *waiting = qc->waiting;
-		qc->waiting = NULL;
-		complete(waiting);
-	}
-
-	if (likely(do_clear))
 		clear_bit(tag, &ap->qactive);
+	}
 }
 
 /**
@@ -3713,7 +3704,6 @@ static void __ata_qc_complete(struct ata
 void ata_qc_free(struct ata_queued_cmd *qc)
 {
 	assert(qc != NULL);	/* ata_qc_from_tag _might_ return NULL */
-	assert(qc->waiting == NULL);	/* nothing should be waiting */
 
 	__ata_qc_complete(qc);
 }
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h	2005-11-29 22:06:20.000000000 +0900
+++ work/include/linux/libata.h	2005-11-29 22:06:38.000000000 +0900
@@ -288,8 +288,6 @@ struct ata_queued_cmd {
 	ata_qc_cb_t		complete_fn;
 	void			*complete_data;
 
-	struct completion	*waiting;
-
 	void			*private_data;
 };
 

Re: [PATCH 1/2] libata: implement ata_exec_internal()

[Jeff Garzik]

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().  This change
> fixes the following bugs.
> 
> * qc not freed on issue failure
> * ap->qactive clearing could race with the next internal command
> * race between timeout handling and irq
> * ignoring error condition not represented in tf->status
> 
> Also, qc & hardware are not accessed anymore once it's completed,
> making internal commands more conformant with general semantics.  This
> change also makes it easy to issue internal commands from multiple
> threads if that becomes necessary.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

This is OK in concept, but there are problems in implementation...

General comment:  would be nice split up as 'implement 
ata.exec.internal' and 'use ata.exec.internal' patches.


> This is take #2 of ata_exec_internal().  qc allocation and tf
> reading-back have also been moved into ata_exec_internal().  Now all
> qc handling is done while properly holding host_set lock removing
> above mentioned rare race conditon (the second item).
> 
> Index: work/drivers/scsi/libata-core.c
> ===================================================================
> --- work.orig/drivers/scsi/libata-core.c	2005-11-29 22:04:15.000000000 +0900
> +++ work/drivers/scsi/libata-core.c	2005-11-29 22:06:20.000000000 +0900
> @@ -1047,28 +1047,119 @@ static unsigned int ata_pio_modes(const 
>  	return modes;
>  }
>  
> -static int ata_qc_wait_err(struct ata_queued_cmd *qc,
> -			   struct completion *wait)
> +struct ata_exec_internal_arg {
> +	unsigned int err_mask;
> +	struct ata_taskfile *tf;
> +	struct completion *waiting;
> +};
> +
> +int ata_qc_complete_internal(struct ata_queued_cmd *qc, unsigned int err_mask)
>  {
> -	int rc = 0;
> +	struct ata_exec_internal_arg *arg = qc->complete_data;

This is what private_data is for.  No need to add 'complete_data'.


> +	struct completion *waiting = arg->waiting;
>  
> -	if (wait_for_completion_timeout(wait, 30 * HZ) < 1) {
> -		/* timeout handling */
> -		unsigned int err_mask = ac_err_mask(ata_chk_status(qc->ap));
> -
> -		if (!err_mask) {
> -			printk(KERN_WARNING "ata%u: slow completion (cmd %x)\n",
> -			       qc->ap->id, qc->tf.command);
> -		} else {
> -			printk(KERN_WARNING "ata%u: qc timeout (cmd %x)\n",
> -			       qc->ap->id, qc->tf.command);
> -			rc = -EIO;
> -		}
> +	if (!(err_mask & ~AC_ERR_DEV))
> +		qc->ap->ops->tf_read(qc->ap, arg->tf);
> +	arg->err_mask = err_mask;
> +	arg->waiting = NULL;
> +	complete(waiting);
> +
> +	return 0;
> +}
> +
> +/**
> + *	ata_exec_internal - execute libata internal command
> + *	@ap: Port to which the command is sent
> + *	@dev: Device to which the command is sent
> + *	@tf: Taskfile registers for the command and the result
> + *	@dma_dir: Data tranfer direction of the command
> + *	@buf: Data buffer of the command
> + *	@buflen: Length of data buffer
> + *
> + *	Executes libata internal command with timeout.  @tf contains
> + *	command on entry and result on return.  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.
> + *
> + *	LOCKING:
> + *	None.  Should be called with kernel context, might sleep.
> + */
> +
> +static unsigned
> +ata_exec_internal(struct ata_port *ap, struct ata_device *dev,
> +		  struct ata_taskfile *tf,
> +		  int dma_dir, void *buf, unsigned int buflen)
> +{
> +	u8 command = tf->command;
> +	struct ata_queued_cmd *qc;
> +	DECLARE_COMPLETION(wait);
> +	unsigned long flags;
> +	struct ata_exec_internal_arg arg;
> +
> +	if (ata_busy_sleep(ap, ATA_TMOUT_INTERNAL_QUICK, ATA_TMOUT_INTERNAL))
> +		return AC_ERR_OTHER;

NAK.  This sort of thing really belongs in the execution engine for each 
driver.  Advanced controllers have zero need for this.

Moreover, why are you adding this, anyway?  This is a distinct 
regression from the nice "do everything inside ata_qc_issue" model 
that's been sprouting.


> +	spin_lock_irqsave(&ap->host_set->lock, flags);
> +
> +	qc = ata_qc_new_init(ap, dev);
> +	BUG_ON(qc == NULL);
> +
> +	qc->tf = *tf;
> +	qc->dma_dir = dma_dir;
> +	if (dma_dir != DMA_NONE) {
> +		ata_sg_init_one(qc, buf, buflen);
> +		qc->nsect = buflen / ATA_SECT_SIZE;
> +		printk("dma_dir=%d buflen=%u nsect=%d\n",
> +		       dma_dir, buflen, qc->nsect);
> +	}
> +
> +	arg.waiting = &wait;
> +	arg.tf = tf;
> +	qc->complete_data = &arg;
> +	qc->complete_fn = ata_qc_complete_internal;
> +
> +	if (ata_qc_issue(qc))
> +		goto issue_fail;
> +
> +	spin_unlock_irqrestore(&ap->host_set->lock, flags);
> +
> +	if (!wait_for_completion_timeout(&wait, ATA_TMOUT_INTERNAL)) {
> +		int timedout;
> +
> +		spin_lock_irqsave(&ap->host_set->lock, flags);
> +
> +		/* 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.
> +		 */
> +		timedout = arg.waiting != NULL;
> +		if (timedout)
> +			ata_qc_complete(qc, 0);

This is not very wise, to pass 'nothing is wrong' to ata_qc_complete() 
upon timeout.


> +		spin_unlock_irqrestore(&ap->host_set->lock, flags);
>  
> -		ata_qc_complete(qc, err_mask);
> +		if (timedout) {
> +			arg.err_mask = ac_err_mask(tf->command);
> +			if (!arg.err_mask)
> +				printk(KERN_WARNING
> +				       "ata%u: slow completion (cmd %x)\n",
> +				       ap->id, command);
> +			else
> +				printk(KERN_WARNING
> +				       "ata%u: qc timeout (cmd %x)\n",
> +				       ap->id, command);
> +		}
>  	}
>  
> -	return rc;
> +	return arg.err_mask;
> +
> + issue_fail:
> +	ata_qc_free(qc);
> +	spin_unlock_irqrestore(&ap->host_set->lock, flags);
> +	return AC_ERR_OTHER;
>  }
>  
>  /**
> @@ -1100,9 +1191,8 @@ static void ata_dev_identify(struct ata_
>  	u16 tmp;
>  	unsigned long xfer_modes;
>  	unsigned int using_edd;
> -	DECLARE_COMPLETION(wait);
> -	struct ata_queued_cmd *qc;
> -	unsigned long flags;
> +	struct ata_taskfile tf;
> +	unsigned int err_mask;
>  	int rc;
>  
>  	if (!ata_dev_present(dev)) {
> @@ -1123,40 +1213,26 @@ static void ata_dev_identify(struct ata_
>  
>  	ata_dev_select(ap, device, 1, 1); /* select device 0/1 */
>  
> -	qc = ata_qc_new_init(ap, dev);
> -	BUG_ON(qc == NULL);
> -
> -	ata_sg_init_one(qc, dev->id, sizeof(dev->id));
> -	qc->dma_dir = DMA_FROM_DEVICE;
> -	qc->tf.protocol = ATA_PROT_PIO;
> -	qc->nsect = 1;
> -
>  retry:
> +	ata_tf_init(ap, &tf, device);
>  	if (dev->class == ATA_DEV_ATA) {
> -		qc->tf.command = ATA_CMD_ID_ATA;
> +		tf.command = ATA_CMD_ID_ATA;
>  		DPRINTK("do ATA identify\n");
>  	} else {
> -		qc->tf.command = ATA_CMD_ID_ATAPI;
> +		tf.command = ATA_CMD_ID_ATAPI;
>  		DPRINTK("do ATAPI identify\n");
>  	}
>  
> -	qc->waiting = &wait;
> -	qc->complete_fn = ata_qc_complete_noop;
> +	tf.protocol = ATA_PROT_PIO;
>  
> -	spin_lock_irqsave(&ap->host_set->lock, flags);
> -	rc = ata_qc_issue(qc);
> -	spin_unlock_irqrestore(&ap->host_set->lock, flags);
> +	err_mask = ata_exec_internal(ap, dev, &tf, DMA_FROM_DEVICE,
> +				     dev->id, sizeof(dev->id));
>  
> -	if (rc)
> -		goto err_out;
> -	else
> -		ata_qc_wait_err(qc, &wait);
> -
> -	spin_lock_irqsave(&ap->host_set->lock, flags);
> -	ap->ops->tf_read(ap, &qc->tf);
> -	spin_unlock_irqrestore(&ap->host_set->lock, flags);
> +	if (err_mask) {
> +		if (err_mask & ~AC_ERR_DEV)
> +			goto err_out;
>  
> -	if (qc->tf.command & ATA_ERR) {
>  		/*
>  		 * arg!  EDD works for all test cases, but seems to return
>  		 * the ATA signature for some ATAPI devices.  Until the
> @@ -1169,13 +1245,9 @@ retry:
>  		 * to have this problem.
>  		 */
>  		if ((using_edd) && (dev->class == ATA_DEV_ATA)) {
> -			u8 err = qc->tf.feature;
> +			u8 err = tf.feature;
>  			if (err & ATA_ABORTED) {
>  				dev->class = ATA_DEV_ATAPI;
> -				qc->cursg = 0;
> -				qc->cursg_ofs = 0;
> -				qc->cursect = 0;
> -				qc->nsect = 1;
>  				goto retry;
>  			}
>  		}
> @@ -2288,34 +2360,23 @@ static int ata_choose_xfer_mode(const st
>  
>  static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev)
>  {
> -	DECLARE_COMPLETION(wait);
> -	struct ata_queued_cmd *qc;
> -	int rc;
> -	unsigned long flags;
> +	struct ata_taskfile tf;
>  
>  	/* set up set-features taskfile */
>  	DPRINTK("set features - xfer mode\n");
>  
> -	qc = ata_qc_new_init(ap, dev);
> -	BUG_ON(qc == NULL);
> -
> -	qc->tf.command = ATA_CMD_SET_FEATURES;
> -	qc->tf.feature = SETFEATURES_XFER;
> -	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
> -	qc->tf.protocol = ATA_PROT_NODATA;
> -	qc->tf.nsect = dev->xfer_mode;
> -
> -	qc->waiting = &wait;
> -	qc->complete_fn = ata_qc_complete_noop;
> -
> -	spin_lock_irqsave(&ap->host_set->lock, flags);
> -	rc = ata_qc_issue(qc);
> -	spin_unlock_irqrestore(&ap->host_set->lock, flags);
> +	ata_tf_init(ap, &tf, dev->devno);
> +	tf.command = ATA_CMD_SET_FEATURES;
> +	tf.feature = SETFEATURES_XFER;
> +	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
> +	tf.protocol = ATA_PROT_NODATA;
> +	tf.nsect = dev->xfer_mode;
>  
> -	if (rc)
> +	if (ata_exec_internal(ap, dev, &tf, DMA_NONE, NULL, 0)) {
> +		printk(KERN_ERR "ata%u: failed to set xfermode, disabled\n",
> +		       ap->id);
>  		ata_port_disable(ap);
> -	else
> -		ata_qc_wait_err(qc, &wait);
> +	}
>  
>  	DPRINTK("EXIT\n");
>  }
> @@ -2330,41 +2391,25 @@ static void ata_dev_set_xfermode(struct 
>  
>  static void ata_dev_reread_id(struct ata_port *ap, struct ata_device *dev)
>  {
> -	DECLARE_COMPLETION(wait);
> -	struct ata_queued_cmd *qc;
> -	unsigned long flags;
> -	int rc;
> -
> -	qc = ata_qc_new_init(ap, dev);
> -	BUG_ON(qc == NULL);
> +	struct ata_taskfile tf;
>  
> -	ata_sg_init_one(qc, dev->id, sizeof(dev->id));
> -	qc->dma_dir = DMA_FROM_DEVICE;
> +	ata_tf_init(ap, &tf, dev->devno);
>  
>  	if (dev->class == ATA_DEV_ATA) {
> -		qc->tf.command = ATA_CMD_ID_ATA;
> +		tf.command = ATA_CMD_ID_ATA;
>  		DPRINTK("do ATA identify\n");
>  	} else {
> -		qc->tf.command = ATA_CMD_ID_ATAPI;
> +		tf.command = ATA_CMD_ID_ATAPI;
>  		DPRINTK("do ATAPI identify\n");
>  	}
>  
> -	qc->tf.flags |= ATA_TFLAG_DEVICE;
> -	qc->tf.protocol = ATA_PROT_PIO;
> -	qc->nsect = 1;
> -
> -	qc->waiting = &wait;
> -	qc->complete_fn = ata_qc_complete_noop;
> +	tf.flags |= ATA_TFLAG_DEVICE;
> +	tf.protocol = ATA_PROT_PIO;
>  
> -	spin_lock_irqsave(&ap->host_set->lock, flags);
> -	rc = ata_qc_issue(qc);
> -	spin_unlock_irqrestore(&ap->host_set->lock, flags);
> -
> -	if (rc)
> +	if (ata_exec_internal(ap, dev, &tf, DMA_FROM_DEVICE,
> +			      dev->id, sizeof(dev->id)))
>  		goto err_out;
>  
> -	ata_qc_wait_err(qc, &wait);
> -
>  	swap_buf_le16(dev->id, ATA_ID_WORDS);
>  
>  	ata_dump_id(dev);
> @@ -2373,6 +2418,7 @@ static void ata_dev_reread_id(struct ata
>  
>  	return;
>  err_out:
> +	printk(KERN_ERR "ata%u: failed to reread ID, disabled\n", ap->id);
>  	ata_port_disable(ap);
>  }
>  
> @@ -2386,10 +2432,7 @@ err_out:
>  
>  static void ata_dev_init_params(struct ata_port *ap, struct ata_device *dev)
>  {
> -	DECLARE_COMPLETION(wait);
> -	struct ata_queued_cmd *qc;
> -	int rc;
> -	unsigned long flags;
> +	struct ata_taskfile tf;
>  	u16 sectors = dev->id[6];
>  	u16 heads   = dev->id[3];
>  
> @@ -2400,26 +2443,18 @@ static void ata_dev_init_params(struct a
>  	/* set up init dev params taskfile */
>  	DPRINTK("init dev params \n");
>  
> -	qc = ata_qc_new_init(ap, dev);
> -	BUG_ON(qc == NULL);
> -
> -	qc->tf.command = ATA_CMD_INIT_DEV_PARAMS;
> -	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
> -	qc->tf.protocol = ATA_PROT_NODATA;
> -	qc->tf.nsect = sectors;
> -	qc->tf.device |= (heads - 1) & 0x0f; /* max head = num. of heads - 1 */
> -
> -	qc->waiting = &wait;
> -	qc->complete_fn = ata_qc_complete_noop;
> -
> -	spin_lock_irqsave(&ap->host_set->lock, flags);
> -	rc = ata_qc_issue(qc);
> -	spin_unlock_irqrestore(&ap->host_set->lock, flags);
> +	ata_tf_init(ap, &tf, dev->devno);
> +	tf.command = ATA_CMD_INIT_DEV_PARAMS;
> +	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
> +	tf.protocol = ATA_PROT_NODATA;
> +	tf.nsect = sectors;
> +	tf.device |= (heads - 1) & 0x0f; /* max head = num. of heads - 1 */
>  
> -	if (rc)
> +	if (ata_exec_internal(ap, dev, &tf, DMA_NONE, NULL, 0)) {
> +		printk(KERN_ERR "ata%u: failed to init parameters, disabled\n",
> +		       ap->id);
>  		ata_port_disable(ap);
> -	else
> -		ata_qc_wait_err(qc, &wait);
> +	}
>  
>  	DPRINTK("EXIT\n");

Re: [PATCH 1/2] libata: implement ata_exec_internal()

[Tejun Heo]

Hello, Jeff.

Jeff Garzik wrote:
> 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().  This change
>> fixes the following bugs.
>>
>> * qc not freed on issue failure
>> * ap->qactive clearing could race with the next internal command
>> * race between timeout handling and irq
>> * ignoring error condition not represented in tf->status
>>
>> Also, qc & hardware are not accessed anymore once it's completed,
>> making internal commands more conformant with general semantics.  This
>> change also makes it easy to issue internal commands from multiple
>> threads if that becomes necessary.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> 
> This is OK in concept, but there are problems in implementation...
> 
> General comment:  would be nice split up as 'implement 
> ata.exec.internal' and 'use ata.exec.internal' patches.
> 

Sure.

> 
>> This is take #2 of ata_exec_internal().  qc allocation and tf
>> reading-back have also been moved into ata_exec_internal().  Now all
>> qc handling is done while properly holding host_set lock removing
>> above mentioned rare race conditon (the second item).
>>
>> Index: work/drivers/scsi/libata-core.c
>> ===================================================================
>> --- work.orig/drivers/scsi/libata-core.c    2005-11-29 
>> 22:04:15.000000000 +0900
>> +++ work/drivers/scsi/libata-core.c    2005-11-29 22:06:20.000000000 
>> +0900
>> @@ -1047,28 +1047,119 @@ static unsigned int ata_pio_modes(const      
>> return modes;
>>  }
>>  
>> -static int ata_qc_wait_err(struct ata_queued_cmd *qc,
>> -               struct completion *wait)
>> +struct ata_exec_internal_arg {
>> +    unsigned int err_mask;
>> +    struct ata_taskfile *tf;
>> +    struct completion *waiting;
>> +};
>> +
>> +int ata_qc_complete_internal(struct ata_queued_cmd *qc, unsigned int 
>> err_mask)
>>  {
>> -    int rc = 0;
>> +    struct ata_exec_internal_arg *arg = qc->complete_data;
> 
> 
> This is what private_data is for.  No need to add 'complete_data'.
> 

Hmmm... As there currently isn't any in-kernel user of qc->private_data, 
its ownership status isn't very clear.  However, ap->private_data is 
used by most low-level drivers to store its private data, so I assumed 
that qc->private_data belongs to low-level driver too.

Actually, the m15w workaround I'm maintaining uses qc->private_data to 
store context for qc iteration.  This can be substitued easily but I 
think it would be nice / more consistent to reserve private_data fields 
for low-level drivers.

> 
>> +    struct completion *waiting = arg->waiting;
>>  
>> -    if (wait_for_completion_timeout(wait, 30 * HZ) < 1) {
>> -        /* timeout handling */
>> -        unsigned int err_mask = ac_err_mask(ata_chk_status(qc->ap));
>> -
>> -        if (!err_mask) {
>> -            printk(KERN_WARNING "ata%u: slow completion (cmd %x)\n",
>> -                   qc->ap->id, qc->tf.command);
>> -        } else {
>> -            printk(KERN_WARNING "ata%u: qc timeout (cmd %x)\n",
>> -                   qc->ap->id, qc->tf.command);
>> -            rc = -EIO;
>> -        }
>> +    if (!(err_mask & ~AC_ERR_DEV))
>> +        qc->ap->ops->tf_read(qc->ap, arg->tf);
>> +    arg->err_mask = err_mask;
>> +    arg->waiting = NULL;
>> +    complete(waiting);
>> +
>> +    return 0;
>> +}
>> +
>> +/**
>> + *    ata_exec_internal - execute libata internal command
>> + *    @ap: Port to which the command is sent
>> + *    @dev: Device to which the command is sent
>> + *    @tf: Taskfile registers for the command and the result
>> + *    @dma_dir: Data tranfer direction of the command
>> + *    @buf: Data buffer of the command
>> + *    @buflen: Length of data buffer
>> + *
>> + *    Executes libata internal command with timeout.  @tf contains
>> + *    command on entry and result on return.  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.
>> + *
>> + *    LOCKING:
>> + *    None.  Should be called with kernel context, might sleep.
>> + */
>> +
>> +static unsigned
>> +ata_exec_internal(struct ata_port *ap, struct ata_device *dev,
>> +          struct ata_taskfile *tf,
>> +          int dma_dir, void *buf, unsigned int buflen)
>> +{
>> +    u8 command = tf->command;
>> +    struct ata_queued_cmd *qc;
>> +    DECLARE_COMPLETION(wait);
>> +    unsigned long flags;
>> +    struct ata_exec_internal_arg arg;
>> +
>> +    if (ata_busy_sleep(ap, ATA_TMOUT_INTERNAL_QUICK, 
>> ATA_TMOUT_INTERNAL))
>> +        return AC_ERR_OTHER;
> 
> 
> NAK.  This sort of thing really belongs in the execution engine for each 
> driver.  Advanced controllers have zero need for this.
> 
> Moreover, why are you adding this, anyway?  This is a distinct 
> regression from the nice "do everything inside ata_qc_issue" model 
> that's been sprouting.
> 

I was thinking that as these internal commands are used for 
initialization and possibly eh in the future, it would be better to give 
drives longer time to get ready for commands.  So, the ata_busy_sleep 
with ATA_TMOUT_INTERNAL_QUICK, ATA_TMOUT_INTERNAL timeouts.  Too 
paranoid / unnecessary?

> 
>> +    spin_lock_irqsave(&ap->host_set->lock, flags);
>> +
>> +    qc = ata_qc_new_init(ap, dev);
>> +    BUG_ON(qc == NULL);
>> +
>> +    qc->tf = *tf;
>> +    qc->dma_dir = dma_dir;
>> +    if (dma_dir != DMA_NONE) {
>> +        ata_sg_init_one(qc, buf, buflen);
>> +        qc->nsect = buflen / ATA_SECT_SIZE;
>> +        printk("dma_dir=%d buflen=%u nsect=%d\n",
>> +               dma_dir, buflen, qc->nsect);
>> +    }
>> +
>> +    arg.waiting = &wait;
>> +    arg.tf = tf;
>> +    qc->complete_data = &arg;
>> +    qc->complete_fn = ata_qc_complete_internal;
>> +
>> +    if (ata_qc_issue(qc))
>> +        goto issue_fail;
>> +
>> +    spin_unlock_irqrestore(&ap->host_set->lock, flags);
>> +
>> +    if (!wait_for_completion_timeout(&wait, ATA_TMOUT_INTERNAL)) {
>> +        int timedout;
>> +
>> +        spin_lock_irqsave(&ap->host_set->lock, flags);
>> +
>> +        /* 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.
>> +         */
>> +        timedout = arg.waiting != NULL;
>> +        if (timedout)
>> +            ata_qc_complete(qc, 0);
> 
> 
> This is not very wise, to pass 'nothing is wrong' to ata_qc_complete() 
> upon timeout.
> 

The problem is that we may or may not complete the command successfully 
due to the following slow completion handling.  So, completing with 
AC_ERR_OTHER isn't very correct either.  This matters because the tf 
registers are read from the completion callback and the completion 
callback does so only on device errors.  The solutions are....

1. leaving as it is with more comments
2. completing with AC_ERR_OTHER but always reading back tf regs in the 
completion callback

I prefer #1, but I'm okay with #2, too.

On a side note, I'm sort of doubtful about slow completion handling in 
ata_exec_internal() and also in eng_timeout().  For example, if some 
kind of electrical / cabling glitch occurs during command execution and 
results in device internal reset, the device's reset status can easily 
be interpreted as successful completion by slow completion handling.

Actually, in the current libata, this can be easily triggered by just 
unplugging and replugging the cable on a busy drive as we don't handle 
those events.

IMHO, timed out commands should always be finished with AC_ERR_OTHER.  I 
don't really see a lot of benefits in successfully completing a command 
after 30sec timeout.

Thanks for reviewing this.

-- 
tejun

Re: [PATCH 1/2] libata: implement ata_exec_internal()

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> Tejun Heo wrote:

>>> This is take #2 of ata_exec_internal().  qc allocation and tf
>>> reading-back have also been moved into ata_exec_internal().  Now all
>>> qc handling is done while properly holding host_set lock removing
>>> above mentioned rare race conditon (the second item).
>>>
>>> Index: work/drivers/scsi/libata-core.c
>>> ===================================================================
>>> --- work.orig/drivers/scsi/libata-core.c    2005-11-29 
>>> 22:04:15.000000000 +0900
>>> +++ work/drivers/scsi/libata-core.c    2005-11-29 22:06:20.000000000 
>>> +0900
>>> @@ -1047,28 +1047,119 @@ static unsigned int ata_pio_modes(const      
>>> return modes;
>>>  }
>>>  
>>> -static int ata_qc_wait_err(struct ata_queued_cmd *qc,
>>> -               struct completion *wait)
>>> +struct ata_exec_internal_arg {
>>> +    unsigned int err_mask;
>>> +    struct ata_taskfile *tf;
>>> +    struct completion *waiting;
>>> +};
>>> +
>>> +int ata_qc_complete_internal(struct ata_queued_cmd *qc, unsigned int 
>>> err_mask)
>>>  {
>>> -    int rc = 0;
>>> +    struct ata_exec_internal_arg *arg = qc->complete_data;
>>
>>
>>
>> This is what private_data is for.  No need to add 'complete_data'.
>>
> 
> Hmmm... As there currently isn't any in-kernel user of qc->private_data, 
> its ownership status isn't very clear.  However, ap->private_data is 
> used by most low-level drivers to store its private data, so I assumed 
> that qc->private_data belongs to low-level driver too.
> 
> Actually, the m15w workaround I'm maintaining uses qc->private_data to 
> store context for qc iteration.  This can be substitued easily but I 
> think it would be nice / more consistent to reserve private_data fields 
> for low-level drivers.

qc->private_data is for the creator/owner of the ata_queued_cmd, not for 
low-level drivers.


>>> +    struct completion *waiting = arg->waiting;
>>>  
>>> -    if (wait_for_completion_timeout(wait, 30 * HZ) < 1) {
>>> -        /* timeout handling */
>>> -        unsigned int err_mask = ac_err_mask(ata_chk_status(qc->ap));
>>> -
>>> -        if (!err_mask) {
>>> -            printk(KERN_WARNING "ata%u: slow completion (cmd %x)\n",
>>> -                   qc->ap->id, qc->tf.command);
>>> -        } else {
>>> -            printk(KERN_WARNING "ata%u: qc timeout (cmd %x)\n",
>>> -                   qc->ap->id, qc->tf.command);
>>> -            rc = -EIO;
>>> -        }
>>> +    if (!(err_mask & ~AC_ERR_DEV))
>>> +        qc->ap->ops->tf_read(qc->ap, arg->tf);
>>> +    arg->err_mask = err_mask;
>>> +    arg->waiting = NULL;
>>> +    complete(waiting);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/**
>>> + *    ata_exec_internal - execute libata internal command
>>> + *    @ap: Port to which the command is sent
>>> + *    @dev: Device to which the command is sent
>>> + *    @tf: Taskfile registers for the command and the result
>>> + *    @dma_dir: Data tranfer direction of the command
>>> + *    @buf: Data buffer of the command
>>> + *    @buflen: Length of data buffer
>>> + *
>>> + *    Executes libata internal command with timeout.  @tf contains
>>> + *    command on entry and result on return.  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.
>>> + *
>>> + *    LOCKING:
>>> + *    None.  Should be called with kernel context, might sleep.
>>> + */
>>> +
>>> +static unsigned
>>> +ata_exec_internal(struct ata_port *ap, struct ata_device *dev,
>>> +          struct ata_taskfile *tf,
>>> +          int dma_dir, void *buf, unsigned int buflen)
>>> +{
>>> +    u8 command = tf->command;
>>> +    struct ata_queued_cmd *qc;
>>> +    DECLARE_COMPLETION(wait);
>>> +    unsigned long flags;
>>> +    struct ata_exec_internal_arg arg;
>>> +
>>> +    if (ata_busy_sleep(ap, ATA_TMOUT_INTERNAL_QUICK, 
>>> ATA_TMOUT_INTERNAL))
>>> +        return AC_ERR_OTHER;
>>
>>
>>
>> NAK.  This sort of thing really belongs in the execution engine for 
>> each driver.  Advanced controllers have zero need for this.
>>
>> Moreover, why are you adding this, anyway?  This is a distinct 
>> regression from the nice "do everything inside ata_qc_issue" model 
>> that's been sprouting.
>>
> 
> I was thinking that as these internal commands are used for 
> initialization and possibly eh in the future, it would be better to give 
> drives longer time to get ready for commands.  So, the ata_busy_sleep 
> with ATA_TMOUT_INTERNAL_QUICK, ATA_TMOUT_INTERNAL timeouts.  Too 
> paranoid / unnecessary?

Yes, and it breaks the "use ata_queued_cmd for everything" model. 
Polling for BSY, etc. should happen in the time between the call to 
ata_qc_issue() and the call to ata_qc_complete().

There are some violations of this model in the SRST and IDENTIFY DEVICE 
code, but those need to be eliminated, not expanded :)


>>> +    spin_lock_irqsave(&ap->host_set->lock, flags);
>>> +
>>> +    qc = ata_qc_new_init(ap, dev);
>>> +    BUG_ON(qc == NULL);
>>> +
>>> +    qc->tf = *tf;
>>> +    qc->dma_dir = dma_dir;
>>> +    if (dma_dir != DMA_NONE) {
>>> +        ata_sg_init_one(qc, buf, buflen);
>>> +        qc->nsect = buflen / ATA_SECT_SIZE;
>>> +        printk("dma_dir=%d buflen=%u nsect=%d\n",
>>> +               dma_dir, buflen, qc->nsect);
>>> +    }
>>> +
>>> +    arg.waiting = &wait;
>>> +    arg.tf = tf;
>>> +    qc->complete_data = &arg;
>>> +    qc->complete_fn = ata_qc_complete_internal;
>>> +
>>> +    if (ata_qc_issue(qc))
>>> +        goto issue_fail;
>>> +
>>> +    spin_unlock_irqrestore(&ap->host_set->lock, flags);
>>> +
>>> +    if (!wait_for_completion_timeout(&wait, ATA_TMOUT_INTERNAL)) {
>>> +        int timedout;
>>> +
>>> +        spin_lock_irqsave(&ap->host_set->lock, flags);
>>> +
>>> +        /* 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.
>>> +         */
>>> +        timedout = arg.waiting != NULL;
>>> +        if (timedout)
>>> +            ata_qc_complete(qc, 0);
>>
>>
>>
>> This is not very wise, to pass 'nothing is wrong' to ata_qc_complete() 
>> upon timeout.
>>
> 
> The problem is that we may or may not complete the command successfully 
> due to the following slow completion handling.  So, completing with 
> AC_ERR_OTHER isn't very correct either.  This matters because the tf 
> registers are read from the completion callback and the completion 
> callback does so only on device errors.  The solutions are....
> 
> 1. leaving as it is with more comments
> 2. completing with AC_ERR_OTHER but always reading back tf regs in the 
> completion callback
> 
> I prefer #1, but I'm okay with #2, too.

If it times out, call it an error.  Easy enough.


> On a side note, I'm sort of doubtful about slow completion handling in 
> ata_exec_internal() and also in eng_timeout().  For example, if some 
> kind of electrical / cabling glitch occurs during command execution and 
> results in device internal reset, the device's reset status can easily 
> be interpreted as successful completion by slow completion handling.
> 
> Actually, in the current libata, this can be easily triggered by just 
> unplugging and replugging the cable on a busy drive as we don't handle 
> those events.

You're welcome to improve the libata error handling code ;-)

	Jeff

[PATCH 1/3] libata: implement ata_exec_internal()

[Tejun Heo]

This patch implements ata_exec_internal() function which performs
libata internal command execution.  Previously, this was done by each
user by manually initializing a qc, issueing it, waiting for its
completion and handling errors.  In addition to obvious code
factoring, using ata_exec_internal() fixes the following bugs.

* qc not freed on issue failure
* ap->qactive clearing could race with the next internal command
* race between timeout handling and irq
* ignoring error condition not represented in tf->status

Also, qc & hardware are not accessed anymore once it's completed,
making internal commands more conformant with general semantics.
ata_exec_internal() also makes it easy to issue internal commands from
multiple threads if that becomes necessary.

This patch only implements ata_exec_internal().  A following patch
will convert all users.

Signed-off-by: Tejun Heo <htejun@gmail.com>

--

Jeff, changes from the previous posting are...

* no ata_busy_sleep
* no ->complete_data
* timeout is always an AC_ERR_OTHER error
* patches splitted

Thanks.

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2005-12-05 17:05:25.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2005-12-05 17:07:14.000000000 +0900
@@ -1047,6 +1047,106 @@ static unsigned int ata_pio_modes(const 
 	return modes;
 }
 
+struct ata_exec_internal_arg {
+	unsigned int err_mask;
+	struct ata_taskfile *tf;
+	struct completion *waiting;
+};
+
+int ata_qc_complete_internal(struct ata_queued_cmd *qc, unsigned int err_mask)
+{
+	struct ata_exec_internal_arg *arg = qc->private_data;
+	struct completion *waiting = arg->waiting;
+
+	if (!(err_mask & ~AC_ERR_DEV))
+		qc->ap->ops->tf_read(qc->ap, arg->tf);
+	arg->err_mask = err_mask;
+	arg->waiting = NULL;
+	complete(waiting);
+
+	return 0;
+}
+
+/**
+ *	ata_exec_internal - execute libata internal command
+ *	@ap: Port to which the command is sent
+ *	@dev: Device to which the command is sent
+ *	@tf: Taskfile registers for the command and the result
+ *	@dma_dir: Data tranfer direction of the command
+ *	@buf: Data buffer of the command
+ *	@buflen: Length of data buffer
+ *
+ *	Executes libata internal command with timeout.  @tf contains
+ *	command on entry and result on return.  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.
+ *
+ *	LOCKING:
+ *	None.  Should be called with kernel context, might sleep.
+ */
+
+static unsigned
+ata_exec_internal(struct ata_port *ap, struct ata_device *dev,
+		  struct ata_taskfile *tf,
+		  int dma_dir, void *buf, unsigned int buflen)
+{
+	u8 command = tf->command;
+	struct ata_queued_cmd *qc;
+	DECLARE_COMPLETION(wait);
+	unsigned long flags;
+	struct ata_exec_internal_arg arg;
+
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+
+	qc = ata_qc_new_init(ap, dev);
+	BUG_ON(qc == NULL);
+
+	qc->tf = *tf;
+	qc->dma_dir = dma_dir;
+	if (dma_dir != DMA_NONE) {
+		ata_sg_init_one(qc, buf, buflen);
+		qc->nsect = buflen / ATA_SECT_SIZE;
+		printk("dma_dir=%d buflen=%u nsect=%d\n",
+		       dma_dir, buflen, qc->nsect);
+	}
+
+	arg.waiting = &wait;
+	arg.tf = tf;
+	qc->private_data = &arg;
+	qc->complete_fn = ata_qc_complete_internal;
+
+	if (ata_qc_issue(qc))
+		goto issue_fail;
+
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+	if (!wait_for_completion_timeout(&wait, ATA_TMOUT_INTERNAL)) {
+		spin_lock_irqsave(&ap->host_set->lock, flags);
+
+		/* 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 (arg.waiting) {
+			ata_qc_complete(qc, AC_ERR_OTHER);
+			printk(KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n",
+			       ap->id, command);
+		}
+
+		spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	}
+
+	return arg.err_mask;
+
+ issue_fail:
+	ata_qc_free(qc);
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	return AC_ERR_OTHER;
+}
+
 static int ata_qc_wait_err(struct ata_queued_cmd *qc,
 			   struct completion *wait)
 {
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h	2005-12-05 17:05:25.000000000 +0900
+++ work/include/linux/libata.h	2005-12-05 17:05:27.000000000 +0900
@@ -136,6 +136,8 @@ enum {
 	ATA_TMOUT_BOOT_QUICK	= 7 * HZ,	/* hueristic */
 	ATA_TMOUT_DATAOUT	= 30 * HZ,
 	ATA_TMOUT_DATAOUT_QUICK	= 5 * HZ,
+	ATA_TMOUT_INTERNAL	= 30 * HZ,
+	ATA_TMOUT_INTERNAL_QUICK = 5 * HZ,
 
 	/* ATA bus states */
 	BUS_UNKNOWN		= 0,

[PATCH 2/4] libata: use ata_exec_internal()

[Tejun Heo]

This patch converts all users of libata internal commands to use
ata_exec_internal().

Signed-off-by: Tejun Heo <htejun@gmail.com>

--

Jeff, I made a mistake while posting the previous patch.  The subject
line should say [PATCH 1/4] not [PATCH 1/3].  Sorry.

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2005-12-05 17:07:14.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2005-12-05 17:08:25.000000000 +0900
@@ -1200,9 +1200,8 @@ static void ata_dev_identify(struct ata_
 	u16 tmp;
 	unsigned long xfer_modes;
 	unsigned int using_edd;
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	unsigned long flags;
+	struct ata_taskfile tf;
+	unsigned int err_mask;
 	int rc;
 
 	if (!ata_dev_present(dev)) {
@@ -1223,40 +1222,26 @@ static void ata_dev_identify(struct ata_
 
 	ata_dev_select(ap, device, 1, 1); /* select device 0/1 */
 
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
-
-	ata_sg_init_one(qc, dev->id, sizeof(dev->id));
-	qc->dma_dir = DMA_FROM_DEVICE;
-	qc->tf.protocol = ATA_PROT_PIO;
-	qc->nsect = 1;
-
 retry:
+	ata_tf_init(ap, &tf, device);
+
 	if (dev->class == ATA_DEV_ATA) {
-		qc->tf.command = ATA_CMD_ID_ATA;
+		tf.command = ATA_CMD_ID_ATA;
 		DPRINTK("do ATA identify\n");
 	} else {
-		qc->tf.command = ATA_CMD_ID_ATAPI;
+		tf.command = ATA_CMD_ID_ATAPI;
 		DPRINTK("do ATAPI identify\n");
 	}
 
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	tf.protocol = ATA_PROT_PIO;
 
-	if (rc)
-		goto err_out;
-	else
-		ata_qc_wait_err(qc, &wait);
+	err_mask = ata_exec_internal(ap, dev, &tf, DMA_FROM_DEVICE,
+				     dev->id, sizeof(dev->id));
 
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	ap->ops->tf_read(ap, &qc->tf);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	if (err_mask) {
+		if (err_mask & ~AC_ERR_DEV)
+			goto err_out;
 
-	if (qc->tf.command & ATA_ERR) {
 		/*
 		 * arg!  EDD works for all test cases, but seems to return
 		 * the ATA signature for some ATAPI devices.  Until the
@@ -1269,13 +1254,9 @@ retry:
 		 * to have this problem.
 		 */
 		if ((using_edd) && (dev->class == ATA_DEV_ATA)) {
-			u8 err = qc->tf.feature;
+			u8 err = tf.feature;
 			if (err & ATA_ABORTED) {
 				dev->class = ATA_DEV_ATAPI;
-				qc->cursg = 0;
-				qc->cursg_ofs = 0;
-				qc->cursect = 0;
-				qc->nsect = 1;
 				goto retry;
 			}
 		}
@@ -2388,34 +2369,23 @@ static int ata_choose_xfer_mode(const st
 
 static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev)
 {
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	int rc;
-	unsigned long flags;
+	struct ata_taskfile tf;
 
 	/* set up set-features taskfile */
 	DPRINTK("set features - xfer mode\n");
 
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
-
-	qc->tf.command = ATA_CMD_SET_FEATURES;
-	qc->tf.feature = SETFEATURES_XFER;
-	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-	qc->tf.protocol = ATA_PROT_NODATA;
-	qc->tf.nsect = dev->xfer_mode;
-
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	ata_tf_init(ap, &tf, dev->devno);
+	tf.command = ATA_CMD_SET_FEATURES;
+	tf.feature = SETFEATURES_XFER;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.nsect = dev->xfer_mode;
 
-	if (rc)
+	if (ata_exec_internal(ap, dev, &tf, DMA_NONE, NULL, 0)) {
+		printk(KERN_ERR "ata%u: failed to set xfermode, disabled\n",
+		       ap->id);
 		ata_port_disable(ap);
-	else
-		ata_qc_wait_err(qc, &wait);
+	}
 
 	DPRINTK("EXIT\n");
 }
@@ -2430,41 +2400,25 @@ static void ata_dev_set_xfermode(struct 
 
 static void ata_dev_reread_id(struct ata_port *ap, struct ata_device *dev)
 {
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	unsigned long flags;
-	int rc;
-
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
+	struct ata_taskfile tf;
 
-	ata_sg_init_one(qc, dev->id, sizeof(dev->id));
-	qc->dma_dir = DMA_FROM_DEVICE;
+	ata_tf_init(ap, &tf, dev->devno);
 
 	if (dev->class == ATA_DEV_ATA) {
-		qc->tf.command = ATA_CMD_ID_ATA;
+		tf.command = ATA_CMD_ID_ATA;
 		DPRINTK("do ATA identify\n");
 	} else {
-		qc->tf.command = ATA_CMD_ID_ATAPI;
+		tf.command = ATA_CMD_ID_ATAPI;
 		DPRINTK("do ATAPI identify\n");
 	}
 
-	qc->tf.flags |= ATA_TFLAG_DEVICE;
-	qc->tf.protocol = ATA_PROT_PIO;
-	qc->nsect = 1;
+	tf.flags |= ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_PIO;
 
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
-
-	if (rc)
+	if (ata_exec_internal(ap, dev, &tf, DMA_FROM_DEVICE,
+			      dev->id, sizeof(dev->id)))
 		goto err_out;
 
-	ata_qc_wait_err(qc, &wait);
-
 	swap_buf_le16(dev->id, ATA_ID_WORDS);
 
 	ata_dump_id(dev);
@@ -2473,6 +2427,7 @@ static void ata_dev_reread_id(struct ata
 
 	return;
 err_out:
+	printk(KERN_ERR "ata%u: failed to reread ID, disabled\n", ap->id);
 	ata_port_disable(ap);
 }
 
@@ -2486,10 +2441,7 @@ err_out:
 
 static void ata_dev_init_params(struct ata_port *ap, struct ata_device *dev)
 {
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	int rc;
-	unsigned long flags;
+	struct ata_taskfile tf;
 	u16 sectors = dev->id[6];
 	u16 heads   = dev->id[3];
 
@@ -2500,26 +2452,18 @@ static void ata_dev_init_params(struct a
 	/* set up init dev params taskfile */
 	DPRINTK("init dev params \n");
 
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
-
-	qc->tf.command = ATA_CMD_INIT_DEV_PARAMS;
-	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-	qc->tf.protocol = ATA_PROT_NODATA;
-	qc->tf.nsect = sectors;
-	qc->tf.device |= (heads - 1) & 0x0f; /* max head = num. of heads - 1 */
-
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	ata_tf_init(ap, &tf, dev->devno);
+	tf.command = ATA_CMD_INIT_DEV_PARAMS;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.nsect = sectors;
+	tf.device |= (heads - 1) & 0x0f; /* max head = num. of heads - 1 */
 
-	if (rc)
+	if (ata_exec_internal(ap, dev, &tf, DMA_NONE, NULL, 0)) {
+		printk(KERN_ERR "ata%u: failed to init parameters, disabled\n",
+		       ap->id);
 		ata_port_disable(ap);
-	else
-		ata_qc_wait_err(qc, &wait);
+	}
 
 	DPRINTK("EXIT\n");
 }

[PATCH 3/4] libata: remove unused functions

[Tejun Heo]

There is no user of ata_qc_wait_err() and ata_qc_complete_noop() after
ata_qc_exec_internal() change.  Remove unused functions.

Signed-off-by: Tejun Heo <htejun@gmail.com>

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2005-12-05 17:10:36.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2005-12-05 17:10:44.000000000 +0900
@@ -1147,30 +1147,6 @@ ata_exec_internal(struct ata_port *ap, s
 	return AC_ERR_OTHER;
 }
 
-static int ata_qc_wait_err(struct ata_queued_cmd *qc,
-			   struct completion *wait)
-{
-	int rc = 0;
-
-	if (wait_for_completion_timeout(wait, 30 * HZ) < 1) {
-		/* timeout handling */
-		unsigned int err_mask = ac_err_mask(ata_chk_status(qc->ap));
-
-		if (!err_mask) {
-			printk(KERN_WARNING "ata%u: slow completion (cmd %x)\n",
-			       qc->ap->id, qc->tf.command);
-		} 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);
-	}
-
-	return rc;
-}
-
 /**
  *	ata_dev_identify - obtain IDENTIFY x DEVICE page
  *	@ap: port on which device we wish to probe resides
@@ -3685,11 +3661,6 @@ struct ata_queued_cmd *ata_qc_new_init(s
 	return qc;
 }
 
-int ata_qc_complete_noop(struct ata_queued_cmd *qc, unsigned int err_mask)
-{
-	return 0;
-}
-
 static void __ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
Index: work/drivers/scsi/libata.h
===================================================================
--- work.orig/drivers/scsi/libata.h	2005-12-05 17:10:16.000000000 +0900
+++ work/drivers/scsi/libata.h	2005-12-05 17:10:36.000000000 +0900
@@ -39,7 +39,6 @@ struct ata_scsi_args {
 
 /* libata-core.c */
 extern int atapi_enabled;
-extern int ata_qc_complete_noop(struct ata_queued_cmd *qc, unsigned int err_mask);
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
 				      struct ata_device *dev);
 extern void ata_rwcmd_protocol(struct ata_queued_cmd *qc);

[PATCH 4/4] libata: remove unused qc->waiting

[Tejun Heo]

There is no user of qc->waiting left after ata_exec_internal() change.
Kill the field.

Signed-off-by: Tejun Heo <htejun@gmail.com>

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2005-12-05 17:10:44.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2005-12-05 17:10:48.000000000 +0900
@@ -3664,7 +3664,7 @@ struct ata_queued_cmd *ata_qc_new_init(s
 static void __ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
-	unsigned int tag, do_clear = 0;
+	unsigned int tag;
 
 	qc->flags = 0;
 	tag = qc->tag;
@@ -3672,17 +3672,8 @@ static void __ata_qc_complete(struct ata
 		if (tag == ap->active_tag)
 			ap->active_tag = ATA_TAG_POISON;
 		qc->tag = ATA_TAG_POISON;
-		do_clear = 1;
-	}
-
-	if (qc->waiting) {
-		struct completion *waiting = qc->waiting;
-		qc->waiting = NULL;
-		complete(waiting);
-	}
-
-	if (likely(do_clear))
 		clear_bit(tag, &ap->qactive);
+	}
 }
 
 /**
@@ -3698,7 +3689,6 @@ static void __ata_qc_complete(struct ata
 void ata_qc_free(struct ata_queued_cmd *qc)
 {
 	assert(qc != NULL);	/* ata_qc_from_tag _might_ return NULL */
-	assert(qc->waiting == NULL);	/* nothing should be waiting */
 
 	__ata_qc_complete(qc);
 }
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h	2005-12-05 17:10:36.000000000 +0900
+++ work/include/linux/libata.h	2005-12-05 17:10:48.000000000 +0900
@@ -287,8 +287,6 @@ struct ata_queued_cmd {
 
 	ata_qc_cb_t		complete_fn;
 
-	struct completion	*waiting;
-
 	void			*private_data;
 };
 

Re: [PATCH 1/2] libata: implement ata_exec_internal()

[Tejun Heo]

Jeff, can you please ack or nack it?

Thanks.

-- 
tejun

[PATCH 1/4] libata: implement ata_exec_internal()

[Tejun Heo]

This patch implements ata_exec_internal() function which performs
libata internal command execution.  Previously, this was done by each
user by manually initializing a qc, issueing it, waiting for its
completion and handling errors.  In addition to obvious code
factoring, using ata_exec_internal() fixes the following bugs.

* qc not freed on issue failure
* ap->qactive clearing could race with the next internal command
* race between timeout handling and irq
* ignoring error condition not represented in tf->status

Also, qc & hardware are not accessed anymore once it's completed,
making internal commands more conformant with general semantics.
ata_exec_internal() also makes it easy to issue internal commands from
multiple threads if that becomes necessary.

This patch only implements ata_exec_internal().  A following patch
will convert all users.

Signed-off-by: Tejun Heo <htejun@gmail.com>

--

Jeff, this is the third posting of this patchset.  Changes from the
second posting are

* adapted to late qc->err_mask change

Changes to the second posting from the first are

* no ata_busy_sleep
* no ->complete_data
* timeout is always an AC_ERR_OTHER error
* patches splitted

Thanks.


Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2005-12-11 12:54:06.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2005-12-11 12:54:07.000000000 +0900
@@ -1047,6 +1047,107 @@ static unsigned int ata_pio_modes(const 
 	return modes;
 }
 
+struct ata_exec_internal_arg {
+	unsigned int err_mask;
+	struct ata_taskfile *tf;
+	struct completion *waiting;
+};
+
+int ata_qc_complete_internal(struct ata_queued_cmd *qc)
+{
+	struct ata_exec_internal_arg *arg = qc->private_data;
+	struct completion *waiting = arg->waiting;
+
+	if (!(qc->err_mask & ~AC_ERR_DEV))
+		qc->ap->ops->tf_read(qc->ap, arg->tf);
+	arg->err_mask = qc->err_mask;
+	arg->waiting = NULL;
+	complete(waiting);
+
+	return 0;
+}
+
+/**
+ *	ata_exec_internal - execute libata internal command
+ *	@ap: Port to which the command is sent
+ *	@dev: Device to which the command is sent
+ *	@tf: Taskfile registers for the command and the result
+ *	@dma_dir: Data tranfer direction of the command
+ *	@buf: Data buffer of the command
+ *	@buflen: Length of data buffer
+ *
+ *	Executes libata internal command with timeout.  @tf contains
+ *	command on entry and result on return.  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.
+ *
+ *	LOCKING:
+ *	None.  Should be called with kernel context, might sleep.
+ */
+
+static unsigned
+ata_exec_internal(struct ata_port *ap, struct ata_device *dev,
+		  struct ata_taskfile *tf,
+		  int dma_dir, void *buf, unsigned int buflen)
+{
+	u8 command = tf->command;
+	struct ata_queued_cmd *qc;
+	DECLARE_COMPLETION(wait);
+	unsigned long flags;
+	struct ata_exec_internal_arg arg;
+
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+
+	qc = ata_qc_new_init(ap, dev);
+	BUG_ON(qc == NULL);
+
+	qc->tf = *tf;
+	qc->dma_dir = dma_dir;
+	if (dma_dir != DMA_NONE) {
+		ata_sg_init_one(qc, buf, buflen);
+		qc->nsect = buflen / ATA_SECT_SIZE;
+		printk("dma_dir=%d buflen=%u nsect=%d\n",
+		       dma_dir, buflen, qc->nsect);
+	}
+
+	arg.waiting = &wait;
+	arg.tf = tf;
+	qc->private_data = &arg;
+	qc->complete_fn = ata_qc_complete_internal;
+
+	if (ata_qc_issue(qc))
+		goto issue_fail;
+
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+	if (!wait_for_completion_timeout(&wait, ATA_TMOUT_INTERNAL)) {
+		spin_lock_irqsave(&ap->host_set->lock, flags);
+
+		/* 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 (arg.waiting) {
+			qc->err_mask = AC_ERR_OTHER;
+			ata_qc_complete(qc);
+			printk(KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n",
+			       ap->id, command);
+		}
+
+		spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	}
+
+	return arg.err_mask;
+
+ issue_fail:
+	ata_qc_free(qc);
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	return AC_ERR_OTHER;
+}
+
 static int ata_qc_wait_err(struct ata_queued_cmd *qc,
 			   struct completion *wait)
 {
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h	2005-12-11 12:54:06.000000000 +0900
+++ work/include/linux/libata.h	2005-12-11 12:54:07.000000000 +0900
@@ -136,6 +136,8 @@ enum {
 	ATA_TMOUT_BOOT_QUICK	= 7 * HZ,	/* hueristic */
 	ATA_TMOUT_DATAOUT	= 30 * HZ,
 	ATA_TMOUT_DATAOUT_QUICK	= 5 * HZ,
+	ATA_TMOUT_INTERNAL	= 30 * HZ,
+	ATA_TMOUT_INTERNAL_QUICK = 5 * HZ,
 
 	/* ATA bus states */
 	BUS_UNKNOWN		= 0,

[PATCH 2/4] libata: use ata_exec_internal()

[Tejun Heo]

This patch converts all users of libata internal commands to use
ata_exec_internal().

Signed-off-by: Tejun Heo <htejun@gmail.com>

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2005-12-11 12:54:07.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2005-12-11 12:58:39.000000000 +0900
@@ -1201,9 +1201,8 @@ static void ata_dev_identify(struct ata_
 	u16 tmp;
 	unsigned long xfer_modes;
 	unsigned int using_edd;
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	unsigned long flags;
+	struct ata_taskfile tf;
+	unsigned int err_mask;
 	int rc;
 
 	if (!ata_dev_present(dev)) {
@@ -1224,40 +1223,26 @@ static void ata_dev_identify(struct ata_
 
 	ata_dev_select(ap, device, 1, 1); /* select device 0/1 */
 
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
-
-	ata_sg_init_one(qc, dev->id, sizeof(dev->id));
-	qc->dma_dir = DMA_FROM_DEVICE;
-	qc->tf.protocol = ATA_PROT_PIO;
-	qc->nsect = 1;
-
 retry:
+	ata_tf_init(ap, &tf, device);
+
 	if (dev->class == ATA_DEV_ATA) {
-		qc->tf.command = ATA_CMD_ID_ATA;
+		tf.command = ATA_CMD_ID_ATA;
 		DPRINTK("do ATA identify\n");
 	} else {
-		qc->tf.command = ATA_CMD_ID_ATAPI;
+		tf.command = ATA_CMD_ID_ATAPI;
 		DPRINTK("do ATAPI identify\n");
 	}
 
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	tf.protocol = ATA_PROT_PIO;
 
-	if (rc)
-		goto err_out;
-	else
-		ata_qc_wait_err(qc, &wait);
+	err_mask = ata_exec_internal(ap, dev, &tf, DMA_FROM_DEVICE,
+				     dev->id, sizeof(dev->id));
 
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	ap->ops->tf_read(ap, &qc->tf);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	if (err_mask) {
+		if (err_mask & ~AC_ERR_DEV)
+			goto err_out;
 
-	if (qc->tf.command & ATA_ERR) {
 		/*
 		 * arg!  EDD works for all test cases, but seems to return
 		 * the ATA signature for some ATAPI devices.  Until the
@@ -1270,14 +1255,9 @@ retry:
 		 * to have this problem.
 		 */
 		if ((using_edd) && (dev->class == ATA_DEV_ATA)) {
-			u8 err = qc->tf.feature;
+			u8 err = tf.feature;
 			if (err & ATA_ABORTED) {
 				dev->class = ATA_DEV_ATAPI;
-				qc->cursg = 0;
-				qc->cursg_ofs = 0;
-				qc->cursect = 0;
-				qc->nsect = 1;
-				qc->err_mask = 0;
 				goto retry;
 			}
 		}
@@ -2390,34 +2370,23 @@ static int ata_choose_xfer_mode(const st
 
 static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev)
 {
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	int rc;
-	unsigned long flags;
+	struct ata_taskfile tf;
 
 	/* set up set-features taskfile */
 	DPRINTK("set features - xfer mode\n");
 
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
-
-	qc->tf.command = ATA_CMD_SET_FEATURES;
-	qc->tf.feature = SETFEATURES_XFER;
-	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-	qc->tf.protocol = ATA_PROT_NODATA;
-	qc->tf.nsect = dev->xfer_mode;
-
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	ata_tf_init(ap, &tf, dev->devno);
+	tf.command = ATA_CMD_SET_FEATURES;
+	tf.feature = SETFEATURES_XFER;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.nsect = dev->xfer_mode;
 
-	if (rc)
+	if (ata_exec_internal(ap, dev, &tf, DMA_NONE, NULL, 0)) {
+		printk(KERN_ERR "ata%u: failed to set xfermode, disabled\n",
+		       ap->id);
 		ata_port_disable(ap);
-	else
-		ata_qc_wait_err(qc, &wait);
+	}
 
 	DPRINTK("EXIT\n");
 }
@@ -2432,41 +2401,25 @@ static void ata_dev_set_xfermode(struct 
 
 static void ata_dev_reread_id(struct ata_port *ap, struct ata_device *dev)
 {
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	unsigned long flags;
-	int rc;
-
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
+	struct ata_taskfile tf;
 
-	ata_sg_init_one(qc, dev->id, sizeof(dev->id));
-	qc->dma_dir = DMA_FROM_DEVICE;
+	ata_tf_init(ap, &tf, dev->devno);
 
 	if (dev->class == ATA_DEV_ATA) {
-		qc->tf.command = ATA_CMD_ID_ATA;
+		tf.command = ATA_CMD_ID_ATA;
 		DPRINTK("do ATA identify\n");
 	} else {
-		qc->tf.command = ATA_CMD_ID_ATAPI;
+		tf.command = ATA_CMD_ID_ATAPI;
 		DPRINTK("do ATAPI identify\n");
 	}
 
-	qc->tf.flags |= ATA_TFLAG_DEVICE;
-	qc->tf.protocol = ATA_PROT_PIO;
-	qc->nsect = 1;
+	tf.flags |= ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_PIO;
 
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
-
-	if (rc)
+	if (ata_exec_internal(ap, dev, &tf, DMA_FROM_DEVICE,
+			      dev->id, sizeof(dev->id)))
 		goto err_out;
 
-	ata_qc_wait_err(qc, &wait);
-
 	swap_buf_le16(dev->id, ATA_ID_WORDS);
 
 	ata_dump_id(dev);
@@ -2475,6 +2428,7 @@ static void ata_dev_reread_id(struct ata
 
 	return;
 err_out:
+	printk(KERN_ERR "ata%u: failed to reread ID, disabled\n", ap->id);
 	ata_port_disable(ap);
 }
 
@@ -2488,10 +2442,7 @@ err_out:
 
 static void ata_dev_init_params(struct ata_port *ap, struct ata_device *dev)
 {
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	int rc;
-	unsigned long flags;
+	struct ata_taskfile tf;
 	u16 sectors = dev->id[6];
 	u16 heads   = dev->id[3];
 
@@ -2502,26 +2453,18 @@ static void ata_dev_init_params(struct a
 	/* set up init dev params taskfile */
 	DPRINTK("init dev params \n");
 
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
-
-	qc->tf.command = ATA_CMD_INIT_DEV_PARAMS;
-	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-	qc->tf.protocol = ATA_PROT_NODATA;
-	qc->tf.nsect = sectors;
-	qc->tf.device |= (heads - 1) & 0x0f; /* max head = num. of heads - 1 */
-
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	ata_tf_init(ap, &tf, dev->devno);
+	tf.command = ATA_CMD_INIT_DEV_PARAMS;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.nsect = sectors;
+	tf.device |= (heads - 1) & 0x0f; /* max head = num. of heads - 1 */
 
-	if (rc)
+	if (ata_exec_internal(ap, dev, &tf, DMA_NONE, NULL, 0)) {
+		printk(KERN_ERR "ata%u: failed to init parameters, disabled\n",
+		       ap->id);
 		ata_port_disable(ap);
-	else
-		ata_qc_wait_err(qc, &wait);
+	}
 
 	DPRINTK("EXIT\n");
 }

[PATCH 3/4] libata: removed unused functions

[Tejun Heo]

There is no user of ata_qc_wait_err() and ata_qc_complete_noop() after
ata_exec_internal() changes.  Remove unused functions.

Signed-off-by: Tejun Heo <htejun@gmail.com>

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2005-12-11 12:58:39.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2005-12-11 12:58:59.000000000 +0900
@@ -1148,30 +1148,6 @@ ata_exec_internal(struct ata_port *ap, s
 	return AC_ERR_OTHER;
 }
 
-static int ata_qc_wait_err(struct ata_queued_cmd *qc,
-			   struct completion *wait)
-{
-	int rc = 0;
-
-	if (wait_for_completion_timeout(wait, 30 * HZ) < 1) {
-		/* timeout handling */
-		qc->err_mask |= ac_err_mask(ata_chk_status(qc->ap));
-
-		if (!qc->err_mask) {
-			printk(KERN_WARNING "ata%u: slow completion (cmd %x)\n",
-			       qc->ap->id, qc->tf.command);
-		} else {
-			printk(KERN_WARNING "ata%u: qc timeout (cmd %x)\n",
-			       qc->ap->id, qc->tf.command);
-			rc = -EIO;
-		}
-
-		ata_qc_complete(qc);
-	}
-
-	return rc;
-}
-
 /**
  *	ata_dev_identify - obtain IDENTIFY x DEVICE page
  *	@ap: port on which device we wish to probe resides
@@ -3709,11 +3685,6 @@ struct ata_queued_cmd *ata_qc_new_init(s
 	return qc;
 }
 
-int ata_qc_complete_noop(struct ata_queued_cmd *qc)
-{
-	return 0;
-}
-
 static void __ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
Index: work/drivers/scsi/libata.h
===================================================================
--- work.orig/drivers/scsi/libata.h	2005-12-11 12:34:20.000000000 +0900
+++ work/drivers/scsi/libata.h	2005-12-11 12:58:59.000000000 +0900
@@ -39,7 +39,6 @@ struct ata_scsi_args {
 
 /* libata-core.c */
 extern int atapi_enabled;
-extern int ata_qc_complete_noop(struct ata_queued_cmd *qc);
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
 				      struct ata_device *dev);
 extern void ata_rwcmd_protocol(struct ata_queued_cmd *qc);

[PATCH 4/4] libata: remove unused qc->waiting

[Tejun Heo]

There is no user of qc->waiting left after ata_exec_internal()
changes.  Kill the field.

Signed-off-by: Tejun Heo <htejun@gmail.com>

Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2005-12-11 12:58:59.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2005-12-11 12:59:19.000000000 +0900
@@ -3688,7 +3688,7 @@ struct ata_queued_cmd *ata_qc_new_init(s
 static void __ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
-	unsigned int tag, do_clear = 0;
+	unsigned int tag;
 
 	qc->flags = 0;
 	tag = qc->tag;
@@ -3696,17 +3696,8 @@ static void __ata_qc_complete(struct ata
 		if (tag == ap->active_tag)
 			ap->active_tag = ATA_TAG_POISON;
 		qc->tag = ATA_TAG_POISON;
-		do_clear = 1;
-	}
-
-	if (qc->waiting) {
-		struct completion *waiting = qc->waiting;
-		qc->waiting = NULL;
-		complete(waiting);
-	}
-
-	if (likely(do_clear))
 		clear_bit(tag, &ap->qactive);
+	}
 }
 
 /**
@@ -3722,7 +3713,6 @@ static void __ata_qc_complete(struct ata
 void ata_qc_free(struct ata_queued_cmd *qc)
 {
 	assert(qc != NULL);	/* ata_qc_from_tag _might_ return NULL */
-	assert(qc->waiting == NULL);	/* nothing should be waiting */
 
 	__ata_qc_complete(qc);
 }
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h	2005-12-11 12:54:07.000000000 +0900
+++ work/include/linux/libata.h	2005-12-11 12:59:19.000000000 +0900
@@ -289,8 +289,6 @@ struct ata_queued_cmd {
 
 	ata_qc_cb_t		complete_fn;
 
-	struct completion	*waiting;
-
 	void			*private_data;
 };
 

Re: [PATCH 1/4] libata: implement ata_exec_internal()

[Jeff Garzik]

Tejun Heo wrote:
> This patch implements ata_exec_internal() function which performs
> libata internal command execution.  Previously, this was done by each
> user by manually initializing a qc, issueing it, waiting for its
> completion and handling errors.  In addition to obvious code
> factoring, using ata_exec_internal() fixes the following bugs.
> 
> * qc not freed on issue failure
> * ap->qactive clearing could race with the next internal command
> * race between timeout handling and irq
> * ignoring error condition not represented in tf->status
> 
> Also, qc & hardware are not accessed anymore once it's completed,
> making internal commands more conformant with general semantics.
> ata_exec_internal() also makes it easy to issue internal commands from
> multiple threads if that becomes necessary.
> 
> This patch only implements ata_exec_internal().  A following patch
> will convert all users.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

I ACK this version of the patchset, patches 1-4, but they don't seem to 
apply to 'upstream' branch:

[jgarzik@pretzel libata-dev]$ git-applymbox /g/tmp/mbox ~/info/signoff.txt
4 patch(es) to process.

Applying 'libata: implement ata_exec_internal()'

error: patch failed: include/linux/libata.h:136
error: include/linux/libata.h: patch does not apply

Re: [PATCH 1/4] libata: implement ata_exec_internal()

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
> 
>> This patch implements ata_exec_internal() function which performs
>> libata internal command execution.  Previously, this was done by each
>> user by manually initializing a qc, issueing it, waiting for its
>> completion and handling errors.  In addition to obvious code
>> factoring, using ata_exec_internal() fixes the following bugs.
>>
>> * qc not freed on issue failure
>> * ap->qactive clearing could race with the next internal command
>> * race between timeout handling and irq
>> * ignoring error condition not represented in tf->status
>>
>> Also, qc & hardware are not accessed anymore once it's completed,
>> making internal commands more conformant with general semantics.
>> ata_exec_internal() also makes it easy to issue internal commands from
>> multiple threads if that becomes necessary.
>>
>> This patch only implements ata_exec_internal().  A following patch
>> will convert all users.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> 
> I ACK this version of the patchset, patches 1-4, but they don't seem to 
> apply to 'upstream' branch:
> 
> [jgarzik@pretzel libata-dev]$ git-applymbox /g/tmp/mbox ~/info/signoff.txt
> 4 patch(es) to process.
> 
> Applying 'libata: implement ata_exec_internal()'
> 
> error: patch failed: include/linux/libata.h:136
> error: include/linux/libata.h: patch does not apply
> 

Oh... Sorry, all patches are against the ALL branch.  Do you want me to 
regenerate them against upstream?

Also, I'm currently working on a series of EH improvement patches 
against the ALL branch.  Would generating patches against upstream be 
better?

Thanks.

-- 
tejun

Re: [PATCH 1/4] libata: implement ata_exec_internal()

[Jeff Garzik]

Tejun Heo wrote:
> Oh... Sorry, all patches are against the ALL branch.  Do you want me to 
> regenerate them against upstream?

Yes, please.


> Also, I'm currently working on a series of EH improvement patches 
> against the ALL branch.  Would generating patches against upstream be 
> better?

Unfortunately, generating patches against the ALL branch makes a lot of 
sense -- but it's also the worst choice you could make.

Generating patches against the ALL branch means that your patch becomes 
dependent on -all- branches in libata-dev.git, even the branches that 
aren't going upstream for quite a while...  which is generally not what 
you want at all.

ALL is largely for testing and automated push to -mm, but not really for 
developers.

Sometimes you will run into cases where I slacked off on the merge of 
upstream+irq-pio+etc and created a bug, but most of the time the merge 
is easy.  If there are ALL-specific patches, you are welcome to submit 
them separately, with the recogition that any such ALL-specific patch 
would likely be dropped and recreated each time ALL gets blown away, and 
recreated.

	Jeff

Re: [PATCH 1/4] libata: implement ata_exec_internal()

[Jeff Garzik]

Tejun Heo wrote:
> Oh... Sorry, all patches are against the ALL branch.  Do you want me to 
> regenerate them against upstream?

Yes, please.


> Also, I'm currently working on a series of EH improvement patches 
> against the ALL branch.  Would generating patches against upstream be 
> better?

Diffing against ALL means that your patches become dependent on all 
libata-dev.git branches, even ones that aren't going upstream for a long 
time.  Plus, ALL is routinely blown away and recreated, when the merge 
gets too messy.

	Jeff

[PATCH 1/4] libata: implement ata_exec_internal()

[Tejun Heo]

This patch implements ata_exec_internal() function which performs
libata internal command execution.  Previously, this was done by each
user by manually initializing a qc, issueing it, waiting for its
completion and handling errors.  In addition to obvious code
factoring, using ata_exec_internal() fixes the following bugs.

* qc not freed on issue failure
* ap->qactive clearing could race with the next internal command
* race between timeout handling and irq
* ignoring error condition not represented in tf->status

Also, qc & hardware are not accessed anymore once it's completed,
making internal commands more conformant with general semantics.
ata_exec_internal() also makes it easy to issue internal commands from
multiple threads if that becomes necessary.

This patch only implements ata_exec_internal().  A following patch
will convert all users.

Signed-off-by: Tejun Heo <htejun@gmail.com>

--

Jeff, all patches have been regenerated against upstream branch as of
today.  (575ab52a218e4ff0667a6cbd972c3af443ee8713)

Also, I took out a debug printk from ata_exec_internal (don't know how
that one got left there).  Other than that, all patches are identical
to the previous posting.

Thanks. :-)

Index: work2/drivers/scsi/libata-core.c
===================================================================
--- work2.orig/drivers/scsi/libata-core.c	2005-12-13 14:37:10.000000000 +0900
+++ work2/drivers/scsi/libata-core.c	2005-12-13 14:37:41.000000000 +0900
@@ -1046,6 +1046,105 @@ static unsigned int ata_pio_modes(const 
 	return modes;
 }
 
+struct ata_exec_internal_arg {
+	unsigned int err_mask;
+	struct ata_taskfile *tf;
+	struct completion *waiting;
+};
+
+int ata_qc_complete_internal(struct ata_queued_cmd *qc)
+{
+	struct ata_exec_internal_arg *arg = qc->private_data;
+	struct completion *waiting = arg->waiting;
+
+	if (!(qc->err_mask & ~AC_ERR_DEV))
+		qc->ap->ops->tf_read(qc->ap, arg->tf);
+	arg->err_mask = qc->err_mask;
+	arg->waiting = NULL;
+	complete(waiting);
+
+	return 0;
+}
+
+/**
+ *	ata_exec_internal - execute libata internal command
+ *	@ap: Port to which the command is sent
+ *	@dev: Device to which the command is sent
+ *	@tf: Taskfile registers for the command and the result
+ *	@dma_dir: Data tranfer direction of the command
+ *	@buf: Data buffer of the command
+ *	@buflen: Length of data buffer
+ *
+ *	Executes libata internal command with timeout.  @tf contains
+ *	command on entry and result on return.  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.
+ *
+ *	LOCKING:
+ *	None.  Should be called with kernel context, might sleep.
+ */
+
+static unsigned
+ata_exec_internal(struct ata_port *ap, struct ata_device *dev,
+		  struct ata_taskfile *tf,
+		  int dma_dir, void *buf, unsigned int buflen)
+{
+	u8 command = tf->command;
+	struct ata_queued_cmd *qc;
+	DECLARE_COMPLETION(wait);
+	unsigned long flags;
+	struct ata_exec_internal_arg arg;
+
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+
+	qc = ata_qc_new_init(ap, dev);
+	BUG_ON(qc == NULL);
+
+	qc->tf = *tf;
+	qc->dma_dir = dma_dir;
+	if (dma_dir != DMA_NONE) {
+		ata_sg_init_one(qc, buf, buflen);
+		qc->nsect = buflen / ATA_SECT_SIZE;
+	}
+
+	arg.waiting = &wait;
+	arg.tf = tf;
+	qc->private_data = &arg;
+	qc->complete_fn = ata_qc_complete_internal;
+
+	if (ata_qc_issue(qc))
+		goto issue_fail;
+
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+	if (!wait_for_completion_timeout(&wait, ATA_TMOUT_INTERNAL)) {
+		spin_lock_irqsave(&ap->host_set->lock, flags);
+
+		/* 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 (arg.waiting) {
+			qc->err_mask = AC_ERR_OTHER;
+			ata_qc_complete(qc);
+			printk(KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n",
+			       ap->id, command);
+		}
+
+		spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	}
+
+	return arg.err_mask;
+
+ issue_fail:
+	ata_qc_free(qc);
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	return AC_ERR_OTHER;
+}
+
 static int ata_qc_wait_err(struct ata_queued_cmd *qc,
 			   struct completion *wait)
 {
Index: work2/include/linux/libata.h
===================================================================
--- work2.orig/include/linux/libata.h	2005-12-13 14:37:10.000000000 +0900
+++ work2/include/linux/libata.h	2005-12-13 14:37:11.000000000 +0900
@@ -135,6 +135,8 @@ enum {
 	ATA_TMOUT_BOOT_QUICK	= 7 * HZ,	/* hueristic */
 	ATA_TMOUT_CDB		= 30 * HZ,
 	ATA_TMOUT_CDB_QUICK	= 5 * HZ,
+	ATA_TMOUT_INTERNAL	= 30 * HZ,
+	ATA_TMOUT_INTERNAL_QUICK = 5 * HZ,
 
 	/* ATA bus states */
 	BUS_UNKNOWN		= 0,

[PATCH 2/4] libata: use ata_exec_internal()

[Tejun Heo]

This patch converts all users of libata internal commands to use
ata_exec_internal().

Signed-off-by: Tejun Heo <htejun@gmail.com>

Index: work2/drivers/scsi/libata-core.c
===================================================================
--- work2.orig/drivers/scsi/libata-core.c	2005-12-13 14:37:41.000000000 +0900
+++ work2/drivers/scsi/libata-core.c	2005-12-13 14:37:49.000000000 +0900
@@ -1198,9 +1198,8 @@ static void ata_dev_identify(struct ata_
 	u16 tmp;
 	unsigned long xfer_modes;
 	unsigned int using_edd;
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	unsigned long flags;
+	struct ata_taskfile tf;
+	unsigned int err_mask;
 	int rc;
 
 	if (!ata_dev_present(dev)) {
@@ -1221,40 +1220,26 @@ static void ata_dev_identify(struct ata_
 
 	ata_dev_select(ap, device, 1, 1); /* select device 0/1 */
 
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
-
-	ata_sg_init_one(qc, dev->id, sizeof(dev->id));
-	qc->dma_dir = DMA_FROM_DEVICE;
-	qc->tf.protocol = ATA_PROT_PIO;
-	qc->nsect = 1;
-
 retry:
+	ata_tf_init(ap, &tf, device);
+
 	if (dev->class == ATA_DEV_ATA) {
-		qc->tf.command = ATA_CMD_ID_ATA;
+		tf.command = ATA_CMD_ID_ATA;
 		DPRINTK("do ATA identify\n");
 	} else {
-		qc->tf.command = ATA_CMD_ID_ATAPI;
+		tf.command = ATA_CMD_ID_ATAPI;
 		DPRINTK("do ATAPI identify\n");
 	}
 
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	tf.protocol = ATA_PROT_PIO;
 
-	if (rc)
-		goto err_out;
-	else
-		ata_qc_wait_err(qc, &wait);
+	err_mask = ata_exec_internal(ap, dev, &tf, DMA_FROM_DEVICE,
+				     dev->id, sizeof(dev->id));
 
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	ap->ops->tf_read(ap, &qc->tf);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	if (err_mask) {
+		if (err_mask & ~AC_ERR_DEV)
+			goto err_out;
 
-	if (qc->tf.command & ATA_ERR) {
 		/*
 		 * arg!  EDD works for all test cases, but seems to return
 		 * the ATA signature for some ATAPI devices.  Until the
@@ -1267,14 +1252,9 @@ retry:
 		 * to have this problem.
 		 */
 		if ((using_edd) && (dev->class == ATA_DEV_ATA)) {
-			u8 err = qc->tf.feature;
+			u8 err = tf.feature;
 			if (err & ATA_ABORTED) {
 				dev->class = ATA_DEV_ATAPI;
-				qc->cursg = 0;
-				qc->cursg_ofs = 0;
-				qc->cursect = 0;
-				qc->nsect = 1;
-				qc->err_mask = 0;
 				goto retry;
 			}
 		}
@@ -2378,34 +2358,23 @@ static int ata_choose_xfer_mode(const st
 
 static void ata_dev_set_xfermode(struct ata_port *ap, struct ata_device *dev)
 {
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	int rc;
-	unsigned long flags;
+	struct ata_taskfile tf;
 
 	/* set up set-features taskfile */
 	DPRINTK("set features - xfer mode\n");
 
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
-
-	qc->tf.command = ATA_CMD_SET_FEATURES;
-	qc->tf.feature = SETFEATURES_XFER;
-	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-	qc->tf.protocol = ATA_PROT_NODATA;
-	qc->tf.nsect = dev->xfer_mode;
-
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	ata_tf_init(ap, &tf, dev->devno);
+	tf.command = ATA_CMD_SET_FEATURES;
+	tf.feature = SETFEATURES_XFER;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.nsect = dev->xfer_mode;
 
-	if (rc)
+	if (ata_exec_internal(ap, dev, &tf, DMA_NONE, NULL, 0)) {
+		printk(KERN_ERR "ata%u: failed to set xfermode, disabled\n",
+		       ap->id);
 		ata_port_disable(ap);
-	else
-		ata_qc_wait_err(qc, &wait);
+	}
 
 	DPRINTK("EXIT\n");
 }
@@ -2420,41 +2389,25 @@ static void ata_dev_set_xfermode(struct 
 
 static void ata_dev_reread_id(struct ata_port *ap, struct ata_device *dev)
 {
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	unsigned long flags;
-	int rc;
-
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
+	struct ata_taskfile tf;
 
-	ata_sg_init_one(qc, dev->id, sizeof(dev->id));
-	qc->dma_dir = DMA_FROM_DEVICE;
+	ata_tf_init(ap, &tf, dev->devno);
 
 	if (dev->class == ATA_DEV_ATA) {
-		qc->tf.command = ATA_CMD_ID_ATA;
+		tf.command = ATA_CMD_ID_ATA;
 		DPRINTK("do ATA identify\n");
 	} else {
-		qc->tf.command = ATA_CMD_ID_ATAPI;
+		tf.command = ATA_CMD_ID_ATAPI;
 		DPRINTK("do ATAPI identify\n");
 	}
 
-	qc->tf.flags |= ATA_TFLAG_DEVICE;
-	qc->tf.protocol = ATA_PROT_PIO;
-	qc->nsect = 1;
+	tf.flags |= ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_PIO;
 
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
-
-	if (rc)
+	if (ata_exec_internal(ap, dev, &tf, DMA_FROM_DEVICE,
+			      dev->id, sizeof(dev->id)))
 		goto err_out;
 
-	ata_qc_wait_err(qc, &wait);
-
 	swap_buf_le16(dev->id, ATA_ID_WORDS);
 
 	ata_dump_id(dev);
@@ -2463,6 +2416,7 @@ static void ata_dev_reread_id(struct ata
 
 	return;
 err_out:
+	printk(KERN_ERR "ata%u: failed to reread ID, disabled\n", ap->id);
 	ata_port_disable(ap);
 }
 
@@ -2476,10 +2430,7 @@ err_out:
 
 static void ata_dev_init_params(struct ata_port *ap, struct ata_device *dev)
 {
-	DECLARE_COMPLETION(wait);
-	struct ata_queued_cmd *qc;
-	int rc;
-	unsigned long flags;
+	struct ata_taskfile tf;
 	u16 sectors = dev->id[6];
 	u16 heads   = dev->id[3];
 
@@ -2490,26 +2441,18 @@ static void ata_dev_init_params(struct a
 	/* set up init dev params taskfile */
 	DPRINTK("init dev params \n");
 
-	qc = ata_qc_new_init(ap, dev);
-	BUG_ON(qc == NULL);
-
-	qc->tf.command = ATA_CMD_INIT_DEV_PARAMS;
-	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
-	qc->tf.protocol = ATA_PROT_NODATA;
-	qc->tf.nsect = sectors;
-	qc->tf.device |= (heads - 1) & 0x0f; /* max head = num. of heads - 1 */
-
-	qc->waiting = &wait;
-	qc->complete_fn = ata_qc_complete_noop;
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	rc = ata_qc_issue(qc);
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	ata_tf_init(ap, &tf, dev->devno);
+	tf.command = ATA_CMD_INIT_DEV_PARAMS;
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.nsect = sectors;
+	tf.device |= (heads - 1) & 0x0f; /* max head = num. of heads - 1 */
 
-	if (rc)
+	if (ata_exec_internal(ap, dev, &tf, DMA_NONE, NULL, 0)) {
+		printk(KERN_ERR "ata%u: failed to init parameters, disabled\n",
+		       ap->id);
 		ata_port_disable(ap);
-	else
-		ata_qc_wait_err(qc, &wait);
+	}
 
 	DPRINTK("EXIT\n");
 }

[PATCH 3/4] libata: remove unused functions

[Tejun Heo]

There is no user of ata_qc_wait_err() and ata_qc_complete_noop() after
ata_exec_internal() changes.  Remove unused functions.

Signed-off-by: Tejun Heo <htejun@gmail.com>

Index: work2/drivers/scsi/libata-core.c
===================================================================
--- work2.orig/drivers/scsi/libata-core.c	2005-12-13 14:37:49.000000000 +0900
+++ work2/drivers/scsi/libata-core.c	2005-12-13 14:37:51.000000000 +0900
@@ -1145,30 +1145,6 @@ ata_exec_internal(struct ata_port *ap, s
 	return AC_ERR_OTHER;
 }
 
-static int ata_qc_wait_err(struct ata_queued_cmd *qc,
-			   struct completion *wait)
-{
-	int rc = 0;
-
-	if (wait_for_completion_timeout(wait, 30 * HZ) < 1) {
-		/* timeout handling */
-		qc->err_mask |= ac_err_mask(ata_chk_status(qc->ap));
-
-		if (!qc->err_mask) {
-			printk(KERN_WARNING "ata%u: slow completion (cmd %x)\n",
-			       qc->ap->id, qc->tf.command);
-		} else {
-			printk(KERN_WARNING "ata%u: qc timeout (cmd %x)\n",
-			       qc->ap->id, qc->tf.command);
-			rc = -EIO;
-		}
-
-		ata_qc_complete(qc);
-	}
-
-	return rc;
-}
-
 /**
  *	ata_dev_identify - obtain IDENTIFY x DEVICE page
  *	@ap: port on which device we wish to probe resides
@@ -3524,11 +3500,6 @@ struct ata_queued_cmd *ata_qc_new_init(s
 	return qc;
 }
 
-int ata_qc_complete_noop(struct ata_queued_cmd *qc)
-{
-	return 0;
-}
-
 static void __ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
Index: work2/drivers/scsi/libata.h
===================================================================
--- work2.orig/drivers/scsi/libata.h	2005-12-13 14:37:10.000000000 +0900
+++ work2/drivers/scsi/libata.h	2005-12-13 14:37:51.000000000 +0900
@@ -39,7 +39,6 @@ struct ata_scsi_args {
 
 /* libata-core.c */
 extern int atapi_enabled;
-extern int ata_qc_complete_noop(struct ata_queued_cmd *qc);
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
 				      struct ata_device *dev);
 extern void ata_rwcmd_protocol(struct ata_queued_cmd *qc);

[PATCH 4/4] libata: remove unused qc->waiting

[Tejun Heo]

There is no user of qc->waiting left after ata_exec_internal()
changes.  Kill the field.

Signed-off-by: Tejun Heo <htejun@gmail.com>

Index: work2/drivers/scsi/libata-core.c
===================================================================
--- work2.orig/drivers/scsi/libata-core.c	2005-12-13 14:37:51.000000000 +0900
+++ work2/drivers/scsi/libata-core.c	2005-12-13 14:37:54.000000000 +0900
@@ -3503,7 +3503,7 @@ struct ata_queued_cmd *ata_qc_new_init(s
 static void __ata_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
-	unsigned int tag, do_clear = 0;
+	unsigned int tag;
 
 	qc->flags = 0;
 	tag = qc->tag;
@@ -3511,17 +3511,8 @@ static void __ata_qc_complete(struct ata
 		if (tag == ap->active_tag)
 			ap->active_tag = ATA_TAG_POISON;
 		qc->tag = ATA_TAG_POISON;
-		do_clear = 1;
-	}
-
-	if (qc->waiting) {
-		struct completion *waiting = qc->waiting;
-		qc->waiting = NULL;
-		complete(waiting);
-	}
-
-	if (likely(do_clear))
 		clear_bit(tag, &ap->qactive);
+	}
 }
 
 /**
@@ -3537,7 +3528,6 @@ static void __ata_qc_complete(struct ata
 void ata_qc_free(struct ata_queued_cmd *qc)
 {
 	assert(qc != NULL);	/* ata_qc_from_tag _might_ return NULL */
-	assert(qc->waiting == NULL);	/* nothing should be waiting */
 
 	__ata_qc_complete(qc);
 }
Index: work2/include/linux/libata.h
===================================================================
--- work2.orig/include/linux/libata.h	2005-12-13 14:37:11.000000000 +0900
+++ work2/include/linux/libata.h	2005-12-13 14:37:54.000000000 +0900
@@ -285,8 +285,6 @@ struct ata_queued_cmd {
 
 	ata_qc_cb_t		complete_fn;
 
-	struct completion	*waiting;
-
 	void			*private_data;
 };
 

Re: [PATCH 1/4] libata: implement ata_exec_internal()

[Jeff Garzik]

applied 1-4

Re: [PATCH 1/4] libata: implement ata_exec_internal()

[Albert Lee]

Tejun Heo wrote:
> This patch implements ata_exec_internal() function which performs
> libata internal command execution.  Previously, this was done by each
> user by manually initializing a qc, issueing it, waiting for its
> completion and handling errors.  In addition to obvious code
> factoring, using ata_exec_internal() fixes the following bugs.
> 
> * qc not freed on issue failure
> * ap->qactive clearing could race with the next internal command
> * race between timeout handling and irq
> * ignoring error condition not represented in tf->status

Thanks, this fixes the "phantom slave device" problem on my machine. :)

> 
> Also, qc & hardware are not accessed anymore once it's completed,
> making internal commands more conformant with general semantics.
> ata_exec_internal() also makes it easy to issue internal commands from
> multiple threads if that becomes necessary.
> 
> This patch only implements ata_exec_internal().  A following patch
> will convert all users.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
>