Skip to contentSkip to conversation

libata: implement ata_qc_exec_internal()

PATCH In review Tejun Heo 4 participants 30 messages Nov 28, 2005 – Dec 14, 2005

Subsystems (2)
lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/1 libata: implement ata_qc_exec_internal()

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.

drivers/scsi/libata-core.c +76 -66
index f1047a0..40c488e 100644
@@ -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");
 }
drivers/scsi/libata.h +0 -0
include/linux/libata.h +2 -0
index 9aa10df..2fe64f6 100644
@@ -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,
#1 lore raw

does it solve the cpu usage ?

Quoted 260 lines · 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.

drivers/scsi/libata-core.c +76 -66
index f1047a0..40c488e 100644
@@ -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");
 }
drivers/scsi/libata.h +0 -0
include/linux/libata.h +2 -1
index 9aa10df..2fe64f6 100644
@@ -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,
-
Quoted 4 lines

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

--
Raz

author
#2 lore raw
Quoted 11 lines · Raz Ben-Jehuda(caro)

does it solve the cpu usage ?

Quoted 7 lines · 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.

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.

Signature · 1 line
tejun
author
#3 lore raw
Quoted 14 lines · Tejun Heo
Quoted 11 lines · Raz Ben-Jehuda(caro)

does it solve the cpu usage ?

Quoted 7 lines · 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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Signature · 1 line
tejun
author
#4 lore raw
Quoted 18 lines · Tejun Heo
Quoted 16 lines · Tejun Heo
Quoted 11 lines · Raz Ben-Jehuda(caro)

does it solve the cpu usage ?

Quoted 7 lines · 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.

Hi, Raz.

Quoted 8 lines
Quoted 5 lines

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. :-)

Signature · 1 line
tejun
#5 lore raw
In reply to Tejun Heo
Quoted 54 lines · 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.

drivers/scsi/libata-core.c +29 -7
index f1047a0..40c488e 100644
@@ -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;

drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1091

Quoted 10 lines
drivers/scsi/libata-core.c +10 -0
+
+	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

Quoted 32 lines
drivers/scsi/libata-core.c +18 -5
+
+	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;
 }
 
author
#6 lore raw

PATCH 1/2 libata: implement ata_exec_internal()

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.

--

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).

drivers/scsi/libata-core.c +152 -122
Index: work/drivers/scsi/libata-core.c
===================================================================
@@ -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;
drivers/scsi/libata.h +0 -1
Index: work/drivers/scsi/libata.h
===================================================================
@@ -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);
include/linux/libata.h +3 -0
Index: work/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,
@@ -284,6 +286,7 @@ struct ata_queued_cmd {
 	struct scatterlist	*__sg;
 
 	ata_qc_cb_t		complete_fn;
+	void			*complete_data;
 
 	struct completion	*waiting;
 
#7 lore raw

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

Quoted 10 lines · 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

Quoted 6 lines

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.

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.

Quoted 15 lines

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
===================================================================

drivers/scsi/libata-core.c +0 -2
@@ -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)

drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1058

Quoted 10 lines
drivers/scsi/libata-core.c +8 -1
+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'.

Quoted 45 lines
drivers/scsi/libata-core.c +32 -12
+	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,

drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1101

Quoted 10 lines
drivers/scsi/libata-core.c +10 -0
+		  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.

Quoted 28 lines
drivers/scsi/libata-core.c +28 -0
+	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);

drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1140

Quoted 10 lines
drivers/scsi/libata-core.c +10 -0
+
+		/* 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.

Quoted 255 lines
drivers/scsi/libata-core.c +61 -102
+		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");
author
#8 lore raw

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

Hello, Jeff.

Quoted 17 lines · Jeff Garzik
Quoted 15 lines · 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.

Quoted 7 lines
Quoted 1 line

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.

Quoted 16 lines
Quoted 16 lines

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

drivers/scsi/libata-core.c +0 -0
@@ -1047,28 +1047,119 @@ static unsigned int ata_pio_modes(const      
 return modes;
 }
 
Quoted 17 lines

drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1058

Quoted 13 lines
drivers/scsi/libata-core.c +8 -3
-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.

Quoted 61 lines
Quoted 47 lines
drivers/scsi/libata-core.c +33 -12
+    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)

drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1101

Quoted 10 lines
drivers/scsi/libata-core.c +9 -0
+{
+    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.

Quoted 2 lines

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?

Quoted 34 lines
Quoted 34 lines
drivers/scsi/libata-core.c +33 -0
+    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
Quoted 9 lines

drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1140

Quoted 5 lines
drivers/scsi/libata-core.c +5 -0
+         * 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.

Signature · 1 line
tejun
#9 lore raw

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

Quoted 37 lines · Tejun Heo
Quoted 26 lines · Jeff Garzik
Quoted 24 lines · Tejun Heo

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

drivers/scsi/libata-core.c +7 -2
@@ -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 
Quoted 9 lines

drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1058

Quoted 4 lines
drivers/scsi/libata-core.c +1 -1
 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'.

Quoted 3 lines

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.

Quoted 61 lines
Quoted 61 lines
Quoted 46 lines
drivers/scsi/libata-core.c +33 -12
+    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)

drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1101

Quoted 10 lines
drivers/scsi/libata-core.c +9 -0
+{
+    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.

Quoted 4 lines
Quoted 2 lines

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 :)

Quoted 44 lines
Quoted 34 lines
Quoted 34 lines
drivers/scsi/libata-core.c +34 -0
+    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.
Quoted 9 lines

drivers/scsi/libata-core.c static unsigned int ata_pio_modes(const · line 1140

Quoted 4 lines
drivers/scsi/libata-core.c +4 -0
+         */
+        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.

Quoted 6 lines

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.

Quoted 3 lines

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

author
#10 lore raw

PATCH 1/3 libata: implement ata_exec_internal()

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.

--

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.

drivers/scsi/libata-core.c +100 -0
Index: work/drivers/scsi/libata-core.c
===================================================================
@@ -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)
 {
include/linux/libata.h +2 -0
Index: work/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,
author
#11 lore raw

PATCH 2/4 libata: use ata_exec_internal()

In reply to Jeff Garzik

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

--

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

drivers/scsi/libata-core.c +44 -100
Index: work/drivers/scsi/libata-core.c
===================================================================
@@ -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");
 }
author
#12 lore raw

PATCH 3/4 libata: remove unused functions

In reply to Jeff Garzik

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

drivers/scsi/libata-core.c +0 -29
Index: work/drivers/scsi/libata-core.c
===================================================================
@@ -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;
drivers/scsi/libata.h +0 -1
Index: work/drivers/scsi/libata.h
===================================================================
@@ -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);
author
#13 lore raw

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

In reply to Jeff Garzik

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

drivers/scsi/libata-core.c +2 -12
Index: work/drivers/scsi/libata-core.c
===================================================================
@@ -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);
 }
include/linux/libata.h +0 -2
Index: work/include/linux/libata.h
===================================================================
@@ -287,8 +287,6 @@ struct ata_queued_cmd {
 
 	ata_qc_cb_t		complete_fn;
 
-	struct completion	*waiting;
-
 	void			*private_data;
 };
 
author
#15 lore raw

PATCH 1/4 libata: implement ata_exec_internal()

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.

--

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.

drivers/scsi/libata-core.c +101 -0
Index: work/drivers/scsi/libata-core.c
===================================================================
@@ -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)
 {
include/linux/libata.h +2 -0
Index: work/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,
#16 lore raw

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

Quoted 16 lines · 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.

Quoted 4 lines

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

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

author
#17 lore raw

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

Quoted 26 lines · Jeff Garzik
Quoted 16 lines · 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.

Quoted 4 lines

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

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

Quoted 8 lines

[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.

Signature · 1 line
tejun
#18 lore raw

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

Quoted 1 line · Tejun Heo

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

Yes, please.

Quoted 1 line

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

author
#19 lore raw

PATCH 1/4 libata: implement ata_exec_internal()

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.

--

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. :-)

drivers/scsi/libata-core.c +99 -0
Index: work2/drivers/scsi/libata-core.c
===================================================================
@@ -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)
 {
include/linux/libata.h +2 -0
Index: work2/include/linux/libata.h
===================================================================
@@ -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,
author
#20 lore raw

PATCH 2/4 libata: use ata_exec_internal()

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

drivers/scsi/libata-core.c +44 -101
Index: work2/drivers/scsi/libata-core.c
===================================================================
@@ -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");
 }
author
#21 lore raw

PATCH 3/4 libata: remove unused functions

In reply to Tejun Heo

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

drivers/scsi/libata-core.c +0 -29
Index: work2/drivers/scsi/libata-core.c
===================================================================
@@ -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;
drivers/scsi/libata.h +0 -1
Index: work2/drivers/scsi/libata.h
===================================================================
@@ -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);
author
#22 lore raw

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

In reply to Tejun Heo

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

drivers/scsi/libata-core.c +2 -12
Index: work2/drivers/scsi/libata-core.c
===================================================================
@@ -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);
 }
include/linux/libata.h +0 -2
Index: work2/include/linux/libata.h
===================================================================
@@ -285,8 +285,6 @@ struct ata_queued_cmd {
 
 	ata_qc_cb_t		complete_fn;
 
-	struct completion	*waiting;
-
 	void			*private_data;
 };
 
#24 lore raw

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

In reply to Tejun Heo
Quoted 10 lines · 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

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

Quoted 11 lines

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.

#25 lore raw

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

In reply to Tejun Heo
Quoted 1 line · Tejun Heo

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

Yes, please.

Quoted 1 line

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

author
#26 lore raw

PATCH 2/4 libata: use ata_exec_internal()

In reply to Tejun Heo

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

drivers/scsi/libata-core.c +44 -101
Index: work/drivers/scsi/libata-core.c
===================================================================
@@ -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");
 }
author
#27 lore raw

PATCH 3/4 libata: removed unused functions

In reply to Tejun Heo

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

drivers/scsi/libata-core.c +0 -29
Index: work/drivers/scsi/libata-core.c
===================================================================
@@ -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;
drivers/scsi/libata.h +0 -1
Index: work/drivers/scsi/libata.h
===================================================================
@@ -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);
author
#28 lore raw

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

In reply to Tejun Heo

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

drivers/scsi/libata-core.c +2 -12
Index: work/drivers/scsi/libata-core.c
===================================================================
@@ -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);
 }
include/linux/libata.h +0 -2
Index: work/include/linux/libata.h
===================================================================
@@ -289,8 +289,6 @@ struct ata_queued_cmd {
 
 	ata_qc_cb_t		complete_fn;
 
-	struct completion	*waiting;
-
 	void			*private_data;
 };
 
author
#29 lore raw

PATCH 2/2 libata: remove qc->waiting

In reply to Albert Lee

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

drivers/scsi/libata-core.c +2 -12
Index: work/drivers/scsi/libata-core.c
===================================================================
@@ -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);
 }
include/linux/libata.h +0 -2
Index: work/include/linux/libata.h
===================================================================
@@ -288,8 +288,6 @@ struct ata_queued_cmd {
 	ata_qc_cb_t		complete_fn;
 	void			*complete_data;
 
-	struct completion	*waiting;
-
 	void			*private_data;
 };