[PATCHSET] implement and use port_task

[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

[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

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

[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

Re: [PATCHSET] implement and use port_task

[Albert Lee]

Hi Jeff,
> 
> 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
> 

Something related to the PIO HSM.
After the task renaming by this patchset, the task name is changed from pio_task to port_task.
Should we add polling DMA support to the polling HSM?
It could make the polling HSM more complete.

Polling DMA does no performance gain. However, DMA has crc check,
so it won't be worse than PIO when irq is not working.

Albert

Re: [PATCHSET] implement and use port_task

[Tejun Heo]

Albert Lee wrote:
> Hi Jeff,
> 
>>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
>>
> 
> 
> Something related to the PIO HSM.
> After the task renaming by this patchset, the task name is changed from pio_task to port_task.
> Should we add polling DMA support to the polling HSM?
> It could make the polling HSM more complete.
> 
> Polling DMA does no performance gain. However, DMA has crc check,
> so it won't be worse than PIO when irq is not working.
> 

Hmmm... Does irq-pio branch still have polling pio support? I hope we 
can implement polling without impacting normal code path in some generic 
way (pseudo periodic interrupts and each interrupt handler returning 
without doing any harm if nothing has happened should work). I don't 
think complicating HSM with DMA-by-polling is a good idea. It's not like 
all low level drivers can share single implementation of HSM.

Thanks.

-- 
tejun

Re: [PATCHSET] implement and use port_task

[Jeff Garzik]

Albert Lee wrote:
> Something related to the PIO HSM.
> After the task renaming by this patchset, the task name is changed from pio_task to port_task.
> Should we add polling DMA support to the polling HSM?
> It could make the polling HSM more complete.
> 
> Polling DMA does no performance gain. However, DMA has crc check,
> so it won't be worse than PIO when irq is not working.


Polling DMA support with nIEN=1 is theoretically invalid.  You are not 
supposed to touch the ATA shadow register set until after receiving an 
irq event.

On chipsets where nIEN=0 and the chip's IntrMask is also zero, its doable.

I'm satisfied with simply never doing polling DMA, and change our mind 
only if a critical need arises.

	Jeff

Re: [PATCHSET] implement and use port_task

[Albert Lee]

Jeff Garzik wrote:
> 
> 
> Polling DMA support with nIEN=1 is theoretically invalid.  You are not
> supposed to touch the ATA shadow register set until after receiving an
> irq event.
> 
> On chipsets where nIEN=0 and the chip's IntrMask is also zero, its doable.
> 
> I'm satisfied with simply never doing polling DMA, and change our mind
> only if a critical need arises.
> 
> 

Understand. After checking the code, polling DMA looks bad idea.
Thanks for the advice.

BTW, Tejun's proposal of pseudo periodic interrupts reminds me
of some patches to integrate the polling pio with irq-pio.
Will submit the patches for your review later.

Albert