Skip to contentSkip to conversation

libata extension

PATCH In review Vitaliyi 10 participants 40 messages Mar 11, 2007 – Apr 4, 2007 3 messages not in the archive libata

lore
Share & tools
lore

Vitaliyi

author
#0 lore raw

Good Day

Say i want to implement extended set of ATA commands available to userspace for building diagnostic tools.
I need 0x40 -- read verify and 0x32 -- write long with error handling, for example. I was trying ide driver through ioctl's, but seems it lack of functionality and full of gotchas. Furthermore it oopses sometimes.

Is it possible to use libata for such purpose or i need to write separate IDE driver ?
By the way, i'm sure it should be done in kernel space since i'm going to deal with some hdd manufacturer commands.

P.S. I was looking through libata and ide sources and documentation but still dont have broad picture.

Thanks

#1 lore raw

Hi,

Quoted 5 lines · Vitaliyi

Good Day

Say i want to implement extended set of ATA commands available to userspace for building diagnostic tools.
I need 0x40 -- read verify and 0x32 -- write long with error handling,

Mark Lord is working on READ/WRITE_LONG support for libata, he has posted draft patch recently on linux-ide mailing list.

[ Please consider reading/joining [email protected] ML,
it is where Linux ATA discussion happens... ]

Quoted 3 lines

for example. I was trying ide driver through ioctl's, but seems it lack of functionality and full of gotchas. Furthermore it oopses sometimes.

READ/WRITE_LONG is unsupported and as you've already noticed TASKFILE ioctls are full of gotchas...

Quoted 2 lines

Is it possible to use libata for such purpose or i need to write separate IDE driver ?

It should be possible using ATA pass-through, some libata changes may be required but it is the right way to go IMO.

Bart

#2 lore raw
In reply to Vitaliyi
Quoted 8 lines · Vitaliyi

Good Day

Say i want to implement extended set of ATA commands available to userspace for building diagnostic tools.
I need 0x40 -- read verify and 0x32 -- write long with error handling, for example. I was trying ide driver through ioctl's, but seems it lack of functionality and full of gotchas. Furthermore it oopses sometimes.

Use the SCSI SG_IO ioctl() with opcode=ATA_16, which gives you access to the ATA Passthrough mechanism. This will work for most ATA commands.

I already use it in hdparm and in some other utilities for scanning/repairing drives.

A notable exeception are the READ/WRITE LONG opcodes, which require an extra kernel patch from me, awaiting merge into libata some year.

Cheers

-ml

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#4 lore raw

---------- Forwarded message ----------
From: Vitaliyi <[email protected]>
Date: Mar 12, 2007 7:03 PM
Subject: Re: libata extension
To: Mark Lord <[email protected]>

Quoted 3 lines

Use the SCSI SG_IO ioctl() with opcode=ATA_16, which gives you access to the ATA Passthrough mechanism. This will work for most ATA commands.

How about soft-reset, is it possible to reach Device Control Register ? Seems not. I was thinking about patch to libata which allow to use Control Register.
In the perfect case i would like to be able to execute vendor command set (reverse engineered).

Is it possible to control timeouts in user space precisely enough and get rid of unnecessary kernel error messages without touching syslogd/klogd?

By the way, how to make libata to not probe some ide channel or drive ?

Quoted 6 lines

I already use it in hdparm and in some other utilities for scanning/repairing drives.

A notable exeception are the READ/WRITE LONG opcodes, which require an extra kernel patch from me, awaiting merge into libata some year.

Could you please send me this patch once it's ready ?

Thank you

#5 lore raw

PATCH 1/1 libata: add support for READ/WRITE LONG

In reply to a message not in the archive

The READ/WRITE LONG commands are theoretically obsolete, but the majority of drives in existance still implement them.

The WRITE_LONG and WRITE_LONG_ONCE commands are of particular interest for fault injection testing -- eg. creating "media errors" at specific locations on a disk.

The fussy bit is that these commands require a non-standard sector size, usually 520 bytes instead of 512.

This patch adds support to libata for READ/WRITE LONG commands issued via SG_IO/ATA_16.

This patch was generated against a 2.6.21-rc3-git7 base:

drivers/ata/libata-core.c |   20 ++++++++++----------
drivers/ata/libata-scsi.c |   12 ++++++++++++
include/linux/ata.h       |    6 ++++++
include/linux/libata.h    |    2 ++
4 files changed, 30 insertions(+), 10 deletions(-)

---

include/linux/libata.h renamed from include/linux/libata.h.orig +0 -0
@@ -425,6 +425,7 @@
	int			dma_dir;

	unsigned int		pad_len;

+ unsigned int sect_size;

	unsigned int		nbytes;
	unsigned int		curbytes;
@@ -1171,6 +1172,7 @@
	qc->n_elem = 0;
	qc->err_mask = 0;
	qc->pad_len = 0;

+ qc->sect_size = ATA_SECT_SIZE;

	ata_tf_init(qc->dev, &qc->tf);
include/linux/ata.h renamed from include/linux/ata.h.orig +0 -0
@@ -163,6 +163,12 @@
	/* READ_LOG_EXT pages */
	ATA_LOG_SATA_NCQ	= 0x10,

+ /* READ/WRITE LONG (obsolete) */
+ ATA_CMD_READ_LONG = 0x22,
+ ATA_CMD_READ_LONG_ONCE = 0x23,
+ ATA_CMD_WRITE_LONG = 0x32,
+ ATA_CMD_WRITE_LONG_ONCE = 0x33,
+

	/* SETFEATURES stuff */
	SETFEATURES_XFER	= 0x03,
	XFER_UDMA_7		= 0x47,
drivers/ata/libata-scsi.c renamed from drivers/ata/libata-scsi.c.orig +0 -0
@@ -2678,6 +2678,18 @@
		tf->device = qc->dev->devno ?
			tf->device | ATA_DEV1 : tf->device & ~ATA_DEV1;

+ /* READ/WRITE LONG use a non-standard sect_size */
+ qc->sect_size = ATA_SECT_SIZE;
+ switch (tf->command) {
+ case ATA_CMD_READ_LONG:
+ case ATA_CMD_READ_LONG_ONCE:
+ case ATA_CMD_WRITE_LONG:
+ case ATA_CMD_WRITE_LONG_ONCE:
+ if (tf->protocol != ATA_PROT_PIO || tf->nsect != 1)
+ goto invalid_fld;
+ qc->sect_size = scmd->request_bufflen;
+ }
+
/*
* Filter SET_FEATURES - XFER MODE command -- otherwise,
* SET_FEATURES - XFER MODE must be preceded/succeeded

drivers/ata/libata-core.c renamed from drivers/ata/libata-core.c.orig +0 -0
@@ -4032,10 +4032,10 @@
 
 

/**
- * ata_pio_sector - Transfer ATA_SECT_SIZE (512 bytes) of data.
+ * ata_pio_sector - Transfer a sector of data.
* @qc: Command on going
*
- * Transfer ATA_SECT_SIZE of data from/to the ATA device.
+ * Transfer qc->sect_size bytes of data from/to the ATA device.
*
* LOCKING:
* Inherited from caller.
@@ -4050,7 +4050,7 @@

	unsigned int offset;
	unsigned char *buf;

- if (qc->curbytes == qc->nbytes - ATA_SECT_SIZE)
+ if (qc->curbytes == qc->nbytes - qc->sect_size)

		ap->hsm_task_state = HSM_ST_LAST;

	page = sg[qc->cursg].page;
@@ -4070,17 +4070,17 @@
		buf = kmap_atomic(page, KM_IRQ0);

		/* do the actual data transfer */

- ap->ops->data_xfer(qc->dev, buf + offset, ATA_SECT_SIZE, do_write);
+ ap->ops->data_xfer(qc->dev, buf + offset, qc->sect_size, do_write);

		kunmap_atomic(buf, KM_IRQ0);
		local_irq_restore(flags);
	} else {
		buf = page_address(page);
-		ap->ops->data_xfer(qc->dev, buf + offset, ATA_SECT_SIZE, do_write);
+		ap->ops->data_xfer(qc->dev, buf + offset, qc->sect_size, do_write);
	}

- qc->curbytes += ATA_SECT_SIZE;
- qc->cursg_ofs += ATA_SECT_SIZE;
+ qc->curbytes += qc->sect_size;
+ qc->cursg_ofs += qc->sect_size;

	if (qc->cursg_ofs == (&sg[qc->cursg])->length) {
		qc->cursg++;

@@ -4089,10 +4089,10 @@
}

/**
- * ata_pio_sectors - Transfer one or many 512-byte sectors.
+ * ata_pio_sectors - Transfer one or many sectors.
* @qc: Command on going
*
- * Transfer one or many ATA_SECT_SIZE of data from/to the
+ * Transfer one or many sectors of data from/to the
* ATA device for the DRQ request.
*
* LOCKING:
@@ -4107,7 +4107,7 @@

		WARN_ON(qc->dev->multi_count == 0);

- nsect = min((qc->nbytes - qc->curbytes) / ATA_SECT_SIZE,
+ nsect = min((qc->nbytes - qc->curbytes) / qc->sect_size,

			    qc->dev->multi_count);
		while (nsect--)
			ata_pio_sector(qc);
#6 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 9 lines · Mark Lord

The READ/WRITE LONG commands are theoretically obsolete, but the majority of drives in existance still implement them.

The WRITE_LONG and WRITE_LONG_ONCE commands are of particular interest for fault injection testing -- eg. creating "media errors" at specific locations on a disk.

The fussy bit is that these commands require a non-standard sector size, usually 520 bytes instead of 512.

..

A possible nice side-benefit, is that this patch also prepares libata for future extension to use drives with a mix of physical sector sizes.

More work required for that, of course, but not *much* more.

Cheers

#7 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

In reply to Mark Lord
Quoted 6 lines · On Mon, 12 Mar 2007 15:08:19 -0400 Mark Lord <[email protected]> wrote:

The READ/WRITE LONG commands are theoretically obsolete, but the majority of drives in existance still implement them.

The WRITE_LONG and WRITE_LONG_ONCE commands are of particular interest for fault injection testing -- eg. creating "media errors" at specific locations on a disk.

Do you still get hangs with ata_piix if you use R/W LONG and these changes ?

Alan

#8 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 6 lines · Alan Cox
Quoted 3 lines · On Mon, 12 Mar 2007 15:08:19 -0400 Mark Lord <[email protected]> wrote:

The READ/WRITE LONG commands are theoretically obsolete, but the majority of drives in existance still implement them.

Quoted 6 lines
Quoted 3 lines

The WRITE_LONG and WRITE_LONG_ONCE commands are of particular interest for fault injection testing -- eg. creating "media errors" at specific locations on a disk.

Do you still get hangs with ata_piix if you use R/W LONG and these changes ?

Err.. I *never* got hangs with ata_piix and these patches. Either that, or the Alzheimer's is getting worse. ;)

???

#9 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 11 lines · On Mon, 12 Mar 2007 18:23:22 -0400 Mark Lord <[email protected]> wrote:
Quoted 10 lines · Alan Cox
Quoted 6 lines · On Mon, 12 Mar 2007 15:08:19 -0400 Mark Lord <[email protected]> wrote:

The READ/WRITE LONG commands are theoretically obsolete, but the majority of drives in existance still implement them.

The WRITE_LONG and WRITE_LONG_ONCE commands are of particular interest for fault injection testing -- eg. creating "media errors" at specific locations on a disk.

Quoted 5 lines
Quoted 2 lines

Do you still get hangs with ata_piix if you use R/W LONG and these changes ?

Err.. I *never* got hangs with ata_piix and these patches. Either that, or the Alzheimer's is getting worse. ;)

Umm... someone reported originally getting problems with R/W LONG and PIIX but not AHCI and we discussed whether we might need to turn the prefetch/postwrite off for it. Would have been about October last year.

If it works ok then I'll just cross it off the list

Alan

#10 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 4 lines · Alan Cox

Umm... someone reported originally getting problems with R/W LONG and PIIX but not AHCI and we discussed whether we might need to turn the prefetch/postwrite off for it. Would have been about October last year.

Ah, well, yes, they would have problems unless my patch had been applied. And not only on ata_piix.

Quoted 1 line

If it works ok then I'll just cross it off the list

It works for me, on a variety of hardware.

Cheers

#11 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

In reply to Mark Lord
Quoted 7 lines · Mark Lord

The READ/WRITE LONG commands are theoretically obsolete, but the majority of drives in existance still implement them.

The WRITE_LONG and WRITE_LONG_ONCE commands are of particular interest for fault injection testing -- eg. creating "media errors" at specific locations on a disk.

Quoted 7 lines

The fussy bit is that these commands require a non-standard sector size, usually 520 bytes instead of 512.

This patch adds support to libata for READ/WRITE LONG commands issued via SG_IO/ATA_16.

This patch was generated against a 2.6.21-rc3-git7 base:

I think it would be better if this comes in two patches. One to add qc->sect_size and convert all users of ATA_SECT_SIZE to qc->sect_size and the other one to implement READ/WRITE LONG. Another question is whether this needs to be included into mainline. This is definitely useful but it is mostly for debugging/testing.

Hmmm... But we're gonna need qc->sect_size anyway for devices with larger sector sizes and overhead for supporting READ/WRITE LONG is nearly nill, so I'm voting for inclusion.

Thanks.

Signature · 1 line
tejun
#12 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 24 lines · Tejun Heo
Quoted 7 lines · Mark Lord

The READ/WRITE LONG commands are theoretically obsolete, but the majority of drives in existance still implement them.

The WRITE_LONG and WRITE_LONG_ONCE commands are of particular interest for fault injection testing -- eg. creating "media errors" at specific locations on a disk.

Quoted 8 lines

The fussy bit is that these commands require a non-standard sector size, usually 520 bytes instead of 512.

This patch adds support to libata for READ/WRITE LONG commands issued via SG_IO/ATA_16.

This patch was generated against a 2.6.21-rc3-git7 base:

I think it would be better if this comes in two patches. One to add qc->sect_size and convert all users of ATA_SECT_SIZE to qc->sect_size and the other one to implement READ/WRITE LONG. Another question is whether this needs to be included into mainline. This is definitely useful but it is mostly for debugging/testing.

Quoted 6 lines

Hmmm... But we're gonna need qc->sect_size anyway for devices with larger sector sizes and overhead for supporting READ/WRITE LONG is nearly nill, so I'm voting for inclusion.

Thanks.

I just want to add that this patch has been incredibly useful for us in testing the error handling & RAID. Nothing like "real" media errors on demand to validate your assumptions ;-)

ric

#13 lore raw

PATCH 1/1 libata: add support for READ/WRITE LONG

In reply to Mark Lord

(resending, again. why does this take months of wasted efforts?)

The READ/WRITE LONG commands are theoretically obsolete, but the majority of drives in existance still implement them.

The WRITE_LONG and WRITE_LONG_ONCE commands are of particular interest for fault injection testing -- eg. creating "media errors" at specific locations on a disk.

The fussy bit is that these commands require a non-standard sector size, usually 520 bytes instead of 512.

This patch adds support to libata for READ/WRITE LONG commands issued via SG_IO/ATA_16.

This patch was generated against a 2.6.21-rc3-git7 base:

drivers/ata/libata-core.c |   20 ++++++++++----------
drivers/ata/libata-scsi.c |   12 ++++++++++++
include/linux/ata.h       |    6 ++++++
include/linux/libata.h    |    2 ++
4 files changed, 30 insertions(+), 10 deletions(-)

---

include/linux/libata.h renamed from include/linux/libata.h.orig +0 -0
@@ -425,6 +425,7 @@
	int			dma_dir;

	unsigned int		pad_len;

+ unsigned int sect_size;

	unsigned int		nbytes;
	unsigned int		curbytes;
@@ -1171,6 +1172,7 @@
	qc->n_elem = 0;
	qc->err_mask = 0;
	qc->pad_len = 0;

+ qc->sect_size = ATA_SECT_SIZE;

	ata_tf_init(qc->dev, &qc->tf);
include/linux/ata.h renamed from include/linux/ata.h.orig +0 -0
@@ -163,6 +163,12 @@
	/* READ_LOG_EXT pages */
	ATA_LOG_SATA_NCQ	= 0x10,

+ /* READ/WRITE LONG (obsolete) */
+ ATA_CMD_READ_LONG = 0x22,
+ ATA_CMD_READ_LONG_ONCE = 0x23,
+ ATA_CMD_WRITE_LONG = 0x32,
+ ATA_CMD_WRITE_LONG_ONCE = 0x33,
+

	/* SETFEATURES stuff */
	SETFEATURES_XFER	= 0x03,
	XFER_UDMA_7		= 0x47,
drivers/ata/libata-scsi.c renamed from drivers/ata/libata-scsi.c.orig +0 -0
@@ -2678,6 +2678,18 @@
		tf->device = qc->dev->devno ?
			tf->device | ATA_DEV1 : tf->device & ~ATA_DEV1;

+ /* READ/WRITE LONG use a non-standard sect_size */
+ qc->sect_size = ATA_SECT_SIZE;
+ switch (tf->command) {
+ case ATA_CMD_READ_LONG:
+ case ATA_CMD_READ_LONG_ONCE:
+ case ATA_CMD_WRITE_LONG:
+ case ATA_CMD_WRITE_LONG_ONCE:
+ if (tf->protocol != ATA_PROT_PIO || tf->nsect != 1)
+ goto invalid_fld;
+ qc->sect_size = scmd->request_bufflen;
+ }
+
/*
* Filter SET_FEATURES - XFER MODE command -- otherwise,
* SET_FEATURES - XFER MODE must be preceded/succeeded

drivers/ata/libata-core.c renamed from drivers/ata/libata-core.c.orig +0 -0
@@ -4032,10 +4032,10 @@
 
 

/**
- * ata_pio_sector - Transfer ATA_SECT_SIZE (512 bytes) of data.
+ * ata_pio_sector - Transfer a sector of data.
* @qc: Command on going
*
- * Transfer ATA_SECT_SIZE of data from/to the ATA device.
+ * Transfer qc->sect_size bytes of data from/to the ATA device.
*
* LOCKING:
* Inherited from caller.
@@ -4050,7 +4050,7 @@

	unsigned int offset;
	unsigned char *buf;

- if (qc->curbytes == qc->nbytes - ATA_SECT_SIZE)
+ if (qc->curbytes == qc->nbytes - qc->sect_size)

		ap->hsm_task_state = HSM_ST_LAST;

	page = sg[qc->cursg].page;
@@ -4070,17 +4070,17 @@
		buf = kmap_atomic(page, KM_IRQ0);

		/* do the actual data transfer */

- ap->ops->data_xfer(qc->dev, buf + offset, ATA_SECT_SIZE, do_write);
+ ap->ops->data_xfer(qc->dev, buf + offset, qc->sect_size, do_write);

		kunmap_atomic(buf, KM_IRQ0);
		local_irq_restore(flags);
	} else {
		buf = page_address(page);
-		ap->ops->data_xfer(qc->dev, buf + offset, ATA_SECT_SIZE, do_write);
+		ap->ops->data_xfer(qc->dev, buf + offset, qc->sect_size, do_write);
	}

- qc->curbytes += ATA_SECT_SIZE;
- qc->cursg_ofs += ATA_SECT_SIZE;
+ qc->curbytes += qc->sect_size;
+ qc->cursg_ofs += qc->sect_size;

	if (qc->cursg_ofs == (&sg[qc->cursg])->length) {
		qc->cursg++;

@@ -4089,10 +4089,10 @@
}

/**
- * ata_pio_sectors - Transfer one or many 512-byte sectors.
+ * ata_pio_sectors - Transfer one or many sectors.
* @qc: Command on going
*
- * Transfer one or many ATA_SECT_SIZE of data from/to the
+ * Transfer one or many sectors of data from/to the
* ATA device for the DRQ request.
*
* LOCKING:
@@ -4107,7 +4107,7 @@

		WARN_ON(qc->dev->multi_count == 0);

- nsect = min((qc->nbytes - qc->curbytes) / ATA_SECT_SIZE,
+ nsect = min((qc->nbytes - qc->curbytes) / qc->sect_size,

			    qc->dev->multi_count);
		while (nsect--)
			ata_pio_sector(qc);
Acked-by on v1 1/1
#14 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 2 lines

The fussy bit is that these commands require a non-standard sector size, usually 520 bytes instead of 512.

Do we always know the worst case here as this breaks my pending patch to use bounce buffers for PIO so we transfer with IRQ enabled and I need to know the correct new worst case size.

Quoted 5 lines

This patch adds support to libata for READ/WRITE LONG commands issued via SG_IO/ATA_16.

This patch was generated against a 2.6.21-rc3-git7 base:

Quoted 7 lines
drivers/ata/libata-core.c |   20 ++++++++++----------
drivers/ata/libata-scsi.c |   12 ++++++++++++
include/linux/ata.h       |    6 ++++++
include/linux/libata.h    |    2 ++
4 files changed, 30 insertions(+), 10 deletions(-)

I'll fix my bounce patches up to match and queue them to Jeff after this is all in and settled.

#15 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 6 lines · Alan Cox
Quoted 2 lines

The fussy bit is that these commands require a non-standard sector size, usually 520 bytes instead of 512.

Do we always know the worst case here as this breaks my pending patch to use bounce buffers for PIO so we transfer with IRQ enabled and I need to know the correct new worst case size.

Ugh. The drives default to 520 bytes (always), but can specify a different, preferred, length in the IDENTIFY data. A SETFEATURES command is required to change the setting from the default of 520.

In practice, I only ever use 520, because (1) it always works, and (2) I'm only trying to corrupt drives, not write correct stuff. ;)

So we could modify the patch to limit it to a specific maximum, or always require exactly 520 bytes without any hardship.

???

Cheers

#16 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 4 lines

Ugh. The drives default to 520 bytes (always), but can specify a different, preferred, length in the IDENTIFY data. A SETFEATURES command is required to change the setting from the default of 520.

Thats fine so long as we know the "worst case" length - or I can make it fall back if the worst case length is over 4K.

Alan

#17 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

In reply to Mark Lord
Quoted 10 lines · Mark Lord
Quoted 6 lines · Alan Cox
Quoted 2 lines

The fussy bit is that these commands require a non-standard sector size, usually 520 bytes instead of 512.

Do we always know the worst case here as this breaks my pending patch to use bounce buffers for PIO so we transfer with IRQ enabled and I need to know the correct new worst case size.

Ugh. The drives default to 520 bytes (always),

The old IDE drives used to have 4 and 7 byte ECC (seen in ancient Phoenix
BIOS).

Quoted 3 lines

but can specify a different, preferred, length in the IDENTIFY data. A SETFEATURES command is required to change the setting from the default of 520.

You're wrong here (which is explainable by your patch bneing boken
protocol wise) -- there used to be two subcodes, one to selecyt vendor spcific length (0x44), and one to selectt 4-byte ECC (0xBB). Not 8-byte!

Quoted 7 lines

In practice, I only ever use 520, because (1) it always works, and (2) I'm only trying to corrupt drives, not write correct stuff. ;)

So we could modify the patch to limit it to a specific maximum, or always require exactly 520 bytes without any hardship.

???

Certainly not the second. Vendor ECC length is indicated in the word 22
identify device info. Not sure how this gets translated to SCSI...

WBR, Sergei

#18 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 6 lines · Sergei Shtylyov
Quoted 1 line · Mark Lord

Ugh. The drives default to 520 bytes (always),

The old IDE drives used to have 4 and 7 byte ECC (seen in ancient
Phoenix BIOS).

Yes -- 4-bytes means a 520-byte transfer when using 16-bit words.

The ATA standard specifies a default of 520 bytes, or a SETFEATURES command to enable a different length.

Here it is from ATA-1:

Quoted 4 lines

9.9.8 Word 22: ECC bytes available on read/write long commands

If the contents of this field are set to a value other than 4, the only way to use this information is via the Set Features commands.

And again from ATA-3:

Quoted 3 lines

Section 2.1.7: ... The default length of the vendor specific bytpes associated with the READ LONG and WRITE LONG commands is four bytes, but may be changed by use of the SET FEATURES command.

It helps to verify stuff like this. The best ways to do
that are (1) to read the specs, and (2) actually implement and test the code using both ancient and modern drives on multiple controllers. All of which has been done here.

Cheers

#19 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

In reply to Mark Lord

Hello.

Quoted 9 lines · Mark Lord

The READ/WRITE LONG commands are theoretically obsolete, but the majority of drives in existance still implement them.

The WRITE_LONG and WRITE_LONG_ONCE commands are of particular interest for fault injection testing -- eg. creating "media errors" at specific locations on a disk.

The fussy bit is that these commands require a non-standard sector size, usually 520 bytes instead of 512.

Which requires from the drivers to be able to turn off IDE prefetch (and
maybe posting too). I don't see that in this patch (or are you expecting them to just "snoop' the commands and do it automagically?).

Quoted 9 lines

This patch adds support to libata for READ/WRITE LONG commands issued via SG_IO/ATA_16.

---
--- linux/include/linux/libata.h.orig 2007-03-07 09:20:04.000000000
-0500
+++ linux/include/linux/libata.h 2007-03-12 13:16:29.000000000 -0400

Review line 431

Quoted 7 lines
(quoted diff) +1 -0
@@ -425,6 +425,7 @@
    int            dma_dir;
 
    unsigned int        pad_len;
+    unsigned int        sect_size;
 
    unsigned int        nbytes;
    unsigned int        curbytes;

Tabs are spoilt. :-/

Quoted 87 lines
(quoted diff) +1 -0
@@ -1171,6 +1172,7 @@
    qc->n_elem = 0;
    qc->err_mask = 0;
    qc->pad_len = 0;
+    qc->sect_size = ATA_SECT_SIZE;
 
    ata_tf_init(qc->dev, &qc->tf);
 
include/linux/ata.h 2007-03-12 13:15:45.000000000 -0400 renamed from include/linux/ata.h.orig 2007-03-07 09:21:25.000000000 -0500 +6 -1
@@ -163,6 +163,12 @@
    /* READ_LOG_EXT pages */
    ATA_LOG_SATA_NCQ    = 0x10,
 
+    /* READ/WRITE LONG (obsolete) */
+    ATA_CMD_READ_LONG    = 0x22,
+    ATA_CMD_READ_LONG_ONCE    = 0x23,
+    ATA_CMD_WRITE_LONG    = 0x32,
+    ATA_CMD_WRITE_LONG_ONCE    = 0x33,
+
    /* SETFEATURES stuff */
    SETFEATURES_XFER    = 0x03,
    XFER_UDMA_7        = 0x47,
--- linux/drivers/ata/libata-scsi.c.orig    2007-03-07 

09:21:13.000000000 -0500
+++ linux/drivers/ata/libata-scsi.c 2007-03-12 13:14:46.000000000 -0400

include/linux/ata.h 2007-03-12 13:15:45.000000000 -0400 renamed from include/linux/ata.h.orig 2007-03-07 09:21:25.000000000 -0500 +12 -1
@@ -2678,6 +2678,18 @@
        tf->device = qc->dev->devno ?
            tf->device | ATA_DEV1 : tf->device & ~ATA_DEV1;
 
+    /* READ/WRITE LONG use a non-standard sect_size */
+    qc->sect_size = ATA_SECT_SIZE;
+    switch (tf->command) {
+    case ATA_CMD_READ_LONG:
+    case ATA_CMD_READ_LONG_ONCE:
+    case ATA_CMD_WRITE_LONG:
+    case ATA_CMD_WRITE_LONG_ONCE:
+        if (tf->protocol != ATA_PROT_PIO || tf->nsect != 1)
+            goto invalid_fld;
+        qc->sect_size = scmd->request_bufflen;
+    }
+
    /*
     * Filter SET_FEATURES - XFER MODE command -- otherwise,
     * SET_FEATURES - XFER MODE must be preceded/succeeded
--- linux/drivers/ata/libata-core.c.orig    2007-03-12 

11:22:43.000000000 -0400
+++ linux/drivers/ata/libata-core.c 2007-03-12 13:08:28.000000000 -0400

include/linux/ata.h 2007-03-12 13:15:45.000000000 -0400 renamed from include/linux/ata.h.orig 2007-03-07 09:21:25.000000000 -0500 +5 -5
@@ -4032,10 +4032,10 @@
 
 
 /**
- *    ata_pio_sector - Transfer ATA_SECT_SIZE (512 bytes) of data.
+ *    ata_pio_sector - Transfer a sector of data.
 *    @qc: Command on going
 *
- *    Transfer ATA_SECT_SIZE of data from/to the ATA device.
+ *    Transfer qc->sect_size bytes of data from/to the ATA device.
 *
 *    LOCKING:
 *    Inherited from caller.
@@ -4050,7 +4050,7 @@
    unsigned int offset;
    unsigned char *buf;
 
-    if (qc->curbytes == qc->nbytes - ATA_SECT_SIZE)
+    if (qc->curbytes == qc->nbytes - qc->sect_size)
        ap->hsm_task_state = HSM_ST_LAST;
 
    page = sg[qc->cursg].page;
@@ -4070,17 +4070,17 @@
        buf = kmap_atomic(page, KM_IRQ0);
 
        /* do the actual data transfer */
-        ap->ops->data_xfer(qc->dev, buf + offset, ATA_SECT_SIZE, 
 do_write);
+        ap->ops->data_xfer(qc->dev, buf + offset, qc->sect_size, 
 do_write);
 
 
        kunmap_atomic(buf, KM_IRQ0);
        local_irq_restore(flags);
    } else {
        buf = page_address(page);
-        ap->ops->data_xfer(qc->dev, buf + offset, ATA_SECT_SIZE, 
 do_write);
+        ap->ops->data_xfer(qc->dev, buf + offset, qc->sect_size, 

include/linux/ata.h 2007-03-12 13:15:45.000000000 -0400

Quoted 10 lines
include/linux/ata.h 2007-03-12 13:15:45.000000000 -0400 renamed from include/linux/ata.h.orig 2007-03-07 09:21:25.000000000 -0500 +2 -2
 do_write);
    }
 
-    qc->curbytes += ATA_SECT_SIZE;
-    qc->cursg_ofs += ATA_SECT_SIZE;
+    qc->curbytes += qc->sect_size;
+    qc->cursg_ofs += qc->sect_size;
 
    if (qc->cursg_ofs == (&sg[qc->cursg])->length) {
        qc->cursg++;

Again, ata_data_xfer() doesn't seem capable of performing ECC read/writes
-- the ECC bytes must be transferred in 8-bit mode, AFAIR. ata_data_xfer() can oinly do that for optionally trailing odd byte. NAK.

MBR, Sergei

#20 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 3 lines

Which requires from the drivers to be able to turn off IDE prefetch (and
maybe posting too). I don't see that in this patch (or are you expecting them to just "snoop' the commands and do it automagically?).

We need to see which drivers have that problem and if its only the odd one then we can handle it in qc_issue. We've already got controllers that wrap qc_issue to peek at commands and reject some of them (eg the 8212 in hardware raid mode)

Alan

#21 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Hello.

Quoted 7 lines · Alan Cox
Quoted 2 lines

Which requires from the drivers to be able to turn off IDE prefetch (and
maybe posting too). I don't see that in this patch (or are you expecting them to just "snoop' the commands and do it automagically?).

We need to see which drivers have that problem and if its only the odd one then we can handle it in qc_issue. We've already got controllers that wrap qc_issue to peek at commands and reject some of them (eg the 8212 in hardware raid mode)

I'd expect this to be needed with all drives, since IIUC prefetch is
working based on either fixed or programmable sector size. And here the out-of-band PIO protocol itself (which fails to be taken into account by the patch) implies mixed 8- and 16- bit I/O which should also be muddling up the prefetch logic.

Quoted 1 line

Alan

WBR, Sergei

#22 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

In reply to Sergei Shtylyov
Quoted 5 lines · Sergei Shtylyov
Quoted 2 lines

The fussy bit is that these commands require a non-standard sector size, usually 520 bytes instead of 512.

Which requires from the drivers to be able to turn off IDE prefetch
(and maybe posting too). I don't see that in this patch (or are you expecting them to just "snoop' the commands and do it automagically?).

Most controllers seem to be behave well enough
and just not muck up. And if it fails on any "too clever" controllers, then no big deal -- we just switch to a controller that works instead.

This is not an essential command for normal operation. What it is for, is injecting errors so that we can experience firsthand the (excellent or crappy) error handling in the kernel and in applications. And then work out and test better strategies.

So 100% working everywhere, for an obsolete opcode, isn't really essential to me. But it should work most places, and it does.

Quoted 3 lines

Again, ata_data_xfer() doesn't seem capable of performing ECC read/writes
-- the ECC bytes must be transferred in 8-bit mode, AFAIR. ata_data_xfer() can oinly do that for optionally trailing odd byte.

I have no idea what that was all about. Care to explain again? RWLONG transfer the ECC info 8-bits at a time, using 16-bit words to do so, no different from normal. ???

Here it is again with the whitespace not so messed up.

Thanks.

The READ/WRITE LONG commands are theoretically obsolete, but the majority of drives in existance still implement them.

The WRITE_LONG and WRITE_LONG_ONCE commands are of particular interest for fault injection testing -- eg. creating "media errors" at specific locations on a disk.

The fussy bit is that these commands require a non-standard sector size, usually 520 bytes instead of 512.

This patch adds support to libata for READ/WRITE LONG commands issued via SG_IO/ATA_16.

This patch was generated against a 2.6.21-rc3-git7 base:

drivers/ata/libata-core.c |   20 ++++++++++----------
drivers/ata/libata-scsi.c |   12 ++++++++++++
include/linux/ata.h       |    6 ++++++
include/linux/libata.h    |    2 ++
4 files changed, 30 insertions(+), 10 deletions(-)

---

include/linux/libata.h renamed from include/linux/libata.h.orig +0 -0
@@ -425,6 +425,7 @@
	int			dma_dir;

	unsigned int		pad_len;

+ unsigned int sect_size;

	unsigned int		nbytes;
	unsigned int		curbytes;
@@ -1171,6 +1172,7 @@
	qc->n_elem = 0;
	qc->err_mask = 0;
	qc->pad_len = 0;

+ qc->sect_size = ATA_SECT_SIZE;

	ata_tf_init(qc->dev, &qc->tf);
include/linux/ata.h renamed from include/linux/ata.h.orig +0 -0
@@ -163,6 +163,12 @@
	/* READ_LOG_EXT pages */
	ATA_LOG_SATA_NCQ	= 0x10,

+ /* READ/WRITE LONG (obsolete) */
+ ATA_CMD_READ_LONG = 0x22,
+ ATA_CMD_READ_LONG_ONCE = 0x23,
+ ATA_CMD_WRITE_LONG = 0x32,
+ ATA_CMD_WRITE_LONG_ONCE = 0x33,
+

	/* SETFEATURES stuff */
	SETFEATURES_XFER	= 0x03,
	XFER_UDMA_7		= 0x47,
drivers/ata/libata-scsi.c renamed from drivers/ata/libata-scsi.c.orig +0 -0
@@ -2678,6 +2678,18 @@
		tf->device = qc->dev->devno ?
			tf->device | ATA_DEV1 : tf->device & ~ATA_DEV1;

+ /* READ/WRITE LONG use a non-standard sect_size */
+ qc->sect_size = ATA_SECT_SIZE;
+ switch (tf->command) {
+ case ATA_CMD_READ_LONG:
+ case ATA_CMD_READ_LONG_ONCE:
+ case ATA_CMD_WRITE_LONG:
+ case ATA_CMD_WRITE_LONG_ONCE:
+ if (tf->protocol != ATA_PROT_PIO || tf->nsect != 1)
+ goto invalid_fld;
+ qc->sect_size = scmd->request_bufflen;
+ }
+
/*
* Filter SET_FEATURES - XFER MODE command -- otherwise,
* SET_FEATURES - XFER MODE must be preceded/succeeded

drivers/ata/libata-core.c renamed from drivers/ata/libata-core.c.orig +0 -0
@@ -4032,10 +4032,10 @@
 
 

/**
- * ata_pio_sector - Transfer ATA_SECT_SIZE (512 bytes) of data.
+ * ata_pio_sector - Transfer a sector of data.
* @qc: Command on going
*
- * Transfer ATA_SECT_SIZE of data from/to the ATA device.
+ * Transfer qc->sect_size bytes of data from/to the ATA device.
*
* LOCKING:
* Inherited from caller.
@@ -4050,7 +4050,7 @@

	unsigned int offset;
	unsigned char *buf;

- if (qc->curbytes == qc->nbytes - ATA_SECT_SIZE)
+ if (qc->curbytes == qc->nbytes - qc->sect_size)

		ap->hsm_task_state = HSM_ST_LAST;

	page = sg[qc->cursg].page;
@@ -4070,17 +4070,17 @@
		buf = kmap_atomic(page, KM_IRQ0);

		/* do the actual data transfer */

- ap->ops->data_xfer(qc->dev, buf + offset, ATA_SECT_SIZE, do_write);
+ ap->ops->data_xfer(qc->dev, buf + offset, qc->sect_size, do_write);

		kunmap_atomic(buf, KM_IRQ0);
		local_irq_restore(flags);
	} else {
		buf = page_address(page);
-		ap->ops->data_xfer(qc->dev, buf + offset, ATA_SECT_SIZE, do_write);
+		ap->ops->data_xfer(qc->dev, buf + offset, qc->sect_size, do_write);
	}

- qc->curbytes += ATA_SECT_SIZE;
- qc->cursg_ofs += ATA_SECT_SIZE;
+ qc->curbytes += qc->sect_size;
+ qc->cursg_ofs += qc->sect_size;

	if (qc->cursg_ofs == (&sg[qc->cursg])->length) {
		qc->cursg++;

@@ -4089,10 +4089,10 @@
}

/**
- * ata_pio_sectors - Transfer one or many 512-byte sectors.
+ * ata_pio_sectors - Transfer one or many sectors.
* @qc: Command on going
*
- * Transfer one or many ATA_SECT_SIZE of data from/to the
+ * Transfer one or many sectors of data from/to the
* ATA device for the DRQ request.
*
* LOCKING:
@@ -4107,7 +4107,7 @@

		WARN_ON(qc->dev->multi_count == 0);

- nsect = min((qc->nbytes - qc->curbytes) / ATA_SECT_SIZE,
+ nsect = min((qc->nbytes - qc->curbytes) / qc->sect_size,

			    qc->dev->multi_count);
		while (nsect--)
			ata_pio_sector(qc);
#23 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Hello.

Quoted 9 lines · Mark Lord
Quoted 5 lines
Quoted 2 lines

The fussy bit is that these commands require a non-standard sector size, usually 520 bytes instead of 512.

Which requires from the drivers to be able to turn off IDE prefetch
(and maybe posting too). I don't see that in this patch (or are you expecting them to just "snoop' the commands and do it automagically?).

Most controllers seem to be behave well enough
and just not muck up. And if it fails on any "too clever" controllers, then no big deal -- we just switch to a controller that works instead.

IDE controllers are generally not known as "clever" -- and that's what's
frightening me... :-)

Quoted 7 lines

This is not an essential command for normal operation. What it is for, is injecting errors so that we can experience firsthand the (excellent or crappy) error handling in the kernel and in applications. And then work out and test better strategies.

So 100% working everywhere, for an obsolete opcode, isn't really essential to me. But it should work most places, and it does.

I don't think it should *correctly* work anywhere w/prefetch on, IDE bus
wise. Maybe that just doesn't show up on host side (but well might have been).

Quoted 7 lines
Quoted 5 lines

Again, ata_data_xfer() doesn't seem capable of performing ECC
read/writes
-- the ECC bytes must be transferred in 8-bit mode, AFAIR. ata_data_xfer()
can oinly do that for optionally trailing odd byte.

I have no idea what that was all about. Care to explain again?

Care to read the standards? :-/

Quoted 2 lines

RWLONG transfer the ECC info 8-bits at a time, using 16-bit words to do so, no different from normal. ???

From ATA-1:

"The transfer of the vendor specific bytes shall be one byte at a time
over bits DD0-7 only (8-bits wide)."

Quoted 1 line

Here it is again with the whitespace not so messed up.

I'm sorry, it seems only being messed on my side, due to format=flowed. :-/

Quoted 9 lines

The READ/WRITE LONG commands are theoretically obsolete, but the majority of drives in existance still implement them.

The WRITE_LONG and WRITE_LONG_ONCE commands are of particular interest for fault injection testing -- eg. creating "media errors" at specific locations on a disk.

The fussy bit is that these commands require a non-standard sector size, usually 520 bytes instead of 512.

It's 516 bytes, not 512 as you claim. Well, maybe the extra odd bytes
don't matter anyway in this GIGO case... :-)

Quoted 4 lines

This patch adds support to libata for READ/WRITE LONG commands issued via SG_IO/ATA_16.

This patch was generated against a 2.6.21-rc3-git7 base:

MBR, Sergei

#24 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 3 lines · Sergei Shtylyov

Hello.

Mark Lord wrote:

..

Quoted 18 lines
Quoted 7 lines
Quoted 5 lines

Again, ata_data_xfer() doesn't seem capable of performing ECC
read/writes
-- the ECC bytes must be transferred in 8-bit mode, AFAIR. ata_data_xfer()
can oinly do that for optionally trailing odd byte.

I have no idea what that was all about. Care to explain again?

Care to read the standards? :-/

Quoted 2 lines

RWLONG transfer the ECC info 8-bits at a time, using 16-bit words to do so, no different from normal. ???

From ATA-1:

"The transfer of the vendor specific bytes shall be one byte at a
time over bits DD0-7 only (8-bits wide)."

Quoted 6 lines

Yes, I said that already. 8-bits at a time, but using 16-bit transfers. Kinda like it says here, in ATA-3 section 7.16:

The transfer of the vendor specifc bytes shall be 16 bit transfers with the vendor specific byte in bits 7 through 0. Bits 15 through 8 shall be ignored by the host.

But if we really want to be 100% compliant, we could consider dropping to PIO0 for the command. Not worth it, though, as in practice this is not necessary, and it would mess up libata far more than is worthwhile for this effort.

Cheers

#25 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 3 lines · Sergei Shtylyov

Hello.

Mark Lord wrote:

..

Quoted 6 lines
Quoted 5 lines
Quoted 3 lines

Again, ata_data_xfer() doesn't seem capable of performing ECC read/writes
-- the ECC bytes must be transferred in 8-bit mode, AFAIR. ata_data_xfer() can oinly do that for optionally trailing odd byte.

I have no idea what that was all about. Care to explain again?

Quoted 8 lines

Care to read the standards? :-/

Quoted 2 lines

RWLONG transfer the ECC info 8-bits at a time, using 16-bit words to do so, no different from normal. ???

From ATA-1:

"The transfer of the vendor specific bytes shall be one byte at a time over bits DD0-7 only (8-bits wide)."

Yes, I said that already. 8-bits at a time, but using 16-bit transfers. Kinda like it says here, in ATA-3 section 7.16:

Quoted 3 lines

The transfer of the vendor specifc bytes shall be 16 bit transfers with the vendor specific byte in bits 7 through 0. Bits 15 through 8 shall be ignored by the host.

But if we really want to be 100% compliant, we could consider dropping to PIO0 for the command. Not worth it, though, as in practice this is not necessary, and it would mess up libata far more than is worthwhile for this effort.

Cheers

#26 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Hello.

Quoted 13 lines · Mark Lord
Quoted 12 lines
Quoted 7 lines
Quoted 5 lines

Again, ata_data_xfer() doesn't seem capable of performing ECC
read/writes
-- the ECC bytes must be transferred in 8-bit mode, AFAIR. ata_data_xfer()
can oinly do that for optionally trailing odd byte.

I have no idea what that was all about. Care to explain again?

Care to read the standards? :-/

Quoted 2 lines

RWLONG transfer the ECC info 8-bits at a time, using 16-bit words to do so, no different from normal. ???

Quoted 7 lines
Quoted 4 lines

From ATA-1:

"The transfer of the vendor specific bytes shall be one byte at a
time over bits DD0-7 only (8-bits wide)."

Yes, I said that already. 8-bits at a time, but using 16-bit transfers. Kinda like it says here, in ATA-3 section 7.16:

Heh, I give up. There was no word of 16-bit transfers before ATA-3. And
waht I saw was the code doing this by 8-bit I/O (evem with added delays).
Well, those ATA specs have always been quite messy: for example, polling
protocol had an unnoticed race for years (device was allowed to clear BSY before asserting INTRQ, so there was no guarantee that the host's reading of the status reg. will actually clear drive's interrupt)...

Quoted 6 lines
Quoted 3 lines

The transfer of the vendor specifc bytes shall be 16 bit transfers with the vendor specific byte in bits 7 through 0. Bits 15 through 8 shall be ignored by the host.

But if we really want to be 100% compliant, we could consider dropping to PIO0 for the command.

Yeah, reading ATA-2 we see:

NOTE 31 - Some ATA-1 devices are not capable of delivering the 8 bit ECC immediately after the word sector data. BIOS and driver developers should use PIO mode 0 for 8 bit ECC accesses.

That explains the delays I saw...

Quoted 3 lines

Not worth it, though, as in practice this is
not necessary, and it would mess up libata far more than is worthwhile for this effort.

Agreed.

Quoted 1 line

Cheers

MBR, Sergei

#27 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 3 lines · Sergei Shtylyov

..
Well, those ATA specs have always been quite messy: for example,
polling protocol had an unnoticed race for years (device was allowed to clear BSY before asserting INTRQ, so there was no guarantee that the host's reading of the status reg. will actually clear drive's interrupt)...

Hey.. a chap after my own heart! Been there, done that!

Quoted 1 line

NOTE 31 - Some ATA-1 devices are not capable of delivering the 8 bit ECC immediately after the word sector data. BIOS and driver developers should use PIO mode 0 for 8 bit ECC accesses.

Exactly. But we're not really interested so much in the rare ATA1 drive that fails (the ATA1 Conner unit I have here works fine with this patch).

The idea here is to give us a way to inject errors into (more or less) current drives (ata6/7 vintages) so we can see what happens when FPDMA on a SATA drive hits a bad sector, etc..

For which it seems to work rather well.

Cheers

#28 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

In reply to Mark Lord

Hello.

Quoted 4 lines · Mark Lord

But if we really want to be 100% compliant, we could consider dropping to PIO0 for the command. Not worth it, though, as in practice this is not necessary, and it would mess up libata far more than is worthwhile for this effort.

I think it only requires the *host* to drop to PIO0 timings. In which
case it should be achievable w/o libata modification -- if the driver has to "snoop" command and turn off prefetch, why not switch to PIO0 temporarily?

Quoted 1 line

Cheers

MBR, Sergei

#29 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 4 lines

I think it only requires the *host* to drop to PIO0 timings. In which case it should be achievable w/o libata modification -- if the driver has to "snoop" command and turn off prefetch, why not switch to PIO0 temporarily?

Because (1) it is not actually necessary in practice, and (2) see point (1),
and (3) that would only be half of the story,
in that it would need to be turned back on again afterwards, and (4) not all drivers support PIO mode changes.

Cheers

#30 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

In reply to Sergei Shtylyov
Quoted 3 lines

I think it only requires the *host* to drop to PIO0 timings. In which
case it should be achievable w/o libata modification -- if the driver has to "snoop" command and turn off prefetch, why not switch to PIO0 temporarily?

This isn't a big issue. Eventually we have to support sending speed change commands and once we do that the caller will be able to switch to PIO0 and back again.

Alan

#31 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 6 lines · Alan Cox
Quoted 2 lines

I think it only requires the *host* to drop to PIO0 timings. In which
case it should be achievable w/o libata modification -- if the driver has to "snoop" command and turn off prefetch, why not switch to PIO0 temporarily?

This isn't a big issue. Eventually we have to support sending speed change commands and once we do that the caller will be able to switch to PIO0 and back again.

We should be pretty close to that. One of the bigger blockages is needing to stop (freeze) all ports, before performing an out-of-band SET FEATURES and related tuning.

For controllers that don't need such synchronization (though I argue it is wise practice for all controllers) -- generally the ones that do not snoop SET FEATURES -- you could probably get it going with a few lines of code change: removing the current SET FEATURES - XFER MODE filter, and calling the requisite callbacks.

Even in the current EH we don't get this quite right. The main culprit is Promise controllers. SET FEATURES is snooped on these controllers, and registers are programmed. It is known to corrupt data if a mode change occurs while data is transferring on /other/ ports. If new-EH decides to change the mode on a Promise controller, we might hit this scenario, because the apparatus does not know to stop all ports on that controller, rather than just the single port that is current frozen and inside EH.

Jeff

#32 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 9 lines · Jeff Garzik
Quoted 6 lines · Alan Cox
Quoted 2 lines

I think it only requires the *host* to drop to PIO0 timings. In
which case it should be achievable w/o libata modification -- if the driver has to "snoop" command and turn off prefetch, why not switch to PIO0 temporarily?

This isn't a big issue. Eventually we have to support sending speed change commands and once we do that the caller will be able to switch to PIO0 and back again.

We should be pretty close to that. One of the bigger blockages is needing to stop (freeze) all ports, before performing an out-of-band SET FEATURES and related tuning.

For this particular feature, READ/WRITE LONG,
none of this is actually necessary in practice on common controllers.

I've tested ATA1 and newer PATA drives, and various SATA drives with these commands without bothering to drop to PIO0, and none of them had issues.

Cheers

#33 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 9 lines · Mark Lord
Quoted 8 lines · Jeff Garzik
Quoted 6 lines · Alan Cox
Quoted 2 lines

I think it only requires the *host* to drop to PIO0 timings. In
which case it should be achievable w/o libata modification -- if the driver has to "snoop" command and turn off prefetch, why not switch to PIO0 temporarily?

This isn't a big issue. Eventually we have to support sending speed change commands and once we do that the caller will be able to switch to PIO0 and back again.

Quoted 8 lines
Quoted 1 line

We should be pretty close to that. One of the bigger blockages is needing to stop (freeze) all ports, before performing an out-of-band SET FEATURES and related tuning.

For this particular feature, READ/WRITE LONG,
none of this is actually necessary in practice on common controllers.

I've tested ATA1 and newer PATA drives, and various SATA drives with these commands without bothering to drop to PIO0, and none of them had issues.

I wasn't implying a blockage of your patch, just describing a general problem within libata.

Jeff

#34 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

In reply to Mark Lord
Quoted 11 lines · Mark Lord
Quoted 10 lines · Jeff Garzik
Quoted 8 lines · Alan Cox
Quoted 4 lines

I think it only requires the *host* to drop to PIO0 timings. In
which case it should be achievable w/o libata modification -- if the driver has to "snoop" command and turn off prefetch, why not switch to PIO0 temporarily?

This isn't a big issue. Eventually we have to support sending speed change commands and once we do that the caller will be able to switch to PIO0 and back again.

Quoted 6 lines
Quoted 3 lines

We should be pretty close to that. One of the bigger blockages is needing to stop (freeze) all ports, before performing an out-of-band SET FEATURES and related tuning.

For this particular feature, READ/WRITE LONG,
none of this is actually necessary in practice on common controllers.

It still would be nice to add a comment documenting issues raised by Sergei so if somebody actually hits a problem on not-so-common controller she/he will know what to look for.

Quoted 3 lines

I've tested ATA1 and newer PATA drives, and various SATA drives with these commands without bothering to drop to PIO0, and none of them had issues.

This is what really matters wrt libata READ/WRITE LONG support.

These commands are not supposed to be used by normal users anyway so patch while not being "perfect" is "good enough" to be merged IMO.

Bart

#35 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 1 line · Bartlomiej Zolnierkiewicz

On Friday 16 March 2007, Mark Lord wrote:

..

Quoted 8 lines
Quoted 3 lines

I've tested ATA1 and newer PATA drives, and various SATA drives with these commands without bothering to drop to PIO0, and none of them had issues.

This is what really matters wrt libata READ/WRITE LONG support.

These commands are not supposed to be used by normal users anyway so patch while not being "perfect" is "good enough" to be merged IMO.

Heh.. I have a similar patch for IDE as well,
but not up-to-date for the latest kernels.

Cheers

#36 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

Quoted 7 lines · Mark Lord
Quoted 1 line · Bartlomiej Zolnierkiewicz

On Friday 16 March 2007, Mark Lord wrote:

..

Quoted 4 lines
Quoted 3 lines

I've tested ATA1 and newer PATA drives, and various SATA drives with these commands without bothering to drop to PIO0, and none of them had issues.

Quoted 6 lines
Quoted 4 lines

This is what really matters wrt libata READ/WRITE LONG support.

These commands are not supposed to be used by normal users anyway so patch while not being "perfect" is "good enough" to be merged IMO.

Heh.. I have a similar patch for IDE as well,

Please send it out, I will happily apply it.

Quoted 1 line

but not up-to-date for the latest kernels.

The most changes recently have been in host drivers so probably the patch should apply/work fine (or require minimal fixups)...

Thanks,
Bart

#37 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

In reply to Mark Lord

Hello.

Quoted 3 lines · Mark Lord

I've tested ATA1 and newer PATA drives, and various SATA drives with these commands without bothering to drop to PIO0, and none of them had issues.

Of course -- that PIO0 warning was for the sake of "some ATA-1 drives". :-)

Quoted 1 line

Cheers

MBR, Sergei

Applied
#38 lore raw

Re: [PATCH] libata: add support for READ/WRITE LONG

In reply to Mark Lord
Quoted 48 lines · Mark Lord
Quoted 5 lines · Sergei Shtylyov
Quoted 2 lines

The fussy bit is that these commands require a non-standard sector size, usually 520 bytes instead of 512.

Which requires from the drivers to be able to turn off IDE prefetch
(and maybe posting too). I don't see that in this patch (or are you expecting them to just "snoop' the commands and do it automagically?).

Most controllers seem to be behave well enough
and just not muck up. And if it fails on any "too clever" controllers, then no big deal -- we just switch to a controller that works instead.

This is not an essential command for normal operation. What it is for, is injecting errors so that we can experience firsthand the (excellent or crappy) error handling in the kernel and in applications. And then work out and test better strategies.

So 100% working everywhere, for an obsolete opcode, isn't really essential to me. But it should work most places, and it does.

Quoted 5 lines

Again, ata_data_xfer() doesn't seem capable of performing ECC
read/writes
-- the ECC bytes must be transferred in 8-bit mode, AFAIR. ata_data_xfer()
can oinly do that for optionally trailing odd byte.

I have no idea what that was all about. Care to explain again? RWLONG transfer the ECC info 8-bits at a time, using 16-bit words to do so, no different from normal. ???

Here it is again with the whitespace not so messed up.

Thanks.

The READ/WRITE LONG commands are theoretically obsolete, but the majority of drives in existance still implement them.

The WRITE_LONG and WRITE_LONG_ONCE commands are of particular interest for fault injection testing -- eg. creating "media errors" at specific locations on a disk.

The fussy bit is that these commands require a non-standard sector size, usually 520 bytes instead of 512.

This patch adds support to libata for READ/WRITE LONG commands issued via SG_IO/ATA_16.

This patch was generated against a 2.6.21-rc3-git7 base:

Quoted 7 lines
drivers/ata/libata-core.c |   20 ++++++++++----------
drivers/ata/libata-scsi.c |   12 ++++++++++++
include/linux/ata.h       |    6 ++++++
include/linux/libata.h    |    2 ++
4 files changed, 30 insertions(+), 10 deletions(-)

applied

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#41 lore raw
Quoted 1 line

Why is the access to Control register needed?

To execute soft reset for example.

Quoted 6 lines
Quoted 2 lines

In the perfect case i would like to be able to execute vendor command set (reverse engineered).

Sounds interesting. :-)

Could you give some more details on what are you going to implement?

Reading/writing service area, uploading, downloading modules, working with flash etc.

#42 lore raw
Quoted 7 lines · Vitaliyi
Quoted 1 line

Why is the access to Control register needed?

To execute soft reset for example.

Quoted 3 lines
Quoted 2 lines

In the perfect case i would like to be able to execute vendor command set (reverse engineered).

Quoted 6 lines
Quoted 3 lines

Sounds interesting. :-)

Could you give some more details on what are you going to implement?

Reading/writing service area, uploading, downloading modules, working with flash etc.

SAT (aka ATA passthru) defines how to do soft-reset.

SG_IO supports the ATA_12 and ATA_16 commands which permit soft-reset and similar tasks. libata supports this interface, but does not yet support soft-reset and similar non-comment-oriented tasks. This would be the best area to add such features, though.

Jeff