[PATCHSET #upstream] libata: improve ATAPI data transfer handling, take #4

[PATCHSET #upstream] libata: improve ATAPI data transfer handling, take #4

[Tejun Heo]

Hello,

This is the fourth take of improve-ATAPI-data-transfer patchset.
Changes from the last take [L] are

* Patches other than 8, 13, 14 and 15 have been committed to #upstream
  and thus dropped from this patchset.

* Due to merge order, pata_pcmcia is now out-of-sync with data_xfer
  prototype update.  A patch was added to convert pata_pcmcia.

* Patch 13, 14 and 15 are updated to apply on top of the current
  #upstream.

This patchset is agaisnt the current #upstream (32a03e13).

Thanks.

--
tejun

[L] http://thread.gmane.org/gmane.linux.ide/25938

[PATCH 1/4] sata_qstor: convert to new data_xfer prototype

[Tejun Heo]

While merging data_xfer prototype change, pata_pcmcia was left out.
Convert it.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/pata_pcmcia.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/pata_pcmcia.c b/drivers/ata/pata_pcmcia.c
index ceba93b..3e7f6a9 100644
--- a/drivers/ata/pata_pcmcia.c
+++ b/drivers/ata/pata_pcmcia.c
@@ -102,10 +102,10 @@ static int pcmcia_set_mode_8bit(struct ata_link *link,
 
 /**
  *	ata_data_xfer_8bit	 -	Transfer data by 8bit PIO
- *	@adev: device to target
+ *	@dev: device to target
  *	@buf: data buffer
  *	@buflen: buffer length
- *	@write_data: read/write
+ *	@rw: read/write
  *
  *	Transfer data from/to the device data register by 8 bit PIO.
  *
@@ -113,14 +113,17 @@ static int pcmcia_set_mode_8bit(struct ata_link *link,
  *	Inherited from caller.
  */
 
-static void ata_data_xfer_8bit(struct ata_device *adev, unsigned char *buf,
-		   unsigned int buflen, int write_data)
+static unsigned int ata_data_xfer_8bit(struct ata_device *dev,
+				unsigned char *buf, unsigned int buflen, int rw)
 {
-	struct ata_port *ap = adev->link->ap;
-	if (write_data)
-		iowrite8_rep(ap->ioaddr.data_addr, buf, buflen);
-	else
+	struct ata_port *ap = dev->link->ap;
+
+	if (rw == READ)
 		ioread8_rep(ap->ioaddr.data_addr, buf, buflen);
+	else
+		iowrite8_rep(ap->ioaddr.data_addr, buf, buflen);
+
+	return buflen;
 }
 
 
-- 
1.5.2.4

[PATCH 1/4] pata_pcmcia: convert to new data_xfer prototype

[Tejun Heo]

While merging data_xfer prototype change, pata_pcmcia was left out.
Convert it.

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

 drivers/ata/pata_pcmcia.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/pata_pcmcia.c b/drivers/ata/pata_pcmcia.c
index ceba93b..3e7f6a9 100644
--- a/drivers/ata/pata_pcmcia.c
+++ b/drivers/ata/pata_pcmcia.c
@@ -102,10 +102,10 @@ static int pcmcia_set_mode_8bit(struct ata_link *link,
 
 /**
  *	ata_data_xfer_8bit	 -	Transfer data by 8bit PIO
- *	@adev: device to target
+ *	@dev: device to target
  *	@buf: data buffer
  *	@buflen: buffer length
- *	@write_data: read/write
+ *	@rw: read/write
  *
  *	Transfer data from/to the device data register by 8 bit PIO.
  *
@@ -113,14 +113,17 @@ static int pcmcia_set_mode_8bit(struct ata_link *link,
  *	Inherited from caller.
  */
 
-static void ata_data_xfer_8bit(struct ata_device *adev, unsigned char *buf,
-		   unsigned int buflen, int write_data)
+static unsigned int ata_data_xfer_8bit(struct ata_device *dev,
+				unsigned char *buf, unsigned int buflen, int rw)
 {
-	struct ata_port *ap = adev->link->ap;
-	if (write_data)
-		iowrite8_rep(ap->ioaddr.data_addr, buf, buflen);
-	else
+	struct ata_port *ap = dev->link->ap;
+
+	if (rw == READ)
 		ioread8_rep(ap->ioaddr.data_addr, buf, buflen);
+	else
+		iowrite8_rep(ap->ioaddr.data_addr, buf, buflen);
+
+	return buflen;
 }
 
 
--
1.5.2.4

Re: [PATCH 1/4] sata_qstor: convert to new data_xfer prototype

[Jeff Garzik]

Tejun Heo wrote:
> While merging data_xfer prototype change, pata_pcmcia was left out.
> Convert it.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/pata_pcmcia.c |   19 +++++++++++--------
>  1 files changed, 11 insertions(+), 8 deletions(-)

applied

[PATCH 2/4] libata: update ATAPI overflow draining

[Tejun Heo]

For misc ATAPI commands which transfer variable length data to the
host, overflow can occur due to application or hardware bug.  Such
overflows can be ignored safely as long as overflow data is properly
drained.  libata HSM implementation has this implemented in
__atapi_pio_bytes() and recently updated for 2.6.24-rc but it requires
further improvements.  Improve drain logic such that...

* Report overflow errors using ehi desc mechanism instead of printing
  directly.

* Properly calculate the number of bytes to be drained considering
  actual number of consumed bytes for partial draining.

* Add and use ata_drain_page for draining.  This change fixes the
  problem where LLDs which do 32bit IOs consumes 4 bytes on each 2
  byte draining resulting in draining twice more data than requested.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Acked-by: Albert Lee <albertcc@tw.ibm.com>
---
 drivers/ata/libata-core.c |  104 +++++++++++++++++++++++----------------------
 1 files changed, 53 insertions(+), 51 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9d040a3..3dbac19 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -84,8 +84,8 @@ static unsigned long ata_dev_blacklisted(const struct ata_device *dev);
 
 unsigned int ata_print_id = 1;
 static struct workqueue_struct *ata_wq;
-
 struct workqueue_struct *ata_aux_wq;
+static void *ata_drain_page;
 
 int atapi_enabled = 1;
 module_param(atapi_enabled, int, 0444);
@@ -4658,24 +4658,9 @@ int ata_check_atapi_dma(struct ata_queued_cmd *qc)
  */
 static int atapi_qc_may_overflow(struct ata_queued_cmd *qc)
 {
-	if (qc->tf.protocol != ATAPI_PROT_PIO &&
-	    qc->tf.protocol != ATAPI_PROT_DMA)
-		return 0;
-
-	if (qc->tf.flags & ATA_TFLAG_WRITE)
-		return 0;
-
-	switch (qc->cdb[0]) {
-	case READ_10:
-	case READ_12:
-	case WRITE_10:
-	case WRITE_12:
-	case GPCMD_READ_CD:
-	case GPCMD_READ_CD_MSF:
-		return 0;
-	}
-
-	return 1;
+	return ata_is_atapi(qc->tf.protocol) && ata_is_data(qc->tf.protocol) &&
+		atapi_cmd_type(qc->cdb[0]) == ATAPI_MISC &&
+		!(qc->tf.flags & ATA_TFLAG_WRITE);
 }
 
 /**
@@ -5129,13 +5114,14 @@ static void atapi_send_cdb(struct ata_port *ap, struct ata_queued_cmd *qc)
  */
 static int __atapi_pio_bytes(struct ata_queued_cmd *qc, unsigned int bytes)
 {
-	int do_write = (qc->tf.flags & ATA_TFLAG_WRITE);
+	int rw = (qc->tf.flags & ATA_TFLAG_WRITE) ? WRITE : READ;
 	struct ata_port *ap = qc->ap;
-	struct ata_eh_info *ehi = &qc->dev->link->eh_info;
+	struct ata_device *dev = qc->dev;
+	struct ata_eh_info *ehi = &dev->link->eh_info;
 	struct scatterlist *sg;
 	struct page *page;
 	unsigned char *buf;
-	unsigned int offset, count;
+	unsigned int offset, count, consumed;
 
 next_sg:
 	sg = qc->cursg;
@@ -5147,27 +5133,29 @@ next_sg:
 		 *    - for read case, discard trailing data from the device
 		 *    - for write case, padding zero data to the device
 		 */
-		u16 pad_buf[1] = { 0 };
-		unsigned int i;
-
-		if (bytes > qc->curbytes - qc->nbytes + ATAPI_MAX_DRAIN) {
+		if (qc->curbytes + bytes > qc->nbytes + ATAPI_MAX_DRAIN) {
 			ata_ehi_push_desc(ehi, "too much trailing data "
 					  "buf=%u cur=%u bytes=%u",
 					  qc->nbytes, qc->curbytes, bytes);
 			return -1;
 		}
 
-		 /* overflow is exptected for misc ATAPI commands */
-		if (bytes && !atapi_qc_may_overflow(qc))
-			ata_dev_printk(qc->dev, KERN_WARNING, "ATAPI %u bytes "
-				       "trailing data (cdb=%02x nbytes=%u)\n",
-				       bytes, qc->cdb[0], qc->nbytes);
+		/* allow overflow only for misc ATAPI commands */
+		if (!atapi_qc_may_overflow(qc)) {
+			ata_ehi_push_desc(ehi, "unexpected trailing data "
+					  "%u bytes", bytes);
+			return -1;
+		}
 
-		for (i = 0; i < (bytes + 1) / 2; i++)
-			ap->ops->data_xfer(qc->dev, (unsigned char *)pad_buf, 2, do_write);
+		consumed = 0;
+		while (consumed < bytes) {
+			count = min_t(unsigned int,
+				      bytes - consumed, PAGE_SIZE);
+			consumed += ap->ops->data_xfer(dev, ata_drain_page,
+						       count, rw);
+		}
 
 		qc->curbytes += bytes;
-
 		return 0;
 	}
 
@@ -5194,18 +5182,16 @@ next_sg:
 		buf = kmap_atomic(page, KM_IRQ0);
 
 		/* do the actual data transfer */
-		ap->ops->data_xfer(qc->dev,  buf + offset, count, do_write);
+		consumed = ap->ops->data_xfer(dev,  buf + offset, count, rw);
 
 		kunmap_atomic(buf, KM_IRQ0);
 		local_irq_restore(flags);
 	} else {
 		buf = page_address(page);
-		ap->ops->data_xfer(qc->dev,  buf + offset, count, do_write);
+		consumed = ap->ops->data_xfer(dev,  buf + offset, count, rw);
 	}
 
-	bytes -= count;
-	if ((count & 1) && bytes)
-		bytes--;
+	bytes -= min(bytes, consumed);
 	qc->curbytes += count;
 	qc->cursg_ofs += count;
 
@@ -5214,9 +5200,11 @@ next_sg:
 		qc->cursg_ofs = 0;
 	}
 
+	/* consumed can be larger than count only for the last transfer */
+	WARN_ON(qc->cursg && count != consumed);
+
 	if (bytes)
 		goto next_sg;
-
 	return 0;
 }
 
@@ -5234,6 +5222,7 @@ static void atapi_pio_bytes(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 	struct ata_device *dev = qc->dev;
+	struct ata_eh_info *ehi = &dev->link->eh_info;
 	unsigned int ireason, bc_lo, bc_hi, bytes;
 	int i_write, do_write = (qc->tf.flags & ATA_TFLAG_WRITE) ? 1 : 0;
 
@@ -5251,26 +5240,28 @@ static void atapi_pio_bytes(struct ata_queued_cmd *qc)
 
 	/* shall be cleared to zero, indicating xfer of data */
 	if (unlikely(ireason & (1 << 0)))
-		goto err_out;
+		goto atapi_check;
 
 	/* make sure transfer direction matches expected */
 	i_write = ((ireason & (1 << 1)) == 0) ? 1 : 0;
 	if (unlikely(do_write != i_write))
-		goto err_out;
+		goto atapi_check;
 
 	if (unlikely(!bytes))
-		goto err_out;
+		goto atapi_check;
 
 	VPRINTK("ata%u: xfering %d bytes\n", ap->print_id, bytes);
 
-	if (__atapi_pio_bytes(qc, bytes))
+	if (unlikely(__atapi_pio_bytes(qc, bytes)))
 		goto err_out;
 	ata_altstatus(ap); /* flush */
 
 	return;
 
-err_out:
-	ata_dev_printk(dev, KERN_INFO, "ATAPI check failed\n");
+ atapi_check:
+	ata_ehi_push_desc(ehi, "ATAPI check failed (ireason=0x%x bytes=%u)",
+			  ireason, bytes);
+ err_out:
 	qc->err_mask |= AC_ERR_HSM;
 	ap->hsm_task_state = HSM_ST_ERR;
 }
@@ -7404,24 +7395,35 @@ int ata_pci_device_resume(struct pci_dev *pdev)
 static int __init ata_init(void)
 {
 	ata_probe_timeout *= HZ;
+
+	ata_drain_page = (void *)__get_free_page(GFP_DMA32);
+	if (!ata_drain_page)
+		goto err_drain_page;
+
 	ata_wq = create_workqueue("ata");
 	if (!ata_wq)
-		return -ENOMEM;
+		goto err_ata_wq;
 
 	ata_aux_wq = create_singlethread_workqueue("ata_aux");
-	if (!ata_aux_wq) {
-		destroy_workqueue(ata_wq);
-		return -ENOMEM;
-	}
+	if (!ata_aux_wq)
+		goto err_ata_aux_wq;
 
 	printk(KERN_DEBUG "libata version " DRV_VERSION " loaded.\n");
 	return 0;
+
+ err_ata_aux_wq:
+	destroy_workqueue(ata_wq);
+ err_ata_wq:
+	free_page((unsigned long)ata_drain_page);
+ err_drain_page:
+	return -ENOMEM;
 }
 
 static void __exit ata_exit(void)
 {
 	destroy_workqueue(ata_wq);
 	destroy_workqueue(ata_aux_wq);
+	free_page((unsigned long)ata_drain_page);
 }
 
 subsys_initcall(ata_init);
-- 
1.5.2.4

Re: [PATCH 2/4] libata: update ATAPI overflow draining

[Jeff Garzik]

Tejun,

If you could explain your objection to James's patches in this area, I 
would really appreciate it.

We have several conflicting patches in this area, and we need to get the 
details sorted.

I think you mentioned a key objection that was we actually need to know 
/two/ sizes at the block layer:  (1) the size including all padding and 
drain buffers, and (2) the raw or "true" size.

	Jeff

Re: [PATCH 2/4] libata: update ATAPI overflow draining

[Jeff Garzik]

Tejun Heo wrote:
> For misc ATAPI commands which transfer variable length data to the
> host, overflow can occur due to application or hardware bug.  Such
> overflows can be ignored safely as long as overflow data is properly
> drained.  libata HSM implementation has this implemented in
> __atapi_pio_bytes() and recently updated for 2.6.24-rc but it requires
> further improvements.  Improve drain logic such that...
> 
> * Report overflow errors using ehi desc mechanism instead of printing
>   directly.
> 
> * Properly calculate the number of bytes to be drained considering
>   actual number of consumed bytes for partial draining.
> 
> * Add and use ata_drain_page for draining.  This change fixes the
>   problem where LLDs which do 32bit IOs consumes 4 bytes on each 2
>   byte draining resulting in draining twice more data than requested.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Acked-by: Albert Lee <albertcc@tw.ibm.com>
> ---
>  drivers/ata/libata-core.c |  104 +++++++++++++++++++++++----------------------
>  1 files changed, 53 insertions(+), 51 deletions(-)

(updating status on an old patch)

dropped, due to your current work

[PATCH 3/4] libata: implement ATAPI drain buffer

[Tejun Heo]

Misc ATAPI commands may try to transfer more bytes than requested.
For PIO which is performed by libata HSM, this is worked around by
draining extra bytes from __atapi_pio_bytes().

This patch implements drain buffer to perform draining for DMA and
PIO-over-DMA cases.  One page is allocated w/ GFP_DMA32 during libata
core layer initialization.  On host registration, this drain page is
DMA mapped and ATAPI_MAX_DRAIN_PAGES sg entries are reserved.

ata_sg_setup_extra() uses these extra sg entries to map the drain page
ATAPI_MAX_DRAIN_PAGES times, extending sg list by ATAPI_MAX_DRAIN
bytes.  This allows both DMA and PIO-over-DMA misc ATAPI commands to
overflow by ATAPI_MAX_DRAIN bytes just like PIO commands.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-core.c |  116 ++++++++++++++++++++++++++++++++++++++++-----
 drivers/ata/libata-scsi.c |   14 ++++--
 include/linux/libata.h    |    4 +-
 3 files changed, 116 insertions(+), 18 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3dbac19..d763c07 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4750,6 +4750,60 @@ void ata_sg_init(struct ata_queued_cmd *qc, struct scatterlist *sg,
 	qc->cursg = qc->sg;
 }
 
+/**
+ *	ata_sg_setup_extra - Setup extra sg entries
+ *	@qc: Command to setup extra sg entries for
+ *	@n_elem_extra: Out parameter for the number of extra sg entries
+ *	@nbytes_extra: Out parameter for the number of extra bytes
+ *
+ *	Extra sg entries are used to deal with ATAPI peculiarities.
+ *	First, the content to be transferred can be of any size but
+ *	transfer length should be aligned to 4 bytes, so if data size
+ *	isn't aligned, it needs to be padded.
+ *
+ *	Second, for commands whose repsonse can be variable, due to
+ *	userland bugs (more likely) and hardware bugs, devices can try
+ *	to transfer more bytes than requested.  This can be worked
+ *	around by appending drain buffers at the end.
+ *
+ *	This function sets up both padding and draining sg entries.
+ *	For this purpose, each qc has 2 + ATAPI_MAX_DRAIN_PAGES extra
+ *	sg entries.  Each extra sg has assigned function.
+ *
+ *	   e[0]  |   e[1]  |   e[2]  | ... | e[2 + ATAPI_MAX_DRAIN_PAGES - 1]
+ *	----------------------------------------------------------------------
+ *	   link  | padding | draining  ...
+ *		   or link
+ *
+ *	After sg setup is complete, sg list looks like the following.
+ *
+ *	1. Padding necessary, padding doesn't replace the last sg
+ *
+ *	o[0][1][2]...[last]   e[0][1]([2]... if draining is necessary)
+ *	               |        ^
+ *                      \------/
+ *	   e[0] carries the original content of o[last].
+ *
+ *	2. Padding necessary, padding replaces the last sg
+ *
+ *	o[0][1][2]...[last]   e[0][1]([2]... if draining is necessary)
+ *	               |           ^
+ *                      \---------/
+ *	   e[1] completely includes what o[last] used to point to.
+ *
+ *	3. Only draining is necessary.
+ *
+ *	[0][1][2]...[last]   e[0][1][2]...
+ *	              |           ^
+ *                     \---------/
+ *	   e[1] carries the original conetent of o[last].
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host lock)
+ *
+ *	RETURNS:
+ *	Adjusted n_elem which should be mapped.
+ */
 static unsigned int ata_sg_setup_extra(struct ata_queued_cmd *qc,
 				       unsigned int *n_elem_extra,
 				       unsigned int *nbytes_extra)
@@ -4757,6 +4811,7 @@ static unsigned int ata_sg_setup_extra(struct ata_queued_cmd *qc,
 	struct ata_port *ap = qc->ap;
 	unsigned int n_elem = qc->n_elem;
 	struct scatterlist *lsg, *copy_lsg = NULL, *tsg = NULL, *esg = NULL;
+	int drain;
 
 	*n_elem_extra = 0;
 	*nbytes_extra = 0;
@@ -4764,7 +4819,10 @@ static unsigned int ata_sg_setup_extra(struct ata_queued_cmd *qc,
 	/* needs padding? */
 	qc->pad_len = qc->nbytes & 3;
 
-	if (likely(!qc->pad_len))
+	/* needs drain? */
+	drain = atapi_qc_may_overflow(qc);
+
+	if (likely(!qc->pad_len && !drain))
 		return n_elem;
 
 	/* locate last sg and save it */
@@ -4826,6 +4884,29 @@ static unsigned int ata_sg_setup_extra(struct ata_queued_cmd *qc,
 		(*nbytes_extra) += 4 - qc->pad_len;
 	}
 
+	if (drain) {
+		struct scatterlist *dsg = qc->extra_sg + 2;
+		int i;
+
+		for (i = 0; i < ATAPI_MAX_DRAIN_PAGES; i++) {
+			sg_set_page(dsg, virt_to_page(ata_drain_page),
+				    PAGE_SIZE, 0);
+			sg_dma_address(dsg) = ap->host->drain_page_dma;
+			sg_dma_len(dsg) = PAGE_SIZE;
+			dsg++;
+		}
+
+		if (!tsg) {
+			copy_lsg = &qc->extra_sg[1];
+			tsg = &qc->extra_sg[1];
+		}
+
+		esg = dsg - 1;
+
+		(*n_elem_extra) += ATAPI_MAX_DRAIN_PAGES;
+		(*nbytes_extra) += ATAPI_MAX_DRAIN_PAGES * PAGE_SIZE;
+	}
+
 	if (copy_lsg)
 		sg_set_page(copy_lsg, sg_page(lsg), lsg->length, lsg->offset);
 
@@ -6898,6 +6979,9 @@ static void ata_host_stop(struct device *gendev, void *res)
 			ap->ops->port_stop(ap);
 	}
 
+	dma_unmap_single(host->dev, host->drain_page_dma, PAGE_SIZE,
+			 DMA_FROM_DEVICE);
+
 	if (host->ops->host_stop)
 		host->ops->host_stop(host);
 }
@@ -6920,8 +7004,8 @@ static void ata_host_stop(struct device *gendev, void *res)
  */
 int ata_host_start(struct ata_host *host)
 {
-	int have_stop = 0;
 	void *start_dr = NULL;
+	dma_addr_t dma;
 	int i, rc;
 
 	if (host->flags & ATA_HOST_STARTED)
@@ -6932,20 +7016,23 @@ int ata_host_start(struct ata_host *host)
 
 		if (!host->ops && !ata_port_is_dummy(ap))
 			host->ops = ap->ops;
-
-		if (ap->ops->port_stop)
-			have_stop = 1;
 	}
 
-	if (host->ops->host_stop)
-		have_stop = 1;
+	start_dr = devres_alloc(ata_host_stop, 0, GFP_KERNEL);
+	if (!start_dr)
+		return -ENOMEM;
 
-	if (have_stop) {
-		start_dr = devres_alloc(ata_host_stop, 0, GFP_KERNEL);
-		if (!start_dr)
-			return -ENOMEM;
+	/* map drain page */
+	dma = dma_map_single(host->dev, ata_drain_page, PAGE_SIZE,
+			     DMA_FROM_DEVICE);
+	if (dma_mapping_error(dma)) {
+		dev_printk(KERN_ERR, host->dev, "failed to map drain page\n");
+		rc = -ENOMEM;
+		goto err_map;
 	}
+	host->drain_page_dma = dma;
 
+	/* start ports */
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
@@ -6954,7 +7041,7 @@ int ata_host_start(struct ata_host *host)
 			if (rc) {
 				if (rc != -ENODEV)
 					dev_printk(KERN_ERR, host->dev, "failed to start port %d (errno=%d)\n", i, rc);
-				goto err_out;
+				goto err_start;
 			}
 		}
 		ata_eh_freeze_port(ap);
@@ -6965,13 +7052,16 @@ int ata_host_start(struct ata_host *host)
 	host->flags |= ATA_HOST_STARTED;
 	return 0;
 
- err_out:
+ err_start:
 	while (--i >= 0) {
 		struct ata_port *ap = host->ports[i];
 
 		if (ap->ops->port_stop)
 			ap->ops->port_stop(ap);
 	}
+ err_map:
+	dma_unmap_single(host->dev, host->drain_page_dma, PAGE_SIZE,
+			 DMA_FROM_DEVICE);
 	devres_free(start_dr);
 	return rc;
 }
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index f523e66..f07704c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -832,13 +832,19 @@ static void ata_scsi_dev_config(struct scsi_device *sdev,
 	/* configure max sectors */
 	blk_queue_max_sectors(sdev->request_queue, dev->max_sectors);
 
-	/* SATA DMA transfers must be multiples of 4 byte, so
-	 * we need to pad ATAPI transfers using an extra sg.
-	 * Decrement max hw segments accordingly.
+	/* SATA DMA transfers must be multiples of 4 byte, so we need
+	 * to pad ATAPI transfers using an extra sg.  Also, ATAPI
+	 * commands with variable length reponse needs draining of
+	 * extra data.  Decrement max hw segments accordingly.
 	 */
 	if (dev->class == ATA_DEV_ATAPI) {
 		struct request_queue *q = sdev->request_queue;
-		blk_queue_max_hw_segments(q, q->max_hw_segments - 1);
+		unsigned short hw_segments = q->max_hw_segments;
+
+		BUG_ON(hw_segments <= 1 + ATAPI_MAX_DRAIN_PAGES);
+		hw_segments -= 1 + ATAPI_MAX_DRAIN_PAGES;
+
+		blk_queue_max_hw_segments(q, hw_segments);
 	}
 
 	if (dev->flags & ATA_DFLAG_AN)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index ccb0556..9fa49e9 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -121,6 +121,7 @@ enum {
 	ATA_SHORT_PAUSE		= (HZ >> 6) + 1,
 
 	ATAPI_MAX_DRAIN		= 16 << 10,
+	ATAPI_MAX_DRAIN_PAGES	= ATAPI_MAX_DRAIN >> PAGE_SHIFT,
 
 	ATA_SHT_EMULATED	= 1,
 	ATA_SHT_CMD_PER_LUN	= 1,
@@ -437,6 +438,7 @@ struct ata_host {
 	void			*private_data;
 	const struct ata_port_operations *ops;
 	unsigned long		flags;
+	dma_addr_t		drain_page_dma;
 #ifdef CONFIG_ATA_ACPI
 	acpi_handle		acpi_handle;
 #endif
@@ -475,7 +477,7 @@ struct ata_queued_cmd {
 	struct scatterlist	*last_sg;
 	struct scatterlist	saved_last_sg;
 	struct scatterlist	sgent;
-	struct scatterlist	extra_sg[2];
+	struct scatterlist	extra_sg[2 + ATAPI_MAX_DRAIN_PAGES];
 
 	struct scatterlist	*sg;
 
-- 
1.5.2.4

[RFC 1/2] block: implement drain buffers

[James Bottomley]

On Wed, 2008-01-02 at 20:12 +0900, Tejun Heo wrote:
> Misc ATAPI commands may try to transfer more bytes than requested.
> For PIO which is performed by libata HSM, this is worked around by
> draining extra bytes from __atapi_pio_bytes().
> 
> This patch implements drain buffer to perform draining for DMA and
> PIO-over-DMA cases.  One page is allocated w/ GFP_DMA32 during libata
> core layer initialization.  On host registration, this drain page is
> DMA mapped and ATAPI_MAX_DRAIN_PAGES sg entries are reserved.
> 
> ata_sg_setup_extra() uses these extra sg entries to map the drain page
> ATAPI_MAX_DRAIN_PAGES times, extending sg list by ATAPI_MAX_DRAIN
> bytes.  This allows both DMA and PIO-over-DMA misc ATAPI commands to
> overflow by ATAPI_MAX_DRAIN bytes just like PIO commands.

These DMA drain buffer implementations are pretty horrible to do in
terms of manipulating the scatterlist.  Plus they're being done at least
in drivers/ide, so we now have code duplication.

The one use case for this, as I understand it is AHCI controllers doing
PIO mode to mmc devices but translating this to DMA at the controller
level.

So, what about adding a callback to the block layer that permits the
adding of the drain buffer for the problem devices.  The idea is that
you'd do this in slave_configure after you find one of these devices.

The beauty of doing it in the block layer is that it quietly adds the
drain buffer to the end of the sg list, so it automatically gets mapped
(and unmapped) without anything unusual having to be done to the
scatterlist in driver/scsi or drivers/ata and without any alteration to
the transfer length.

Jens, could you check I've got the logic of adding and removing the
sizes correct (anything I've missed that could result in a size leak on
request processing?) and that you are OK with the basic concept?  I'm
afraid it's ugly, but it's a lot less ugly than having drivers/ata
trying to append to the scatterlist.

James

---
 block/elevator.c       |   26 +++++++++++++++++++++++++-
 block/ll_rw_blk.c      |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |    4 ++++
 3 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/block/elevator.c b/block/elevator.c
index e452deb..5440da1 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -743,7 +743,21 @@ struct request *elv_next_request(struct request_queue *q)
 			q->boundary_rq = NULL;
 		}
 
-		if ((rq->cmd_flags & REQ_DONTPREP) || !q->prep_rq_fn)
+		if (rq->cmd_flags & REQ_DONTPREP)
+			break;
+
+		if (q->dma_drain_size && rq->data_len) {
+			/*
+			 * make sure space for the drain appears we
+			 * know we can do this because max_hw_segments
+			 * has been adjusted to be one fewer than the
+			 * device can handle
+			 */
+			rq->nr_phys_segments++;
+			rq->nr_hw_segments++;
+		}
+
+		if (!q->prep_rq_fn)
 			break;
 
 		ret = q->prep_rq_fn(q, rq);
@@ -756,6 +770,16 @@ struct request *elv_next_request(struct request_queue *q)
 			 * avoid resource deadlock.  REQ_STARTED will
 			 * prevent other fs requests from passing this one.
 			 */
+			if (q->dma_drain_size && rq->data_len &&
+			    !(rq->cmd_flags & REQ_DONTPREP)) {
+				/*
+				 * remove the space for the drain we added
+				 * so that we don't add it again
+				 */
+				--rq->nr_phys_segments;
+				--rq->nr_hw_segments;
+			}
+	
 			rq = NULL;
 			break;
 		} else if (ret == BLKPREP_KILL) {
diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index 8b91994..b9e7862 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -726,6 +726,43 @@ void blk_queue_stack_limits(struct request_queue *t, struct request_queue *b)
 EXPORT_SYMBOL(blk_queue_stack_limits);
 
 /**
+ * blk_queue_dma_drain - Set up a drain buffer for excess dma.
+ *
+ * @q:  the request queue for the device
+ * @buf:	physically contiguous buffer
+ * @size:	size of the buffer in bytes
+ *
+ * Some devices have excess DMA problems and can't simply discard (or
+ * zero fill) the unwanted piece of the transfer.  They have to have a
+ * real area of memory to transfer it into.  The use case for this is
+ * ATAPI devices in DMA mode.  If the packet command causes a transfer
+ * bigger than the transfer size some HBAs will lock up if there
+ * aren't DMA elements to contain the excess transfer.  What this API
+ * does is adjust the queue so that the buf is always appended
+ * silently to the scatterlist.
+ *
+ * Note: This routine adjusts max_hw_segments to make room for
+ * appending the drain buffer.  If you call
+ * blk_queue_max_hw_segments() after calling this routine, you must
+ * set the limit to one fewer than your device can support otherwise
+ * there won't be room for the drain buffer.
+ */
+int blk_queue_dma_drain(struct request_queue *q, void *buf,
+				unsigned int size)
+{
+	if (q->max_hw_segments < 2)
+		return -EINVAL;
+	/* make room for appending the drain */
+	--q->max_hw_segments;
+	q->dma_drain_buffer = buf;
+	q->dma_drain_size = size;
+
+	return 0;
+}
+
+EXPORT_SYMBOL_GPL(blk_queue_dma_drain);
+
+/**
  * blk_queue_segment_boundary - set boundary rules for segment merging
  * @q:  the request queue for the device
  * @mask:  the memory boundary mask
@@ -1355,6 +1392,16 @@ new_segment:
 		bvprv = bvec;
 	} /* segments in rq */
 
+	if (q->dma_drain_size) {
+		sg->page_link &= ~0x02;
+		sg = sg_next(sg);
+		sg_set_page(sg, virt_to_page(q->dma_drain_buffer),
+			    q->dma_drain_size,
+			    ((unsigned long)q->dma_drain_buffer) &
+			    (PAGE_SIZE - 1));
+		nsegs++;
+	}
+
 	if (sg)
 		sg_mark_end(sg);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index d18ee67..00166f5 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -431,6 +431,8 @@ struct request_queue
 	unsigned int		max_segment_size;
 
 	unsigned long		seg_boundary_mask;
+	void			*dma_drain_buffer;
+	unsigned int		dma_drain_size;
 	unsigned int		dma_alignment;
 
 	struct blk_queue_tag	*queue_tags;
@@ -762,6 +764,8 @@ extern void blk_queue_max_hw_segments(struct request_queue *, unsigned short);
 extern void blk_queue_max_segment_size(struct request_queue *, unsigned int);
 extern void blk_queue_hardsect_size(struct request_queue *, unsigned short);
 extern void blk_queue_stack_limits(struct request_queue *t, struct request_queue *b);
+extern int blk_queue_dma_drain(struct request_queue *q, void *buf,
+			       unsigned int size);
 extern void blk_queue_segment_boundary(struct request_queue *, unsigned long);
 extern void blk_queue_prep_rq(struct request_queue *, prep_rq_fn *pfn);
 extern void blk_queue_merge_bvec(struct request_queue *, merge_bvec_fn *);

Re: [RFC 2/2] libata: implement drain buffers

[James Bottomley]

This just updates the libata slave configure routine to take advantage
of the block layer drain buffers.

I suspect I should also be checking for AHCI as well as ATA_DEV_ATAPI,
but I couldn't see how to do that easily.

James

---

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a883bb0..ce8f63c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -826,8 +826,8 @@ static void ata_scsi_sdev_config(struct scsi_device *sdev)
 	sdev->max_device_blocked = 1;
 }
 
-static void ata_scsi_dev_config(struct scsi_device *sdev,
-				struct ata_device *dev)
+static int ata_scsi_dev_config(struct scsi_device *sdev,
+			       struct ata_device *dev)
 {
 	/* configure max sectors */
 	blk_queue_max_sectors(sdev->request_queue, dev->max_sectors);
@@ -838,7 +838,12 @@ static void ata_scsi_dev_config(struct scsi_device *sdev,
 	 */
 	if (dev->class == ATA_DEV_ATAPI) {
 		struct request_queue *q = sdev->request_queue;
-		blk_queue_max_hw_segments(q, q->max_hw_segments - 1);
+		void *buf = kmalloc(ATAPI_MAX_DRAIN, GFP_KERNEL);
+		if (!buf) {
+			sdev_printk(KERN_ERR, sdev, "drain buffer allocation failed\n");
+			return -ENOMEM;
+		}
+		blk_queue_dma_drain(q, buf, ATAPI_MAX_DRAIN);
 	}
 
 	if (dev->flags & ATA_DFLAG_AN)
@@ -851,6 +856,8 @@ static void ata_scsi_dev_config(struct scsi_device *sdev,
 		depth = min(ATA_MAX_QUEUE - 1, depth);
 		scsi_adjust_queue_depth(sdev, MSG_SIMPLE_TAG, depth);
 	}
+
+	return 0;
 }
 
 /**
@@ -869,15 +876,16 @@ int ata_scsi_slave_config(struct scsi_device *sdev)
 {
 	struct ata_port *ap = ata_shost_to_port(sdev->host);
 	struct ata_device *dev = __ata_scsi_find_dev(ap, sdev);
+	int rc = 0;
 
 	ata_scsi_sdev_config(sdev);
 
 	sdev->manage_start_stop = 1;
 
 	if (dev)
-		ata_scsi_dev_config(sdev, dev);
+		rc = ata_scsi_dev_config(sdev, dev);
 
-	return 0;	/* scsi layer doesn't check return value, sigh */
+	return rc;
 }
 
 /**
@@ -899,6 +907,7 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev)
 	struct ata_port *ap = ata_shost_to_port(sdev->host);
 	unsigned long flags;
 	struct ata_device *dev;
+	struct request_queue *q = sdev->request_queue;
 
 	if (!ap->ops->error_handler)
 		return;
@@ -912,6 +921,10 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev)
 		ata_port_schedule_eh(ap);
 	}
 	spin_unlock_irqrestore(ap->lock, flags);
+
+	kfree(q->dma_drain_buffer);
+	q->dma_drain_buffer = NULL;
+	q->dma_drain_size = 0;
 }
 
 /**

Re: [RFC 1/2] block: implement drain buffers

[James Bottomley]

Jens pointed out that I need to lower both phys and hw segments in the
drain set up, so an updated patch is attached.

James

---

>From 7d5cff0af8a2d4017d0b5246aaef33327b053495 Mon Sep 17 00:00:00 2001
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Thu, 10 Jan 2008 11:30:36 -0600
Subject: block: implement drain buffers

These DMA drain buffer implementations in drivers are pretty horrible
to do in terms of manipulating the scatterlist.  Plus they're being
done at least in drivers/ide and drivers/ata, so we now have code
duplication.

The one use case for this, as I understand it is AHCI controllers doing
PIO mode to mmc devices but translating this to DMA at the controller
level.

So, what about adding a callback to the block layer that permits the
adding of the drain buffer for the problem devices.  The idea is that
you'd do this in slave_configure after you find one of these devices.

The beauty of doing it in the block layer is that it quietly adds the
drain buffer to the end of the sg list, so it automatically gets mapped
(and unmapped) without anything unusual having to be done to the
scatterlist in driver/scsi or drivers/ata and without any alteration to
the transfer length.

Jens, could you check I've got the logic of adding and removing the
sizes correct (anything I've missed that could result in a size leak on
request processing?) and that you are OK with the basic concept?  I'm
afraid it's ugly, but it's a lot less ugly than having drivers/ata
trying to append to the scatterlist.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 block/elevator.c       |   26 ++++++++++++++++++++++++-
 block/ll_rw_blk.c      |   49 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |    4 +++
 3 files changed, 78 insertions(+), 1 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index e452deb..9897f20 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -743,7 +743,21 @@ struct request *elv_next_request(struct request_queue *q)
 			q->boundary_rq = NULL;
 		}
 
-		if ((rq->cmd_flags & REQ_DONTPREP) || !q->prep_rq_fn)
+		if (rq->cmd_flags & REQ_DONTPREP)
+			break;
+
+		if (q->dma_drain_size && rq->data_len) {
+			/*
+			 * make sure space for the drain appears we
+			 * know we can do this because max_hw_segments
+			 * has been adjusted to be one fewer than the
+			 * device can handle
+			 */
+			rq->nr_phys_segments++;
+			rq->nr_hw_segments++;
+		}
+
+		if (!q->prep_rq_fn)
 			break;
 
 		ret = q->prep_rq_fn(q, rq);
@@ -756,6 +770,16 @@ struct request *elv_next_request(struct request_queue *q)
 			 * avoid resource deadlock.  REQ_STARTED will
 			 * prevent other fs requests from passing this one.
 			 */
+			if (q->dma_drain_size && rq->data_len &&
+			    !(rq->cmd_flags & REQ_DONTPREP)) {
+				/*
+				 * remove the space for the drain we added
+				 * so that we don't add it again
+				 */
+				--rq->nr_phys_segments;
+				--rq->nr_hw_segments;
+			}
+
 			rq = NULL;
 			break;
 		} else if (ret == BLKPREP_KILL) {
diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index 14af36c..72f536c 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -726,6 +726,45 @@ void blk_queue_stack_limits(struct request_queue *t, struct request_queue *b)
 EXPORT_SYMBOL(blk_queue_stack_limits);
 
 /**
+ * blk_queue_dma_drain - Set up a drain buffer for excess dma.
+ *
+ * @q:  the request queue for the device
+ * @buf:	physically contiguous buffer
+ * @size:	size of the buffer in bytes
+ *
+ * Some devices have excess DMA problems and can't simply discard (or
+ * zero fill) the unwanted piece of the transfer.  They have to have a
+ * real area of memory to transfer it into.  The use case for this is
+ * ATAPI devices in DMA mode.  If the packet command causes a transfer
+ * bigger than the transfer size some HBAs will lock up if there
+ * aren't DMA elements to contain the excess transfer.  What this API
+ * does is adjust the queue so that the buf is always appended
+ * silently to the scatterlist.
+ *
+ * Note: This routine adjusts max_hw_segments to make room for
+ * appending the drain buffer.  If you call
+ * blk_queue_max_hw_segments() or blk_queue_max_phys_segments() after
+ * calling this routine, you must set the limit to one fewer than your
+ * device can support otherwise there won't be room for the drain
+ * buffer.
+ */
+int blk_queue_dma_drain(struct request_queue *q, void *buf,
+				unsigned int size)
+{
+	if (q->max_hw_segments < 2 || q->max_phys_segments < 2)
+		return -EINVAL;
+	/* make room for appending the drain */
+	--q->max_hw_segments;
+	--q->max_phys_segments;
+	q->dma_drain_buffer = buf;
+	q->dma_drain_size = size;
+
+	return 0;
+}
+
+EXPORT_SYMBOL_GPL(blk_queue_dma_drain);
+
+/**
  * blk_queue_segment_boundary - set boundary rules for segment merging
  * @q:  the request queue for the device
  * @mask:  the memory boundary mask
@@ -1379,6 +1418,16 @@ new_segment:
 		bvprv = bvec;
 	} /* segments in rq */
 
+	if (q->dma_drain_size) {
+		sg->page_link &= ~0x02;
+		sg = sg_next(sg);
+		sg_set_page(sg, virt_to_page(q->dma_drain_buffer),
+			    q->dma_drain_size,
+			    ((unsigned long)q->dma_drain_buffer) &
+			    (PAGE_SIZE - 1));
+		nsegs++;
+	}
+
 	if (sg)
 		sg_mark_end(sg);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 81e99e5..e8bd5cf 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -431,6 +431,8 @@ struct request_queue
 	unsigned int		max_segment_size;
 
 	unsigned long		seg_boundary_mask;
+	void			*dma_drain_buffer;
+	unsigned int		dma_drain_size;
 	unsigned int		dma_alignment;
 
 	struct blk_queue_tag	*queue_tags;
@@ -762,6 +764,8 @@ extern void blk_queue_max_hw_segments(struct request_queue *, unsigned short);
 extern void blk_queue_max_segment_size(struct request_queue *, unsigned int);
 extern void blk_queue_hardsect_size(struct request_queue *, unsigned short);
 extern void blk_queue_stack_limits(struct request_queue *t, struct request_queue *b);
+extern int blk_queue_dma_drain(struct request_queue *q, void *buf,
+			       unsigned int size);
 extern void blk_queue_segment_boundary(struct request_queue *, unsigned long);
 extern void blk_queue_prep_rq(struct request_queue *, prep_rq_fn *pfn);
 extern void blk_queue_merge_bvec(struct request_queue *, merge_bvec_fn *);
-- 
1.5.3.7

Re: [PATCH 3/4] libata: implement ATAPI drain buffer

[Jeff Garzik]

Tejun Heo wrote:
> Misc ATAPI commands may try to transfer more bytes than requested.
> For PIO which is performed by libata HSM, this is worked around by
> draining extra bytes from __atapi_pio_bytes().
> 
> This patch implements drain buffer to perform draining for DMA and
> PIO-over-DMA cases.  One page is allocated w/ GFP_DMA32 during libata
> core layer initialization.  On host registration, this drain page is
> DMA mapped and ATAPI_MAX_DRAIN_PAGES sg entries are reserved.
> 
> ata_sg_setup_extra() uses these extra sg entries to map the drain page
> ATAPI_MAX_DRAIN_PAGES times, extending sg list by ATAPI_MAX_DRAIN
> bytes.  This allows both DMA and PIO-over-DMA misc ATAPI commands to
> overflow by ATAPI_MAX_DRAIN bytes just like PIO commands.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/libata-core.c |  116 ++++++++++++++++++++++++++++++++++++++++-----
>  drivers/ata/libata-scsi.c |   14 ++++--
>  include/linux/libata.h    |    4 +-
>  3 files changed, 116 insertions(+), 18 deletions(-)


(updating status on an old patch)

dropped, due to your current work

[PATCH 4/4] libata: implement ATAPI per-command-type DMA horkages

[Tejun Heo]

ATAPI DMA is filled with mines.  Sector aligned READ transfers usually
work but many other commands transfer non-sector aligned or variable
number of bytes, and there are devices and controllers which have
problems dealing with such non-aligned DMA transactions.

This patch implement ATAPI per-command-type DMA horkages and EH logic
to set those quickly.  This way, failures localized to certain command
type don't affect other operations (most importantly READs) and
working configuration is found quickly such that the device can be
used.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-core.c |   21 +++++++++++++++++++++
 drivers/ata/libata-eh.c   |   35 +++++++++++++++++++++++++++++++++++
 include/linux/libata.h    |    4 ++++
 3 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d763c07..32dde5b 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4628,6 +4628,27 @@ static void ata_fill_sg_dumb(struct ata_queued_cmd *qc)
 int ata_check_atapi_dma(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
+	unsigned int horkage = qc->dev->horkage;
+
+	switch (atapi_cmd_type(qc->cdb[0])) {
+	case ATAPI_READ:
+		break;
+
+	case ATAPI_WRITE:
+		if (horkage & ATAPI_DMA_HORKAGE_WRITE)
+			return 1;
+		break;
+
+	case ATAPI_READ_CD:
+		if (horkage & ATAPI_DMA_HORKAGE_READ_CD)
+			return 1;
+		break;
+
+	case ATAPI_MISC:
+		if (horkage & ATAPI_DMA_HORKAGE_MISC)
+			return 1;
+		break;
+	}
 
 	/* Don't allow DMA if it isn't multiple of 16 bytes.  Quite a
 	 * few ATAPI devices choke on such DMA requests.
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index e93dde1..4399e9e 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1490,6 +1490,38 @@ static unsigned int ata_eh_analyze_tf(struct ata_queued_cmd *qc,
 	return action;
 }
 
+static void atapi_eh_dma_horkages(struct ata_queued_cmd *qc)
+{
+	struct ata_device *dev = qc->dev;
+	const char *type;
+
+	if (!ata_is_atapi(qc->tf.protocol) || !ata_is_dma(qc->tf.protocol) ||
+	    !(qc->err_mask & (AC_ERR_HSM | AC_ERR_TIMEOUT)))
+		return;
+
+	switch (atapi_cmd_type(qc->cdb[0])) {
+	case ATAPI_WRITE:
+		type = "WRITE";
+		dev->horkage |= ATAPI_DMA_HORKAGE_WRITE;
+		break;
+	case ATAPI_READ_CD:
+		type = "READ CD [MSF]";
+		dev->horkage |= ATAPI_DMA_HORKAGE_READ_CD;
+		break;
+	case ATAPI_MISC:
+		type = "MISC";
+		dev->horkage |= ATAPI_DMA_HORKAGE_MISC;
+		break;
+	default:
+		return;
+	}
+
+	ata_dev_printk(dev, KERN_WARNING, "ATAPI DMA for %s disabled (0x%x). \n"
+		"         If this continues to happen, please report to\n"
+		"         linux-ide@vger.kernel.org\n",
+		       type, dev->horkage);
+}
+
 static int ata_eh_categorize_error(unsigned int eflags, unsigned int err_mask,
 				   int *xfer_ok)
 {
@@ -1810,6 +1842,9 @@ static void ata_eh_link_autopsy(struct ata_link *link)
 		all_err_mask |= qc->err_mask;
 		if (qc->flags & ATA_QCFLAG_IO)
 			eflags |= ATA_EFLAG_IS_IO;
+
+		/* handle ATAPI DMA horkages */
+		atapi_eh_dma_horkages(qc);
 	}
 
 	/* enforce default EH actions */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 9fa49e9..1601bbd 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -341,6 +341,10 @@ enum {
 	ATA_HORKAGE_IVB		= (1 << 8),	/* cbl det validity bit bugs */
 	ATA_HORKAGE_STUCK_ERR	= (1 << 9),	/* stuck ERR on next PACKET */
 
+	ATAPI_DMA_HORKAGE_WRITE		= (1 << 29), /* PIO for WRITEs */
+	ATAPI_DMA_HORKAGE_READ_CD	= (1 << 30), /* PIO for READ_CDs */
+	ATAPI_DMA_HORKAGE_MISC		= (1 << 31), /* PIO for MISC commands */
+
 	 /* DMA mask for user DMA control: User visible values; DO NOT
 	    renumber */
 	ATA_DMA_MASK_ATA	= (1 << 0),	/* DMA on ATA Disk */
-- 
1.5.2.4

Re: [PATCH 4/4] libata: implement ATAPI per-command-type DMA horkages

[Alan Cox]

> This patch implement ATAPI per-command-type DMA horkages and EH logic
> to set those quickly.  This way, failures localized to certain command
> type don't affect other operations (most importantly READs) and
> working configuration is found quickly such that the device can be
> used.

Seems sensible to treat them differently for different controllers. Have
you considered re-issuing failing ATAPI DMA commands as PIO as an EH
response ?

Re: [PATCH 4/4] libata: implement ATAPI per-command-type DMA horkages

[Tejun Heo]

Alan Cox wrote:
>> This patch implement ATAPI per-command-type DMA horkages and EH logic
>> to set those quickly.  This way, failures localized to certain command
>> type don't affect other operations (most importantly READs) and
>> working configuration is found quickly such that the device can be
>> used.
> 
> Seems sensible to treat them differently for different controllers. Have
> you considered re-issuing failing ATAPI DMA commands as PIO as an EH
> response ?

I think determining whether to retry or not belongs to upper layer.
Retrying random command can result in unexpected behavior.  For things
like device/media readiness testing, SCSI / cdrom layer will retry.  For
pass-through commands, I think it's better to leave it to the application.

Thanks.

-- 
tejun