[PATCH 2.6.19 2/3] sata_promise: new EH conversion

[PATCH 2.6.19 2/3] sata_promise: new EH conversion

[Mikael Pettersson]

This patch converts sata_promise to use new-style libata error
handling for its SATA ports. PATA is left unchanged.

* ATA_FLAG_SRST is no longer set for SATA ports
* ->phy_reset is no longer set as it is unused when ->error_handler
   is present, and pdc_sata_phy_reset() has been removed
* pdc_freeze() masks interrupts and halts DMA via PDC_CTLSTAT
* pdc_thaw() clears interrupt status in PDC_INT_SEQMASK and then
  unmasks interrupts in PDC_CTLSTAT
* pdc_error_handler() simply freezes the port and then invokes
  ata_do_eh() with standard {s,}ata reset methods
* pdc_post_internal_cmd() resets the port in case of errors

The changes are primarily modelled after ahci and sata_sil24.

These changes have been tested on Promise SATAII (205xx) chips.
I strongly believe they should work on SATAI chips as well, and
probably also on PATA chips (20619) and ports. However, since I
have no documentation for the PATA-only 20619, I let the driver
continue using old-style EH for it (easily changed).

This patch is intended to be applied on top of the sata_promise
SATAII updates patch I sent recently, but will apply and work
even if that patch has not been applied.

Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>

diff -rupN linux-2.6.19.sata_promise-2-PHYMODE4-fixup/drivers/ata/sata_promise.c linux-2.6.19.sata_promise-3-new_EH/drivers/ata/sata_promise.c
--- linux-2.6.19.sata_promise-2-PHYMODE4-fixup/drivers/ata/sata_promise.c	2006-11-30 23:36:57.000000000 +0100
+++ linux-2.6.19.sata_promise-3-new_EH/drivers/ata/sata_promise.c	2006-11-30 23:56:32.000000000 +0100
@@ -73,9 +73,12 @@ enum {
 
 	PDC_HAS_PATA		= (1 << 1), /* PDC20375/20575 has PATA */
 
+	/* PDC_CTLSTAT bit definitions */
+	PDC_DMA_ENABLE		= (1 << 7),
+	PDC_IRQ_DISABLE		= (1 << 10),
 	PDC_RESET		= (1 << 11), /* HDMA reset */
 
-	PDC_COMMON_FLAGS	= ATA_FLAG_NO_LEGACY | ATA_FLAG_SRST |
+	PDC_COMMON_FLAGS	= ATA_FLAG_NO_LEGACY |
 				  ATA_FLAG_MMIO | ATA_FLAG_NO_ATAPI |
 				  ATA_FLAG_PIO_POLLING,
 
@@ -102,13 +105,16 @@ static void pdc_eng_timeout(struct ata_p
 static int pdc_port_start(struct ata_port *ap);
 static void pdc_port_stop(struct ata_port *ap);
 static void pdc_pata_phy_reset(struct ata_port *ap);
-static void pdc_sata_phy_reset(struct ata_port *ap);
 static void pdc_qc_prep(struct ata_queued_cmd *qc);
 static void pdc_tf_load_mmio(struct ata_port *ap, const struct ata_taskfile *tf);
 static void pdc_exec_command_mmio(struct ata_port *ap, const struct ata_taskfile *tf);
 static void pdc_irq_clear(struct ata_port *ap);
 static unsigned int pdc_qc_issue_prot(struct ata_queued_cmd *qc);
 static void pdc_host_stop(struct ata_host *host);
+static void pdc_freeze(struct ata_port *ap);
+static void pdc_thaw(struct ata_port *ap);
+static void pdc_error_handler(struct ata_port *ap);
+static void pdc_post_internal_cmd(struct ata_queued_cmd *qc);
 
 
 static struct scsi_host_template pdc_ata_sht = {
@@ -137,11 +143,12 @@ static const struct ata_port_operations 
 	.exec_command		= pdc_exec_command_mmio,
 	.dev_select		= ata_std_dev_select,
 
-	.phy_reset		= pdc_sata_phy_reset,
-
 	.qc_prep		= pdc_qc_prep,
 	.qc_issue		= pdc_qc_issue_prot,
-	.eng_timeout		= pdc_eng_timeout,
+	.freeze			= pdc_freeze,
+	.thaw			= pdc_thaw,
+	.error_handler		= pdc_error_handler,
+	.post_internal_cmd	= pdc_post_internal_cmd,
 	.data_xfer		= ata_mmio_data_xfer,
 	.irq_handler		= pdc_interrupt,
 	.irq_clear		= pdc_irq_clear,
@@ -199,7 +206,7 @@ static const struct ata_port_info pdc_po
 	/* board_20619 */
 	{
 		.sht		= &pdc_ata_sht,
-		.flags		= PDC_COMMON_FLAGS | ATA_FLAG_SLAVE_POSS,
+		.flags		= PDC_COMMON_FLAGS | ATA_FLAG_SRST | ATA_FLAG_SLAVE_POSS,
 		.pio_mask	= 0x1f, /* pio0-4 */
 		.mwdma_mask	= 0x07, /* mwdma0-2 */
 		.udma_mask	= 0x7f, /* udma0-6 ; FIXME */
@@ -367,12 +374,6 @@ static void pdc_reset_port(struct ata_po
 	readl(mmio);	/* flush */
 }
 
-static void pdc_sata_phy_reset(struct ata_port *ap)
-{
-	pdc_reset_port(ap);
-	sata_phy_reset(ap);
-}
-
 static void pdc_pata_cbl_detect(struct ata_port *ap)
 {
 	u8 tmp;
@@ -440,6 +441,63 @@ static void pdc_qc_prep(struct ata_queue
 	}
 }
 
+static void pdc_freeze(struct ata_port *ap)
+{
+	void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr;
+	u32 tmp;
+
+	tmp = readl(mmio + PDC_CTLSTAT);
+	tmp |= PDC_IRQ_DISABLE;
+	tmp &= ~PDC_DMA_ENABLE;
+	writel(tmp, mmio + PDC_CTLSTAT);
+	readl(mmio + PDC_CTLSTAT); /* flush *//* XXX: needed? sata_sil does this */
+}
+
+static void pdc_thaw(struct ata_port *ap)
+{
+	void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr;
+	u32 tmp;
+
+	/* clear IRQ */
+	readl(mmio + PDC_INT_SEQMASK);
+
+	/* turn IRQ back on */
+	tmp = readl(mmio + PDC_CTLSTAT);
+	tmp &= ~PDC_IRQ_DISABLE;
+	writel(tmp, mmio + PDC_CTLSTAT);
+	readl(mmio + PDC_CTLSTAT); /* flush *//* XXX: needed? */
+}
+
+static void pdc_error_handler(struct ata_port *ap)
+{
+	struct ata_eh_context *ehc = &ap->eh_context;
+	ata_reset_fn_t hardreset;
+
+	/* stop DMA, mask IRQ, don't clobber anything else */
+	ata_eh_freeze_port(ap);
+
+	hardreset = NULL;
+	if (sata_scr_valid(ap)) {
+		ehc->i.action |= ATA_EH_HARDRESET;
+		hardreset = sata_std_hardreset;
+	}
+
+	ata_do_eh(ap, ata_std_prereset, ata_std_softreset, hardreset,
+		  ata_std_postreset);
+}
+
+static void pdc_post_internal_cmd(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+
+	if (qc->flags & ATA_QCFLAG_FAILED)
+		qc->err_mask |= AC_ERR_OTHER;
+
+	/* make DMA engine forget about the failed command */
+	if (qc->err_mask)
+		pdc_reset_port(ap);
+}
+
 static void pdc_eng_timeout(struct ata_port *ap)
 {
 	struct ata_host *host = ap->host;

Re: [PATCH 2.6.19 2/3] sata_promise: new EH conversion

[Tejun Heo]

Hello, Mikael.

Thanks for doing this.

Mikael Pettersson wrote:
[--snip--]
> +static void pdc_freeze(struct ata_port *ap)
> +{
> +	void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr;
> +	u32 tmp;
> +
> +	tmp = readl(mmio + PDC_CTLSTAT);
> +	tmp |= PDC_IRQ_DISABLE;
> +	tmp &= ~PDC_DMA_ENABLE;
> +	writel(tmp, mmio + PDC_CTLSTAT);
> +	readl(mmio + PDC_CTLSTAT); /* flush *//* XXX: needed? sata_sil does this */

Just drop the above line.

> +}
> +
> +static void pdc_thaw(struct ata_port *ap)
> +{
> +	void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr;
> +	u32 tmp;
> +
> +	/* clear IRQ */
> +	readl(mmio + PDC_INT_SEQMASK);
> +
> +	/* turn IRQ back on */
> +	tmp = readl(mmio + PDC_CTLSTAT);
> +	tmp &= ~PDC_IRQ_DISABLE;
> +	writel(tmp, mmio + PDC_CTLSTAT);
> +	readl(mmio + PDC_CTLSTAT); /* flush *//* XXX: needed? */

Ditto.

> +}
> +
> +static void pdc_error_handler(struct ata_port *ap)
> +{
> +	struct ata_eh_context *ehc = &ap->eh_context;
> +	ata_reset_fn_t hardreset;
> +
> +	/* stop DMA, mask IRQ, don't clobber anything else */
> +	ata_eh_freeze_port(ap);

Don't freeze port unconditionally.  You'll end up hardresetting on every
error.  Just make sure DMA engine is stopped and the controller is in a
sane state.  If that fails, then, the port should be frozen.

> +	hardreset = NULL;
> +	if (sata_scr_valid(ap)) {
> +		ehc->i.action |= ATA_EH_HARDRESET;

Why always force HARDRESET?

> +		hardreset = sata_std_hardreset;
> +	}

-- 
tejun

Re: [PATCH 2.6.19 2/3] sata_promise: new EH conversion

[Jeff Garzik]

Tejun Heo wrote:
> Hello, Mikael.
> 
> Thanks for doing this.
> 
> Mikael Pettersson wrote:
> [--snip--]
>> +static void pdc_freeze(struct ata_port *ap)
>> +{
>> +	void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr;
>> +	u32 tmp;
>> +
>> +	tmp = readl(mmio + PDC_CTLSTAT);
>> +	tmp |= PDC_IRQ_DISABLE;
>> +	tmp &= ~PDC_DMA_ENABLE;
>> +	writel(tmp, mmio + PDC_CTLSTAT);
>> +	readl(mmio + PDC_CTLSTAT); /* flush *//* XXX: needed? sata_sil does this */
> 
> Just drop the above line.
> 
>> +}
>> +
>> +static void pdc_thaw(struct ata_port *ap)
>> +{
>> +	void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr;
>> +	u32 tmp;
>> +
>> +	/* clear IRQ */
>> +	readl(mmio + PDC_INT_SEQMASK);
>> +
>> +	/* turn IRQ back on */
>> +	tmp = readl(mmio + PDC_CTLSTAT);
>> +	tmp &= ~PDC_IRQ_DISABLE;
>> +	writel(tmp, mmio + PDC_CTLSTAT);
>> +	readl(mmio + PDC_CTLSTAT); /* flush *//* XXX: needed? */
> 
> Ditto.

Why do you think these flushes are not needed?

	Jeff

Re: [PATCH 2.6.19 2/3] sata_promise: new EH conversion

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Hello, Mikael.
>>
>> Thanks for doing this.
>>
>> Mikael Pettersson wrote:
>> [--snip--]
>>> +static void pdc_freeze(struct ata_port *ap)
>>> +{
>>> +    void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr;
>>> +    u32 tmp;
>>> +
>>> +    tmp = readl(mmio + PDC_CTLSTAT);
>>> +    tmp |= PDC_IRQ_DISABLE;
>>> +    tmp &= ~PDC_DMA_ENABLE;
>>> +    writel(tmp, mmio + PDC_CTLSTAT);
>>> +    readl(mmio + PDC_CTLSTAT); /* flush *//* XXX: needed? sata_sil
>>> does this */
>>
>> Just drop the above line.
>>
>>> +}
>>> +
>>> +static void pdc_thaw(struct ata_port *ap)
>>> +{
>>> +    void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr;
>>> +    u32 tmp;
>>> +
>>> +    /* clear IRQ */
>>> +    readl(mmio + PDC_INT_SEQMASK);
>>> +
>>> +    /* turn IRQ back on */
>>> +    tmp = readl(mmio + PDC_CTLSTAT);
>>> +    tmp &= ~PDC_IRQ_DISABLE;
>>> +    writel(tmp, mmio + PDC_CTLSTAT);
>>> +    readl(mmio + PDC_CTLSTAT); /* flush *//* XXX: needed? */
>>
>> Ditto.
> 
> Why do you think these flushes are not needed?

1. for thaw, it doesn't matter.  it's always followed by further IO
operations.

2. for freeze, interrupt delivery is asynchronous to IO anyway and
freeze is also called from outside of interrupt handler.  IRQ handler
must be ready to handle spurious interrupts on a frozen port (Note they
automatically are because they can't access aborted qc's).  As long as
it gets quiesced after finite number of interrupts, it's okay.

3. we don't have them in ahci nor sata_sil24.

But, having those flushes won't hurt either.  What was the conclusion of
mmio <-> spinlock sync discussion?  I always feel kind of uncomfortable
about readl() flushes.  I think they're too subtle.

-- 
tejun

Re: [PATCH 2.6.19 2/3] sata_promise: new EH conversion

[Arjan van de Ven]

> But, having those flushes won't hurt either.  What was the conclusion of
> mmio <-> spinlock sync discussion?  I always feel kind of uncomfortable
> about readl() flushes.  I think they're too subtle.

those are orthogonal!
The posting flushes have nothing to do with spinlock-vs-mmio; that
discussion was about the CPU, while posting flushes are about the
chipset / bridges / etc....

> 
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

Re: [PATCH 2.6.19 2/3] sata_promise: new EH conversion

[Tejun Heo]

Arjan van de Ven wrote:
>> But, having those flushes won't hurt either.  What was the conclusion of
>> mmio <-> spinlock sync discussion?  I always feel kind of uncomfortable
>> about readl() flushes.  I think they're too subtle.
> 
> those are orthogonal!
> The posting flushes have nothing to do with spinlock-vs-mmio; that
> discussion was about the CPU, while posting flushes are about the
> chipset / bridges / etc....

The problem is that it's not clear what those posting flushes actually
achieve.  Do they achieve IRQ disabling on completion?  Hardly, IRQ can
use whole different channel anyway.

And, as for spinlock/IO ordering, libata currently depends on IO
instructions not leaking outside of spinlock (ordering-wise).  We have
posting flushes at several places and some of them clearly make sense
(e.g. timed wait) but some others aren't that clear while majority of
places just don't do anything about ordering other than wrapping them
inside spinlock.

So, I don't really know.  Do we have to add mmiowb() before every
spin_unlock after IO?  Or, do we have to do explicit flushes everytime?
 Or, is it something to be taken care of in the upper layer?

-- 
tejun

Re: [PATCH 2.6.19 2/3] sata_promise: new EH conversion

[Mikael Pettersson]

On Sun, 03 Dec 2006 22:19:35 +0900, Tejun Heo wrote:
>Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> Hello, Mikael.
>>>
>>> Thanks for doing this.
>>>
>>> Mikael Pettersson wrote:
>>> [--snip--]
>>>> +static void pdc_freeze(struct ata_port *ap)
>>>> +{
>>>> +    void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr;
>>>> +    u32 tmp;
>>>> +
>>>> +    tmp = readl(mmio + PDC_CTLSTAT);
>>>> +    tmp |= PDC_IRQ_DISABLE;
>>>> +    tmp &= ~PDC_DMA_ENABLE;
>>>> +    writel(tmp, mmio + PDC_CTLSTAT);
>>>> +    readl(mmio + PDC_CTLSTAT); /* flush *//* XXX: needed? sata_sil
>>>> does this */
>>>
>>> Just drop the above line.
>>>
>>>> +}
>>>> +
>>>> +static void pdc_thaw(struct ata_port *ap)
>>>> +{
>>>> +    void __iomem *mmio = (void __iomem *) ap->ioaddr.cmd_addr;
>>>> +    u32 tmp;
>>>> +
>>>> +    /* clear IRQ */
>>>> +    readl(mmio + PDC_INT_SEQMASK);
>>>> +
>>>> +    /* turn IRQ back on */
>>>> +    tmp = readl(mmio + PDC_CTLSTAT);
>>>> +    tmp &= ~PDC_IRQ_DISABLE;
>>>> +    writel(tmp, mmio + PDC_CTLSTAT);
>>>> +    readl(mmio + PDC_CTLSTAT); /* flush *//* XXX: needed? */
>>>
>>> Ditto.
>> 
>> Why do you think these flushes are not needed?
>
>1. for thaw, it doesn't matter.  it's always followed by further IO
>operations.
>
>2. for freeze, interrupt delivery is asynchronous to IO anyway and
>freeze is also called from outside of interrupt handler.  IRQ handler
>must be ready to handle spurious interrupts on a frozen port (Note they
>automatically are because they can't access aborted qc's).  As long as
>it gets quiesced after finite number of interrupts, it's okay.
>
>3. we don't have them in ahci nor sata_sil24.

But you do in sata_sil.c:sil_freeze().

>But, having those flushes won't hurt either.

libata doesn't specify in its documentation that these driver
operations may have delayed behaviour, so I have to assume that
side-effects are to be completed when the operations return.
In fact, the documentation for __ata_port_freeze explicitly
requires the port to not perform any operations until thawed.
If I didn't flush, the port could do just that.

Since the flushes clearly are safe I'd prefer to keep them, but
of course I'll remove them if you or Jeff can guarantee that not
flushing the PCI writes is OK.

/Mikael

Re: [PATCH 2.6.19 2/3] sata_promise: new EH conversion

[Tejun Heo]

Mikael Pettersson wrote:
>> 3. we don't have them in ahci nor sata_sil24.
> 
> But you do in sata_sil.c:sil_freeze().

Two versus one.  It's democracy.  :-)

>> But, having those flushes won't hurt either.
> 
> libata doesn't specify in its documentation that these driver
> operations may have delayed behaviour, so I have to assume that
> side-effects are to be completed when the operations return.
> In fact, the documentation for __ata_port_freeze explicitly
> requires the port to not perform any operations until thawed.
> If I didn't flush, the port could do just that.

I can't fully understand the above paragraph.  Please elaborate.

> Since the flushes clearly are safe I'd prefer to keep them, but
> of course I'll remove them if you or Jeff can guarantee that not
> flushing the PCI writes is OK.

I don't really know.  AFAICT, things should work without those flushes.
 ATA devices are often crappy and drivers are built to deal with those
kinds of spurious interrupts while frozen.  And, yes, having it never
hurts.  I'm just not quite sure what they guarantee.  IRQ signals are
asynchronous to IOs, so flushing IO doesn't guarantee immediate IRQ
quiescence.

If you leave those flushes, that makes it two versus two.  Universe will
be in balance.  :-)

-- 
tejun

Re: [PATCH 2.6.19 2/3] sata_promise: new EH conversion

[Mikael Pettersson]

On Sun, 03 Dec 2006 22:00:42 +0900, Tejun Heo wrote:
>Mikael Pettersson wrote:
>> +}
>> +
>> +static void pdc_error_handler(struct ata_port *ap)
>> +{
>> +	struct ata_eh_context *ehc = &ap->eh_context;
>> +	ata_reset_fn_t hardreset;
>> +
>> +	/* stop DMA, mask IRQ, don't clobber anything else */
>> +	ata_eh_freeze_port(ap);
>
>Don't freeze port unconditionally.  You'll end up hardresetting on every
>error.  Just make sure DMA engine is stopped and the controller is in a
>sane state.  If that fails, then, the port should be frozen.

I'm looking into this now, but so far it seems only a reset
(what Promise calls software reset, I don't know if libata
considers it a soft or hard reset) of the ATA channel will do.

>> +	hardreset = NULL;
>> +	if (sata_scr_valid(ap)) {
>> +		ehc->i.action |= ATA_EH_HARDRESET;
>
>Why always force HARDRESET?

I based that on sata_sil24:

	if (sil24_init_port(ap)) {
		ata_eh_freeze_port(ap);
		ehc->i.action |= ATA_EH_HARDRESET;
	}

I interpreted the ATA_EH_HARDRESET as being required due to
the ata_eh_freeze_port(), but perhaps it's only there because
sil24_init_port() returned failure?

A different issue, but of practical importance, is which
libata branch I should base the EH conversion on: #upstream
or #ALL? Andrew Morton's -mm kernels include the ALL patches,
but they in turn include the promise-sata-pata patches, and
there is a conflict between the PATA patch and the EH conversion.
Currently my EH conversion is based on #upstream, and I've ported
the PATA patch to apply on top of it.

/Mikael

Re: [PATCH 2.6.19 2/3] sata_promise: new EH conversion

[Jeff Garzik]

Mikael Pettersson wrote:
> On Sun, 03 Dec 2006 22:00:42 +0900, Tejun Heo wrote:
>> Mikael Pettersson wrote:
>>> +}
>>> +
>>> +static void pdc_error_handler(struct ata_port *ap)
>>> +{
>>> +	struct ata_eh_context *ehc = &ap->eh_context;
>>> +	ata_reset_fn_t hardreset;
>>> +
>>> +	/* stop DMA, mask IRQ, don't clobber anything else */
>>> +	ata_eh_freeze_port(ap);
>> Don't freeze port unconditionally.  You'll end up hardresetting on every
>> error.  Just make sure DMA engine is stopped and the controller is in a
>> sane state.  If that fails, then, the port should be frozen.
> 
> I'm looking into this now, but so far it seems only a reset
> (what Promise calls software reset, I don't know if libata
> considers it a soft or hard reset) of the ATA channel will do.
> 
>>> +	hardreset = NULL;
>>> +	if (sata_scr_valid(ap)) {
>>> +		ehc->i.action |= ATA_EH_HARDRESET;
>> Why always force HARDRESET?
> 
> I based that on sata_sil24:
> 
> 	if (sil24_init_port(ap)) {
> 		ata_eh_freeze_port(ap);
> 		ehc->i.action |= ATA_EH_HARDRESET;
> 	}
> 
> I interpreted the ATA_EH_HARDRESET as being required due to
> the ata_eh_freeze_port(), but perhaps it's only there because
> sil24_init_port() returned failure?
> 
> A different issue, but of practical importance, is which
> libata branch I should base the EH conversion on: #upstream
> or #ALL? Andrew Morton's -mm kernels include the ALL patches,
> but they in turn include the promise-sata-pata patches, and
> there is a conflict between the PATA patch and the EH conversion.
> Currently my EH conversion is based on #upstream, and I've ported
> the PATA patch to apply on top of it.

It's a tiered system ;-)

* if at all possible, provide patches against the latest linux-2.6.git

* if there are dependencies in #upstream-fixes or #upstream (i.e. I 
already applied some of your patches), provide patches against 
#upstream-fixes or #upstream

#ALL is a branch that is blown away at will, and is really more of a 
testing and akpm sync point.  Don't worry about conflicts with 
promise-sata-pata, I take care of those when I merge the #ALL branch 
together.

	Jeff

Re: [PATCH 2.6.19 2/3] sata_promise: new EH conversion

[Tejun Heo]

Mikael Pettersson wrote:
>> Don't freeze port unconditionally.  You'll end up hardresetting on every
>> error.  Just make sure DMA engine is stopped and the controller is in a
>> sane state.  If that fails, then, the port should be frozen.

Sorry, s/hardresetting/resetting/

> I'm looking into this now, but so far it seems only a reset
> (what Promise calls software reset, I don't know if libata
> considers it a soft or hard reset) of the ATA channel will do.

Errors properly reported by the device shouldn't cause resets.  Think
about ATAPI check condition.

>>> +	hardreset = NULL;
>>> +	if (sata_scr_valid(ap)) {
>>> +		ehc->i.action |= ATA_EH_HARDRESET;
>> Why always force HARDRESET?
> 
> I based that on sata_sil24:
> 
> 	if (sil24_init_port(ap)) {
> 		ata_eh_freeze_port(ap);
> 		ehc->i.action |= ATA_EH_HARDRESET;
> 	}
> 
> I interpreted the ATA_EH_HARDRESET as being required due to
> the ata_eh_freeze_port(), but perhaps it's only there because
> sil24_init_port() returned failure?

Yeap, that's right.

> A different issue, but of practical importance, is which
> libata branch I should base the EH conversion on: #upstream
> or #ALL? Andrew Morton's -mm kernels include the ALL patches,
> but they in turn include the promise-sata-pata patches, and
> there is a conflict between the PATA patch and the EH conversion.
> Currently my EH conversion is based on #upstream, and I've ported
> the PATA patch to apply on top of it.

#upstream, It is.  #ALL is merge of all libata-dev devel branches and no
development work occurs there directly.

Thanks.

-- 
tejun