[PATCH 3/4] libata: kill unused pio_task and packet_task

[PATCH 3/4] libata: kill unused pio_task and packet_task

[Tejun Heo]

Kill unused pio_task and packet_task.

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

---

 drivers/scsi/libata-core.c |   68 --------------------------------------------
 include/linux/libata.h     |    3 --
 2 files changed, 0 insertions(+), 71 deletions(-)

aaf1bca7a373df0a4e301da177d6c71329b98196
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index ebd4072..657b196 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -796,71 +796,6 @@ void ata_port_flush_task(struct ata_port
 	DPRINTK("EXIT\n");
 }
 
-static inline void
-ata_queue_packet_task(struct ata_port *ap)
-{
-	if (!(ap->flags & ATA_FLAG_FLUSH_PIO_TASK))
-		queue_work(ata_wq, &ap->packet_task);
-}
-
-static inline void
-ata_queue_pio_task(struct ata_port *ap)
-{
-	if (!(ap->flags & ATA_FLAG_FLUSH_PIO_TASK))
-		queue_work(ata_wq, &ap->pio_task);
-}
-
-static inline void
-ata_queue_delayed_pio_task(struct ata_port *ap, unsigned long delay)
-{
-	if (!(ap->flags & ATA_FLAG_FLUSH_PIO_TASK))
-		queue_delayed_work(ata_wq, &ap->pio_task, delay);
-}
-
-/**
- *	ata_flush_pio_tasks - Flush pio_task and packet_task
- *	@ap: the target ata_port
- *
- *	After this function completes, pio_task and packet_task are
- *	guranteed not to be running or scheduled.
- *
- *	LOCKING:
- *	Kernel thread context (may sleep)
- */
-
-static void ata_flush_pio_tasks(struct ata_port *ap)
-{
-	int tmp = 0;
-	unsigned long flags;
-
-	DPRINTK("ENTER\n");
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	ap->flags |= ATA_FLAG_FLUSH_PIO_TASK;
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
-
-	DPRINTK("flush #1\n");
-	flush_workqueue(ata_wq);
-
-	/*
-	 * At this point, if a task is running, it's guaranteed to see
-	 * the FLUSH flag; thus, it will never queue pio tasks again.
-	 * Cancel and flush.
-	 */
-	tmp |= cancel_delayed_work(&ap->pio_task);
-	tmp |= cancel_delayed_work(&ap->packet_task);
-	if (!tmp) {
-		DPRINTK("flush #2\n");
-		flush_workqueue(ata_wq);
-	}
-
-	spin_lock_irqsave(&ap->host_set->lock, flags);
-	ap->flags &= ~ATA_FLAG_FLUSH_PIO_TASK;
-	spin_unlock_irqrestore(&ap->host_set->lock, flags);
-
-	DPRINTK("EXIT\n");
-}
-
 void ata_qc_complete_internal(struct ata_queued_cmd *qc)
 {
 	struct completion *waiting = qc->private_data;
@@ -3675,7 +3610,6 @@ static void ata_qc_timeout(struct ata_qu
 
 	DPRINTK("ENTER\n");
 
-	ata_flush_pio_tasks(ap);
 	ap->hsm_task_state = HSM_ST_IDLE;
 
 	spin_lock_irqsave(&host_set->lock, flags);
@@ -4554,8 +4488,6 @@ static void ata_host_init(struct ata_por
 	ap->last_ctl = 0xFF;
 
 	INIT_WORK(&ap->port_task, NULL, NULL);
-	INIT_WORK(&ap->packet_task, atapi_packet_task, ap);
-	INIT_WORK(&ap->pio_task, ata_pio_task, ap);
 	INIT_LIST_HEAD(&ap->eh_done_q);
 
 	for (i = 0; i < ATA_MAX_DEVICES; i++)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index abc5c6e..d856660 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -390,9 +390,6 @@ struct ata_port {
 
 	struct work_struct	port_task;
 
-	struct work_struct	packet_task;
-
-	struct work_struct	pio_task;
 	unsigned int		hsm_task_state;
 	unsigned long		pio_task_timeout;
 
-- 
1.2.1

[PATCH 2/4] libata: convert pio_task and packet_task to port_task

[Tejun Heo]

Make pio_task and atapi_packet_task use port_task.
atapi_packet_task() is moved upward such that it's right after
ata_pio_task().  This position is more natural and makes adding
prototype for ata_qc_issue_prot() unnecessary.

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

---

 drivers/scsi/libata-core.c |  154 ++++++++++++++++++++++----------------------
 1 files changed, 77 insertions(+), 77 deletions(-)

b5b1469010fc9cd99e149f0503cbf6e6f1ac17b2
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 3575d68..ebd4072 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3570,12 +3570,84 @@ fsm_start:
 	}
 
 	if (timeout)
-		ata_queue_delayed_pio_task(ap, timeout);
+		ata_port_queue_task(ap, ata_pio_task, ap, timeout);
 	else if (!qc_completed)
 		goto fsm_start;
 }
 
 /**
+ *	atapi_packet_task - Write CDB bytes to hardware
+ *	@_data: Port to which ATAPI device is attached.
+ *
+ *	When device has indicated its readiness to accept
+ *	a CDB, this function is called.  Send the CDB.
+ *	If DMA is to be performed, exit immediately.
+ *	Otherwise, we are in polling mode, so poll
+ *	status under operation succeeds or fails.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ */
+
+static void atapi_packet_task(void *_data)
+{
+	struct ata_port *ap = _data;
+	struct ata_queued_cmd *qc;
+	u8 status;
+
+	qc = ata_qc_from_tag(ap, ap->active_tag);
+	WARN_ON(qc == NULL);
+	WARN_ON(!(qc->flags & ATA_QCFLAG_ACTIVE));
+
+	/* sleep-wait for BSY to clear */
+	DPRINTK("busy wait\n");
+	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB)) {
+		qc->err_mask |= AC_ERR_TIMEOUT;
+		goto err_out;
+	}
+
+	/* make sure DRQ is set */
+	status = ata_chk_status(ap);
+	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) {
+		qc->err_mask |= AC_ERR_HSM;
+		goto err_out;
+	}
+
+	/* send SCSI cdb */
+	DPRINTK("send cdb\n");
+	WARN_ON(qc->dev->cdb_len < 12);
+
+	if (qc->tf.protocol == ATA_PROT_ATAPI_DMA ||
+	    qc->tf.protocol == ATA_PROT_ATAPI_NODATA) {
+		unsigned long flags;
+
+		/* Once we're done issuing command and kicking bmdma,
+		 * irq handler takes over.  To not lose irq, we need
+		 * to clear NOINTR flag before sending cdb, but
+		 * interrupt handler shouldn't be invoked before we're
+		 * finished.  Hence, the following locking.
+		 */
+		spin_lock_irqsave(&ap->host_set->lock, flags);
+		ap->flags &= ~ATA_FLAG_NOINTR;
+		ata_data_xfer(ap, qc->cdb, qc->dev->cdb_len, 1);
+		if (qc->tf.protocol == ATA_PROT_ATAPI_DMA)
+			ap->ops->bmdma_start(qc);	/* initiate bmdma */
+		spin_unlock_irqrestore(&ap->host_set->lock, flags);
+	} else {
+		ata_data_xfer(ap, qc->cdb, qc->dev->cdb_len, 1);
+
+		/* PIO commands are handled by polling */
+		ap->hsm_task_state = HSM_ST;
+		ata_port_queue_task(ap, ata_pio_task, ap, 0);
+	}
+
+	return;
+
+err_out:
+	ata_poll_qc_complete(qc);
+}
+
+/**
  *	ata_qc_timeout - Handle timeout of queued command
  *	@qc: Command that timed out
  *
@@ -3874,26 +3946,26 @@ unsigned int ata_qc_issue_prot(struct at
 		ata_qc_set_polling(qc);
 		ata_tf_to_host(ap, &qc->tf);
 		ap->hsm_task_state = HSM_ST;
-		ata_queue_pio_task(ap);
+		ata_port_queue_task(ap, ata_pio_task, ap, 0);
 		break;
 
 	case ATA_PROT_ATAPI:
 		ata_qc_set_polling(qc);
 		ata_tf_to_host(ap, &qc->tf);
-		ata_queue_packet_task(ap);
+		ata_port_queue_task(ap, atapi_packet_task, ap, 0);
 		break;
 
 	case ATA_PROT_ATAPI_NODATA:
 		ap->flags |= ATA_FLAG_NOINTR;
 		ata_tf_to_host(ap, &qc->tf);
-		ata_queue_packet_task(ap);
+		ata_port_queue_task(ap, atapi_packet_task, ap, 0);
 		break;
 
 	case ATA_PROT_ATAPI_DMA:
 		ap->flags |= ATA_FLAG_NOINTR;
 		ap->ops->tf_load(ap, &qc->tf);	 /* load tf registers */
 		ap->ops->bmdma_setup(qc);	    /* set up bmdma */
-		ata_queue_packet_task(ap);
+		ata_port_queue_task(ap, atapi_packet_task, ap, 0);
 		break;
 
 	default:
@@ -4261,78 +4333,6 @@ irqreturn_t ata_interrupt (int irq, void
 	return IRQ_RETVAL(handled);
 }
 
-/**
- *	atapi_packet_task - Write CDB bytes to hardware
- *	@_data: Port to which ATAPI device is attached.
- *
- *	When device has indicated its readiness to accept
- *	a CDB, this function is called.  Send the CDB.
- *	If DMA is to be performed, exit immediately.
- *	Otherwise, we are in polling mode, so poll
- *	status under operation succeeds or fails.
- *
- *	LOCKING:
- *	Kernel thread context (may sleep)
- */
-
-static void atapi_packet_task(void *_data)
-{
-	struct ata_port *ap = _data;
-	struct ata_queued_cmd *qc;
-	u8 status;
-
-	qc = ata_qc_from_tag(ap, ap->active_tag);
-	WARN_ON(qc == NULL);
-	WARN_ON(!(qc->flags & ATA_QCFLAG_ACTIVE));
-
-	/* sleep-wait for BSY to clear */
-	DPRINTK("busy wait\n");
-	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB)) {
-		qc->err_mask |= AC_ERR_TIMEOUT;
-		goto err_out;
-	}
-
-	/* make sure DRQ is set */
-	status = ata_chk_status(ap);
-	if ((status & (ATA_BUSY | ATA_DRQ)) != ATA_DRQ) {
-		qc->err_mask |= AC_ERR_HSM;
-		goto err_out;
-	}
-
-	/* send SCSI cdb */
-	DPRINTK("send cdb\n");
-	WARN_ON(qc->dev->cdb_len < 12);
-
-	if (qc->tf.protocol == ATA_PROT_ATAPI_DMA ||
-	    qc->tf.protocol == ATA_PROT_ATAPI_NODATA) {
-		unsigned long flags;
-
-		/* Once we're done issuing command and kicking bmdma,
-		 * irq handler takes over.  To not lose irq, we need
-		 * to clear NOINTR flag before sending cdb, but
-		 * interrupt handler shouldn't be invoked before we're
-		 * finished.  Hence, the following locking.
-		 */
-		spin_lock_irqsave(&ap->host_set->lock, flags);
-		ap->flags &= ~ATA_FLAG_NOINTR;
-		ata_data_xfer(ap, qc->cdb, qc->dev->cdb_len, 1);
-		if (qc->tf.protocol == ATA_PROT_ATAPI_DMA)
-			ap->ops->bmdma_start(qc);	/* initiate bmdma */
-		spin_unlock_irqrestore(&ap->host_set->lock, flags);
-	} else {
-		ata_data_xfer(ap, qc->cdb, qc->dev->cdb_len, 1);
-
-		/* PIO commands are handled by polling */
-		ap->hsm_task_state = HSM_ST;
-		ata_queue_pio_task(ap);
-	}
-
-	return;
-
-err_out:
-	ata_poll_qc_complete(qc);
-}
-
 
 /*
  * Execute a 'simple' command, that only consists of the opcode 'cmd' itself,
-- 
1.2.1

[PATCH 4/4] libata: rename ATA_FLAG_FLUSH_PIO_TASK to ATA_FLAG_FLUSH_PORT_TASK

[Tejun Heo]

Rename ATA_FLAG_FLUSH_PIO_TASK to ATA_FLAG_FLUSH_PORT_TASK.

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

---

 drivers/scsi/libata-core.c |    6 +++---
 include/linux/libata.h     |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

4347020e90c1e2ae5d8d48b27e9d52f7143730e1
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 657b196..6a06c47 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -742,7 +742,7 @@ void ata_port_queue_task(struct ata_port
 {
 	int rc;
 
-	if (ap->flags & ATA_FLAG_FLUSH_PIO_TASK)
+	if (ap->flags & ATA_FLAG_FLUSH_PORT_TASK)
 		return;
 
 	PREPARE_WORK(&ap->port_task, fn, data);
@@ -773,7 +773,7 @@ void ata_port_flush_task(struct ata_port
 	DPRINTK("ENTER\n");
 
 	spin_lock_irqsave(&ap->host_set->lock, flags);
-	ap->flags |= ATA_FLAG_FLUSH_PIO_TASK;
+	ap->flags |= ATA_FLAG_FLUSH_PORT_TASK;
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
 	DPRINTK("flush #1\n");
@@ -790,7 +790,7 @@ void ata_port_flush_task(struct ata_port
 	}
 
 	spin_lock_irqsave(&ap->host_set->lock, flags);
-	ap->flags &= ~ATA_FLAG_FLUSH_PIO_TASK;
+	ap->flags &= ~ATA_FLAG_FLUSH_PORT_TASK;
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
 	DPRINTK("EXIT\n");
diff --git a/include/linux/libata.h b/include/linux/libata.h
index d856660..034f515 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -151,7 +151,7 @@ enum {
 	ATA_FLAG_PIO_LBA48	= (1 << 13), /* Host DMA engine is LBA28 only */
 	ATA_FLAG_IRQ_MASK	= (1 << 14), /* Mask IRQ in PIO xfers */
 
-	ATA_FLAG_FLUSH_PIO_TASK	= (1 << 15), /* Flush PIO task */
+	ATA_FLAG_FLUSH_PORT_TASK = (1 << 15), /* Flush port task */
 	ATA_FLAG_IN_EH		= (1 << 16), /* EH in progress */
 
 	ATA_QCFLAG_ACTIVE	= (1 << 1), /* cmd not yet ack'd to scsi lyer */
-- 
1.2.1

[PATCHSET] implement and use port_task

[Tejun Heo]

Hello, Jeff and Albert.

This patchset implements port_task and replace pio_task and
packet_task with it.  port_task isn't very different from pio_task or
packet_task - it's straight forward generalization.  The port_task is
a per-port task avaliable to low level drivers which is managed by
libata core layer.

Differences from pio/packet task.

* Not limited to PIO HSM implementation.  Other drivers can use it.

* Synchronization against EH core layer is done within libata core
  layer, not in PIO HSM implementation (ata_qc_timeout()).

Thanks.

--
tejun

[PATCH 1/4] libata: implement port_task

[Tejun Heo]

Implement port_task.  LLDD's can schedule a function to be executed
with context after specified delay.  libata core takes care of
synchronization against EH.  This is generalized form of pio_task and
packet_task which are tied to PIO hsm implementation.

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

---

 drivers/scsi/libata-core.c |   77 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/libata-scsi.c |    2 +
 drivers/scsi/libata.h      |    1 +
 include/linux/libata.h     |    4 ++
 4 files changed, 84 insertions(+), 0 deletions(-)

4d83950873856672ed469f25c6421de1eb98ba97
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index b710fc4..3575d68 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -721,6 +721,81 @@ static unsigned int ata_pio_modes(const 
 	   timing API will get this right anyway */
 }
 
+/**
+ *	ata_port_queue_task - Queue port_task
+ *	@ap: The ata_port to queue port_task for
+ *
+ *	Schedule @fn(@data) for execution after @delay jiffies using
+ *	port_task.  There is one port_task per port and it's the
+ *	user(low level driver)'s responsibility to make sure that only
+ *	one task is active at any given time.
+ *
+ *	libata core layer takes care of synchronization between
+ *	port_task and EH.  ata_port_queue_task() may be ignored for EH
+ *	synchronization.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+void ata_port_queue_task(struct ata_port *ap, void (*fn)(void *), void *data,
+			 unsigned long delay)
+{
+	int rc;
+
+	if (ap->flags & ATA_FLAG_FLUSH_PIO_TASK)
+		return;
+
+	PREPARE_WORK(&ap->port_task, fn, data);
+
+	if (!delay)
+		rc = queue_work(ata_wq, &ap->port_task);
+	else
+		rc = queue_delayed_work(ata_wq, &ap->port_task, delay);
+
+	/* rc == 0 means that another user is using port task */
+	WARN_ON(rc == 0);
+}
+
+/**
+ *	ata_port_flush_task - Flush port_task
+ *	@ap: The ata_port to flush port_task for
+ *
+ *	After this function completes, port_task is guranteed not to
+ *	be running or scheduled.
+ *
+ *	LOCKING:
+ *	Kernel thread context (may sleep)
+ */
+void ata_port_flush_task(struct ata_port *ap)
+{
+	unsigned long flags;
+
+	DPRINTK("ENTER\n");
+
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+	ap->flags |= ATA_FLAG_FLUSH_PIO_TASK;
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+	DPRINTK("flush #1\n");
+	flush_workqueue(ata_wq);
+
+	/*
+	 * At this point, if a task is running, it's guaranteed to see
+	 * the FLUSH flag; thus, it will never queue pio tasks again.
+	 * Cancel and flush.
+	 */
+	if (!cancel_delayed_work(&ap->port_task)) {
+		DPRINTK("flush #2\n");
+		flush_workqueue(ata_wq);
+	}
+
+	spin_lock_irqsave(&ap->host_set->lock, flags);
+	ap->flags &= ~ATA_FLAG_FLUSH_PIO_TASK;
+	spin_unlock_irqrestore(&ap->host_set->lock, flags);
+
+	DPRINTK("EXIT\n");
+}
+
 static inline void
 ata_queue_packet_task(struct ata_port *ap)
 {
@@ -4478,6 +4553,7 @@ static void ata_host_init(struct ata_por
 	ap->active_tag = ATA_TAG_POISON;
 	ap->last_ctl = 0xFF;
 
+	INIT_WORK(&ap->port_task, NULL, NULL);
 	INIT_WORK(&ap->packet_task, atapi_packet_task, ap);
 	INIT_WORK(&ap->pio_task, ata_pio_task, ap);
 	INIT_LIST_HEAD(&ap->eh_done_q);
@@ -4948,6 +5024,7 @@ EXPORT_SYMBOL_GPL(ata_drive_probe_reset)
 EXPORT_SYMBOL_GPL(ata_port_disable);
 EXPORT_SYMBOL_GPL(ata_ratelimit);
 EXPORT_SYMBOL_GPL(ata_busy_sleep);
+EXPORT_SYMBOL_GPL(ata_port_queue_task);
 EXPORT_SYMBOL_GPL(ata_scsi_ioctl);
 EXPORT_SYMBOL_GPL(ata_scsi_queuecmd);
 EXPORT_SYMBOL_GPL(ata_scsi_timed_out);
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
index d0bd94a..ccedb45 100644
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -785,6 +785,8 @@ int ata_scsi_error(struct Scsi_Host *hos
 	WARN_ON(ata_qc_from_tag(ap, ap->active_tag) == NULL);
 	spin_unlock_irqrestore(&ap->host_set->lock, flags);
 
+	ata_port_flush_task(ap);
+
 	ap->ops->eng_timeout(ap);
 
 	WARN_ON(host->host_failed || !list_empty(&host->eh_cmd_q));
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
index d822eba..f4c48c9 100644
--- a/drivers/scsi/libata.h
+++ b/drivers/scsi/libata.h
@@ -45,6 +45,7 @@ extern int libata_fua;
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
 				      struct ata_device *dev);
 extern int ata_rwcmd_protocol(struct ata_queued_cmd *qc);
+extern void ata_port_flush_task(struct ata_port *ap);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
 extern unsigned int ata_qc_issue(struct ata_queued_cmd *qc);
 extern int ata_check_atapi_dma(struct ata_queued_cmd *qc);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 86a504f..abc5c6e 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -388,6 +388,8 @@ struct ata_port {
 	struct ata_host_stats	stats;
 	struct ata_host_set	*host_set;
 
+	struct work_struct	port_task;
+
 	struct work_struct	packet_task;
 
 	struct work_struct	pio_task;
@@ -513,6 +515,8 @@ extern int ata_ratelimit(void);
 extern unsigned int ata_busy_sleep(struct ata_port *ap,
 				   unsigned long timeout_pat,
 				   unsigned long timeout);
+extern void ata_port_queue_task(struct ata_port *ap, void (*fn)(void *),
+				void *data, unsigned long delay);
 
 /*
  * Default driver ops implementations
-- 
1.2.1

Re: [PATCH 1/4] libata: implement port_task

[Jeff Garzik]

Tejun Heo wrote:
> Implement port_task.  LLDD's can schedule a function to be executed
> with context after specified delay.  libata core takes care of
> synchronization against EH.  This is generalized form of pio_task and
> packet_task which are tied to PIO hsm implementation.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> ---
> 
>  drivers/scsi/libata-core.c |   77 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/libata-scsi.c |    2 +
>  drivers/scsi/libata.h      |    1 +
>  include/linux/libata.h     |    4 ++
>  4 files changed, 84 insertions(+), 0 deletions(-)
> 
> 4d83950873856672ed469f25c6421de1eb98ba97
> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
> index b710fc4..3575d68 100644
> --- a/drivers/scsi/libata-core.c
> +++ b/drivers/scsi/libata-core.c
> @@ -721,6 +721,81 @@ static unsigned int ata_pio_modes(const 
>  	   timing API will get this right anyway */
>  }
>  
> +/**
> + *	ata_port_queue_task - Queue port_task
> + *	@ap: The ata_port to queue port_task for
> + *
> + *	Schedule @fn(@data) for execution after @delay jiffies using
> + *	port_task.  There is one port_task per port and it's the
> + *	user(low level driver)'s responsibility to make sure that only
> + *	one task is active at any given time.
> + *
> + *	libata core layer takes care of synchronization between
> + *	port_task and EH.  ata_port_queue_task() may be ignored for EH
> + *	synchronization.
> + *
> + *	LOCKING:
> + *	Inherited from caller.
> + */
> +void ata_port_queue_task(struct ata_port *ap, void (*fn)(void *), void *data,
> +			 unsigned long delay)
> +{
> +	int rc;
> +
> +	if (ap->flags & ATA_FLAG_FLUSH_PIO_TASK)
> +		return;
> +
> +	PREPARE_WORK(&ap->port_task, fn, data);
> +
> +	if (!delay)
> +		rc = queue_work(ata_wq, &ap->port_task);
> +	else
> +		rc = queue_delayed_work(ata_wq, &ap->port_task, delay);

Two worries here, though not quite a NAK:

1) This is abuse of the PREPARE_WORK(), which is usually not used 
because INIT_WORK() took care of the initialization.  However, it should 
be OK if...

2) This will fall apart if anything tries to queue a task while a 
previous task is still pending.  Are you certain a double-queue never 
ever happens?

	Jeff

Re: [PATCH 1/4] libata: implement port_task

[Tejun Heo]

On Sun, Mar 05, 2006 at 11:09:08AM -0500, Jeff Garzik wrote:
> >+/**
> >+ *	ata_port_queue_task - Queue port_task
> >+ *	@ap: The ata_port to queue port_task for
> >+ *
> >+ *	Schedule @fn(@data) for execution after @delay jiffies using
> >+ *	port_task.  There is one port_task per port and it's the
> >+ *	user(low level driver)'s responsibility to make sure that only
> >+ *	one task is active at any given time.
> >+ *
> >+ *	libata core layer takes care of synchronization between
> >+ *	port_task and EH.  ata_port_queue_task() may be ignored for EH
> >+ *	synchronization.
> >+ *
> >+ *	LOCKING:
> >+ *	Inherited from caller.
> >+ */
> >+void ata_port_queue_task(struct ata_port *ap, void (*fn)(void *), void 
> >*data,
> >+			 unsigned long delay)
> >+{
> >+	int rc;
> >+
> >+	if (ap->flags & ATA_FLAG_FLUSH_PIO_TASK)
> >+		return;
> >+
> >+	PREPARE_WORK(&ap->port_task, fn, data);
> >+
> >+	if (!delay)
> >+		rc = queue_work(ata_wq, &ap->port_task);
> >+	else
> >+		rc = queue_delayed_work(ata_wq, &ap->port_task, delay);
> 
> Two worries here, though not quite a NAK:
> 
> 1) This is abuse of the PREPARE_WORK(), which is usually not used 
> because INIT_WORK() took care of the initialization.  However, it should 
> be OK if...

I first used separate atomic flag and INIT_WORK() for this but seemed
like an overkill as task->pending does mostly the same thing.

> 2) This will fall apart if anything tries to queue a task while a 
> previous task is still pending.  Are you certain a double-queue never 
> ever happens?

The current code never does and neither does irq-pio branch (irq-pio
actually uses only one task, so...).  Also, if any such event occurs,
WARN_ON(rc == 0) should almost always trigger and whine about it.

We can also choose to generalize it further such that there can be
more than one libata-managed tasks but there's no need yet.

-- 
tejun

Re: [PATCH 2/4] libata: convert pio_task and packet_task to port_task

[Jeff Garzik]

Tejun Heo wrote:
> Make pio_task and atapi_packet_task use port_task.
> atapi_packet_task() is moved upward such that it's right after
> ata_pio_task().  This position is more natural and makes adding
> prototype for ata_qc_issue_prot() unnecessary.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

Applied 1-4, though in the future, please -do not- include function 
shuffling in the same patch as other changes.  atapi_packet_task() has 
been moved all over the place, over the lifetime of libata, and it just 
clutters up cumulative diffs at review time.

	Jeff