linux-ide.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Tejun Heo <htejun@gmail.com>
To: Jeff Garzik <jgarzik@pobox.com>
Cc: Albert Lee <albertcc@tw.ibm.com>, linux-ide@vger.kernel.org
Subject: [PATCH 1/4] libata: implement ata_exec_internal()
Date: Sun, 11 Dec 2005 13:23:03 +0900	[thread overview]
Message-ID: <20051211042303.GA29403@htj.dyndns.org> (raw)
In-Reply-To: <43994F85.7040300@gmail.com>

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

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

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

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

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

--

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

* adapted to late qc->err_mask change

Changes to the second posting from the first are

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

Thanks.


Index: work/drivers/scsi/libata-core.c
===================================================================
--- work.orig/drivers/scsi/libata-core.c	2005-12-11 12:54:06.000000000 +0900
+++ work/drivers/scsi/libata-core.c	2005-12-11 12:54:07.000000000 +0900
@@ -1047,6 +1047,107 @@ static unsigned int ata_pio_modes(const 
 	return modes;
 }
 
+struct ata_exec_internal_arg {
+	unsigned int err_mask;
+	struct ata_taskfile *tf;
+	struct completion *waiting;
+};
+
+int ata_qc_complete_internal(struct ata_queued_cmd *qc)
+{
+	struct ata_exec_internal_arg *arg = qc->private_data;
+	struct completion *waiting = arg->waiting;
+
+	if (!(qc->err_mask & ~AC_ERR_DEV))
+		qc->ap->ops->tf_read(qc->ap, arg->tf);
+	arg->err_mask = qc->err_mask;
+	arg->waiting = NULL;
+	complete(waiting);
+
+	return 0;
+}
+
+/**
+ *	ata_exec_internal - execute libata internal command
+ *	@ap: Port to which the command is sent
+ *	@dev: Device to which the command is sent
+ *	@tf: Taskfile registers for the command and the result
+ *	@dma_dir: Data tranfer direction of the command
+ *	@buf: Data buffer of the command
+ *	@buflen: Length of data buffer
+ *
+ *	Executes libata internal command with timeout.  @tf contains
+ *	command on entry and result on return.  Timeout and error
+ *	conditions are reported via return value.  No recovery action
+ *	is taken after a command times out.  It's caller's duty to
+ *	clean up after timeout.
+ *
+ *	LOCKING:
+ *	None.  Should be called with kernel context, might sleep.
+ */
+
+static unsigned
+ata_exec_internal(struct ata_port *ap, struct ata_device *dev,
+		  struct ata_taskfile *tf,
+		  int dma_dir, void *buf, unsigned int buflen)
+{
+	u8 command = tf->command;
+	struct ata_queued_cmd *qc;
+	DECLARE_COMPLETION(wait);
+	unsigned long flags;
+	struct ata_exec_internal_arg arg;
+
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+
+	qc = ata_qc_new_init(ap, dev);
+	BUG_ON(qc == NULL);
+
+	qc->tf = *tf;
+	qc->dma_dir = dma_dir;
+	if (dma_dir != DMA_NONE) {
+		ata_sg_init_one(qc, buf, buflen);
+		qc->nsect = buflen / ATA_SECT_SIZE;
+		printk("dma_dir=%d buflen=%u nsect=%d\n",
+		       dma_dir, buflen, qc->nsect);
+	}
+
+	arg.waiting = &wait;
+	arg.tf = tf;
+	qc->private_data = &arg;
+	qc->complete_fn = ata_qc_complete_internal;
+
+	if (ata_qc_issue(qc))
+		goto issue_fail;
+
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+	if (!wait_for_completion_timeout(&wait, ATA_TMOUT_INTERNAL)) {
+		spin_lock_irqsave(&ap->host_set->lock, flags);
+
+		/* We're racing with irq here.  If we lose, the
+		 * following test prevents us from completing the qc
+		 * again.  If completion irq occurs after here but
+		 * before the caller cleans up, it will result in a
+		 * spurious interrupt.  We can live with that.
+		 */
+		if (arg.waiting) {
+			qc->err_mask = AC_ERR_OTHER;
+			ata_qc_complete(qc);
+			printk(KERN_WARNING "ata%u: qc timeout (cmd 0x%x)\n",
+			       ap->id, command);
+		}
+
+		spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	}
+
+	return arg.err_mask;
+
+ issue_fail:
+	ata_qc_free(qc);
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	return AC_ERR_OTHER;
+}
+
 static int ata_qc_wait_err(struct ata_queued_cmd *qc,
 			   struct completion *wait)
 {
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h	2005-12-11 12:54:06.000000000 +0900
+++ work/include/linux/libata.h	2005-12-11 12:54:07.000000000 +0900
@@ -136,6 +136,8 @@ enum {
 	ATA_TMOUT_BOOT_QUICK	= 7 * HZ,	/* hueristic */
 	ATA_TMOUT_DATAOUT	= 30 * HZ,
 	ATA_TMOUT_DATAOUT_QUICK	= 5 * HZ,
+	ATA_TMOUT_INTERNAL	= 30 * HZ,
+	ATA_TMOUT_INTERNAL_QUICK = 5 * HZ,
 
 	/* ATA bus states */
 	BUS_UNKNOWN		= 0,

  reply	other threads:[~2005-12-11  4:23 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2005-11-28 16:42 [PATCH] libata: implement ata_qc_exec_internal() Tejun Heo
2005-11-28 17:48 ` Raz Ben-Jehuda(caro)
2005-11-29  3:08   ` Tejun Heo
2005-11-29  3:09     ` Tejun Heo
2005-11-29  3:12       ` Tejun Heo
2005-11-29  7:41 ` Albert Lee
2005-11-29 13:17   ` [PATCH 1/2] libata: implement ata_exec_internal() Tejun Heo
2005-12-04  1:50     ` Jeff Garzik
2005-12-04 14:04       ` Tejun Heo
2005-12-04 19:21         ` Jeff Garzik
2005-12-05  8:26           ` [PATCH 1/3] " Tejun Heo
2005-12-05  8:28           ` [PATCH 2/4] libata: use ata_exec_internal() Tejun Heo
2005-12-05  8:30           ` [PATCH 3/4] libata: remove unused functions Tejun Heo
2005-12-05  8:31           ` [PATCH 4/4] libata: remove unused qc->waiting Tejun Heo
2005-12-09  9:33           ` [PATCH 1/2] libata: implement ata_exec_internal() Tejun Heo
2005-12-11  4:23             ` Tejun Heo [this message]
2005-12-13  4:58               ` [PATCH 1/4] " Jeff Garzik
2005-12-13  5:06                 ` Tejun Heo
2005-12-13  5:18                   ` Jeff Garzik
2005-12-13  5:48                     ` Tejun Heo
2005-12-13  5:49                       ` [PATCH 2/4] libata: use ata_exec_internal() Tejun Heo
2005-12-13  5:50                       ` [PATCH 3/4] libata: remove unused functions Tejun Heo
2005-12-13  5:51                       ` [PATCH 4/4] libata: remove unused qc->waiting Tejun Heo
2005-12-13  6:34                       ` [PATCH 1/4] libata: implement ata_exec_internal() Jeff Garzik
2005-12-14 10:01                       ` Albert Lee
2005-12-13  5:22                   ` Jeff Garzik
2005-12-11  4:24             ` [PATCH 2/4] libata: use ata_exec_internal() Tejun Heo
2005-12-11  4:25             ` [PATCH 3/4] libata: removed unused functions Tejun Heo
2005-12-11  4:26             ` [PATCH 4/4] libata: remove unused qc->waiting Tejun Heo
2005-11-29 13:19   ` [PATCH 2/2] libata: remove qc->waiting Tejun Heo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20051211042303.GA29403@htj.dyndns.org \
    --to=htejun@gmail.com \
    --cc=albertcc@tw.ibm.com \
    --cc=jgarzik@pobox.com \
    --cc=linux-ide@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).