Skip to contentSkip to conversation

libata ATAPI transfer size cleanups

PATCH In review Jeff Garzik 3 participants 6 messages Nov 1, 2007 – Nov 2, 2007 libata

lore
Share & tools
lore

Jeff Garzik

author
#0 lore raw

PATCH 1/1 libata ATAPI transfer size cleanups

This is purely for comment and testing, not for merging (yet?).

A common recipe in several vendor drivers (either GPL'd, or I have NDA'd access to use them as a documentation-like reference) for ATAPI was slightly different from ours. This recipe can be found in atapi_tf_xfer_size(), and it's slightly different from Alan's.

This patch also adds some byte count checks, consolidated ata_pio_use_silly() use, and adds a dmadir-related FIXME.

The 'xfer-size' branch of master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/libata-dev.git xfer-size

to receive the following updates:

 drivers/ata/libata-core.c |   53 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/ata/libata-eh.c   |   11 +--------
 drivers/ata/libata-scsi.c |   48 +++++++++-------------------------------
 drivers/ata/libata.h      |    7 ++++++
 4 files changed, 72 insertions(+), 47 deletions(-)

Jeff Garzik (1):
[libata] Standardize ATAPI byte count [limit] handling

drivers/ata/libata-core.c +53 -0
index 63035d7..5c616a8 100644
@@ -5258,6 +5258,55 @@ next_sg:
 		goto next_sg;
 }
 
+void atapi_tf_xfer_size(struct ata_taskfile *tf,
+			unsigned int total_xfer_len,
+			bool dma)
+{
+	if (dma) {
+		tf->protocol = ATA_PROT_ATAPI_DMA;
+		tf->feature |= ATAPI_PKT_DMA;
+		tf->lbam = 0;
+		tf->lbah = 0;
+	} else {
+		tf->protocol = ATA_PROT_ATAPI;
+		tf->feature &= ~ATAPI_PKT_DMA;
+		if (total_xfer_len <= 0xffff) {
+			tf->lbam = total_xfer_len;
+			tf->lbah = total_xfer_len >> 8;
+		} else {
+			tf->lbam = 0xff;
+			tf->lbah = 0xff;
+		}
+	}
+}
+
+static bool atapi_valid_bcount(const struct ata_taskfile *tf,
+			       const struct ata_taskfile *result_tf)
+{
+	unsigned int ibyte, obyte, lo, hi;
+
+	lo = tf->lbam;
+	hi = tf->lbam;
+	ibyte = (hi << 8) | lo;
+
+	lo = result_tf->lbam;
+	hi = result_tf->lbam;
+	obyte = (hi << 8) | lo;
+
+	if (unlikely(obyte > ibyte))
+		return false;
+	if (unlikely(obyte == 0))
+		return false;
+	if (unlikely(obyte > 0xfffe))
+		return false;
+
+	/* another test, omitted due to lack of data point:
+	 * obyte should be even, if it is not the last DRQ block
+	 */
+
+	return true;
+}
+
 /**
  *	atapi_pio_bytes - Transfer data from/to the ATAPI device.
  *	@qc: Command on going

@@ -5282,6 +5331,10 @@ static void atapi_pio_bytes(struct ata_queued_cmd *qc)
 	 * So, the correctness of qc->result_tf is not affected.
 	 */
 	ap->ops->tf_read(ap, &qc->result_tf);
+
+	if (!atapi_valid_bcount(&qc->tf, &qc->result_tf))
+		goto err_out;
+
 	ireason = qc->result_tf.nsect;
 	bc_lo = qc->result_tf.lbam;
 	bc_hi = qc->result_tf.lbah;
drivers/ata/libata-eh.c +1 -10
index 8d64f8f..505b2e9 100644
@@ -1352,16 +1352,7 @@ static unsigned int atapi_eh_request_sense(struct ata_queued_cmd *qc)
 
 	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
 	tf.command = ATA_CMD_PACKET;
-
-	/* is it pointless to prefer PIO for "safety reasons"? */
-	if (ap->flags & ATA_FLAG_PIO_DMA) {
-		tf.protocol = ATA_PROT_ATAPI_DMA;
-		tf.feature |= ATAPI_PKT_DMA;
-	} else {
-		tf.protocol = ATA_PROT_ATAPI;
-		tf.lbam = (8 * 1024) & 0xff;
-		tf.lbah = (8 * 1024) >> 8;
-	}
+	atapi_tf_xfer_size(&tf, SCSI_SENSE_BUFFERSIZE, ata_pio_use_silly(ap));
 
 	return ata_exec_internal(dev, &tf, cdb, DMA_FROM_DEVICE,
 				 sense_buf, SCSI_SENSE_BUFFERSIZE, 0);
drivers/ata/libata-scsi.c +11 -37

76 lines not on this page · raw message

drivers/ata/libata.h +7 -0
index 0e6cf3a..f62029a 100644
@@ -103,6 +103,8 @@ extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);
 extern struct ata_port *ata_port_alloc(struct ata_host *host);
 extern void ata_dev_enable_pm(struct ata_device *dev, enum link_pm policy);
 extern void ata_lpm_schedule(struct ata_port *ap, enum link_pm);
+extern void atapi_tf_xfer_size(struct ata_taskfile *tf,
+				unsigned int total_xfer_len, bool dma);
 
 /* libata-acpi.c */
 #ifdef CONFIG_ATA_ACPI

@@ -188,5 +190,10 @@ extern void ata_eh_finish(struct ata_port *ap);
 /* libata-sff.c */
 extern u8 ata_irq_on(struct ata_port *ap);
 
+/* is it pointless to prefer PIO for "safety reasons"? */
+static inline int ata_pio_use_silly(struct ata_port *ap)
+{
+	return (ap->flags & ATA_FLAG_PIO_DMA);
+}
 
 #endif /* __LIBATA_H__ */
#1 lore raw
Quoted 7 lines · On Thu, 1 Nov 2007 05:07:33 -0400 Jeff Garzik <[email protected]> wrote:

This is purely for comment and testing, not for merging (yet?).

A common recipe in several vendor drivers (either GPL'd, or I have NDA'd access to use them as a documentation-like reference) for ATAPI was slightly different from ours. This recipe can be found in atapi_tf_xfer_size(), and it's slightly different from Alan's.

Looks mostly good. Might cause breakage on one or two controllers by setting lbam/lbah to 0 for DMA but we can test that and find out.

Note however - the scheme in use now has been tested for over ten years in drivers/ide. The scheme below has not...

Having an ata_tf_xfer_size method is definitely a good idea whatever methods we eventually decide on.

author
#2 lore raw
Quoted 5 lines · Alan Cox
Quoted 2 lines · On Thu, 1 Nov 2007 05:07:33 -0400 Jeff Garzik <[email protected]> wrote:

This is purely for comment and testing, not for merging (yet?).

Quoted 7 lines
Quoted 4 lines

A common recipe in several vendor drivers (either GPL'd, or I have NDA'd access to use them as a documentation-like reference) for ATAPI was slightly different from ours. This recipe can be found in atapi_tf_xfer_size(), and it's slightly different from Alan's.

Looks mostly good. Might cause breakage on one or two controllers by setting lbam/lbah to 0 for DMA but we can test that and find out.

DMA always zeroed lbam/lbah thanks to memset(), my patch merely made it explicit.

Quoted 2 lines

Note however - the scheme in use now has been tested for over ten years in drivers/ide. The scheme below has not...

See my other email just posted, differences and errata abound :)

Jeff

#3 lore raw
Quoted 11 lines · On Thu, 01 Nov 2007 05:54:06 -0400 Jeff Garzik <[email protected]> wrote:
Quoted 10 lines · Alan Cox
Quoted 6 lines · On Thu, 1 Nov 2007 05:07:33 -0400 Jeff Garzik <[email protected]> wrote:

This is purely for comment and testing, not for merging (yet?).

A common recipe in several vendor drivers (either GPL'd, or I have NDA'd access to use them as a documentation-like reference) for ATAPI was slightly different from ours. This recipe can be found in atapi_tf_xfer_size(), and it's slightly different from Alan's.

Quoted 5 lines
Quoted 2 lines

Looks mostly good. Might cause breakage on one or two controllers by setting lbam/lbah to 0 for DMA but we can test that and find out.

DMA always zeroed lbam/lbah thanks to memset(), my patch merely made it explicit.

That would be a bug if so - need to try fixing that and see if it sorts out the remaining recalcitrant ALi chips.

Alan

#4 lore raw
In reply to Jeff Garzik

drivers/ata/libata-core.c next_sg: · line 5293

Quoted 6 lines · Jeff Garzik
drivers/ata/libata-core.c +6 -0
+       lo = tf->lbam;
+       hi = tf->lbam;
+       ibyte = (hi << 8) | lo;
+
+       lo = result_tf->lbam;
+       hi = result_tf->lbam;

That doesn't look right.
I suspect this was intended:

lo = tf->lbam;
hi = tf->lbah;

Torsten

author
#5 lore raw
Quoted 8 lines · Torsten Kaiser

drivers/ata/libata-core.c next_sg: · line 5293

Quoted 6 lines · Jeff Garzik
drivers/ata/libata-core.c +6 -0
+       lo = tf->lbam;
+       hi = tf->lbam;
+       ibyte = (hi << 8) | lo;
+
+       lo = result_tf->lbam;
+       hi = result_tf->lbam;
Quoted 5 lines

That doesn't look right.
I suspect this was intended:

lo = tf->lbam;
hi = tf->lbah;

Agreed, will correct.

Thanks,

Jeff