[PATCH]

[PATCH]

[Alan Cox]

Correct handling of SRST reset sequences. After an SRST it is undefined
whether the drive has gone back to PIO0. In order to talk safely we should
talk slowly and carefully until we know.

Thus when we do the reset if the controller has a pio setup method we call
it to flip back to PIO 0 and a known state. After the reset completes the
identify will then be done at the safe speed and the drive/controller will
pick suitable faster modes and reconfigure the controller to these timings.

As a side effect it means we force the controller to PIO 0 as we bring it
up which fixes funnies on a few systems where the BIOS firmware leaves us
in an interesting choice of modes, or embedded boxes with no firmware which
come up in random states.

For smart controllers there is nothing to do - they know about this internally.

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


diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc3-mm1/drivers/ata/libata-core.c linux-2.6.23rc3-mm1/drivers/ata/libata-core.c
--- linux.vanilla-2.6.23rc3-mm1/drivers/ata/libata-core.c	2007-08-22 17:23:00.000000000 +0100
+++ linux-2.6.23rc3-mm1/drivers/ata/libata-core.c	2007-08-22 18:17:31.321738376 +0100
@@ -3163,6 +3191,8 @@
 			     unsigned long deadline)
 {
 	struct ata_ioports *ioaddr = &ap->ioaddr;
+	struct ata_device *dev;
+	int i = 0;
 
 	DPRINTK("ata%u: bus reset via SRST\n", ap->print_id);
 
@@ -3173,6 +3203,25 @@
 	udelay(20);	/* FIXME: flush */
 	iowrite8(ap->ctl, ioaddr->ctl_addr);
 
+	/* If we issued an SRST then an ATA drive (not ATAPI)
+	 * may have changed configuration and be in PIO0 timing. If
+	 * we did a hard reset (or are coming from power on) this is
+	 * true for ATA or ATAPI. Until we've set a suitable controller
+	 * mode we should not touch the bus as we may be talking too fast.
+	 */
+
+	ata_link_for_each_dev(dev, &ap->link)
+		dev->pio_mode = XFER_PIO_0;
+
+	/* If the controller has a pio mode setup function then use
+	   it to set the chipset to rights. Don't touch the DMA setup
+	   as that will be dealt with when revalidating */
+	if (ap->ops->set_piomode) {
+		ata_link_for_each_dev(dev, &ap->link)
+			if (devmask & (1 << i++))
+				ap->ops->set_piomode(ap, dev);
+	}
+
 	/* wait a while before checking status */
 	ata_wait_after_reset(ap, deadline);
 

Re: [PATCH]

[Jeff Garzik]

Alan Cox wrote:
> Correct handling of SRST reset sequences. After an SRST it is undefined
> whether the drive has gone back to PIO0. In order to talk safely we should
> talk slowly and carefully until we know.
> 
> Thus when we do the reset if the controller has a pio setup method we call
> it to flip back to PIO 0 and a known state. After the reset completes the
> identify will then be done at the safe speed and the drive/controller will
> pick suitable faster modes and reconfigure the controller to these timings.
> 
> As a side effect it means we force the controller to PIO 0 as we bring it
> up which fixes funnies on a few systems where the BIOS firmware leaves us
> in an interesting choice of modes, or embedded boxes with no firmware which
> come up in random states.
> 
> For smart controllers there is nothing to do - they know about this internally.
> 
> Signed-off-by: Alan Cox <alan@redhat.com>
> 
> 
> diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc3-mm1/drivers/ata/libata-core.c linux-2.6.23rc3-mm1/drivers/ata/libata-core.c
> --- linux.vanilla-2.6.23rc3-mm1/drivers/ata/libata-core.c	2007-08-22 17:23:00.000000000 +0100
> +++ linux-2.6.23rc3-mm1/drivers/ata/libata-core.c	2007-08-22 18:17:31.321738376 +0100
> @@ -3163,6 +3191,8 @@
>  			     unsigned long deadline)
>  {
>  	struct ata_ioports *ioaddr = &ap->ioaddr;
> +	struct ata_device *dev;
> +	int i = 0;
>  
>  	DPRINTK("ata%u: bus reset via SRST\n", ap->print_id);
>  
> @@ -3173,6 +3203,25 @@
>  	udelay(20);	/* FIXME: flush */
>  	iowrite8(ap->ctl, ioaddr->ctl_addr);
>  
> +	/* If we issued an SRST then an ATA drive (not ATAPI)
> +	 * may have changed configuration and be in PIO0 timing. If
> +	 * we did a hard reset (or are coming from power on) this is
> +	 * true for ATA or ATAPI. Until we've set a suitable controller
> +	 * mode we should not touch the bus as we may be talking too fast.
> +	 */
> +
> +	ata_link_for_each_dev(dev, &ap->link)
> +		dev->pio_mode = XFER_PIO_0;
> +
> +	/* If the controller has a pio mode setup function then use
> +	   it to set the chipset to rights. Don't touch the DMA setup
> +	   as that will be dealt with when revalidating */
> +	if (ap->ops->set_piomode) {
> +		ata_link_for_each_dev(dev, &ap->link)
> +			if (devmask & (1 << i++))
> +				ap->ops->set_piomode(ap, dev);
> +	}
> +

Should this be queued for 2.6.24?

The note I have on this is "low priority, make sure it gets testing in 
-mm first"

Re: [PATCH]

[Alan Cox]

> Should this be queued for 2.6.24?
> 
> The note I have on this is "low priority, make sure it gets testing in 
> -mm first"


Its been tested without any reports. There are two things it sorts out

#1 non x86 systems with firmware that doesn't init the chip into PIO0
(not many as most chips power up in pio0)

#2 A few obscure cases where we error and then can't recover an ATA drive
as we are talking PIO4 at it and its in PIO0 so we are burbling too fast
(the other way around is of course ok)

Alan

Re: [PATCH]

[Jeff Garzik]

Alan Cox wrote:
> Correct handling of SRST reset sequences. After an SRST it is undefined
> whether the drive has gone back to PIO0. In order to talk safely we should
> talk slowly and carefully until we know.
> 
> Thus when we do the reset if the controller has a pio setup method we call
> it to flip back to PIO 0 and a known state. After the reset completes the
> identify will then be done at the safe speed and the drive/controller will
> pick suitable faster modes and reconfigure the controller to these timings.
> 
> As a side effect it means we force the controller to PIO 0 as we bring it
> up which fixes funnies on a few systems where the BIOS firmware leaves us
> in an interesting choice of modes, or embedded boxes with no firmware which
> come up in random states.
> 
> For smart controllers there is nothing to do - they know about this internally.
> 
> Signed-off-by: Alan Cox <alan@redhat.com>
> 
> 
> diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.23rc3-mm1/drivers/ata/libata-core.c linux-2.6.23rc3-mm1/drivers/ata/libata-core.c
> --- linux.vanilla-2.6.23rc3-mm1/drivers/ata/libata-core.c	2007-08-22 17:23:00.000000000 +0100
> +++ linux-2.6.23rc3-mm1/drivers/ata/libata-core.c	2007-08-22 18:17:31.321738376 +0100
> @@ -3163,6 +3191,8 @@
>  			     unsigned long deadline)
>  {
>  	struct ata_ioports *ioaddr = &ap->ioaddr;
> +	struct ata_device *dev;
> +	int i = 0;
>  
>  	DPRINTK("ata%u: bus reset via SRST\n", ap->print_id);
>  
> @@ -3173,6 +3203,25 @@
>  	udelay(20);	/* FIXME: flush */
>  	iowrite8(ap->ctl, ioaddr->ctl_addr);
>  
> +	/* If we issued an SRST then an ATA drive (not ATAPI)
> +	 * may have changed configuration and be in PIO0 timing. If
> +	 * we did a hard reset (or are coming from power on) this is
> +	 * true for ATA or ATAPI. Until we've set a suitable controller
> +	 * mode we should not touch the bus as we may be talking too fast.
> +	 */
> +
> +	ata_link_for_each_dev(dev, &ap->link)
> +		dev->pio_mode = XFER_PIO_0;
> +
> +	/* If the controller has a pio mode setup function then use
> +	   it to set the chipset to rights. Don't touch the DMA setup
> +	   as that will be dealt with when revalidating */
> +	if (ap->ops->set_piomode) {
> +		ata_link_for_each_dev(dev, &ap->link)
> +			if (devmask & (1 << i++))
> +				ap->ops->set_piomode(ap, dev);
> +	}
> +
>  	/* wait a while before checking status */
>  	ata_wait_after_reset(ap, deadline);

ACK.  Requesting a patch that applies to libata-dev.git#upstream.

[PATCH]

[Sergey Shtylyov]

Add myself as a reviewer for the libata PATA drivers -- there has been some
activity in this area still... 8-)
Having been hacking on ATA from the early 90s, I think I deserved this
highly responsible position, at last! :-)

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

---
This patch is against the 'master' branch of Damien Le Moal's 'libata.git'
repo.

 MAINTAINERS |    6 ++++++
 1 file changed, 6 insertions(+)

Index: libata/MAINTAINERS
===================================================================
--- libata.orig/MAINTAINERS
+++ libata/MAINTAINERS
@@ -10880,6 +10880,12 @@ T:	git git://git.kernel.org/pub/scm/linu
 F:	drivers/ata/pata_arasan_cf.c
 F:	include/linux/pata_arasan_cf_data.h
 
+LIBATA PATA DRIVERS
+R:	Sergey Shtylyov <s.shtylyov@omp.ru>
+L:	linux-ide@vger.kernel.org
+F:	drivers/ata/ata_*.c
+F:	drivers/ata/pata_*.c
+
 LIBATA PATA FARADAY FTIDE010 AND GEMINI SATA BRIDGE DRIVERS
 M:	Linus Walleij <linus.walleij@linaro.org>
 L:	linux-ide@vger.kernel.org