atapi_dmadir: why do we not (also) auto-detect it from IDENTIFY PACKET ??

PATCH In review Mark Lord 6 participants 12 messages Feb 18, 2008 – Feb 24, 2008 libata

Share & tools

Mark Lord

author

2008-02-18 19:02 UTC #0 lore raw

Albert / Jeff,

Way back in 2.6.17-rc2, you guys signed-off on 95de719adc94392a95c3c4d0a2d6b8b1ea39d236, which added a libata module parameter for atapi_dmadir.

That's nice, but many modern drives (or bridge chips) flag this in their IDENTIFY PACKET response, as bit-15 of word-62 of the returned data.

So for those at lease, we should be automatically setting atapi_dmadir=1. But on a per-device basis, not globally as done now.

Albert: do you want to cook up a bug fix for this?

Cheers

Mark Lord

author

2008-02-18 20:53 UTC #1 lore raw

PATCH 1/1 libata-scsi: automatically use DMADIR if drive/bridge requires it

Back in 2.6.17-rc2, a libata module parameter was added for atapi_dmadir.

That's nice, but most SATA devices which need it will tell us about it in their IDENTIFY PACKET response, as bit-15 of word-62 of the returned data (as per ATA7, ATA8 specifications).

So for those which specify it, we should automatically use the DMADIR bit. Otherwise, disc writing will fail by default on many SATA-ATAPI drives.

Signed-off-by Mark Lord <[email protected]>

drivers/ata/libata-scsi.c +0 -0

@@ -2543,9 +2543,10 @@

		qc->tf.protocol = ATAPI_PROT_DMA;
		qc->tf.feature |= ATAPI_PKT_DMA;

- if (atapi_dmadir && (scmd->sc_data_direction != DMA_TO_DEVICE))
- /* some SATA bridges need us to indicate data xfer direction */
- qc->tf.feature |= ATAPI_DMADIR;
+ /* some SATA bridges need us to indicate data xfer direction */
+ if (atapi_dmadir || (dev->id[62] & 0x8000))
+ if (scmd->sc_data_direction != DMA_TO_DEVICE)

+				qc->tf.feature |= ATAPI_DMADIR;
	}

Alan Cox

2008-02-18 22:27 UTC #2 lore raw

Re: [PATCH] libata-scsi: automatically use DMADIR if drive/bridge requires it

Review

Quoted 3 lines · Mark Lord

(quoted diff) +1 -0

+		/* some SATA bridges need us to indicate data xfer direction

(quoted diff) +1 -0

+		if (atapi_dmadir || (dev->id[62] & 0x8000))

The rest of the code uses ata_ inlines/defines in ata.h for things like this and religiously checks validity bits and ATA versions.

Mark Lord

author

2008-02-19 00:41 UTC #3 lore raw

Re: [PATCH] libata-scsi: automatically use DMADIR if drive/bridge requires it

Quoted 7 lines · Alan Cox

Review
Quoted 3 lines · Mark Lord
(quoted diff) +1 -0
+		/* some SATA bridges need us to indicate data xfer direction 
*/
(quoted diff) +1 -0
+		if (atapi_dmadir || (dev->id[62] & 0x8000))
The rest of the code uses ata_ inlines/defines in ata.h for things like this and religiously checks validity bits and ATA versions.

Feel free. I don't actually have a device like that here to test on, so I'm not particularly motivated about doing more than the above right now.

Perhaps you or Albert may be, though. :)

That particular bit has always been zero (reserved), from ATA1 through present, except when used as a DMADIR bit, beginning somewhere around ATA7.

Cheers

Jeff Garzik

2008-02-20 17:19 UTC #4 lore raw

Re: [PATCH] libata-scsi: automatically use DMADIR if drive/bridge requires it

In reply to Mark Lord

Quoted 20 lines · Mark Lord

Back in 2.6.17-rc2, a libata module parameter was added for atapi_dmadir.
That's nice, but most SATA devices which need it will tell us about it in their IDENTIFY PACKET response, as bit-15 of word-62 of the returned data (as per ATA7, ATA8 specifications).
So for those which specify it, we should automatically use the DMADIR bit. Otherwise, disc writing will fail by default on many SATA-ATAPI drives.
Signed-off-by Mark Lord <[email protected]>
drivers/ata/libata-scsi.c 2008-02-18 14:53:02.000000000 -0500 renamed from drivers/ata/libata-scsi.c 2008-02-18 14:45:08.000000000 -0500 +0 -2
@@ -2543,9 +2543,10 @@
        qc->tf.protocol = ATAPI_PROT_DMA;
        qc->tf.feature |= ATAPI_PKT_DMA;
 
-        if (atapi_dmadir && (scmd->sc_data_direction != DMA_TO_DEVICE))
-            /* some SATA bridges need us to indicate data xfer 
direction */

drivers/ata/libata-scsi.c 2008-02-18 14:53:02.000000000 -0500

Quoted 5 lines

drivers/ata/libata-scsi.c 2008-02-18 14:53:02.000000000 -0500 renamed from drivers/ata/libata-scsi.c 2008-02-18 14:45:08.000000000 -0500 +4 -1
-            qc->tf.feature |= ATAPI_DMADIR;
+        /* some SATA bridges need us to indicate data xfer direction */
+        if (atapi_dmadir || (dev->id[62] & 0x8000))
+            if (scmd->sc_data_direction != DMA_TO_DEVICE)
+                qc->tf.feature |= ATAPI_DMADIR;

Regardless of the historical usage (or lack thereof) of that I.D. word, I would prefer

1) an ata version check

2) a simple wrapper in linux/ata.h, similar to the many that already exist

So, conditional ACK...

Mark Lord

author

2008-02-20 18:53 UTC #5 lore raw

Re: [PATCH] libata-scsi: automatically use DMADIR if drive/bridge requires it

Quoted 30 lines · Jeff Garzik

Quoted 21 lines · Mark Lord
Back in 2.6.17-rc2, a libata module parameter was added for atapi_dmadir.
That's nice, but most SATA devices which need it will tell us about it in their IDENTIFY PACKET response, as bit-15 of word-62 of the returned data (as per ATA7, ATA8 specifications).
So for those which specify it, we should automatically use the DMADIR bit. Otherwise, disc writing will fail by default on many SATA-ATAPI drives.
Signed-off-by Mark Lord <[email protected]>
drivers/ata/libata-scsi.c 2008-02-18 14:53:02.000000000 renamed from drivers/ata/libata-scsi.c 2008-02-18 14:45:08.000000000 -0500 +0 -3
-0500
@@ -2543,9 +2543,10 @@
        qc->tf.protocol = ATAPI_PROT_DMA;
        qc->tf.feature |= ATAPI_PKT_DMA;
 
-        if (atapi_dmadir && (scmd->sc_data_direction != DMA_TO_DEVICE))
-            /* some SATA bridges need us to indicate data xfer 
direction */
drivers/ata/libata-scsi.c 2008-02-18 14:53:02.000000000
Quoted 5 lines
drivers/ata/libata-scsi.c 2008-02-18 14:53:02.000000000 renamed from drivers/ata/libata-scsi.c 2008-02-18 14:45:08.000000000 -0500 +4 -1
-            qc->tf.feature |= ATAPI_DMADIR;
+        /* some SATA bridges need us to indicate data xfer direction */
+        if (atapi_dmadir || (dev->id[62] & 0x8000))
+            if (scmd->sc_data_direction != DMA_TO_DEVICE)
+                qc->tf.feature |= ATAPI_DMADIR;
Regardless of the historical usage (or lack thereof) of that I.D. word, I would prefer

Quoted 5 lines

1) an ata version check
2) a simple wrapper in linux/ata.h, similar to the many that already exist
So, conditional ACK...

Good. As I said, I don't expect to be doing this, so if anyone else wants to fix this issue in libata they are more than welcome to take the above and build on it.

Cheers

Tejun Heo

2008-02-21 04:25 UTC #6 lore raw

PATCH 1/1 libata: automatically use DMADIR if drive/bridge requires it

Back in 2.6.17-rc2, a libata module parameter was added for atapi_dmadir.

That's nice, but most SATA devices which need it will tell us about it in their IDENTIFY PACKET response, as bit-15 of word-62 of the returned data (as per ATA7, ATA8 specifications).

So for those which specify it, we should automatically use the DMADIR bit. Otherwise, disc writing will fail by default on many SATA-ATAPI drives.

This patch adds ATA_DFLAG_DMADIR and make ata_dev_configure() set it if atapi_dmadir is set or identify data indicates DMADIR is necessary. atapi_xlat() is converted to check ATA_DFLAG_DMADIR before setting DMADIR.

Original patch is from Mark Lord.

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

Cc Mark Lord <[email protected]>

---
I don't have a bridge which sets DMADIR but so only checked atapi_dmadir parameter. Thanks.

 drivers/ata/libata-core.c |   11 +++++++++--
 drivers/ata/libata-scsi.c |    3 ++-
 include/linux/ata.h       |    5 +++++
 include/linux/libata.h    |    1 +
 4 files changed, 17 insertions(+), 3 deletions(-)

drivers/ata/libata-core.c +9 -2

index 3011919..d38f113 100644
@@ -2200,6 +2200,7 @@ int ata_dev_configure(struct ata_device *dev)
 	else if (dev->class == ATA_DEV_ATAPI) {
 		const char *cdb_intr_string = "";
 		const char *atapi_an_string = "";
+		const char *dma_dir_string = "";
 		u32 sntf;
 
 		rc = atapi_cdb_len(id);

@@ -2240,13 +2241,19 @@ int ata_dev_configure(struct ata_device *dev)
 			cdb_intr_string = ", CDB intr";
 		}
 
+		if (atapi_dmadir || atapi_id_dmadir(dev->id)) {
+			dev->flags |= ATA_DFLAG_DMADIR;
+			dma_dir_string = ", DMADIR";
+		}
+
 		/* print device info to dmesg */
 		if (ata_msg_drv(ap) && print_info)
 			ata_dev_printk(dev, KERN_INFO,
-				       "ATAPI: %s, %s, max %s%s%s\n",
+				       "ATAPI: %s, %s, max %s%s%s%s\n",
 				       modelbuf, fwrevbuf,
 				       ata_mode_string(xfer_mask),
-				       cdb_intr_string, atapi_an_string);
+				       cdb_intr_string, atapi_an_string,
+				       dma_dir_string);
 	}
 
 	/* determine max_sectors */

drivers/ata/libata-scsi.c +2 -1

index c02c490..f50b96b 100644
@@ -2543,7 +2543,8 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
 		qc->tf.protocol = ATAPI_PROT_DMA;
 		qc->tf.feature |= ATAPI_PKT_DMA;
 
-		if (atapi_dmadir && (scmd->sc_data_direction != DMA_TO_DEVICE))
+		if ((dev->flags & ATA_DFLAG_DMADIR) &&
+		    (scmd->sc_data_direction != DMA_TO_DEVICE))
 			/* some SATA bridges need us to indicate data xfer direction */
 			qc->tf.feature |= ATAPI_DMADIR;
 	}

include/linux/ata.h +5 -0

index 78bbaca..1c622e2 100644
@@ -659,6 +659,11 @@ static inline int atapi_command_packet_set(const u16 *dev_id)
 	return (dev_id[0] >> 8) & 0x1f;
 }
 
+static inline int atapi_id_dmadir(const u16 *dev_id)
+{
+	return ata_id_major_version(dev_id) >= 7 && (dev_id[62] & 0x8000);
+}
+
 static inline int is_multi_taskfile(struct ata_taskfile *tf)
 {
 	return (tf->command == ATA_CMD_READ_MULTI) ||

include/linux/libata.h +1 -0

index bc5a8d0..b942e90 100644
@@ -138,6 +138,7 @@ enum {
 	ATA_DFLAG_AN		= (1 << 7), /* AN configured */
 	ATA_DFLAG_HIPM		= (1 << 8), /* device supports HIPM */
 	ATA_DFLAG_DIPM		= (1 << 9), /* device supports DIPM */
+	ATA_DFLAG_DMADIR	= (1 << 10), /* device requires DMADIR */
 	ATA_DFLAG_CFG_MASK	= (1 << 12) - 1,
 
 	ATA_DFLAG_PIO		= (1 << 12), /* device limited to PIO mode */

Albert Lee

2008-02-22 10:10 UTC #7 lore raw

Re: [PATCH] libata: automatically use DMADIR if drive/bridge requires it

Quoted 14 lines · Tejun Heo

Back in 2.6.17-rc2, a libata module parameter was added for atapi_dmadir.
That's nice, but most SATA devices which need it will tell us about it in their IDENTIFY PACKET response, as bit-15 of word-62 of the returned data (as per ATA7, ATA8 specifications).
So for those which specify it, we should automatically use the DMADIR bit. Otherwise, disc writing will fail by default on many SATA-ATAPI drives.
This patch adds ATA_DFLAG_DMADIR and make ata_dev_configure() set it if atapi_dmadir is set or identify data indicates DMADIR is necessary. atapi_xlat() is converted to check ATA_DFLAG_DMADIR before setting DMADIR.

Quoted 8 lines

Original patch is from Mark Lord.
Signed-off-by Tejun Heo <[email protected]>
Cc Mark Lord <[email protected]>
---
I don't have a bridge which sets DMADIR but so only checked atapi_dmadir parameter. Thanks.

The patch looks good. However, it seems there is no realworld IDE-to-SATA bridge that requires DMADIR and also mangles IDENTIFY PACKET bit-15 of word-62 to indicate its presence.

Quoted 1 line

From the previous test of the IDE-to-SATA bridges, only the Sil 3611

requires the host software to hint on ATAPI DMADIR. But Sil 3611 doesn't mangle IDENTIFY PACKET word-62:
http://www.spinics.net/lists/linux-ide/msg01514.html

Maybe we could use ata_dev_knobble() instead? i.e. If it's an ATAPI device && ata_dev_knobble() then we turn on DMADIR (regardless the bridge chip used since setting DMADIR won't hurt those don't need it.) --
albert

Mark Lord

author

2008-02-22 13:33 UTC #8 lore raw

Re: [PATCH] libata: automatically use DMADIR if drive/bridge requires it

Quoted 24 lines · Albert Lee

Quoted 14 lines · Tejun Heo
Back in 2.6.17-rc2, a libata module parameter was added for atapi_dmadir.
That's nice, but most SATA devices which need it will tell us about it in their IDENTIFY PACKET response, as bit-15 of word-62 of the returned data (as per ATA7, ATA8 specifications).
So for those which specify it, we should automatically use the DMADIR bit. Otherwise, disc writing will fail by default on many SATA-ATAPI drives.
This patch adds ATA_DFLAG_DMADIR and make ata_dev_configure() set it if atapi_dmadir is set or identify data indicates DMADIR is necessary. atapi_xlat() is converted to check ATA_DFLAG_DMADIR before setting DMADIR.
Quoted 8 lines
Original patch is from Mark Lord.
Signed-off-by Tejun Heo <[email protected]>
Cc Mark Lord <[email protected]>
---
I don't have a bridge which sets DMADIR but so only checked atapi_dmadir parameter. Thanks.

Quoted 8 lines

The patch looks good. However, it seems there is no realworld IDE-to-SATA bridge that requires DMADIR and also mangles IDENTIFY PACKET bit-15 of word-62 to indicate its presence.
Quoted 1 line
From the previous test of the IDE-to-SATA bridges, only the Sil 3611
requires the host software to hint on ATAPI DMADIR. But Sil 3611 doesn't mangle IDENTIFY PACKET word-62:
http://www.spinics.net/lists/linux-ide/msg01514.html

Ahh, great! We have "history" here!

But there are more SATA bridges out there than the ones Alber has there (SiI 3611/3811, Marvell 88i8030/88SA8040, Acard ARC770, JMicron JM20330).

There's the Marvell 88SA8050, and I have a funky looking thing here that says "SataLink SPiF223A" on it (????). I wonder what it does? And how many other ones are there?

The Word62 patch (from Tejun) is simple enough that the "complexity" point from before really doesn't hold water.

To be ATA compliant, we really should add it in. But yes, it would also be good to see it in action.

That's more likely to happen in the general user community, than in our limited circle of kernel folk with limited hardware.

Cheers

Mark Lord

author

2008-02-22 14:11 UTC #9 lore raw

Re: [PATCH] libata: automatically use DMADIR if drive/bridge requires it

Quoted 2 lines · Mark Lord

I have a funky looking thing here
that says "SataLink SPiF223A" on it (????). I wonder what it does?

Well, after some experimentation, this one appears to be ATA-only, with no ATAPI support at all.

Cheers

Jeff Garzik

2008-02-22 16:32 UTC #10 lore raw

Re: [PATCH] libata: automatically use DMADIR if drive/bridge requires it

In reply to Mark Lord

Quoted 40 lines · Mark Lord

Quoted 24 lines · Albert Lee
Quoted 14 lines · Tejun Heo
Back in 2.6.17-rc2, a libata module parameter was added for atapi_dmadir.
That's nice, but most SATA devices which need it will tell us about it in their IDENTIFY PACKET response, as bit-15 of word-62 of the returned data (as per ATA7, ATA8 specifications).
So for those which specify it, we should automatically use the DMADIR bit. Otherwise, disc writing will fail by default on many SATA-ATAPI drives.
This patch adds ATA_DFLAG_DMADIR and make ata_dev_configure() set it if atapi_dmadir is set or identify data indicates DMADIR is necessary. atapi_xlat() is converted to check ATA_DFLAG_DMADIR before setting DMADIR.
Quoted 8 lines
Original patch is from Mark Lord.
Signed-off-by Tejun Heo <[email protected]>
Cc Mark Lord <[email protected]>
---
I don't have a bridge which sets DMADIR but so only checked atapi_dmadir parameter. Thanks.
Quoted 8 lines
The patch looks good. However, it seems there is no realworld IDE-to-SATA bridge that requires DMADIR and also mangles IDENTIFY PACKET bit-15 of word-62 to indicate its presence.
Quoted 1 line
From the previous test of the IDE-to-SATA bridges, only the Sil 3611
requires the host software to hint on ATAPI DMADIR. But Sil 3611 doesn't mangle IDENTIFY PACKET word-62:
http://www.spinics.net/lists/linux-ide/msg01514.html
..
Ahh, great! We have "history" here!
But there are more SATA bridges out there than the ones Alber has there (SiI 3611/3811, Marvell 88i8030/88SA8040, Acard ARC770, JMicron JM20330).

Quoted 6 lines

There's the Marvell 88SA8050, and I have a funky looking thing here that says "SataLink SPiF223A" on it (????). I wonder what it does? And how many other ones are there?
The Word62 patch (from Tejun) is simple enough that the "complexity" point from before really doesn't hold water.

I'm going to apply Tejun's patch. It's fine (as I noted I conditionally ACK'd your original patch).

My opinion, though, it that this is merely coding to the spec, rather than making a bunch of real-world hardware work. All the SATA<->PATA bridges I have, that support ATAPI, do not flag DMADIR.

Jeff

Jeff Garzik

Applied

2008-02-24 05:29 UTC #11 lore raw

Re: [PATCH] libata: automatically use DMADIR if drive/bridge requires it

In reply to Tejun Heo

Quoted 22 lines · Tejun Heo

Back in 2.6.17-rc2, a libata module parameter was added for atapi_dmadir.
That's nice, but most SATA devices which need it will tell us about it in their IDENTIFY PACKET response, as bit-15 of word-62 of the returned data (as per ATA7, ATA8 specifications).
So for those which specify it, we should automatically use the DMADIR bit. Otherwise, disc writing will fail by default on many SATA-ATAPI drives.
This patch adds ATA_DFLAG_DMADIR and make ata_dev_configure() set it if atapi_dmadir is set or identify data indicates DMADIR is necessary. atapi_xlat() is converted to check ATA_DFLAG_DMADIR before setting DMADIR.
Original patch is from Mark Lord.
Signed-off-by Tejun Heo <[email protected]>
Cc Mark Lord <[email protected]>
---
I don't have a bridge which sets DMADIR but so only checked atapi_dmadir parameter. Thanks.

Quoted 5 lines

 drivers/ata/libata-core.c |   11 +++++++++--
 drivers/ata/libata-scsi.c |    3 ++-
 include/linux/ata.h       |    5 +++++
 include/linux/libata.h    |    1 +
 4 files changed, 17 insertions(+), 3 deletions(-)

applied