[PATCH 2.6.26-rc2 1/3] sata_promise: fix irq clearing buglets

[PATCH 2.6.26-rc2 1/3] sata_promise: fix irq clearing buglets

[Mikael Pettersson]

This patch fixes two bugs in sata_promise's irq status clearing paths:
1. When clearing the irq status for a specific port, the driver
   read the global SEQMASK register. This is wrong because that
   clears the irq status for _all_ ports.
2. pdc_thaw() incorrectly added the PDC_INT_SEQMASK host register
   offset to a per-port ata engine base address. This resulted in
   it reading the unrelated PDC_PKT_SUBMIT register, which did not
   have the desired irq status clearing effect.

In both cases the fix is to read from the port's Command/Status
register. This also matches what Promise's own driver does.

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

diff -rupN linux-2.6.26-rc2/drivers/ata/sata_promise.c linux-2.6.26-rc2.sata_promise-irqclear/drivers/ata/sata_promise.c
--- linux-2.6.26-rc2/drivers/ata/sata_promise.c	2008-05-16 12:44:28.000000000 +0200
+++ linux-2.6.26-rc2.sata_promise-irqclear/drivers/ata/sata_promise.c	2008-05-17 15:02:01.000000000 +0200
@@ -663,7 +663,7 @@ static void pdc_thaw(struct ata_port *ap
 	u32 tmp;
 
 	/* clear IRQ */
-	readl(mmio + PDC_INT_SEQMASK);
+	readl(mmio + PDC_COMMAND);
 
 	/* turn IRQ back on */
 	tmp = readl(mmio + PDC_CTLSTAT);
@@ -781,10 +781,9 @@ static inline unsigned int pdc_host_intr
 
 static void pdc_irq_clear(struct ata_port *ap)
 {
-	struct ata_host *host = ap->host;
-	void __iomem *mmio = host->iomap[PDC_MMIO_BAR];
+	void __iomem *mmio = ap->ioaddr.cmd_addr;
 
-	readl(mmio + PDC_INT_SEQMASK);
+	readl(mmio + PDC_COMMAND);
 }
 
 static irqreturn_t pdc_interrupt(int irq, void *dev_instance)

Re: [PATCH 2.6.26-rc2 1/3] sata_promise: fix irq clearing buglets

[Jeff Garzik]

Mikael Pettersson wrote:
> This patch fixes two bugs in sata_promise's irq status clearing paths:
> 1. When clearing the irq status for a specific port, the driver
>    read the global SEQMASK register. This is wrong because that
>    clears the irq status for _all_ ports.
> 2. pdc_thaw() incorrectly added the PDC_INT_SEQMASK host register
>    offset to a per-port ata engine base address. This resulted in
>    it reading the unrelated PDC_PKT_SUBMIT register, which did not
>    have the desired irq status clearing effect.
> 
> In both cases the fix is to read from the port's Command/Status
> register. This also matches what Promise's own driver does.
> 
> Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
> ---
>  drivers/ata/sata_promise.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

applied 1-3

Re: [PATCH 2.6.26-rc2 1/3] sata_promise: fix irq clearing buglets

[Jeff Garzik]

Mikael Pettersson wrote:
> This patch fixes two bugs in sata_promise's irq status clearing paths:
> 1. When clearing the irq status for a specific port, the driver
>    read the global SEQMASK register. This is wrong because that
>    clears the irq status for _all_ ports.
> 2. pdc_thaw() incorrectly added the PDC_INT_SEQMASK host register
>    offset to a per-port ata engine base address. This resulted in
>    it reading the unrelated PDC_PKT_SUBMIT register, which did not
>    have the desired irq status clearing effect.
> 
> In both cases the fix is to read from the port's Command/Status
> register. This also matches what Promise's own driver does.
> 
> Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
> ---
>  drivers/ata/sata_promise.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

is sata_sx4 similarly affected?

Re: [PATCH 2.6.26-rc2 1/3] sata_promise: fix irq clearing buglets

[Mikael Pettersson]

Jeff Garzik writes:
 > Mikael Pettersson wrote:
 > > This patch fixes two bugs in sata_promise's irq status clearing paths:
 > > 1. When clearing the irq status for a specific port, the driver
 > >    read the global SEQMASK register. This is wrong because that
 > >    clears the irq status for _all_ ports.
 > > 2. pdc_thaw() incorrectly added the PDC_INT_SEQMASK host register
 > >    offset to a per-port ata engine base address. This resulted in
 > >    it reading the unrelated PDC_PKT_SUBMIT register, which did not
 > >    have the desired irq status clearing effect.
 > > 
 > > In both cases the fix is to read from the port's Command/Status
 > > register. This also matches what Promise's own driver does.
 > > 
 > > Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
 > > ---
 > >  drivers/ata/sata_promise.c |    7 +++----
 > >  1 file changed, 3 insertions(+), 4 deletions(-)
 > 
 > is sata_sx4 similarly affected?

For issue 2, pdc_thaw(), the answer is No since that's
a sata_promise only thing I added as part of new-EH.

For ->irq_clear() I'll have to check. I'll do that
this evening and let you know tomorrow.

/Mikael

Re: [PATCH 2.6.26-rc2 1/3] sata_promise: fix irq clearing buglets

[Jeff Garzik]

Mikael Pettersson wrote:
> Jeff Garzik writes:
>  > Mikael Pettersson wrote:
>  > > This patch fixes two bugs in sata_promise's irq status clearing paths:
>  > > 1. When clearing the irq status for a specific port, the driver
>  > >    read the global SEQMASK register. This is wrong because that
>  > >    clears the irq status for _all_ ports.
>  > > 2. pdc_thaw() incorrectly added the PDC_INT_SEQMASK host register
>  > >    offset to a per-port ata engine base address. This resulted in
>  > >    it reading the unrelated PDC_PKT_SUBMIT register, which did not
>  > >    have the desired irq status clearing effect.
>  > > 
>  > > In both cases the fix is to read from the port's Command/Status
>  > > register. This also matches what Promise's own driver does.
>  > > 
>  > > Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
>  > > ---
>  > >  drivers/ata/sata_promise.c |    7 +++----
>  > >  1 file changed, 3 insertions(+), 4 deletions(-)
>  > 
>  > is sata_sx4 similarly affected?
> 
> For issue 2, pdc_thaw(), the answer is No since that's
> a sata_promise only thing I added as part of new-EH.

probably relevant to libata-dev.git#new-eh which contains the sata_sx4 
new-eh conversion.


> For ->irq_clear() I'll have to check. I'll do that
> this evening and let you know tomorrow.


Thanks,

	Jeff

Re: [PATCH 2.6.26-rc2 1/3] sata_promise: fix irq clearing buglets

[Mikael Pettersson]

Jeff Garzik writes:
 > Mikael Pettersson wrote:
 > > Jeff Garzik writes:
 > >  > Mikael Pettersson wrote:
 > >  > > This patch fixes two bugs in sata_promise's irq status clearing paths:
 > >  > > 1. When clearing the irq status for a specific port, the driver
 > >  > >    read the global SEQMASK register. This is wrong because that
 > >  > >    clears the irq status for _all_ ports.
 > >  > > 2. pdc_thaw() incorrectly added the PDC_INT_SEQMASK host register
 > >  > >    offset to a per-port ata engine base address. This resulted in
 > >  > >    it reading the unrelated PDC_PKT_SUBMIT register, which did not
 > >  > >    have the desired irq status clearing effect.
 > >  > > 
 > >  > > In both cases the fix is to read from the port's Command/Status
 > >  > > register. This also matches what Promise's own driver does.
 > >  > > 
 > >  > > Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
 > >  > > ---
 > >  > >  drivers/ata/sata_promise.c |    7 +++----
 > >  > >  1 file changed, 3 insertions(+), 4 deletions(-)
 > >  > 
 > >  > is sata_sx4 similarly affected?
 > > 
 > > For issue 2, pdc_thaw(), the answer is No since that's
 > > a sata_promise only thing I added as part of new-EH.
 > 
 > probably relevant to libata-dev.git#new-eh which contains the sata_sx4 
 > new-eh conversion.

I forgot about that branch. I'll check it out later today.

 > 
 > > For ->irq_clear() I'll have to check. I'll do that
 > > this evening and let you know tomorrow.
 > 
 > 
 > Thanks,

Based on the pdc20261 programming guide (SX4 has SEQMASK and
ATA Control/Status just like the TX chips), and that sata_sx4
calls ata_wait_idle() to "get drive status, clear intr", it
appears that sata_sx4's ->clear_irq() needs to be fixed to
read the port's ATA Control/Status not the hosts' SEQMASK.
Otherwise ->clear_irq(ap) will affect _all_ ports.

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

--- linux-2.6.26-rc3/drivers/ata/sata_sx4.c.~1~	2008-05-19 20:36:50.000000000 +0200
+++ linux-2.6.26-rc3/drivers/ata/sata_sx4.c	2008-05-21 10:00:23.000000000 +0200
@@ -786,12 +786,7 @@ static inline unsigned int pdc20621_host
 
 static void pdc20621_irq_clear(struct ata_port *ap)
 {
-	struct ata_host *host = ap->host;
-	void __iomem *mmio = host->iomap[PDC_MMIO_BAR];
-
-	mmio += PDC_CHIP0_OFS;
-
-	readl(mmio + PDC_20621_SEQMASK);
+	ioread8(ap->ioaddr.status_addr);
 }
 
 static irqreturn_t pdc20621_interrupt(int irq, void *dev_instance)

Re: [PATCH 2.6.26-rc2 1/3] sata_promise: fix irq clearing buglets

[Mikael Pettersson]

Jeff Garzik writes:
 > Mikael Pettersson wrote:
 > > Jeff Garzik writes:
 > >  > Mikael Pettersson wrote:
 > >  > > This patch fixes two bugs in sata_promise's irq status clearing paths:
 > >  > > 1. When clearing the irq status for a specific port, the driver
 > >  > >    read the global SEQMASK register. This is wrong because that
 > >  > >    clears the irq status for _all_ ports.
 > >  > > 2. pdc_thaw() incorrectly added the PDC_INT_SEQMASK host register
 > >  > >    offset to a per-port ata engine base address. This resulted in
 > >  > >    it reading the unrelated PDC_PKT_SUBMIT register, which did not
 > >  > >    have the desired irq status clearing effect.
 > >  > > 
 > >  > > In both cases the fix is to read from the port's Command/Status
 > >  > > register. This also matches what Promise's own driver does.
 > >  > > 
 > >  > > Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
 > >  > > ---
 > >  > >  drivers/ata/sata_promise.c |    7 +++----
 > >  > >  1 file changed, 3 insertions(+), 4 deletions(-)
 > >  > 
 > >  > is sata_sx4 similarly affected?
 > > 
 > > For issue 2, pdc_thaw(), the answer is No since that's
 > > a sata_promise only thing I added as part of new-EH.
 > 
 > probably relevant to libata-dev.git#new-eh which contains the sata_sx4 
 > new-eh conversion.

pdc_thaw() in the libata#new-eh version of sata_sx4.c wants to
clear a single port's IRQ status, but reads the host's SEQMASK
register which affects all ports.

Fix by reading the port's Command/Status register instead.

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

--- linux-2.6.26-rc3.libata#new-eh/drivers/ata/sata_sx4.c.~1~	2008-05-21 10:38:01.000000000 +0200
+++ linux-2.6.26-rc3.libata#new-eh/drivers/ata/sata_sx4.c	2008-05-22 09:54:07.000000000 +0200
@@ -872,14 +872,12 @@ static void pdc_freeze(struct ata_port *
 static void pdc_thaw(struct ata_port *ap)
 {
 	void __iomem *mmio = ap->ioaddr.cmd_addr;
-	void __iomem *mmio_base;
 	u32 tmp;
 
 	/* FIXME: this should handle HDMA copy engine thawing */
 
-	/* reading SEQ mask register clears IRQ */
-	mmio_base = ap->host->iomap[PDC_MMIO_BAR] + PDC_CHIP0_OFS;
-	readl(mmio_base + PDC_20621_SEQMASK);
+	/* clear IRQ */
+	ioread8(ap->ioaddr.status_addr);
 
 	/* turn IRQ back on */
 	tmp = readl(mmio + PDC_CTLSTAT);