sata_sil data corruption, possible workarounds

sata_sil data corruption, possible workarounds

[bl0]

I have a PCI card based on Silicon Image 3114 SATA controller. Like many
people in the past I have experienced silent data corruption.
I am lucky to have a hardware configuration where it is easy to reproduce
this behavior with 100% rate by copying a file from a USB stick plugged
into another PCI card. My motherboard has nvidia chipset.

Going through messages and bug reports about this problem, someone mentioned
that PCI cache line size may be relevant. I did some testing with different
CLS values and found that the problem of data corruption is solved if
either
A). CLS is set to 0, before or after sata_sil kernel driver is loaded
  # setpci -d 1095:3114 CACHE_LINE_SIZE=0
where 1095:3114 is the device id as shown by 'lspci -nn'. The same command
can also be used in grub2 (recent versions) shell or configuration file
before booting linux.
or
B). CLS is set to a sufficiently large value, only after sata_sil is loaded.
  # setpci -d 1095:3114 CACHE_LINE_SIZE=28
(value is hexadecimal, in 4-byte units, here it's 160 bytes)
What is a sufficiently large value depends on the value that is set before
the driver is loaded. If the value before the driver is loaded is 32 or 64
bytes, I have to increase it (after the driver is loaded) to 128 or 160
bytes, respectively.

In sata_sil.c source in sil_init_controller it writes some hardware-specific
value depending on PCI cache line size. By lowering this value I can get it
to work with lower CLS. The lowest value 0 works with CLS 64 bytes. If the
CLS is 32 bytes, I have to increase the CLS.

Data corruption is the biggest problem for me and these workarounds help but
another problem remains, sometimes when accessing multiple PCI devices at
the same time sata becomes inaccessible and times out with log messages
similar to:
[  411.351805] ata3.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6
frozen
[  411.351824] ata3.00: cmd c8/00:00:00:af:00/00:00:00:00:00/e0 tag 0 dma
131072 in
[  411.351826]          res 40/00:00:00:00:00/00:00:00:00:00/00 Emask 0x4
(timeout)
[  411.351830] ata3.00: status: { DRDY }
[  411.351843] ata3: hard resetting link
[  411.671775] ata3: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
[  411.697059] ata3.00: configured for UDMA/100
[  411.697080] ata3: EH complete

Reboot is needed to access sata drives again. If I had the root filesystem
on a sata drive it would probably crash the system.

Another thing that may be related. Comparing lspci output reveals that when
accessing multiple PCI devices at the same time, the flag DiscTmrStat
(Discard Timer Status) gets toggled on for device "00:08.0 PCI bridge:
nVidia Corporation nForce2 External PCI Bridge". I don't know if it's
normal or not.

Finally, the same simple test that I use on Linux does not produce data
corruption on FreeBSD. Either this problem doesn't occur there or it's not
trivial to reproduce.

This bug has been around for so long. I hope someone will find this
information useful.

Re: sata_sil data corruption, possible workarounds

[Robert Hancock]

On 12/15/2012 02:02 AM, bl0 wrote:
> I have a PCI card based on Silicon Image 3114 SATA controller. Like many
> people in the past I have experienced silent data corruption.
> I am lucky to have a hardware configuration where it is easy to reproduce
> this behavior with 100% rate by copying a file from a USB stick plugged
> into another PCI card. My motherboard has nvidia chipset.
>
> Going through messages and bug reports about this problem, someone mentioned
> that PCI cache line size may be relevant. I did some testing with different
> CLS values and found that the problem of data corruption is solved if
> either
> A). CLS is set to 0, before or after sata_sil kernel driver is loaded
>    # setpci -d 1095:3114 CACHE_LINE_SIZE=0
> where 1095:3114 is the device id as shown by 'lspci -nn'. The same command
> can also be used in grub2 (recent versions) shell or configuration file
> before booting linux.
> or
> B). CLS is set to a sufficiently large value, only after sata_sil is loaded.
>    # setpci -d 1095:3114 CACHE_LINE_SIZE=28
> (value is hexadecimal, in 4-byte units, here it's 160 bytes)
> What is a sufficiently large value depends on the value that is set before
> the driver is loaded. If the value before the driver is loaded is 32 or 64
> bytes, I have to increase it (after the driver is loaded) to 128 or 160
> bytes, respectively.
>
> In sata_sil.c source in sil_init_controller it writes some hardware-specific
> value depending on PCI cache line size. By lowering this value I can get it
> to work with lower CLS. The lowest value 0 works with CLS 64 bytes. If the
> CLS is 32 bytes, I have to increase the CLS.

The meaning of that value from the datasheet is: "This bit field is used 
to specify the system cacheline size in terms of 32-bit words. The upper 
2 bits are not used, resulting a maximum size of 64 32-bit words. With 
the SiI3114 as a master, initiating a read transaction, it issues PCI 
command Read Multiple in place, when empty space in its FIFO is larger 
than the value programmed in this register."

I think this value is likely the key. The cache line size itself 
shouldn't make any difference with this controller as it only really 
affects Memory Write & Invalidate (MWI) and the driver doesn't try to 
enable that for this device. But it's being used to derive the value 
written into this register.

Can you add in some output to figure out what values are being written 
to this register and see which values are working or not working?

>
> Data corruption is the biggest problem for me and these workarounds help but
> another problem remains, sometimes when accessing multiple PCI devices at
> the same time sata becomes inaccessible and times out with log messages
> similar to:
> [  411.351805] ata3.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6
> frozen
> [  411.351824] ata3.00: cmd c8/00:00:00:af:00/00:00:00:00:00/e0 tag 0 dma
> 131072 in
> [  411.351826]          res 40/00:00:00:00:00/00:00:00:00:00/00 Emask 0x4
> (timeout)
> [  411.351830] ata3.00: status: { DRDY }
> [  411.351843] ata3: hard resetting link
> [  411.671775] ata3: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
> [  411.697059] ata3.00: configured for UDMA/100
> [  411.697080] ata3: EH complete
>
> Reboot is needed to access sata drives again. If I had the root filesystem
> on a sata drive it would probably crash the system.
>
> Another thing that may be related. Comparing lspci output reveals that when
> accessing multiple PCI devices at the same time, the flag DiscTmrStat
> (Discard Timer Status) gets toggled on for device "00:08.0 PCI bridge:
> nVidia Corporation nForce2 External PCI Bridge". I don't know if it's
> normal or not.

I'm not an expert on the whole PCI bridge/delayed completion stuff but 
it appears that this means that a device (either the host bridge/CPU or 
a device behind that bridge) initiated a delayed transaction for a read, 
but then didn't retry the request to pick up the read data later. From 
what I can tell this seems abnormal, at least in most cases.

Can you post the full lspci -vv output? Do the problems only occur if 
there are multiple devices plugged in behind that bridge?

>
> Finally, the same simple test that I use on Linux does not produce data
> corruption on FreeBSD. Either this problem doesn't occur there or it's not
> trivial to reproduce.
>
> This bug has been around for so long. I hope someone will find this
> information useful.
>
>
> --
> 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
>

Re: sata_sil data corruption, possible workarounds

[bl0]

[-- Attachment #1: Type: text/plain, Size: 6252 bytes --]

Thanks for your response.

On Saturday 15 December 2012 22:55, Robert Hancock wrote:

> On 12/15/2012 02:02 AM, bl0 wrote:
>> I have a PCI card based on Silicon Image 3114 SATA controller. Like many
>> people in the past I have experienced silent data corruption.
>> I am lucky to have a hardware configuration where it is easy to reproduce
>> this behavior with 100% rate by copying a file from a USB stick plugged
>> into another PCI card. My motherboard has nvidia chipset.
>>
>> Going through messages and bug reports about this problem, someone
>> mentioned that PCI cache line size may be relevant. I did some testing
>> with different CLS values and found that the problem of data corruption
>> is solved if either
>> A). CLS is set to 0, before or after sata_sil kernel driver is loaded
>>    # setpci -d 1095:3114 CACHE_LINE_SIZE=0
>> where 1095:3114 is the device id as shown by 'lspci -nn'. The same
>> command can also be used in grub2 (recent versions) shell or
>> configuration file before booting linux.
>> or
>> B). CLS is set to a sufficiently large value, only after sata_sil is
>> loaded.
>>    # setpci -d 1095:3114 CACHE_LINE_SIZE=28
>> (value is hexadecimal, in 4-byte units, here it's 160 bytes)
>> What is a sufficiently large value depends on the value that is set
>> before the driver is loaded. If the value before the driver is loaded is
>> 32 or 64 bytes, I have to increase it (after the driver is loaded) to 128
>> or 160 bytes, respectively.
>>
>> In sata_sil.c source in sil_init_controller it writes some
>> hardware-specific value depending on PCI cache line size. By lowering
>> this value I can get it to work with lower CLS. The lowest value 0 works
>> with CLS 64 bytes. If the CLS is 32 bytes, I have to increase the CLS.
> 
> The meaning of that value from the datasheet is: "This bit field is used
> to specify the system cacheline size in terms of 32-bit words. The upper
> 2 bits are not used, resulting a maximum size of 64 32-bit words. With
> the SiI3114 as a master, initiating a read transaction, it issues PCI
> command Read Multiple in place, when empty space in its FIFO is larger
> than the value programmed in this register."
> 
> I think this value is likely the key. The cache line size itself
> shouldn't make any difference with this controller as it only really
> affects Memory Write & Invalidate (MWI) and the driver doesn't try to
> enable that for this device. But it's being used to derive the value
> written into this register.

In practice, on my hardware configuration, increasing the CLS after the
internal value has already been derived does make a difference.

> Can you add in some output to figure out what values are being written
> to this register

If the CLS is 32 or 64 bytes, it writes 2 or 3, respectively.

> and see which values are working or not working? 

That depends on the CLS. If the CLS is 32 bytes, it doesn't work (by work I
mean it's safe from data corruption) no matter what value I write to that
hardware register. If the CLS is 64 bytes, the only value that works is 0.

CLS     A       B       
32      2       none    
64      3       0       
96      4       1       
128     5       2       
160     6       3       

A: value written by default
B: maximum value safe from data corruption, based on my testing, probably
only applies to similar problematic hardware configurations.

Looking at this table you can see that increasing the CLS to a large value
can be a workaround after the driver has set the default value.

By default on my system this part of sata_sil code just overwrites the same
value (2 for 32 bytes CLS) that is already in place (as retrieved using
readw()) because the same value gets set (by the sata controller bios?)
after reboot. Changing this logic can work around data corruption problem.
There is another problem, sata link becoming inaccessible (I wrote more
about it in the first post), not affected by this part of sata_sil code. My
guess is that the main cause of the problems is elsewhere.

>> Data corruption is the biggest problem for me and these workarounds help
>> but another problem remains, sometimes when accessing multiple PCI
>> devices at the same time sata becomes inaccessible and times out with log
>> messages similar to:
>> [  411.351805] ata3.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6
>> frozen
>> [  411.351824] ata3.00: cmd c8/00:00:00:af:00/00:00:00:00:00/e0 tag 0 dma
>> 131072 in
>> [  411.351826]          res 40/00:00:00:00:00/00:00:00:00:00/00 Emask 0x4
>> (timeout)
>> [  411.351830] ata3.00: status: { DRDY }
>> [  411.351843] ata3: hard resetting link
>> [  411.671775] ata3: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
>> [  411.697059] ata3.00: configured for UDMA/100
>> [  411.697080] ata3: EH complete
>>
>> Reboot is needed to access sata drives again. If I had the root
>> filesystem on a sata drive it would probably crash the system.
>>
>> Another thing that may be related. Comparing lspci output reveals that
>> when accessing multiple PCI devices at the same time, the flag
>> DiscTmrStat (Discard Timer Status) gets toggled on for device "00:08.0
>> PCI bridge: nVidia Corporation nForce2 External PCI Bridge". I don't know
>> if it's normal or not.
> 
> I'm not an expert on the whole PCI bridge/delayed completion stuff but
> it appears that this means that a device (either the host bridge/CPU or
> a device behind that bridge) initiated a delayed transaction for a read,
> but then didn't retry the request to pick up the read data later. From
> what I can tell this seems abnormal, at least in most cases.
> 
> Can you post the full lspci -vv output? Do the problems only occur if
> there are multiple devices plugged in behind that bridge?

'lspci -vvv' output attached. Yes, I've only encountered problems with the
sata controller if at least one other external PCI card is in active use.
(The built-in devices which appear as PCI under another bridge do not cause
problems.)

>> Finally, the same simple test that I use on Linux does not produce data
>> corruption on FreeBSD. Either this problem doesn't occur there or it's
>> not trivial to reproduce.
>>
>> This bug has been around for so long. I hope someone will find this
>> information useful.


[-- Attachment #2: 3-lspci-vvv --]
[-- Type: text/plain, Size: 17889 bytes --]

00:00.0 Host bridge: nVidia Corporation nForce2 IGP2 (rev a2)
	Subsystem: ASUSTeK Computer Inc. Unknown device 80ac
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Region 0: Memory at c0000000 (32-bit, prefetchable) [size=256M]
	Capabilities: [40] AGP version 3.0
		Status: RQ=32 Iso- ArqSz=2 Cal=0 SBA+ ITACoh- GART64- HTrans- 64bit- FW- AGP3+ Rate=x4,x8
		Command: RQ=1 ArqSz=0 Cal=0 SBA- AGP- GART64- 64bit- FW- Rate=x4
	Capabilities: [60] HyperTransport: Host or Secondary Interface
		Command: WarmRst+ DblEnd-
		Link Control: CFlE- CST- CFE- <LkFail- Init+ EOC- TXO- <CRCErr=0
		Link Config: MLWI=8bit MLWO=8bit LWI=8bit LWO=8bit
		Revision ID: 0.16
	Kernel driver in use: agpgart-nvidia
	Kernel modules: nvidia-agp

00:00.1 RAM memory: nVidia Corporation nForce2 Memory Controller 1 (rev a2)
	Subsystem: nVidia Corporation Unknown device 0c17
	Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap- 66MHz+ UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-

00:00.2 RAM memory: nVidia Corporation nForce2 Memory Controller 4 (rev a2)
	Subsystem: nVidia Corporation Unknown device 0c17
	Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap- 66MHz+ UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-

00:00.3 RAM memory: nVidia Corporation nForce2 Memory Controller 3 (rev a2)
	Subsystem: nVidia Corporation Unknown device 0c17
	Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap- 66MHz+ UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-

00:00.4 RAM memory: nVidia Corporation nForce2 Memory Controller 2 (rev a2)
	Subsystem: nVidia Corporation Unknown device 0c17
	Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap- 66MHz+ UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-

00:00.5 RAM memory: nVidia Corporation nForce2 Memory Controller 5 (rev a2)
	Subsystem: nVidia Corporation Unknown device 0c17
	Control: I/O- Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap- 66MHz+ UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-

00:01.0 ISA bridge: nVidia Corporation nForce2 ISA Bridge (rev a3)
	Subsystem: ASUSTeK Computer Inc. A7N8X Mainboard
	Control: I/O+ Mem+ BusMaster+ SpecCycle+ MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Capabilities: [48] HyperTransport: Slave or Primary Interface
		Command: BaseUnitID=1 UnitCnt=15 MastHost- DefDir-
		Link Control 0: CFlE- CST- CFE- <LkFail- Init+ EOC+ TXO- <CRCErr=0
		Link Config 0: MLWI=8bit MLWO=8bit LWI=8bit LWO=8bit
		Link Control 1: CFlE- CST- CFE- <LkFail- Init+ EOC- TXO+ <CRCErr=0
		Link Config 1: MLWI=8bit MLWO=8bit LWI=8bit LWO=8bit
		Revision ID: 0.00

00:01.1 SMBus: nVidia Corporation nForce2 SMBus (MCP) (rev a2)
	Subsystem: ASUSTeK Computer Inc. Unknown device 0c11
	Control: I/O+ Mem- BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Interrupt: pin A routed to IRQ 255
	Region 0: I/O ports at ec00 [size=32]
	Capabilities: [44] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: nForce2_smbus
	Kernel modules: i2c-nforce2

00:02.0 USB Controller: nVidia Corporation nForce2 USB Controller (rev a3) (prog-if 10 [OHCI])
	Subsystem: ASUSTeK Computer Inc. A7N8X Mainboard
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0 (750ns min, 250ns max)
	Interrupt: pin A routed to IRQ 20
	Region 0: Memory at e5080000 (32-bit, non-prefetchable) [size=4K]
	Capabilities: [44] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ohci_hcd

00:02.1 USB Controller: nVidia Corporation nForce2 USB Controller (rev a3) (prog-if 10 [OHCI])
	Subsystem: ASUSTeK Computer Inc. A7N8X Mainboard
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0 (750ns min, 250ns max)
	Interrupt: pin B routed to IRQ 22
	Region 0: Memory at e5082000 (32-bit, non-prefetchable) [size=4K]
	Capabilities: [44] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ohci_hcd

00:02.2 USB Controller: nVidia Corporation nForce2 USB Controller (rev a3) (prog-if 20 [EHCI])
	Subsystem: ASUSTeK Computer Inc. A7N8X Mainboard
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0 (750ns min, 250ns max)
	Interrupt: pin C routed to IRQ 21
	Region 0: Memory at e5085000 (32-bit, non-prefetchable) [size=256]
	Capabilities: [44] Debug port: BAR=1 offset=0080
	Capabilities: [80] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ehci_hcd

00:04.0 Ethernet controller: nVidia Corporation nForce2 Ethernet Controller (rev a1)
	Subsystem: ASUSTeK Computer Inc. A7N8X Mainboard onboard nForce2 Ethernet
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0 (250ns min, 5000ns max)
	Interrupt: pin A routed to IRQ 21
	Region 0: Memory at e5086000 (32-bit, non-prefetchable) [size=4K]
	Region 1: I/O ports at e000 [size=8]
	Capabilities: [44] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
		Status: D0 PME-Enable+ DSel=0 DScale=0 PME-
	Kernel driver in use: forcedeth
	Kernel modules: forcedeth

00:05.0 Multimedia audio controller: nVidia Corporation nForce Audio Processing Unit (rev a2)
	Subsystem: ASUSTeK Computer Inc. Unknown device 0c11
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0 (250ns min, 3000ns max)
	Interrupt: pin A routed to IRQ 5
	Region 0: Memory at e5000000 (32-bit, non-prefetchable) [size=512K]
	Capabilities: [44] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-

00:06.0 Multimedia audio controller: nVidia Corporation nForce2 AC97 Audio Controler (MCP) (rev a1)
	Subsystem: ASUSTeK Computer Inc. nForce2 AC97 Audio Controler (MCP)
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0 (500ns min, 1250ns max)
	Interrupt: pin A routed to IRQ 11
	Region 0: I/O ports at e400 [size=256]
	Region 1: I/O ports at e800 [size=128]
	Region 2: Memory at e5081000 (32-bit, non-prefetchable) [size=4K]
	Capabilities: [44] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-

00:08.0 PCI bridge: nVidia Corporation nForce2 External PCI Bridge (rev a3) (prog-if 00 [Normal decode])
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B- DisINTx-
	Status: Cap- 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Bus: primary=00, secondary=01, subordinate=01, sec-latency=32
	I/O behind bridge: 0000c000-0000dfff
	Memory behind bridge: e3000000-e4ffffff
	Prefetchable memory behind bridge: 50000000-500fffff
	Secondary status: 66MHz- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort+ <SERR- <PERR+
	BridgeCtl: Parity- SERR+ NoISA- VGA- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr+ DiscTmrStat+ DiscTmrSERREn-
	Kernel modules: shpchp

00:09.0 IDE interface: nVidia Corporation nForce2 IDE (rev a2) (prog-if 8a [Master SecP PriP])
	Subsystem: ASUSTeK Computer Inc. Unknown device 0c11
	Control: I/O+ Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0 (750ns min, 250ns max)
	Region 0: [virtual] Memory at 000001f0 (32-bit, non-prefetchable) [disabled] [size=8]
	Region 1: [virtual] Memory at 000003f0 (type 3, non-prefetchable) [disabled] [size=1]
	Region 2: [virtual] Memory at 00000170 (32-bit, non-prefetchable) [disabled] [size=8]
	Region 3: [virtual] Memory at 00000370 (type 3, non-prefetchable) [disabled] [size=1]
	Region 4: I/O ports at f000 [size=16]
	Capabilities: [44] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: AMD_IDE
	Kernel modules: pata_amd

00:0d.0 FireWire (IEEE 1394): nVidia Corporation nForce2 FireWire (IEEE 1394) Controller (rev a3) (prog-if 10 [OHCI])
	Subsystem: ASUSTeK Computer Inc. Unknown device 809a
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0 (750ns min, 250ns max)
	Interrupt: pin A routed to IRQ 22
	Region 0: Memory at e5083000 (32-bit, non-prefetchable) [size=2K]
	Region 1: Memory at e5084000 (32-bit, non-prefetchable) [size=64]
	Capabilities: [44] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ohci1394
	Kernel modules: firewire-ohci

00:1e.0 PCI bridge: nVidia Corporation nForce2 AGP (rev a2) (prog-if 00 [Normal decode])
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B- DisINTx-
	Status: Cap- 66MHz+ UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 32
	Bus: primary=00, secondary=02, subordinate=02, sec-latency=32
	I/O behind bridge: 0000f000-00000fff
	Memory behind bridge: e0000000-e2ffffff
	Prefetchable memory behind bridge: d0000000-dfffffff
	Secondary status: 66MHz+ FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort+ <SERR- <PERR-
	BridgeCtl: Parity- SERR+ NoISA- VGA+ MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Kernel modules: shpchp

01:07.0 Ethernet controller: 3Com Corporation 3c905B 100BaseTX [Cyclone] (rev 30)
	Subsystem: 3Com Corporation 3C905B Fast Etherlink XL 10/100
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 32 (2500ns min, 2500ns max), Cache Line Size: 32 bytes
	Interrupt: pin A routed to IRQ 19
	Region 0: I/O ports at c000 [size=128]
	Region 1: Memory at e4000000 (32-bit, non-prefetchable) [size=128]
	[virtual] Expansion ROM at 50080000 [disabled] [size=128K]
	Capabilities: [dc] Power Management version 1
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0-,D1+,D2+,D3hot+,D3cold+)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: 3c59x
	Kernel modules: 3c59x

01:08.0 RAID bus controller: Silicon Image, Inc. SiI 3114 [SATALink/SATARaid] Serial ATA Controller (rev 02)
	Subsystem: Silicon Image, Inc. Unknown device 7114
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 32, Cache Line Size: 32 bytes
	Interrupt: pin A routed to IRQ 18
	Region 0: I/O ports at c400 [size=8]
	Region 1: I/O ports at c800 [size=4]
	Region 2: I/O ports at cc00 [size=8]
	Region 3: I/O ports at d000 [size=4]
	Region 4: I/O ports at d400 [size=16]
	Region 5: Memory at e4001000 (32-bit, non-prefetchable) [size=1K]
	[virtual] Expansion ROM at 50000000 [disabled] [size=512K]
	Capabilities: [60] Power Management version 2
		Flags: PMEClk- DSI+ D1+ D2+ AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=2 PME-
	Kernel driver in use: sata_sil

01:09.0 Multimedia audio controller: Creative Labs SB Live! EMU10k1 (rev 07)
	Subsystem: Creative Labs SBLive! 5.1 Model SB0100
	Control: I/O+ Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 32 (500ns min, 5000ns max)
	Interrupt: pin A routed to IRQ 17
	Region 0: I/O ports at d800 [size=32]
	Capabilities: [dc] Power Management version 1
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: EMU10K1_Audigy
	Kernel modules: snd-emu10k1

01:09.1 Input device controller: Creative Labs SB Live! Game Port (rev 07)
	Subsystem: Creative Labs Gameport Joystick
	Control: I/O+ Mem- BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 32
	Region 0: I/O ports at dc00 [size=8]
	Capabilities: [dc] Power Management version 1
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: Emu10k1_gameport
	Kernel modules: emu10k1-gp

01:0a.0 USB Controller: NEC Corporation USB (rev 41) (prog-if 10 [OHCI])
	Subsystem: NEC Corporation Hama USB 2.0 CardBus
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 32 (250ns min, 10500ns max), Cache Line Size: 32 bytes
	Interrupt: pin A routed to IRQ 16
	Region 0: Memory at e4002000 (32-bit, non-prefetchable) [size=4K]
	Capabilities: [40] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ohci_hcd

01:0a.1 USB Controller: NEC Corporation USB (rev 41) (prog-if 10 [OHCI])
	Subsystem: NEC Corporation Hama USB 2.0 CardBus
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 32 (250ns min, 10500ns max), Cache Line Size: 32 bytes
	Interrupt: pin B routed to IRQ 17
	Region 0: Memory at e4003000 (32-bit, non-prefetchable) [size=4K]
	Capabilities: [40] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ohci_hcd

01:0a.2 USB Controller: NEC Corporation USB 2.0 (rev 02) (prog-if 20 [EHCI])
	Subsystem: NEC Corporation USB 2.0
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV+ VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 32 (4000ns min, 8500ns max), Cache Line Size: 64 bytes
	Interrupt: pin C routed to IRQ 18
	Region 0: Memory at e4004000 (32-bit, non-prefetchable) [size=256]
	Capabilities: [40] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0+,D1+,D2+,D3hot+,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ehci_hcd

02:00.0 VGA compatible controller: nVidia Corporation G73 [GeForce 7600 GS] (rev a2) (prog-if 00 [VGA controller])
	Subsystem: Giga-byte Technology Unknown device 3428
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 32 (1250ns min, 250ns max)
	Interrupt: pin A routed to IRQ 19
	Region 0: Memory at e0000000 (32-bit, non-prefetchable) [size=16M]
	Region 1: Memory at d0000000 (32-bit, prefetchable) [size=256M]
	Region 2: Memory at e1000000 (32-bit, non-prefetchable) [size=16M]
	[virtual] Expansion ROM at e2000000 [disabled] [size=128K]
	Capabilities: [60] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [44] AGP version 3.0
		Status: RQ=256 Iso- ArqSz=0 Cal=3 SBA+ ITACoh- GART64- HTrans- 64bit- FW+ AGP3+ Rate=x4,x8
		Command: RQ=1 ArqSz=0 Cal=0 SBA- AGP- GART64- 64bit- FW- Rate=<none>
	Kernel driver in use: nvidia
	Kernel modules: nvidia, nvidiafb

Re: sata_sil data corruption, possible workarounds

[Robert Hancock]

On 12/16/2012 06:21 AM, bl0 wrote:
> Thanks for your response.
>
> On Saturday 15 December 2012 22:55, Robert Hancock wrote:
>
>> On 12/15/2012 02:02 AM, bl0 wrote:
>>> I have a PCI card based on Silicon Image 3114 SATA controller. Like many
>>> people in the past I have experienced silent data corruption.
>>> I am lucky to have a hardware configuration where it is easy to reproduce
>>> this behavior with 100% rate by copying a file from a USB stick plugged
>>> into another PCI card. My motherboard has nvidia chipset.
>>>
>>> Going through messages and bug reports about this problem, someone
>>> mentioned that PCI cache line size may be relevant. I did some testing
>>> with different CLS values and found that the problem of data corruption
>>> is solved if either
>>> A). CLS is set to 0, before or after sata_sil kernel driver is loaded
>>>     # setpci -d 1095:3114 CACHE_LINE_SIZE=0
>>> where 1095:3114 is the device id as shown by 'lspci -nn'. The same
>>> command can also be used in grub2 (recent versions) shell or
>>> configuration file before booting linux.
>>> or
>>> B). CLS is set to a sufficiently large value, only after sata_sil is
>>> loaded.
>>>     # setpci -d 1095:3114 CACHE_LINE_SIZE=28
>>> (value is hexadecimal, in 4-byte units, here it's 160 bytes)
>>> What is a sufficiently large value depends on the value that is set
>>> before the driver is loaded. If the value before the driver is loaded is
>>> 32 or 64 bytes, I have to increase it (after the driver is loaded) to 128
>>> or 160 bytes, respectively.
>>>
>>> In sata_sil.c source in sil_init_controller it writes some
>>> hardware-specific value depending on PCI cache line size. By lowering
>>> this value I can get it to work with lower CLS. The lowest value 0 works
>>> with CLS 64 bytes. If the CLS is 32 bytes, I have to increase the CLS.
>>
>> The meaning of that value from the datasheet is: "This bit field is used
>> to specify the system cacheline size in terms of 32-bit words. The upper
>> 2 bits are not used, resulting a maximum size of 64 32-bit words. With
>> the SiI3114 as a master, initiating a read transaction, it issues PCI
>> command Read Multiple in place, when empty space in its FIFO is larger
>> than the value programmed in this register."
>>
>> I think this value is likely the key. The cache line size itself
>> shouldn't make any difference with this controller as it only really
>> affects Memory Write & Invalidate (MWI) and the driver doesn't try to
>> enable that for this device. But it's being used to derive the value
>> written into this register.
>
> In practice, on my hardware configuration, increasing the CLS after the
> internal value has already been derived does make a difference.
>
>> Can you add in some output to figure out what values are being written
>> to this register
>
> If the CLS is 32 or 64 bytes, it writes 2 or 3, respectively.
>
>> and see which values are working or not working?
>
> That depends on the CLS. If the CLS is 32 bytes, it doesn't work (by work I
> mean it's safe from data corruption) no matter what value I write to that
> hardware register. If the CLS is 64 bytes, the only value that works is 0.
>
> CLS     A       B
> 32      2       none
> 64      3       0
> 96      4       1
> 128     5       2
> 160     6       3
>
> A: value written by default
> B: maximum value safe from data corruption, based on my testing, probably
> only applies to similar problematic hardware configurations.
>
> Looking at this table you can see that increasing the CLS to a large value
> can be a workaround after the driver has set the default value.

Hmm, looks like I was looking at the wrong register. The CLS itself is 
described by what I posted, so changing that does affect things (i.e. 
the threshold for Memory Read Multiple). The other value being written 
into fifo_cfg is the FIFO Write Request Control and FIFO Read Request 
Control field (that's why it's written to bits 0-2 and 8-10).

"The FIFO Write Request Control and FIFO Read Request Control fields in 
these registers provide threshold settings for establishing when PCI 
requests are made to the Arbiter. The Arbiter arbitrates among the four
requests using fixed priority with masking. The fixed priority is, from 
highest to lowest: channel 0; channel 1; channel 2; and channel 3. If 
multiple requests are present, the arbiter grants PCI bus access to the 
highest priority channel that is not masked. That channel’s request is 
then masked as long as any unmasked requests are present.

..

FIFO Read Request Control.  This bit field defines the FIFO threshold to 
assign priority when requesting a PCI bus read operation. A value of 00H 
indicates that read request priority is set whenever the FIFO has 
greater than 32 bytes available space, while a value of 07H indicates 
that read request priority is set whenever the FIFO has greater than 
7x32 bytes (=224 bytes) available space. This bit field is useful when 
multiple DMA channels are competing for accessing the PCI bus.


FIFO Write Request Control. This bit field defines the FIFO threshold to 
assign priority when requesting a PCI bus write operation. A value of 
00H indicates that write request priority is set whenever the FIFO 
contains greater than 32 bytes, while a value of 07H indicates that 
write request priority is set whenever the FIFO contains greater than 
7x32 bytes (=224 bytes). This bit field is useful when multiple DMA 
channels are competing for the PCI bus."

The value apparently being written to the register according to the code 
(and given that the value in the CLS register is in units of 32-bit 
words) is (cache line size >> 3) + 1.

 From looking at the history of this code (which dates from the pre-git 
days in 2005) it comes from:

https://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commit;h=fceff08ed7660f9bbe96ee659acb02841a3f1f39

which refers to an issue with DMA FIFO thresholds which could cause data 
corruption. The description is pretty much hand-waving and doesn't 
really describe what is going on. But it seems quite likely that 
whatever magic numbers this code is picking don't work on your system 
for some reason. It appears the root cause is likely a bug in the SiI 
chip. There shouldn't be any region why messing around with these values 
should cause data corruption other than that.

>
> By default on my system this part of sata_sil code just overwrites the same
> value (2 for 32 bytes CLS) that is already in place (as retrieved using
> readw()) because the same value gets set (by the sata controller bios?)
> after reboot. Changing this logic can work around data corruption problem.
> There is another problem, sata link becoming inaccessible (I wrote more
> about it in the first post), not affected by this part of sata_sil code. My
> guess is that the main cause of the problems is elsewhere.
>
>>> Data corruption is the biggest problem for me and these workarounds help
>>> but another problem remains, sometimes when accessing multiple PCI
>>> devices at the same time sata becomes inaccessible and times out with log
>>> messages similar to:
>>> [  411.351805] ata3.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6
>>> frozen
>>> [  411.351824] ata3.00: cmd c8/00:00:00:af:00/00:00:00:00:00/e0 tag 0 dma
>>> 131072 in
>>> [  411.351826]          res 40/00:00:00:00:00/00:00:00:00:00/00 Emask 0x4
>>> (timeout)
>>> [  411.351830] ata3.00: status: { DRDY }
>>> [  411.351843] ata3: hard resetting link
>>> [  411.671775] ata3: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
>>> [  411.697059] ata3.00: configured for UDMA/100
>>> [  411.697080] ata3: EH complete
>>>
>>> Reboot is needed to access sata drives again. If I had the root
>>> filesystem on a sata drive it would probably crash the system.
>>>
>>> Another thing that may be related. Comparing lspci output reveals that
>>> when accessing multiple PCI devices at the same time, the flag
>>> DiscTmrStat (Discard Timer Status) gets toggled on for device "00:08.0
>>> PCI bridge: nVidia Corporation nForce2 External PCI Bridge". I don't know
>>> if it's normal or not.
>>
>> I'm not an expert on the whole PCI bridge/delayed completion stuff but
>> it appears that this means that a device (either the host bridge/CPU or
>> a device behind that bridge) initiated a delayed transaction for a read,
>> but then didn't retry the request to pick up the read data later. From
>> what I can tell this seems abnormal, at least in most cases.
>>
>> Can you post the full lspci -vv output? Do the problems only occur if
>> there are multiple devices plugged in behind that bridge?
>
> 'lspci -vvv' output attached. Yes, I've only encountered problems with the
> sata controller if at least one other external PCI card is in active use.
> (The built-in devices which appear as PCI under another bridge do not cause
> problems.)
>
>>> Finally, the same simple test that I use on Linux does not produce data
>>> corruption on FreeBSD. Either this problem doesn't occur there or it's
>>> not trivial to reproduce.
>>>
>>> This bug has been around for so long. I hope someone will find this
>>> information useful.
>

Re: sata_sil data corruption, possible workarounds

[bl0]

On Monday 17 December 2012 06:44, Robert Hancock wrote:

> Hmm, looks like I was looking at the wrong register. The CLS itself is
> described by what I posted, so changing that does affect things (i.e.
> the threshold for Memory Read Multiple). The other value being written
> into fifo_cfg is the FIFO Write Request Control and FIFO Read Request
> Control field (that's why it's written to bits 0-2 and 8-10).
> 
> "The FIFO Write Request Control and FIFO Read Request Control fields in
> these registers provide threshold settings for establishing when PCI
> requests are made to the Arbiter. The Arbiter arbitrates among the four
> requests using fixed priority with masking. The fixed priority is, from
> highest to lowest: channel 0; channel 1; channel 2; and channel 3. If
> multiple requests are present, the arbiter grants PCI bus access to the
> highest priority channel that is not masked. That channel’s request is
> then masked as long as any unmasked requests are present.
> 
> ..
> 
> FIFO Read Request Control.  This bit field defines the FIFO threshold to
> assign priority when requesting a PCI bus read operation. A value of 00H
> indicates that read request priority is set whenever the FIFO has
> greater than 32 bytes available space, while a value of 07H indicates
> that read request priority is set whenever the FIFO has greater than
> 7x32 bytes (=224 bytes) available space. 

A fencepost error? They probably mean 8x32 bytes...

> This bit field is useful when 
> multiple DMA channels are competing for accessing the PCI bus.
> 
> 
> FIFO Write Request Control. This bit field defines the FIFO threshold to
> assign priority when requesting a PCI bus write operation. A value of
> 00H indicates that write request priority is set whenever the FIFO
> contains greater than 32 bytes, while a value of 07H indicates that
> write request priority is set whenever the FIFO contains greater than
> 7x32 bytes (=224 bytes). This bit field is useful when multiple DMA
> channels are competing for the PCI bus."
> 
> The value apparently being written to the register according to the code
> (and given that the value in the CLS register is in units of 32-bit
> words) is (cache line size >> 3) + 1.
> 
>  From looking at the history of this code (which dates from the pre-git
> days in 2005) it comes from:
> 
>
https://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commit;h=fceff08ed7660f9bbe96ee659acb02841a3f1f39
> 
> which refers to an issue with DMA FIFO thresholds which could cause data
> corruption. The description is pretty much hand-waving and doesn't 
> really describe what is going on. 

From the description: "The patch is to setup the DMA fifo threshold so that
there is no chance for the DMA engine to change protocol." Is there a way to
check (or add debugging messages) if/when this change of protocol is
happening?

> But it seems quite likely that 
> whatever magic numbers this code is picking don't work on your system
> for some reason. It appears the root cause is likely a bug in the SiI
> chip. There shouldn't be any region why messing around with these values
> should cause data corruption other than that.

Do you think something should be done about it in the linux sata_sil driver?
For a lack of a better solution, here is my suggestion. There is already
one option 'slow_down' for problematic disks. Another option, for
example 'cache_line_workaround', could be added for problematic
motherboards. If enabled, the most straightforward way is to set cache line
size to 0 and not worry about the fifo_cfg register. If someone else
confirms that it solves the problem for them, this option could be enabled
automatically if certain motherboard chipset is detected.

A comment in the source of another Silicon Image driver, siimage.c: "If you
have strange problems with nVidia chipset systems please see the SI support
documentation and update your system BIOS if necessary". Do you (or anyone
else) know what support documentation it's refering to?

Re: sata_sil data corruption, possible workarounds

[Robert Hancock]

On 12/18/2012 09:23 AM, bl0 wrote:
> On Monday 17 December 2012 06:44, Robert Hancock wrote:
>
>> Hmm, looks like I was looking at the wrong register. The CLS itself is
>> described by what I posted, so changing that does affect things (i.e.
>> the threshold for Memory Read Multiple). The other value being written
>> into fifo_cfg is the FIFO Write Request Control and FIFO Read Request
>> Control field (that's why it's written to bits 0-2 and 8-10).
>>
>> "The FIFO Write Request Control and FIFO Read Request Control fields in
>> these registers provide threshold settings for establishing when PCI
>> requests are made to the Arbiter. The Arbiter arbitrates among the four
>> requests using fixed priority with masking. The fixed priority is, from
>> highest to lowest: channel 0; channel 1; channel 2; and channel 3. If
>> multiple requests are present, the arbiter grants PCI bus access to the
>> highest priority channel that is not masked. That channel’s request is
>> then masked as long as any unmasked requests are present.
>>
>> ..
>>
>> FIFO Read Request Control.  This bit field defines the FIFO threshold to
>> assign priority when requesting a PCI bus read operation. A value of 00H
>> indicates that read request priority is set whenever the FIFO has
>> greater than 32 bytes available space, while a value of 07H indicates
>> that read request priority is set whenever the FIFO has greater than
>> 7x32 bytes (=224 bytes) available space.
>
> A fencepost error? They probably mean 8x32 bytes...

Yeah, I would think so.

>
>> This bit field is useful when
>> multiple DMA channels are competing for accessing the PCI bus.
>>
>>
>> FIFO Write Request Control. This bit field defines the FIFO threshold to
>> assign priority when requesting a PCI bus write operation. A value of
>> 00H indicates that write request priority is set whenever the FIFO
>> contains greater than 32 bytes, while a value of 07H indicates that
>> write request priority is set whenever the FIFO contains greater than
>> 7x32 bytes (=224 bytes). This bit field is useful when multiple DMA
>> channels are competing for the PCI bus."
>>
>> The value apparently being written to the register according to the code
>> (and given that the value in the CLS register is in units of 32-bit
>> words) is (cache line size >> 3) + 1.
>>
>>   From looking at the history of this code (which dates from the pre-git
>> days in 2005) it comes from:
>>
>>
> https://git.kernel.org/?p=linux/kernel/git/tglx/history.git;a=commit;h=fceff08ed7660f9bbe96ee659acb02841a3f1f39
>>
>> which refers to an issue with DMA FIFO thresholds which could cause data
>> corruption. The description is pretty much hand-waving and doesn't
>> really describe what is going on.
>
>  From the description: "The patch is to setup the DMA fifo threshold so that
> there is no chance for the DMA engine to change protocol." Is there a way to
> check (or add debugging messages) if/when this change of protocol is
> happening?

I'm not sure exactly what they're talking about, but I would imagine 
this is something internal to the chip which we likely wouldn't have any 
visibility into (without specialized equipment like a PCI bus analyzer 
or something).

>
>> But it seems quite likely that
>> whatever magic numbers this code is picking don't work on your system
>> for some reason. It appears the root cause is likely a bug in the SiI
>> chip. There shouldn't be any region why messing around with these values
>> should cause data corruption other than that.
>
> Do you think something should be done about it in the linux sata_sil driver?
> For a lack of a better solution, here is my suggestion. There is already
> one option 'slow_down' for problematic disks. Another option, for
> example 'cache_line_workaround', could be added for problematic
> motherboards. If enabled, the most straightforward way is to set cache line
> size to 0 and not worry about the fifo_cfg register. If someone else
> confirms that it solves the problem for them, this option could be enabled
> automatically if certain motherboard chipset is detected.

We'd have to somehow narrow down which chipsets were involved, which 
might be a hard task. Do you have an idea of how much the performance is 
hurt by these workarounds? If it's not a lot, it might make sense to do 
it by default.

>
> A comment in the source of another Silicon Image driver, siimage.c: "If you
> have strange problems with nVidia chipset systems please see the SI support
> documentation and update your system BIOS if necessary". Do you (or anyone
> else) know what support documentation it's refering to?

I don't remember seeing such a thing before. There might have been 
something on their web site at some point, but who knows if it hasn't 
been reorganized out of existence by now.

Re: sata_sil data corruption, possible workarounds

[bl0]

On Wednesday 19 December 2012 04:44, Robert Hancock wrote:

> On 12/18/2012 09:23 AM, bl0 wrote:
>> Do you think something should be done about it in the linux sata_sil
>> driver? For a lack of a better solution, here is my suggestion. There is
>> already one option 'slow_down' for problematic disks. Another option, for
>> example 'cache_line_workaround', could be added for problematic
>> motherboards. If enabled, the most straightforward way is to set cache
>> line size to 0 and not worry about the fifo_cfg register. If someone else
>> confirms that it solves the problem for them, this option could be
>> enabled automatically if certain motherboard chipset is detected.
> 
> We'd have to somehow narrow down which chipsets were involved, which
> might be a hard task. Do you have an idea of how much the performance is
> hurt by these workarounds? If it's not a lot, it might make sense to do
> it by default.

After setting cache line size to 0, write speed as shown by 'dd
if=/tmpfs/testfile of=/dev/sdc9 bs=1M count=256' goes down from about
45 MB/s to 17 MB/s. Personally I don't care about performance, reliability
and data safety are more important to me.

The other workaround is to increase cache line size to 64 bytes, if
necessary, and set fifo_cfg to 0. No difference in performance measured.
This workaround is more of a hit or miss. It seems to contradict that code
commit made back in 2005, which was also about data corruption. In the
worst case, what solves data corruption problem on some motherboards might
introduce this problem on some other motherboards.

Re: sata_sil data corruption, possible workarounds

[Robert Hancock]

On 12/20/2012 02:54 AM, bl0 wrote:
> On Wednesday 19 December 2012 04:44, Robert Hancock wrote:
>
>> On 12/18/2012 09:23 AM, bl0 wrote:
>>> Do you think something should be done about it in the linux sata_sil
>>> driver? For a lack of a better solution, here is my suggestion. There is
>>> already one option 'slow_down' for problematic disks. Another option, for
>>> example 'cache_line_workaround', could be added for problematic
>>> motherboards. If enabled, the most straightforward way is to set cache
>>> line size to 0 and not worry about the fifo_cfg register. If someone else
>>> confirms that it solves the problem for them, this option could be
>>> enabled automatically if certain motherboard chipset is detected.
>>
>> We'd have to somehow narrow down which chipsets were involved, which
>> might be a hard task. Do you have an idea of how much the performance is
>> hurt by these workarounds? If it's not a lot, it might make sense to do
>> it by default.
>
> After setting cache line size to 0, write speed as shown by 'dd
> if=/tmpfs/testfile of=/dev/sdc9 bs=1M count=256' goes down from about
> 45 MB/s to 17 MB/s. Personally I don't care about performance, reliability
> and data safety are more important to me.

Yeah, cutting performance by 2/3rds is fairly bad though.

>
> The other workaround is to increase cache line size to 64 bytes, if
> necessary, and set fifo_cfg to 0. No difference in performance measured.
> This workaround is more of a hit or miss. It seems to contradict that code
> commit made back in 2005, which was also about data corruption. In the
> worst case, what solves data corruption problem on some motherboards might
> introduce this problem on some other motherboards.

That's possible, which is why I suspect that someone from Silicon Image 
would have to confirm a possible fix - might be hard to get their 
attention about this old chipset..

Re: sata_sil data corruption, possible workarounds

[bl0]

On Monday 07 January 2013 05:11, Robert Hancock wrote:

> On 12/20/2012 02:54 AM, bl0 wrote:
>> On Wednesday 19 December 2012 04:44, Robert Hancock wrote:
>>
>>> On 12/18/2012 09:23 AM, bl0 wrote:
>>>> Do you think something should be done about it in the linux sata_sil
>>>> driver? For a lack of a better solution, here is my suggestion. There
>>>> is already one option 'slow_down' for problematic disks. Another
>>>> option, for example 'cache_line_workaround', could be added for
>>>> problematic motherboards. If enabled, the most straightforward way is
>>>> to set cache line size to 0 and not worry about the fifo_cfg register.
>>>> If someone else confirms that it solves the problem for them, this
>>>> option could be enabled automatically if certain motherboard chipset is
>>>> detected.
>>>
>>> We'd have to somehow narrow down which chipsets were involved, which
>>> might be a hard task. Do you have an idea of how much the performance is
>>> hurt by these workarounds? If it's not a lot, it might make sense to do
>>> it by default.
>>
>> After setting cache line size to 0, write speed as shown by 'dd
>> if=/tmpfs/testfile of=/dev/sdc9 bs=1M count=256' goes down from about
>> 45 MB/s to 17 MB/s. Personally I don't care about performance,
>> reliability and data safety are more important to me.
> 
> Yeah, cutting performance by 2/3rds is fairly bad though.

Yes, it's probably not a good thing to do by default for everyone.

>> The other workaround is to increase cache line size to 64 bytes, if
>> necessary, and set fifo_cfg to 0. No difference in performance measured.
>> This workaround is more of a hit or miss. It seems to contradict that
>> code commit made back in 2005, which was also about data corruption. In
>> the worst case, what solves data corruption problem on some motherboards
>> might introduce this problem on some other motherboards.
> 
> That's possible, which is why I suspect that someone from Silicon Image
> would have to confirm a possible fix - might be hard to get their
> attention about this old chipset..

I still recommend, for a start, a kernel module option, along with a message
in dmesg. (If you haven't seen it yet, the code diff in my last message
shows a possible way to do this.)

Re: sata_sil data corruption, possible workarounds

[bl0]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

On Tuesday 18 December 2012 16:23, bl0 wrote:

> On Monday 17 December 2012 06:44, Robert Hancock wrote:
> 
>> But it seems quite likely that
>> whatever magic numbers this code is picking don't work on your system
>> for some reason. It appears the root cause is likely a bug in the SiI
>> chip. There shouldn't be any region why messing around with these values
>> should cause data corruption other than that.
> 
> Do you think something should be done about it in the linux sata_sil
> driver? For a lack of a better solution, here is my suggestion. There is
> already one option 'slow_down' for problematic disks. Another option, for
> example 'cache_line_workaround', could be added for problematic
> motherboards. If enabled, the most straightforward way is to set cache
> line size to 0 and not worry about the fifo_cfg register.

Here is the code I currently have, attached as a diff. (This diff is not
against the latest git tree, it's against an older linux version which I
use.)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sata_sil.diff --]
[-- Type: text/x-diff; name="sata_sil.diff", Size: 4619 bytes --]

--- linux-2.6.27.27/drivers/ata/sata_sil.0.c	2009-07-20 05:45:22.000000000 +0200
+++ linux-2.6.27.27/drivers/ata/sata_sil.c	2012-12-24 10:09:51.000000000 +0100
@@ -246,17 +246,25 @@
 
 static int slow_down;
 module_param(slow_down, int, 0444);
 MODULE_PARM_DESC(slow_down, "Sledgehammer used to work around random problems, by limiting commands to 15 sectors (0=off, 1=on)");
 
+static int cache_line_workaround = -1;
+module_param(cache_line_workaround, int, 0444);
+MODULE_PARM_DESC(cache_line_workaround, "Work around data corruption problem on some motherboards (0 to disable, 1 or 2 to enable different workarounds)");
+
 
 static unsigned char sil_get_device_cache_line(struct pci_dev *pdev)
 {
 	u8 cache_line = 0;
 	pci_read_config_byte(pdev, PCI_CACHE_LINE_SIZE, &cache_line);
 	return cache_line;
 }
+static void sil_set_device_cache_line(struct pci_dev *pdev, u8 val)
+{
+	pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, val);
+}
 
 /**
  *	sil_set_mode		-	wrap set_mode functions
  *	@link: link to set up
  *	@r_failed: returned device when we fail
@@ -555,30 +563,100 @@
 		dev->udma_mask &= ATA_UDMA5;
 		return;
 	}
 }
 
+static int sil_detect_problematic_chipset(void)
+{
+	/* it could be added later, i wouldn't worry about it now */
+	return 0;
+}
+
+static int sil_get_cache_line_workaround(struct pci_dev *pdev)
+{
+	if (cache_line_workaround != -1) { /* set by user */
+		dev_printk(KERN_INFO, &pdev->dev,
+			"cache_line_workaround=%d given\n", cache_line_workaround);
+		return cache_line_workaround;
+	} else if (slow_down) {
+		/* users have set slow_down because they have experienced problems. lower performance is expected. maybe it makes sense to enable it for them. */
+		dev_printk(KERN_INFO, &pdev->dev,
+			"slow_down given, using cache_line_workaround=1\n");
+		return 1;
+	} else if (sil_detect_problematic_chipset()) {
+		dev_printk(KERN_INFO, &pdev->dev,
+			"problematic motherboard chipset detected, using cache_line_workaround=1\n");
+		return 1;
+	} else {
+		/* default to old behavior */
+		return 0;
+	}
+}
+
 static void sil_init_controller(struct ata_host *host)
 {
 	struct pci_dev *pdev = to_pci_dev(host->dev);
 	void __iomem *mmio_base = host->iomap[SIL_MMIO_BAR];
+	int clw;
 	u8 cls;
+	u8 fifo_cfg_val;
 	u32 tmp;
 	int i;
 
 	/* Initialize FIFO PCI bus arbitration */
-	cls = sil_get_device_cache_line(pdev);
-	if (cls) {
-		cls >>= 3;
-		cls++;  /* cls = (line_size/8)+1 */
-		for (i = 0; i < host->n_ports; i++)
-			writew(cls << 8 | cls,
-			       mmio_base + sil_port[i].fifo_cfg);
-	} else
-		dev_printk(KERN_WARNING, &pdev->dev,
-			   "cache line size not set.  Driver may not function\n");
-
+	clw = sil_get_cache_line_workaround(pdev);
+	switch (clw) {
+	case 0:
+		dev_printk(KERN_WARNING, &pdev->dev,
+			"Data corruption is known to occur in combination with certain motherboards. If you encounter it, try cache_line_workaround=1 parameter.\n");
+		cls = sil_get_device_cache_line(pdev);
+		if (cls) {
+			dev_printk(KERN_DEBUG, &pdev->dev,
+				"cache line size: %d bytes\n", cls*4);
+			fifo_cfg_val = cls/8 + 1;
+			if (fifo_cfg_val > 7) fifo_cfg_val = 7; /* don't go over the maximum value */
+			dev_printk(KERN_DEBUG, &pdev->dev,
+				"setting fifo_cfg to %d\n", fifo_cfg_val);
+			for (i = 0; i < host->n_ports; i++) {
+				writew(fifo_cfg_val << 8 | fifo_cfg_val,
+				       mmio_base + sil_port[i].fifo_cfg);
+			}
+		} else {
+			dev_printk(KERN_INFO, &pdev->dev,
+				"cache line size not set\n");
+		}
+	break;
+	case 1:
+		dev_printk(KERN_INFO, &pdev->dev,
+			"cache_line_workaround=1, setting cache line size to 0\n");
+		sil_set_device_cache_line(pdev, 0);
+	break;
+	case 2:
+		dev_printk(KERN_INFO, &pdev->dev,
+			"cache_line_workaround=2. Increasing cache line size to 64 bytes, if necessary, and setting fifo_cfg to 0.\n");
+		cls = sil_get_device_cache_line(pdev);
+		dev_printk(KERN_DEBUG, &pdev->dev,
+			"current CLS: %d bytes\n", cls*4);
+		if (cls < 0x10) {
+			dev_printk(KERN_DEBUG, &pdev->dev,
+				"increasing CLS to 64 bytes\n");
+			sil_set_device_cache_line(pdev, 0x10);
+		} else {
+			dev_printk(KERN_DEBUG, &pdev->dev,
+				"keeping current CLS\n");
+		}
+		dev_printk(KERN_DEBUG, &pdev->dev,
+			"setting fifo_cfg to 0\n");
+		for (i = 0; i < host->n_ports; i++) {
+			writew(0, mmio_base + sil_port[i].fifo_cfg);
+		}
+	break;
+	default:
+		dev_printk(KERN_ERR, &pdev->dev,
+			"invalid cache_line_workaround: %d\n", clw);
+	}
+	
 	/* Apply R_ERR on DMA activate FIS errata workaround */
 	if (host->ports[0]->flags & SIL_FLAG_RERR_ON_DMA_ACT) {
 		int cnt;
 
 		for (i = 0, cnt = 0; i < host->n_ports; i++) {

Re: sata_sil data corruption, possible workarounds

[Robert Hancock]

On 12/24/2012 08:37 AM, bl0 wrote:
> On Tuesday 18 December 2012 16:23, bl0 wrote:
>
>> On Monday 17 December 2012 06:44, Robert Hancock wrote:
>>
>>> But it seems quite likely that
>>> whatever magic numbers this code is picking don't work on your system
>>> for some reason. It appears the root cause is likely a bug in the SiI
>>> chip. There shouldn't be any region why messing around with these values
>>> should cause data corruption other than that.
>>
>> Do you think something should be done about it in the linux sata_sil
>> driver? For a lack of a better solution, here is my suggestion. There is
>> already one option 'slow_down' for problematic disks. Another option, for
>> example 'cache_line_workaround', could be added for problematic
>> motherboards. If enabled, the most straightforward way is to set cache
>> line size to 0 and not worry about the fifo_cfg register.
>
> Here is the code I currently have, attached as a diff. (This diff is not
> against the latest git tree, it's against an older linux version which I
> use.)

I wouldn't mind something like this as an option, anyway. Jeff, Tejun, 
thoughts?

Re: sata_sil data corruption, possible workarounds

[Tejun Heo]

Hello, Robert, bl0 (would be cool if this were your real name :)

On Tue, Jan 08, 2013 at 10:48:43PM -0600, Robert Hancock wrote:
> >>Do you think something should be done about it in the linux sata_sil
> >>driver? For a lack of a better solution, here is my suggestion. There is
> >>already one option 'slow_down' for problematic disks. Another option, for
> >>example 'cache_line_workaround', could be added for problematic
> >>motherboards. If enabled, the most straightforward way is to set cache
> >>line size to 0 and not worry about the fifo_cfg register.
> >
> >Here is the code I currently have, attached as a diff. (This diff is not
> >against the latest git tree, it's against an older linux version which I
> >use.)
> 
> I wouldn't mind something like this as an option, anyway. Jeff,
> Tejun, thoughts?

I don't know.  This thread makes me want to eat obsessivly, cry in
fetal position and then puke and wet my pants.

The issue has been there from the beginning.  The current code seems
to cope with, I hope, majority of configurations; unfortunately, for
the problematic ones, there hasn't been any sensible explanation or
reliable way to detect what is causing the problem.  I don't even know
how we should blacklist them.  Is it chipset-specific or
system-specific?  ie. Should we blacklist north/south bridges or DMI
system identifiers?

Having a module option is easy and can be helpful for the very rare
cases where the admin is aware of the issue, but, in general, it
doesn't really help that much.

So, let's throw in a module option.  Other than that, no idea.

\pukes

-- 
tejun

Re: sata_sil data corruption, possible workarounds

[bl0]

On Wednesday 09 January 2013 20:17:53 Tejun Heo wrote:
> Hello, Robert, bl0 (would be cool if this were your real name :)
>
> On Tue, Jan 08, 2013 at 10:48:43PM -0600, Robert Hancock wrote:
> > I wouldn't mind something like this as an option, anyway. Jeff,
> > Tejun, thoughts?
>
> I don't know.  This thread makes me want to eat obsessivly, cry in
> fetal position and then puke and wet my pants.
>
> The issue has been there from the beginning.  The current code seems
> to cope with, I hope, majority of configurations; unfortunately, for
> the problematic ones, there hasn't been any sensible explanation or
> reliable way to detect what is causing the problem.  I don't even know
> how we should blacklist them.  Is it chipset-specific or
> system-specific?  ie. Should we blacklist north/south bridges or DMI
> system identifiers?
>
> Having a module option is easy and can be helpful for the very rare
> cases where the admin is aware of the issue, but, in general, it
> doesn't really help that much.
>
> So, let's throw in a module option.  Other than that, no idea.
>
> \pukes

Thanks for bringing much needed enthusiasm into this thread. I too would 
like to know the root cause of this problem. Since the chances of finding 
it are very low, the only thing that remains is to deal with data corruption 
problem using the means available.

Some people asked for help and posted relevant entries from their logs. Now 
they would be able to find a hint about the module option in the logs. Some 
people have set the slow_down option. The code i posted enables the 
workaround for them too.

As for blacklisting, I was thinking about maybe looking at the vendor and 
device ID of the PCI bridge under which the sata_sil device is found.

Re: sata_sil data corruption, possible workarounds

[Mark Lord]

On 13-01-09 02:17 PM, Tejun Heo wrote:
>
> Having a module option is easy and can be helpful for the very rare
> cases where the admin is aware of the issue, but, in general, it
> doesn't really help that much.

How about a sysfs attribute, rather than a boot option?

Re: sata_sil data corruption, possible workarounds

[Mark Lord]

On 13-01-11 08:53 AM, Mark Lord wrote:
> On 13-01-09 02:17 PM, Tejun Heo wrote:
>>
>> Having a module option is easy and can be helpful for the very rare
>> cases where the admin is aware of the issue, but, in general, it
>> doesn't really help that much.
> 
> How about a sysfs attribute, rather than a boot option?

NAK that idea.. a boot-time module flag is the best way.

Re: sata_sil data corruption, possible workarounds

[Jeff Garzik]

On 01/11/2013 08:54 AM, Mark Lord wrote:
> On 13-01-11 08:53 AM, Mark Lord wrote:
>> On 13-01-09 02:17 PM, Tejun Heo wrote:
>>>
>>> Having a module option is easy and can be helpful for the very rare
>>> cases where the admin is aware of the issue, but, in general, it
>>> doesn't really help that much.
>>
>> How about a sysfs attribute, rather than a boot option?
>
> NAK that idea.. a boot-time module flag is the best way.

Module options appear in sysfs anyway.

If the issue continues to be seen, a knob to enable an ugly solution is 
better than nothing, from the user's perspective.

	Jeff

Re: sata_sil data corruption, possible workarounds

[bl0]

On Monday 14 January 2013 18:58:55 Jeff Garzik wrote:
> If the issue continues to be seen, 

You mean, more than it has been seen already?

> a knob to enable an ugly solution is 
> better than nothing, from the user's perspective.

Also, I fail to see how this is any more ugly than what the current code 
does.