Re: [PATCH libata#upstream] sata_promise: fix error decode regression

Re: [PATCH libata#upstream] sata_promise: fix error decode regression

[Mikael Pettersson]

On Mon, 09 Apr 2007 16:52:53 +0900, Tejun Heo wrote:
>Mikael Pettersson wrote:
>> Promise ATA ports should always be reset by pdc_reset_port()
>> when errors are detected, but the recent error reason decoding
>> update to sata_promise replaced that reset with a freeze.
>> 
>> This patch changes the error detection to do a reset again.
>> This makes the error decoding update safer, as it now only
>> adds error decoding without changing any other behaviour.
>> 
>> Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
>
>Not necessarily NAK'ing but I think it's better to do things like that
>in EH thread not in the interrupt handler.  Isn't freezing enough in the
>interrupt handler?

You're right that the reset should be in the EH code.
But it isn't right now (the resets done there are generic
ones, not the Promise-specific one the HW really wants),
so the error decoding change caused a regression that
needs to be fixed.

I intend to change the interrupt handler to just freeze and
add a Promise-specific reset to EH in a separate patch.

/Mikael

Re: [PATCH libata#upstream] sata_promise: fix error decode regression

[Tejun Heo]

Mikael Pettersson wrote:
> On Mon, 09 Apr 2007 16:52:53 +0900, Tejun Heo wrote:
>> Mikael Pettersson wrote:
>>> Promise ATA ports should always be reset by pdc_reset_port()
>>> when errors are detected, but the recent error reason decoding
>>> update to sata_promise replaced that reset with a freeze.
>>>
>>> This patch changes the error detection to do a reset again.
>>> This makes the error decoding update safer, as it now only
>>> adds error decoding without changing any other behaviour.
>>>
>>> Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
>> Not necessarily NAK'ing but I think it's better to do things like that
>> in EH thread not in the interrupt handler.  Isn't freezing enough in the
>> interrupt handler?
> 
> You're right that the reset should be in the EH code.
> But it isn't right now (the resets done there are generic
> ones, not the Promise-specific one the HW really wants),
> so the error decoding change caused a regression that
> needs to be fixed.
> 
> I intend to change the interrupt handler to just freeze and
> add a Promise-specific reset to EH in a separate patch.

Great, then.  No objection from me.

-- 
tejun

[PATCH libata#upstream] sata_promise: fix error decode regression

[Mikael Pettersson]

Promise ATA ports should always be reset by pdc_reset_port()
when errors are detected, but the recent error reason decoding
update to sata_promise replaced that reset with a freeze.

This patch changes the error detection to do a reset again.
This makes the error decoding update safer, as it now only
adds error decoding without changing any other behaviour.

Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
---
 drivers/ata/sata_promise.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

--- linux-2.6.21-rc6+upstream/drivers/ata/sata_promise.c.~1~	2007-04-06 19:40:01.000000000 +0200
+++ linux-2.6.21-rc6+upstream/drivers/ata/sata_promise.c	2007-04-06 19:45:33.000000000 +0200
@@ -45,7 +45,7 @@
 #include "sata_promise.h"
 
 #define DRV_NAME	"sata_promise"
-#define DRV_VERSION	"2.04"
+#define DRV_VERSION	"2.05"
 
 
 enum {
@@ -650,9 +650,12 @@ static void pdc_error_intr(struct ata_po
 			   | PDC_PCI_SYS_ERR | PDC1_PCI_PARITY_ERR))
 		ac_err_mask |= AC_ERR_HOST_BUS;
 
-	ehi->action |= ATA_EH_SOFTRESET;
+	if (sata_scr_valid(ap))
+		ehi->serror |= pdc_sata_scr_read(ap, SCR_ERROR);
+
 	qc->err_mask |= ac_err_mask;
-	ata_port_freeze(ap);
+
+	pdc_reset_port(ap);
 }
 
 static inline unsigned int pdc_host_intr( struct ata_port *ap,

Re: [PATCH libata#upstream] sata_promise: fix error decode regression

[Tejun Heo]

Mikael Pettersson wrote:
> Promise ATA ports should always be reset by pdc_reset_port()
> when errors are detected, but the recent error reason decoding
> update to sata_promise replaced that reset with a freeze.
> 
> This patch changes the error detection to do a reset again.
> This makes the error decoding update safer, as it now only
> adds error decoding without changing any other behaviour.
> 
> Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>

Not necessarily NAK'ing but I think it's better to do things like that
in EH thread not in the interrupt handler.  Isn't freezing enough in the
interrupt handler?

Thanks.

-- 
tejun

Re: [PATCH libata#upstream] sata_promise: fix error decode regression

[Jeff Garzik]

Mikael Pettersson wrote:
> Promise ATA ports should always be reset by pdc_reset_port()
> when errors are detected, but the recent error reason decoding
> update to sata_promise replaced that reset with a freeze.
> 
> This patch changes the error detection to do a reset again.
> This makes the error decoding update safer, as it now only
> adds error decoding without changing any other behaviour.
> 
> Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
> ---
>  drivers/ata/sata_promise.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)

applied