2.6.23 sata_mv EH updates broke my 7042 controller
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
Quoted 6 lines · 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.
Quoted 6 lines
Bisecting (just the file
drivers/ata/sata_mv.c) resulted inbdd4dddee325a7dce3e84cf48201a06aa8508aa4popping out as the culprit (file version4537deb5e90b717a725b3d74b58b4bb1d28443d0is 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.
Quoted 5 lines
Hardware config in my case:
Highpoint 2310 controller
PPC (big endian)
WD Raptor disk
Quoted 8 lines
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
Quoted 12 lines · Jeff Garzik
Quoted 10 lines · Olof Johansson
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.
Quoted 2 lines
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
Quoted 14 lines · Olof Johansson
Quoted 11 lines · Jeff Garzik
Quoted 10 lines · Olof Johansson
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.Quoted 1 line
The IOMMU data point is certainly interesting. Nothing jumps out on a re-review of the patch, so keep digging and let us know ;-)
Quoted 8 lines
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 1/1 libata: fix for sata_mv >64KB DMA segments
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.
---
Quoted 9 lines · Jeff Garzik
Quoted 7 lines · Olof Johansson
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).
Quoted 4 lines
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_BOUNDARYto0xffffUand 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
drivers/ata/sata_mv.c +19 -7
@@ -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;
Jeff Garzik
AppliedRe: [PATCH] libata: fix for sata_mv >64KB DMA segments
Quoted 8 lines · Olof Johansson
Fix bug in
sata_mvfor 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 <[email protected]>
applied, good catch, thanks!