Re: libata extension

Re: libata extension

[Mark Lord]

Vitaliyi wrote:
> 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

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

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

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

Signed-off-by:  Mark Lord <mlord@pobox.com>
---
--- 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
@@ -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);
 
--- linux/include/linux/ata.h.orig	2007-03-07 09:21:25.000000000 -0500
+++ linux/include/linux/ata.h	2007-03-12 13:15:45.000000000 -0400
@@ -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
@@ -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
@@ -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);

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

[Mark Lord]

Mark Lord 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.
> 
> 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

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

[Alan Cox]

On Mon, 12 Mar 2007 15:08:19 -0400
Mark Lord <liml@rtr.ca> 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

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

[Mark Lord]

Alan Cox wrote:
> On Mon, 12 Mar 2007 15:08:19 -0400
> Mark Lord <liml@rtr.ca> 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 ?

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

???

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

[Mark Lord]

Alan Cox wrote:
>
> 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.

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

It works for me, on a variety of hardware.

Cheers

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

[Alan Cox]

On Mon, 12 Mar 2007 18:23:22 -0400
Mark Lord <liml@rtr.ca> wrote:

> Alan Cox wrote:
> > On Mon, 12 Mar 2007 15:08:19 -0400
> > Mark Lord <liml@rtr.ca> 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 ?
> 
> 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

Re: Fwd: libata extension

[Vitaliyi]

> Why is the access to Control register needed?

To execute soft reset for example.

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

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

[Tejun Heo]

Mark Lord 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.
> 
> 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.

-- 
tejun

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

[Ric Wheeler]

Tejun Heo wrote:
> Mark Lord 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.
>>
>> 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.
>   
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

Re: Fwd: libata extension

[Jeff Garzik]

Vitaliyi wrote:
>> Why is the access to Control register needed?
> 
> To execute soft reset for example.
> 
>> > 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.

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

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

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

Signed-off-by:  Mark Lord <mlord@pobox.com>
---
--- 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
@@ -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);
 
--- linux/include/linux/ata.h.orig	2007-03-07 09:21:25.000000000 -0500
+++ linux/include/linux/ata.h	2007-03-12 13:15:45.000000000 -0400
@@ -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
@@ -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
@@ -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);

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

[Sergei Shtylyov]

Hello.

Mark Lord 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.

> 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?).

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

> Signed-off-by:  Mark Lord <mlord@pobox.com>
> ---
> --- 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
> @@ -425,6 +425,7 @@
>     int            dma_dir;
> 
>     unsigned int        pad_len;
> +    unsigned int        sect_size;
> 
>     unsigned int        nbytes;
>     unsigned int        curbytes;

    Tabs are spoilt. :-/

> @@ -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);
> 
> --- linux/include/linux/ata.h.orig    2007-03-07 09:21:25.000000000 -0500
> +++ linux/include/linux/ata.h    2007-03-12 13:15:45.000000000 -0400
> @@ -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
> @@ -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
> @@ -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++;

    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

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

[Mark Lord]

Alan Cox wrote:
>> 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

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

[Sergei Shtylyov]

Hello.

Alan Cox wrote:
>>    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.

> Alan

WBR, Sergei

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

[Mark Lord]

Sergei Shtylyov wrote:
>> 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.

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

Signed-off-by:  Mark Lord <mlord@pobox.com>
---

--- 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
@@ -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);
 
--- linux/include/linux/ata.h.orig	2007-03-07 09:21:25.000000000 -0500
+++ linux/include/linux/ata.h	2007-03-12 13:15:45.000000000 -0400
@@ -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
@@ -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
@@ -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);

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

[Sergei Shtylyov]

Mark Lord wrote:
> Alan Cox wrote:
> 
>>> 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).

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

> 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

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

[Mark Lord]

Sergei Shtylyov wrote:
> Mark Lord wrote:
>
>> 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:

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

> 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

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

[Sergei Shtylyov]

Hello.

Mark Lord wrote:

>>> 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... :-)

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

>>    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? :-/

> 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)."

> 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. :-/

> 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... :-)

> 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

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

[Mark Lord]

Sergei Shtylyov wrote:
> Hello.
> 
> Mark Lord wrote:
..
>>>    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? :-/
> 
>> 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:

> 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

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

[Mark Lord]

Sergei Shtylyov wrote:
> Hello.
>
> Mark Lord wrote:
..
>>>    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? :-/
>
>> 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:

> 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 

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

[Sergei Shtylyov]

Hello.

Mark Lord wrote:

>>>>    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? :-/

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

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

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

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

> Cheers

MBR, Sergei

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

[Alan Cox]

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

> 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(-)
> 
> Signed-off-by:  Mark Lord <mlord@pobox.com>

Acked-by: Alan Cox <alan@redhat.com>

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

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

[Mark Lord]

Sergei Shtylyov wrote:
>..
>    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!

> 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 

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

[Alan Cox]

>     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

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

[Sergei Shtylyov]

Hello.

Mark Lord wrote:

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

> Cheers

MBR, Sergei

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

[Alan Cox]

> 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

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

[Mark Lord]

> 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

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

[Alan Cox]

>     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

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

[Jeff Garzik]

Alan Cox wrote:
>>     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

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

[Mark Lord]

Jeff Garzik wrote:
> Alan Cox wrote:
>>>     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

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

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
>> Alan Cox wrote:
>>>>     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.

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

	Jeff

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

[Sergei Shtylyov]

Hello.

Mark Lord wrote:

> 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". :-)

> Cheers

MBR, Sergei

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

[Bartlomiej Zolnierkiewicz]

On Friday 16 March 2007, Mark Lord wrote:
> Jeff Garzik wrote:
> > Alan Cox wrote:
> >>>     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.

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.

> 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

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

[Mark Lord]

Bartlomiej Zolnierkiewicz wrote:
> On Friday 16 March 2007, Mark Lord wrote:
..
>> 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

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

[Bartlomiej Zolnierkiewicz]

On Friday 16 March 2007, Mark Lord wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > On Friday 16 March 2007, Mark Lord wrote:
> ..
> >> 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,

Please send it out, I will happily apply it.

> 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

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

[Jeff Garzik]

Mark Lord wrote:
> Sergei Shtylyov wrote:
>>> 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.
> 
>>    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(-)
> 
> Signed-off-by:  Mark Lord <mlord@pobox.com>

applied