sata_mv: trial fix for lost NCQ interrupts

sata_mv: trial fix for lost NCQ interrupts

[Mark Lord]

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.

--- 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-13 16:03:04.000000000 -0500
@@ -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);
 }

Re: sata_mv: trial fix for lost NCQ interrupts

[Harri Olin]

Mark Lord wrote:
> 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 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 
read/write load. Patched driver has not seen much use here as the system 
is in production use and it becomes quickly frustrating to wait for 
something to happen.

I also tried removing ATA_PROT_NODATA from libata-sff.c, as mentioned 
earlier, but after that all disks timeout constantly. It also dropped 
all sata_mv connected disks from md array immediately on boot.

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.

Hardware: Supermicro aoc-sat2-mv8 controller (6081 chip) and WD7500AYYS 
drives.

-- 
Harri.

Re: sata_mv: trial fix for lost NCQ interrupts

[Jeff Garzik]

Harri Olin wrote:
> 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)...

	Jeff

Re: sata_mv: trial fix for lost NCQ interrupts

[Brian Rademacher]

No luck with the patch.  Error occurred right on boot:

ata1.00: exception Emask 0x0 SAct 0x1 SErr 0x0 action 0x6 frozen
ata1.00: cmd 61/08:00:cb:d5:42/00:00:25:00:00/40 tag 0 ncq 4096 out
         res 40/00:00:00:00:00/00:00:00:00:00/00 Emask 0x4 (timeout)
ata1.00: status: { DRDY }
ata1: hard resetting link
ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
ata1.00: max_sectors limited to 256 for NCQ
ata1.00: max_sectors limited to 256 for NCQ
ata1.00: configured for UDMA/133
ata1: EH complete


----- Original Message ----- 
From: "Mark Lord" <liml@rtr.ca>
To: "IDE/ATA development list" <linux-ide@vger.kernel.org>; "Brian 
Rademacher" <rad@radfiles.net>; "Eamonn Hamilton" <EAMONN.HAMILTON@saic.com>
Sent: Tuesday, January 13, 2009 2:17 PM
Subject: 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.
>
> --- 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-13 16:03:04.000000000 -0500
> @@ -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);
> } 

Re: sata_mv: trial fix for lost NCQ interrupts

[Mark Lord]

Harri Olin wrote:
..
> 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
@@ -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 */

Re: sata_mv: trial fix for lost NCQ interrupts

[Mark Lord]

Jeff Garzik wrote:
> Harri Olin wrote:
>> 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)...
..

Yup.  As I just noted on the redhat bugzilla entry,
this bug only bites when drives are attached to *both*
the lower and upper halves of the chip.

The second patch fixes that problem.
Once we hear back again from some affected users,
I'll reissue it for upstream/backstream.

Thanks

Re: sata_mv: trial fix for lost NCQ interrupts

[Harri Olin]

Mark Lord wrote:
> Harri Olin wrote:
> ..
>> 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
> @@ -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 :)

-- 
Harri.

Re: sata_mv: trial fix for lost NCQ interrupts

[Brian Rademacher]

Harri, as I reported on Redhat's Bugzilla 
(https://bugzilla.redhat.com/show_bug.cgi?id=462425), I think this is the 
fix as well...

Thanks for your port observation and Mark Lord's hard work on picking 
through the driver!!


----- Original Message ----- 
From: "Harri Olin" <harri.olin@gmail.com>
To: "Mark Lord" <liml@rtr.ca>
Cc: "IDE/ATA development list" <linux-ide@vger.kernel.org>; "Brian 
Rademacher" <rad@radfiles.net>; "Eamonn Hamilton" <EAMONN.HAMILTON@saic.com>
Sent: Wednesday, January 14, 2009 4:18 PM
Subject: Re: sata_mv: trial fix for lost NCQ interrupts


> Mark Lord wrote:
>> Harri Olin wrote:
>> ..
>>> 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
>> @@ -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 :)
>
> -- 
> Harri. 

Re: sata_mv: trial fix for lost NCQ interrupts

[Mark Lord]

Harri Olin wrote:
>..
> 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.

> 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!

Re: sata_mv: trial fix for lost NCQ interrupts

[Artem Bokhan]

Sounds great! Sorry, but I can't check the patch in the near future, I 
don't have necessary hardware at the moment :(

Mark Lord пишет:
> Harri Olin wrote:
>> ..
>> 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.
>
>> 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!
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html

[PATCH] sata_mv: fix timeouts on lower ports of 508x/6081 chips

[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 <mlord@pobox.com>

--- linux-2.6.29-rc1-git6/drivers/ata/sata_mv.c	2009-01-16 11:11:16.000000000 -0500
+++ linux/drivers/ata/sata_mv.c	2009-01-16 11:52:17.000000000 -0500
@@ -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 */

Re: [PATCH] sata_mv: fix timeouts on lower ports of 508x/6081 chips

[Harri Olin]

Mark Lord wrote:
> 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 <mlord@pobox.com>
>

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 <harri.olin@gmail.com>

-- 
Harri.

Re: [PATCH] sata_mv: fix timeouts on lower ports of 508x/6081 chips

[Brian Rademacher]

A little over 2 days here on 2.6.27 with no problems....

----- Original Message ----- 
From: "Harri Olin" <harri.olin@gmail.com>
To: "Mark Lord" <liml@rtr.ca>
Cc: "IDE/ATA development list" <linux-ide@vger.kernel.org>; "Jeff Garzik" 
<jgarzik@pobox.com>; "Artem Bokhan" <aptem@ngs.ru>; "Brian Rademacher" 
<rad@radfiles.net>; "Eamonn Hamilton" <EAMONN.HAMILTON@saic.com>; 
<stable@kernel.org>
Sent: Friday, January 16, 2009 1:08 PM
Subject: Re: [PATCH] sata_mv: fix timeouts on lower ports of 508x/6081 chips


> Mark Lord wrote:
>> 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 <mlord@pobox.com>
>>
>
> 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 <harri.olin@gmail.com>
>
> -- 
> Harri.