[regression] 2.6.23 sata_mv EH updates broke my 7042 controller

[regression] 2.6.23 sata_mv EH updates broke my 7042 controller

[Olof Johansson]

Hi,

I just pulled out my Marvell 7042 SATA controller here to give it a spin
and noticed that it no longer worked (i.e. init segvs when loading from
it), etc. 2.6.22 is fine.

Bisecting (just the file drivers/ata/sata_mv.c) resulted in
bdd4dddee325a7dce3e84cf48201a06aa8508aa4 popping out as the culprit
(file version 4537deb5e90b717a725b3d74b58b4bb1d28443d0 is fine).

Just for sanity checking: Has anyone else used 7042/6042 with the current
mainline driver?

Hardware config in my case:
Highpoint 2310 controller
PPC (big endian)
WD Raptor disk

Works fine with the other controller I've been using (SIL24), and works
fine if I revert the driver.

It also works fine if I disable the IOMMU. This would point towards
either a stale dma mapping, or a missing setup of one.

Not being much at home in the SATA drivers I could keep digging but I
figured I'd bring it up first in case it rings a bell for someone.


-Olof

Re: [regression] 2.6.23 sata_mv EH updates broke my 7042 controller

[Jeff Garzik]

Olof Johansson wrote:
> Hi,
> 
> I just pulled out my Marvell 7042 SATA controller here to give it a spin
> and noticed that it no longer worked (i.e. init segvs when loading from
> it), etc. 2.6.22 is fine.
> 
> Bisecting (just the file drivers/ata/sata_mv.c) resulted in
> bdd4dddee325a7dce3e84cf48201a06aa8508aa4 popping out as the culprit
> (file version 4537deb5e90b717a725b3d74b58b4bb1d28443d0 is fine).
> 
> Just for sanity checking: Has anyone else used 7042/6042 with the current
> mainline driver?

The update was tested extensively on 6042.  I have a 7042, but haven't 
found a compatible PCI slot here.


> Hardware config in my case:
> Highpoint 2310 controller
> PPC (big endian)
> WD Raptor disk
> 
> Works fine with the other controller I've been using (SIL24), and works
> fine if I revert the driver.
> 
> It also works fine if I disable the IOMMU. This would point towards
> either a stale dma mapping, or a missing setup of one.
> 
> Not being much at home in the SATA drivers I could keep digging but I
> figured I'd bring it up first in case it rings a bell for someone.

The IOMMU data point is certainly interesting.  Nothing jumps out on a 
re-review of the patch, so keep digging and let us know ;-)

	Jeff

Re: [regression] 2.6.23 sata_mv EH updates broke my 7042 controller

[Olof Johansson]

On Mon, Oct 01, 2007 at 06:37:44PM -0400, Jeff Garzik wrote:
> Olof Johansson wrote:
>> Hardware config in my case:
>> Highpoint 2310 controller
>> PPC (big endian)
>> WD Raptor disk
>> Works fine with the other controller I've been using (SIL24), and works
>> fine if I revert the driver.
>> It also works fine if I disable the IOMMU. This would point towards
>> either a stale dma mapping, or a missing setup of one.
>> Not being much at home in the SATA drivers I could keep digging but I
>> figured I'd bring it up first in case it rings a bell for someone.
>
> The IOMMU data point is certainly interesting.  Nothing jumps out on a 
> re-review of the patch, so keep digging and let us know ;-)

Looks like it's caused by enabling vmerge (which tends to be on for the
common PPC defconfigs). If I disable it, things look OK.

Perhaps the Marvell controller doesn't like requests larger than 64K,
or wrapping some boundary. Do you have access to erratas/docs?

I have verified it on a powermac now as well (had a quick scare that it
might have been some problem with the PA Semi IOMMU, but no).


-Olof

Re: [regression] 2.6.23 sata_mv EH updates broke my 7042 controller

[Jeff Garzik]

Olof Johansson wrote:
> On Mon, Oct 01, 2007 at 06:37:44PM -0400, Jeff Garzik wrote:
>> Olof Johansson wrote:
>>> Hardware config in my case:
>>> Highpoint 2310 controller
>>> PPC (big endian)
>>> WD Raptor disk
>>> Works fine with the other controller I've been using (SIL24), and works
>>> fine if I revert the driver.
>>> It also works fine if I disable the IOMMU. This would point towards
>>> either a stale dma mapping, or a missing setup of one.
>>> Not being much at home in the SATA drivers I could keep digging but I
>>> figured I'd bring it up first in case it rings a bell for someone.
>> The IOMMU data point is certainly interesting.  Nothing jumps out on a 
>> re-review of the patch, so keep digging and let us know ;-)
> 
> Looks like it's caused by enabling vmerge (which tends to be on for the
> common PPC defconfigs). If I disable it, things look OK.
> 
> Perhaps the Marvell controller doesn't like requests larger than 64K,
> or wrapping some boundary. Do you have access to erratas/docs?
> 
> I have verified it on a powermac now as well (had a quick scare that it
> might have been some problem with the PA Semi IOMMU, but no).

FWIW:  I just tried the 6042 on my AMD64 platform with iommu=force, and 
was unable to reproduce any trouble.

You could try changing MV_DMA_BOUNDARY to 0xffffU and see what happens.

I'll also look through my errata, to see what is different between 6042 
and 7042, if anything.

	Jeff

[PATCH] libata: fix for sata_mv >64KB DMA segments

[Olof Johansson]

Fix bug in sata_mv for cases where the IOMMU layer has merged SG entries
to larger than 64KB. They need to be split up before being sent to
the driver.

Just for simplicity's sake, split up at 64K boundary instead of 64K size,
since that's what the common code does anyway.

Signed-off-by: Olof Johansson <olof@lixom.net>


---

On Tue, Oct 02, 2007 at 07:23:10PM -0400, Jeff Garzik wrote:
> Olof Johansson wrote:
>> On Mon, Oct 01, 2007 at 06:37:44PM -0400, Jeff Garzik wrote:
>> Looks like it's caused by enabling vmerge (which tends to be on for the
>> common PPC defconfigs). If I disable it, things look OK.
>> Perhaps the Marvell controller doesn't like requests larger than 64K,
>> or wrapping some boundary. Do you have access to erratas/docs?
>> I have verified it on a powermac now as well (had a quick scare that it
>> might have been some problem with the PA Semi IOMMU, but no).
>
> FWIW:  I just tried the 6042 on my AMD64 platform with iommu=force, and was 
> unable to reproduce any trouble.
>
> You could try changing MV_DMA_BOUNDARY to 0xffffU and see what happens.

As per discussion on IRC, it was really caused by mv_fill_sg() not
handing SG entries larger than 64K properly. Below patch fixes it to
behave like ata_fill_sg() instead.

Works OK here after some light testing (restoring my corrupted root
filesystem and running a few fscks on it, among other things).


Thanks,

Olof


diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 11bf6c7..1a82e22 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -1139,15 +1139,27 @@ static unsigned int mv_fill_sg(struct ata_queued_cmd *qc)
 		dma_addr_t addr = sg_dma_address(sg);
 		u32 sg_len = sg_dma_len(sg);
 
-		mv_sg->addr = cpu_to_le32(addr & 0xffffffff);
-		mv_sg->addr_hi = cpu_to_le32((addr >> 16) >> 16);
-		mv_sg->flags_size = cpu_to_le32(sg_len & 0xffff);
+		while (sg_len) {
+			u32 offset = addr & 0xffff;
+			u32 len = sg_len;
 
-		if (ata_sg_is_last(sg, qc))
-			mv_sg->flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
+			if ((offset + sg_len > 0x10000))
+				len = 0x10000 - offset;
+
+			mv_sg->addr = cpu_to_le32(addr & 0xffffffff);
+			mv_sg->addr_hi = cpu_to_le32((addr >> 16) >> 16);
+			mv_sg->flags_size = cpu_to_le32(len);
+
+			sg_len -= len;
+			addr += len;
+
+			if (!sg_len && ata_sg_is_last(sg, qc))
+				mv_sg->flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
+
+			mv_sg++;
+			n_sg++;
+		}
 
-		mv_sg++;
-		n_sg++;
 	}
 
 	return n_sg;

Re: [PATCH] libata: fix for sata_mv >64KB DMA segments

[Jeff Garzik]

Olof Johansson wrote:
> Fix bug in sata_mv for cases where the IOMMU layer has merged SG entries
> to larger than 64KB. They need to be split up before being sent to
> the driver.
> 
> Just for simplicity's sake, split up at 64K boundary instead of 64K size,
> since that's what the common code does anyway.
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>

applied, good catch, thanks!