sata_mv: trial fix for lost NCQ interrupts

This patch is for trial/critique use only at the moment. Once I hear back from a few people who actually use it, I'll post an updated fix for upstream/backstream inclusion.

I spent this afternoon nitpicking and bitpicking through the interrupt code in sata_mv.c, and I believe I found a race on the hc_irq_cause register. The code was "helpfully" attempting to use read-modify-write to clear individual port bits there, but this is impossible to do in a race-free fashion.

So.. the obvious fix is to just write the bits being cleared, without touching anything else. This will also be faster, too, since no read is required or desired. I really don't see a downside, as long as it actually works for everyone.

It does work for me here.

I still see timeouts only on 4 lower ports where disk has also been connected to port +4. i.e. if port 7 is empty, port 3 will work just fine. I just would like to see some confirmation that others are seeing this same kind of behavior.

Quoted 1 line · Harri Olin

I still see timeouts only on 4 lower ports where disk has also been connected to port +4. i.e. if port 7 is empty, port 3 will work just fine. I just would like to see some confirmation that others are seeing this same kind of behavior.

hmmmm, that is quite interesting. That seems to indicate something related to the dual-HC mechanism (8-port cards are really two 4-port chips internally)...

I tried the patch and it works as well as it did without the patch. That is to say, timeouts still happen, every couple of minutes on moderate

Harri Olin wrote:
..

Quoted 1 line

I tried the patch and it works as well as it did without the patch. That is to say, timeouts still happen, every couple of minutes on moderate

..

Yup. On reflection, that patch has no effect, other than speeding things up.

However, I think I *have* found the actual bug this time. Try this patch and report back again.

--- linux-2.6.28/drivers/ata/sata_mv.c 2009-01-13
15:57:11.000000000 -0500
+++ linux/drivers/ata/sata_mv.c 2009-01-14 16:56:38.000000000 -0500

Quoted 14 lines · Mark Lord

Harri Olin wrote:
..

Quoted 1 line

I tried the patch and it works as well as it did without the patch. That is to say, timeouts still happen, every couple of minutes on moderate

..

Yup. On reflection, that patch has no effect, other than speeding things up.

However, I think I *have* found the actual bug this time. Try this patch and report back again.

--- linux-2.6.28/drivers/ata/sata_mv.c 2009-01-13
15:57:11.000000000 -0500
+++ linux/drivers/ata/sata_mv.c 2009-01-14 16:56:38.000000000 -0500

Review line 889

Quoted 8 lines

(quoted diff) +1 -1

@@ -883,7 +883,7 @@
        struct mv_host_priv *hpriv = ap->host->private_data;
        int hardport = mv_hardport_from_port(ap->port_no);
        void __iomem *hc_mmio = mv_hc_base_from_port(
-                    mv_host_base(ap->host), hardport);
+                    mv_host_base(ap->host), ap->port_no);
        u32 hc_irq_cause, ipending;
 
        /* clear EDMA event indicators, if any */

After first 55 minutes of testing, no timeouts yet, so I'm quite sure it works ok now (yay!). Without this patch, timeouts happened every couple minutes on similar load. I think I'll let it run for a day and report back tomorrow how things are by then.

Note that I didn't apply the previous patch as they don't apply on each other. If needed, I can test it separately after running this for a while.

Oh, and thanks for the hard work :)

Signature · 1 line

Harri. 

..
After first 55 minutes of testing, no timeouts yet, so I'm quite sure it works ok now (yay!). Without this patch, timeouts happened every couple minutes on similar load. I think I'll let it run for a day and report back tomorrow how things are by then.

Note that I didn't apply the previous patch as they don't apply on each other. If needed, I can test it separately after running this for a while.

Quoted 2 lines · Harri Olin

..
After first 55 minutes of testing, no timeouts yet, so I'm quite sure it works ok now (yay!). Without this patch, timeouts happened every couple minutes on similar load. I think I'll let it run for a day and report back tomorrow how things are by then.

..

Okay, we have a winner!
I just need to get the "okay" from Marvell,
and then I'll feed this patch for 2.6.29 and earlier -stable streams.

Quoted 1 line

Note that I didn't apply the previous patch as they don't apply on each other. If needed, I can test it separately after running this for a while.

..

No need. The earlier patch has no effect, other than speeding up your system by a slight amount. I'll submit that for upstream separately, along with a fix for another "duh!" I noticed in the code.

In the meanwhile, you should keep the working patch (obviously) in your kernels -- the same/similar patch will apply to earlier kernels if needed.

Thanks to you and Brian for the legwork!

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Fix a longstanding bug for the 8-port Marvell Sata controllers (508x/6081), where accesses to the upper 4 ports would cause lost-interrupts / timeouts for the lower 4-ports. With this patch, the 6081 boards should finally be reliable enough for mainstream use with Linux.

This patch is for 2.6.29, but should also get reviewed/released for the -stable branches of 2.6.28, 2.6.27, and 2.6.26.

Signed-off-by Mark Lord <[email protected]>

Quoted 10 lines · Mark Lord

Fix a longstanding bug for the 8-port Marvell Sata controllers (508x/6081), where accesses to the upper 4 ports would cause lost-interrupts / timeouts for the lower 4-ports. With this patch, the 6081 boards should finally be reliable enough for mainstream use with Linux.

This patch is for 2.6.29, but should also get reviewed/released for the -stable branches of 2.6.28, 2.6.27, and 2.6.26.

Signed-off-by Mark Lord <[email protected]>

After patching no timeouts have occurred for 22 hours on 2.6.28 and 24 hours on 2.6.27.6.

Tested-by Harri Olin <[email protected]>

Signature · 1 line

Harri. 

This patch is for trial/critique use only at the moment. Once I hear back from a few people who actually use it, I'll post an updated fix for upstream/backstream inclusion.

I spent this afternoon nitpicking and bitpicking through the interrupt code in sata_mv.c, and I believe I found a race on the hc_irq_cause register. The code was "helpfully" attempting to use read-modify-write to clear individual port bits there, but this is impossible to do in a race-free fashion.

So.. the obvious fix is to just write the bits being cleared, without touching anything else. This will also be faster, too, since no read is required or desired. I really don't see a downside, as long as it actually works for everyone.

It does work for me here.

drivers/ata/sata_mv.c 2009-01-13 16:03:04.000000000 -0500 renamed from drivers/ata/sata_mv.c 2009-01-13 15:57:11.000000000 -0500 +7 -12

@@ -884,18 +884,14 @@
 int hardport = mv_hardport_from_port(ap->port_no);
 void __iomem *hc_mmio = mv_hc_base_from_port(
 mv_host_base(ap->host), hardport);
- u32 hc_irq_cause, ipending;
+ u32 hc_irq_cause;
 
 /* clear EDMA event indicators, if any */
 writelfl(0, port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);
 
- /* clear EDMA interrupt indicator, if any */
- hc_irq_cause = readl(hc_mmio + HC_IRQ_CAUSE_OFS);
- ipending = (DEV_IRQ | DMA_IRQ) << hardport;
- if (hc_irq_cause & ipending) {
- writelfl(hc_irq_cause & ~ipending,
- hc_mmio + HC_IRQ_CAUSE_OFS);
- }
+ /* clear EDMA interrupt indicators */
+ hc_irq_cause = (DEV_IRQ | DMA_IRQ) << hardport;
+ writelfl(~hc_irq_cause, hc_mmio + HC_IRQ_CAUSE_OFS);
 
 mv_edma_cfg(ap, want_ncq);
 

@@ -2821,10 +2817,9 @@
 /* clear EDMA errors on this port */
 writel(0, port_mmio + EDMA_ERR_IRQ_CAUSE_OFS);
 
- /* clear pending irq events */
- hc_irq_cause = readl(hc_mmio + HC_IRQ_CAUSE_OFS);
- hc_irq_cause &= ~((DEV_IRQ | DMA_IRQ) << hardport);
- writelfl(hc_irq_cause, hc_mmio + HC_IRQ_CAUSE_OFS);
+ /* clear EDMA interrupt indicators */
+ hc_irq_cause = (DEV_IRQ | DMA_IRQ) << hardport;
+ writelfl(~hc_irq_cause, hc_mmio + HC_IRQ_CAUSE_OFS);
 
 mv_enable_port_irqs(ap, ERR_IRQ);
 }