libata: improve ATAPI data transfer handling, take #4

PATCH New Tejun Heo 5 participants 15 messages Jan 2, 2008 – Feb 7, 2008

Subsystems (3)

SCSI Block layer libata

lore

Share & tools

lore

Tejun Heo

author

2008-01-02 11:12 UTC #1 lore raw

PATCH 1/4 sata_qstor: convert to new data_xfer prototype

In reply to Tejun Heo

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

Signed-off-by Tejun Heo <[email protected]>

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

drivers/ata/pata_pcmcia.c +11 -8

index ceba93b..3e7f6a9 100644
@@ -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;
 }

Signature · 1 line

1.5.2.4

Tejun Heo

author

2008-01-02 11:12 UTC #0 lore raw

PATCH 1/1 libata: improve ATAPI data transfer handling, take #4

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

Tejun Heo

author

2008-01-02 11:12 UTC #4 lore raw

PATCH 2/4 libata: update ATAPI overflow draining

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 <[email protected]>

Acked-by Albert Lee <[email protected]>

---
 drivers/ata/libata-core.c |  104 +++++++++++++++++++++++----------------------
 1 files changed, 53 insertions(+), 51 deletions(-)

drivers/ata/libata-core.c +53 ~~-51~~

index 9d040a3..3dbac19 100644
@@ -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);

Signature · 1 line

1.5.2.4

Tejun Heo

author

2008-01-02 11:12 UTC #7 lore raw

PATCH 3/4 libata: implement ATAPI drain buffer

In reply to 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 <[email protected]>

---
 drivers/ata/libata-core.c |  116 ++++++++++++++++++++++++++++++++++++++++-----
 drivers/ata/libata-scsi.c |   14 ++++--
 include/linux/libata.h    |    4 +-
 3 files changed, 116 insertions(+), 18 deletions(-)

drivers/ata/libata-core.c +103 ~~-13~~

index 3dbac19..d763c07 100644
@@ -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;
 }

drivers/ata/libata-scsi.c +10 -4

index f523e66..f07704c 100644
@@ -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)

include/linux/libata.h +3 -1

index ccb0556..9fa49e9 100644
@@ -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;

Signature · 1 line

1.5.2.4

Tejun Heo

author

2008-01-02 11:12 UTC #12 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/ata/libata-core.c |   21 +++++++++++++++++++++
 drivers/ata/libata-eh.c   |   35 +++++++++++++++++++++++++++++++++++
 include/linux/libata.h    |    4 ++++
 3 files changed, 60 insertions(+), 0 deletions(-)

drivers/ata/libata-core.c +21 -0

index d763c07..32dde5b 100644
@@ -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.

drivers/ata/libata-eh.c +35 -0

index e93dde1..4399e9e 100644
@@ -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"
+		"         [email protected]\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 */

include/linux/libata.h +4 -0

index 9fa49e9..1601bbd 100644
@@ -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 */

Signature · 1 line

1.5.2.4

Alan Cox

2008-01-02 13:43 UTC #13 lore raw

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

Quoted 5 lines

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 ?

Tejun Heo

author

2008-01-02 13:49 UTC #14 lore raw

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

Quoted 9 lines · Alan Cox

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

Signature · 1 line

tejun

Tejun Heo

author

2008-01-02 13:51 UTC #2 lore raw

PATCH 1/4 pata_pcmcia: convert to new data_xfer prototype

In reply to Tejun Heo

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

Signed-off-by Tejun Heo <[email protected]>

---
Subject updated.

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

drivers/ata/pata_pcmcia.c +11 -8

index ceba93b..3e7f6a9 100644
@@ -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

James Bottomley

maintainer

2008-01-10 17:30 UTC #8 lore raw

RFC 1/2 block: implement drain buffers

In reply to Tejun Heo

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

Quoted 4 lines

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

block/elevator.c +25 -1

index e452deb..5440da1 100644
@@ -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) {

block/ll_rw_blk.c +47 -0

59 lines not on this page · raw message

include/linux/blkdev.h +4 -0

index d18ee67..00166f5 100644
@@ -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 *);

James Bottomley

maintainer

2008-01-10 17:42 UTC #9 lore raw

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

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

---

drivers/ata/libata-scsi.c +18 -5

index a883bb0..ce8f63c 100644
@@ -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;
 }
 
 /**

James Bottomley

maintainer

2008-01-14 16:01 UTC #10 lore raw

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

In reply to 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

---

Quoted 1 line

From 7d5cff0af8a2d4017d0b5246aaef33327b053495 Mon Sep 17 00:00:00 2001

From: James Bottomley <[email protected]>
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.

Signed-off-by James Bottomley <[email protected]>

---
 block/elevator.c       |   26 ++++++++++++++++++++++++-
 block/ll_rw_blk.c      |   49 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |    4 +++
 3 files changed, 78 insertions(+), 1 deletions(-)

block/elevator.c +25 -1

index e452deb..9897f20 100644
@@ -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) {

block/ll_rw_blk.c +49 -0

61 lines not on this page · raw message

include/linux/blkdev.h +4 -0

index 81e99e5..e8bd5cf 100644
@@ -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 *);

Signature · 1 line

1.5.3.7

Jeff Garzik

Applied

2008-01-16 10:24 UTC #3 lore raw

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

In reply to Tejun Heo

Quoted 7 lines · Tejun Heo

While merging data_xfer prototype change, pata_pcmcia was left out. Convert it.
Signed-off-by Tejun Heo <[email protected]>
---
 drivers/ata/pata_pcmcia.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

applied

Jeff Garzik

2008-02-01 20:34 UTC #5 lore raw

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

In reply to Tejun Heo

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

Jeff Garzik

2008-02-07 00:14 UTC #6 lore raw

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

In reply to Tejun Heo

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

Quoted 5 lines

Signed-off-by Tejun Heo <[email protected]>
Acked-by Albert Lee <[email protected]>
---
 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

Jeff Garzik

2008-02-07 00:14 UTC #11 lore raw

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

In reply to Tejun Heo

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

Quoted 6 lines

Signed-off-by Tejun Heo <[email protected]>

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