r8169 mac reading/writing broken

r8169 mac reading/writing broken

[Timo Teräs]

It seems that my r8169 mac address is lost when r8169 module is reloaded,
or system is soft rebooted. Hard power reset recovers the mac address again.
The commit cc098dc705895f6b0109b7e8e026ac2b8ae1c0a1 (r8169: restore mac addr
in rtl8169_remove_one and rtl_shutdown) seems to have broken it. The first four
bytes of mac address go zeroes because of this.

I did some more testing, and added debugging info to rtl_rar_set(). It would
appear that even if I write any mac address (with ifconfig) and reread the
MAC0..MAC5 register, the first four bytes get zeroed. So it would sounds like
the hardware is faulty, or that the rtl_rar_set function is buggy.

Any suggestions to fix this?

The hardware in question is a 3-in-1 NIC. Each three PCI devices shows up as
something like:

00:09.0 Ethernet controller [0200]: Realtek Semiconductor Co., Ltd. RTL-8110SC/8169SC Gigabit Ethernet [10ec:8167] (rev 10)
	Subsystem: Jetway Information Co., Ltd. Device [16f3:10ec]
	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: 64 (8000ns min, 16000ns max), Cache Line Size: 32 bytes
	Interrupt: pin A routed to IRQ 18
	Region 0: I/O ports at f200 [size=256]
	Region 1: Memory at fdfff000 (32-bit, non-prefetchable) [size=256]
	[virtual] Expansion ROM at 3c000000 [disabled] [size=128K]
	Capabilities: [dc] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0-,D1+,D2+,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: r8169

And the kernel drivers prints the following at startup:

r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded
r8169 0000:00:09.0: PCI->APIC IRQ transform: INT A -> IRQ 18
r8169 0000:00:09.0: no PCI Express capability
eth0: RTL8169sc/8110sc at 0xf81ae000, 00:30:18:a6:2b:6c, XID 18000000 IRQ 18
r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded
r8169 0000:00:0b.0: PCI->APIC IRQ transform: INT A -> IRQ 19
r8169 0000:00:0b.0: no PCI Express capability
eth1: RTL8169sc/8110sc at 0xf81b2000, 00:30:18:a6:2b:6d, XID 18000000 IRQ 19
r8169 Gigabit Ethernet driver 2.3LK-NAPI loaded
r8169 0000:00:0c.0: PCI->APIC IRQ transform: INT A -> IRQ 16
r8169 0000:00:0c.0: no PCI Express capability
eth2: RTL8169sc/8110sc at 0xf81b6000, 00:30:18:a6:2b:6e, XID 18000000 IRQ 16

- Timo

Re: r8169 mac reading/writing broken

[François Romieu]

Timo Teräs <timo.teras@iki.fi> :
[...]
> I did some more testing, and added debugging info to rtl_rar_set(). It would
> appear that even if I write any mac address (with ifconfig) and reread the
> MAC0..MAC5 register, the first four bytes get zeroed. So it would sounds like
> the hardware is faulty, or that the rtl_rar_set function is buggy.
> 
> Any suggestions to fix this ?

Try something like the patch below and please send a complete lspci -vvv.

I wonder what the bus controler looks like.

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 9d3ebf3..5db357a 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -2814,6 +2814,7 @@ static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
 	void __iomem *ioaddr = tp->mmio_addr;
 	u32 high;
 	u32 low;
+	int i;
 
 	low  = addr[0] | (addr[1] << 8) | (addr[2] << 16) | (addr[3] << 24);
 	high = addr[4] | (addr[5] << 8);
@@ -2822,7 +2823,17 @@ static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
 
 	RTL_W8(Cfg9346, Cfg9346_Unlock);
 	RTL_W32(MAC0, low);
-	RTL_W32(MAC4, high);
+	for (i = 0; i < 16; i++) {
+		u32 read;
+
+		RTL_W32(MAC4, high);
+		read = RTL_R32(MAC4);
+		if (read != high) {
+			printk(KERN_ERR PFX
+			       "failure %02d: read = 0x%08x, write = 0x%08x\n",
+				i, read, high);
+		}
+	}
 	RTL_W8(Cfg9346, Cfg9346_Lock);
 
 	spin_unlock_irq(&tp->lock);

Re: r8169 mac reading/writing broken

[Timo Teräs]

François Romieu wrote:
> Timo Teräs <timo.teras@iki.fi> :
> [...]
>> I did some more testing, and added debugging info to rtl_rar_set(). It would
>> appear that even if I write any mac address (with ifconfig) and reread the
>> MAC0..MAC5 register, the first four bytes get zeroed. So it would sounds like
>> the hardware is faulty, or that the rtl_rar_set function is buggy.
>>
>> Any suggestions to fix this ?
> 
> Try something like the patch below and please send a complete lspci -vvv.

Attached at the end.

> I wonder what the bus controler looks like.
> 
> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> index 9d3ebf3..5db357a 100644
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
> @@ -2814,6 +2814,7 @@ static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
>  	void __iomem *ioaddr = tp->mmio_addr;
>  	u32 high;
>  	u32 low;
> +	int i;
>  
>  	low  = addr[0] | (addr[1] << 8) | (addr[2] << 16) | (addr[3] << 24);
>  	high = addr[4] | (addr[5] << 8);
> @@ -2822,7 +2823,17 @@ static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
>  
>  	RTL_W8(Cfg9346, Cfg9346_Unlock);
>  	RTL_W32(MAC0, low);
> -	RTL_W32(MAC4, high);
> +	for (i = 0; i < 16; i++) {
> +		u32 read;
> +
> +		RTL_W32(MAC4, high);
> +		read = RTL_R32(MAC4);
> +		if (read != high) {
> +			printk(KERN_ERR PFX
> +			       "failure %02d: read = 0x%08x, write = 0x%08x\n",
> +				i, read, high);
> +		}
> +	}
>  	RTL_W8(Cfg9346, Cfg9346_Lock);
>  
>  	spin_unlock_irq(&tp->lock);

I don't think this would do anything. The high part is recorded correctly always.
It's the 'low' part that gets discarded. I can do similar test if writing it
more times will help. Will post results soon.

- Timo

00:00.0 Host bridge: VIA Technologies, Inc. CN700/VN800/P4M800CE/Pro Host Bridge
	Subsystem: VIA Technologies, Inc. CN700/VN800/P4M800CE/Pro Host Bridge
	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: 8
	Region 0: Memory at e8000000 (32-bit, prefetchable) [size=128M]
	Capabilities: [80] AGP version 3.5
		Status: RQ=8 Iso- ArqSz=0 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=<none>
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: agpgart-via

00:00.1 Host bridge: VIA Technologies, Inc. CN700/VN800/P4M800CE/Pro Host Bridge
	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: 0

00:00.2 Host bridge: VIA Technologies, Inc. CN700/VN800/P4M800CE/Pro Host Bridge
	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: 0

00:00.3 Host bridge: VIA Technologies, Inc. PT890 Host Bridge
	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: 0

00:00.4 Host bridge: VIA Technologies, Inc. CN700/VN800/P4M800CE/Pro Host Bridge
	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: 0

00:00.7 Host bridge: VIA Technologies, Inc. CN700/VN800/P4M800CE/Pro Host Bridge
	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: 0

00:01.0 PCI bridge: VIA Technologies, Inc. VT8237/VX700 PCI Bridge (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: 0
	Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
	I/O behind bridge: 0000d000-0000dfff
	Memory behind bridge: fb000000-fcffffff
	Prefetchable memory behind bridge: f4000000-f7ffffff
	Secondary status: 66MHz+ FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort+ <SERR+ <PERR+
	BridgeCtl: Parity- SERR- NoISA- VGA+ MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: [70] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-

00:09.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL-8110SC/8169SC Gigabit Ethernet (rev 10)
	Subsystem: Jetway Information Co., Ltd. Device 10ec
	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: 64 (8000ns min, 16000ns max), Cache Line Size: 32 bytes
	Interrupt: pin A routed to IRQ 18
	Region 0: I/O ports at f200 [size=256]
	Region 1: Memory at fdfff000 (32-bit, non-prefetchable) [size=256]
	[virtual] Expansion ROM at 3c000000 [disabled] [size=128K]
	Capabilities: [dc] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0-,D1+,D2+,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: r8169

00:0a.0 FireWire (IEEE 1394): VIA Technologies, Inc. VT6306 Fire II IEEE 1394 OHCI Link Layer Controller (rev 80) (prog-if 10 [OHCI])
	Subsystem: VIA Technologies, Inc. VT6306 Fire II IEEE 1394 OHCI Link Layer Controller
	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 (8000ns max), Cache Line Size: 64 bytes
	Interrupt: pin A routed to IRQ 19
	Region 0: Memory at fdffe000 (32-bit, non-prefetchable) [size=2K]
	Region 1: I/O ports at ff00 [size=128]
	Capabilities: [50] Power Management version 2
		Flags: PMEClk- DSI- D1- D2+ AuxCurrent=0mA PME(D0-,D1-,D2+,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: firewire_ohci

00:0b.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL-8110SC/8169SC Gigabit Ethernet (rev 10)
	Subsystem: Jetway Information Co., Ltd. Device 10ec
	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: 64 (8000ns min, 16000ns max), Cache Line Size: 32 bytes
	Interrupt: pin A routed to IRQ 19
	Region 0: I/O ports at f000 [size=256]
	Region 1: Memory at fdffd000 (32-bit, non-prefetchable) [size=256]
	[virtual] Expansion ROM at 3c020000 [disabled] [size=128K]
	Capabilities: [dc] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0-,D1+,D2+,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: r8169

00:0c.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL-8110SC/8169SC Gigabit Ethernet (rev 10)
	Subsystem: Jetway Information Co., Ltd. Device 10ec
	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: 64 (8000ns min, 16000ns max), Cache Line Size: 32 bytes
	Interrupt: pin A routed to IRQ 16
	Region 0: I/O ports at ec00 [size=256]
	Region 1: Memory at fdffc000 (32-bit, non-prefetchable) [size=256]
	[virtual] Expansion ROM at 3c040000 [disabled] [size=128K]
	Capabilities: [dc] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0-,D1+,D2+,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: r8169

00:0f.0 IDE interface: VIA Technologies, Inc. VIA VT6420 SATA RAID Controller (rev 80) (prog-if 8f [Master SecP SecO PriP PriO])
	Subsystem: VIA Technologies, Inc. VIA VT6420 SATA RAID Controller
	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
	Interrupt: pin B routed to IRQ 20
	Region 0: I/O ports at fe00 [size=8]
	Region 1: I/O ports at fd00 [size=4]
	Region 2: I/O ports at fc00 [size=8]
	Region 3: I/O ports at fb00 [size=4]
	Region 4: I/O ports at fa00 [size=16]
	Region 5: I/O ports at ee00 [size=256]
	Capabilities: [c0] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: sata_via

00:0f.1 IDE interface: VIA Technologies, Inc. VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if 8a [Master SecP PriP])
	Subsystem: VIA Technologies, Inc. VT82C586/B/VT82C686/A/B/VT8233/A/C/VT8235 PIPC Bus Master IDE
	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
	Interrupt: pin A routed to IRQ 20
	Region 0: [virtual] Memory at 000001f0 (32-bit, non-prefetchable) [size=8]
	Region 1: [virtual] Memory at 000003f0 (type 3, non-prefetchable) [size=1]
	Region 2: [virtual] Memory at 00000170 (32-bit, non-prefetchable) [size=8]
	Region 3: [virtual] Memory at 00000370 (type 3, non-prefetchable) [size=1]
	Region 4: I/O ports at f900 [size=16]
	Capabilities: [c0] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: pata_via

00:10.0 USB Controller: VIA Technologies, Inc. VT82xxxxx UHCI USB 1.1 Controller (rev 81) (prog-if 00 [UHCI])
	Subsystem: VIA Technologies, Inc. VT82xxxxx UHCI USB 1.1 Controller
	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: 64 bytes
	Interrupt: pin A routed to IRQ 21
	Region 4: I/O ports at f800 [size=32]
	Capabilities: [80] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: uhci_hcd

00:10.1 USB Controller: VIA Technologies, Inc. VT82xxxxx UHCI USB 1.1 Controller (rev 81) (prog-if 00 [UHCI])
	Subsystem: VIA Technologies, Inc. VT82xxxxx UHCI USB 1.1 Controller
	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: 64 bytes
	Interrupt: pin A routed to IRQ 21
	Region 4: I/O ports at f700 [size=32]
	Capabilities: [80] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: uhci_hcd

00:10.2 USB Controller: VIA Technologies, Inc. VT82xxxxx UHCI USB 1.1 Controller (rev 81) (prog-if 00 [UHCI])
	Subsystem: VIA Technologies, Inc. VT82xxxxx UHCI USB 1.1 Controller
	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: 64 bytes
	Interrupt: pin B routed to IRQ 21
	Region 4: I/O ports at f600 [size=32]
	Capabilities: [80] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: uhci_hcd

00:10.3 USB Controller: VIA Technologies, Inc. VT82xxxxx UHCI USB 1.1 Controller (rev 81) (prog-if 00 [UHCI])
	Subsystem: VIA Technologies, Inc. VT82xxxxx UHCI USB 1.1 Controller
	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: 64 bytes
	Interrupt: pin B routed to IRQ 21
	Region 4: I/O ports at f500 [size=32]
	Capabilities: [80] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: uhci_hcd

00:10.4 USB Controller: VIA Technologies, Inc. USB 2.0 (rev 86) (prog-if 20 [EHCI])
	Subsystem: VIA Technologies, Inc. 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, Cache Line Size: 64 bytes
	Interrupt: pin C routed to IRQ 21
	Region 0: Memory at fdffb000 (32-bit, non-prefetchable) [size=256]
	Capabilities: [80] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: ehci_hcd

00:11.0 ISA bridge: VIA Technologies, Inc. VT8237 ISA bridge [KT600/K8T800/K8T890 South]
	Subsystem: VIA Technologies, Inc. DFI KT600-AL / Soltek SL-B9D-FGR Motherboard
	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: 0
	Capabilities: [c0] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-

00:11.5 Multimedia audio controller: VIA Technologies, Inc. VT8233/A/8235/8237 AC97 Audio Controller (rev 60)
	Subsystem: Jetway Information Co., Ltd. Device 4170
	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-
	Interrupt: pin C routed to IRQ 22
	Region 0: I/O ports at ea00 [size=256]
	Capabilities: [c0] Power Management version 2
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: VIA 82xx Audio

00:12.0 Ethernet controller: VIA Technologies, Inc. VT6102 [Rhine-II] (rev 78)
	Subsystem: VIA Technologies, Inc. VT6102 [Rhine II] Embeded Ethernet Controller on VT8235
	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 (750ns min, 2000ns max), Cache Line Size: 64 bytes
	Interrupt: pin A routed to IRQ 23
	Region 0: I/O ports at e800 [size=256]
	Region 1: Memory at fdffa000 (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 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Kernel driver in use: via-rhine

01:00.0 VGA compatible controller: VIA Technologies, Inc. CN700/P4M800 Pro/P4M800 CE/VN800 [S3 UniChrome Pro] (rev 01) (prog-if 00 [VGA controller])
	Subsystem: VIA Technologies, Inc. CN700/P4M800 Pro/P4M800 CE/VN800 [S3 UniChrome Pro]
	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)
	Interrupt: pin A routed to IRQ 5
	Region 0: Memory at f4000000 (32-bit, prefetchable) [size=64M]
	Region 1: Memory at fb000000 (32-bit, non-prefetchable) [size=16M]
	[virtual] Expansion ROM at fc000000 [disabled] [size=64K]
	Capabilities: [60] Power Management version 2
		Flags: PMEClk- DSI+ D1+ D2+ AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [70] AGP version 3.0
		Status: RQ=256 Iso- ArqSz=0 Cal=7 SBA+ ITACoh- GART64- HTrans- 64bit- FW- AGP3+ Rate=x4,x8
		Command: RQ=1 ArqSz=0 Cal=0 SBA+ AGP- GART64- 64bit- FW- Rate=<none>

Re: r8169 mac reading/writing broken

[François Romieu]

Timo Teräs <timo.teras@iki.fi> :
[...]
> I don't think this would do anything. The high part is recorded correctly always.
> It's the 'low' part that gets discarded. I can do similar test if writing it
> more times will help. Will post results soon.

You may check whether writing MAC4 before MAC0 makes a difference
or not as well.

-- 
Ueimor

Re: r8169 mac reading/writing broken

[Timo Teräs]

François Romieu wrote:
> Timo Teräs <timo.teras@iki.fi> :
> [...]
>> I don't think this would do anything. The high part is recorded correctly always.
>> It's the 'low' part that gets discarded. I can do similar test if writing it
>> more times will help. Will post results soon.
> 
> You may check whether writing MAC4 before MAC0 makes a difference
> or not as well.

It seems that adding single printk between writing MAC0 and MAC4 fixes it.
I guess it needs a bit of delay between the writes or something.

Re: r8169 mac reading/writing broken

[Timo Teräs]

Timo Teräs wrote:
> François Romieu wrote:
>> Timo Teräs <timo.teras@iki.fi> :
>> [...]
>>> I don't think this would do anything. The high part is recorded 
>>> correctly always.
>>> It's the 'low' part that gets discarded. I can do similar test if 
>>> writing it
>>> more times will help. Will post results soon.
>>
>> You may check whether writing MAC4 before MAC0 makes a difference
>> or not as well.
> 
> It seems that adding single printk between writing MAC0 and MAC4 fixes it.
> I guess it needs a bit of delay between the writes or something.

Oh, and writing MAC4 first seems to fix it too.

- Timo

Re: r8169 mac reading/writing broken

[François Romieu]

Timo Teräs <timo.teras@iki.fi> :
[...]
> It seems that adding single printk between writing MAC0 and MAC4 fixes it.
> I guess it needs a bit of delay between the writes or something.

Can you test with a single RTL_R32 after each MACx write ?

-- 
Ueimor

Re: r8169 mac reading/writing broken

[Timo Teräs]

François Romieu wrote:
> Timo Teräs <timo.teras@iki.fi> :
> [...]
>> It seems that adding single printk between writing MAC0 and MAC4 fixes it.
>> I guess it needs a bit of delay between the writes or something.
> 
> Can you test with a single RTL_R32 after each MACx write ?

Adding reading back of the written value fixes it too. Though, disassembly
says that it added an extra instructions also (needs to load the 'high' from
stack before writing it) so the added delay is probably slightly more than
just the io read.

Re: r8169 mac reading/writing broken

[Timo Teräs]

Timo Teräs wrote:
> François Romieu wrote:
>> Timo Teräs <timo.teras@iki.fi> :
>> [...]
>>> It seems that adding single printk between writing MAC0 and MAC4 
>>> fixes it.
>>> I guess it needs a bit of delay between the writes or something.
>>
>> Can you test with a single RTL_R32 after each MACx write ?
> 
> Adding reading back of the written value fixes it too. Though,
> disassembly says that it added an extra instructions also (needs to
> load the 'high' from stack before writing it) so the added delay is
> probably slightly more than just the io read.

I'm not too familiar with PCI details, but this smells a bit
like that write-combining is happening and the NIC does not like
that.

Any ideas how to check this?

The system experiencing this is a "VIA Eden 1.2Ghz" box.

Or is swapping MAC0/MAC4 writes, or adding the extra read an
acceptable fix/workaround?

- Timo

Re: r8169 mac reading/writing broken

[François Romieu]

Timo Teräs <timo.teras@iki.fi> :
[...]
> Any ideas how to check this?

Check the datasheet of VIA's chipset for a WC control bit - there
ought to be one - and disable it.

> Or is swapping MAC0/MAC4 writes, or adding the extra read an
> acceptable fix/workaround?

swapping should reliably disable WC. It would be fine.

-- 
Ueimor

Re: r8169 mac reading/writing broken

[Ben Hutchings]

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

On Sat, 2010-03-27 at 22:11 +0100, François Romieu wrote:
> Timo Teräs <timo.teras@iki.fi> :
> [...]
> > Any ideas how to check this?
> 
> Check the datasheet of VIA's chipset for a WC control bit - there
> ought to be one - and disable it.
> 
> > Or is swapping MAC0/MAC4 writes, or adding the extra read an
> > acceptable fix/workaround?
> 
> swapping should reliably disable WC. It would be fine.

This bug was also reported by a Debian user in
<http://bugs.debian.org/573007>, also using a VIA chipset.

This sort of behaviour has been seen before with 64-bit registers
written in two 32-bit chunks, on some ARM platforms.  You worked around
that for the descriptor pointers with:

ommit b39fe41f481d20c201012e4483e76c203802dda7
Author: Francois Romieu <romieu@fr.zoreil.com>
Date:   Mon Sep 11 20:10:58 2006 +0200

    r8169: quirk for the 8110sb on arm platform

A similar problem seems to afflict the multicast hash register on this
platform - see <http://bugs.debian.org/407217>, and sorry I didn't
report this earlier when I got confirmation of my hypothesis.

I wonder whether there are special rules that need to be followed for
updating such registers and which the driver is not following, or a more
general bug in the Realtek chips that should be consistently
worked-around for all 64-bit registers.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: r8169 mac reading/writing broken

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Sat, 27 Mar 2010 23:20:54 +0000

> I wonder whether there are special rules that need to be followed
> for updating such registers and which the driver is not following,
> or a more general bug in the Realtek chips that should be
> consistently worked-around for all 64-bit registers.

I suspect that MMIO to 64-bit registers in 32-bit chunks is not
reliable with these parts, given all of the information we have so
far.

[PATCH] r8169: fix broken register writes

[François Romieu]

This is quite similar to b39fe41f481d20c201012e4483e76c203802dda7
though said registers are not even documented as 64-bit registers
- as opposed to the initial TxDescStartAddress ones - but as single
bytes which must be combined into 32 bits at the MMIO read/write
level before being merged into a 64 bit logical entity.

Credits go to Ben Hutchings <ben@decadent.org.uk> for the MAR
registers (aka "multicast is broken for ages on ARM) and to
Timo Teräs <timo.teras@iki.fi> for the MAC registers.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/r8169.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 9d3ebf3..966407c 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -2821,8 +2821,8 @@ static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
 	spin_lock_irq(&tp->lock);
 
 	RTL_W8(Cfg9346, Cfg9346_Unlock);
-	RTL_W32(MAC0, low);
 	RTL_W32(MAC4, high);
+	RTL_W32(MAC0, low);
 	RTL_W8(Cfg9346, Cfg9346_Lock);
 
 	spin_unlock_irq(&tp->lock);
@@ -4754,8 +4754,8 @@ static void rtl_set_rx_mode(struct net_device *dev)
 		mc_filter[1] = swab32(data);
 	}
 
-	RTL_W32(MAR0 + 0, mc_filter[0]);
 	RTL_W32(MAR0 + 4, mc_filter[1]);
+	RTL_W32(MAR0 + 0, mc_filter[0]);
 
 	RTL_W32(RxConfig, tmp);
 
-- 
1.6.6.1

Re: [PATCH] r8169: fix broken register writes

[Ben Hutchings]

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

On Sun, 2010-03-28 at 02:31 +0100, François Romieu wrote:
> This is quite similar to b39fe41f481d20c201012e4483e76c203802dda7
> though said registers are not even documented as 64-bit registers
> - as opposed to the initial TxDescStartAddress ones - but as single
> bytes which must be combined into 32 bits at the MMIO read/write
> level before being merged into a 64 bit logical entity.
[...]

Thanks François.  Which hardware have you tested this on so far?  I was
hesitant to make changes because of the huge number of variants.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] r8169: fix broken register writes

[François Romieu]

Ben Hutchings <ben@decadent.org.uk> :
[...]
> Thanks François.  Which hardware have you tested this on so far ?

10ec:8169 (rev 10) / RTL8169sb/8110sb / XID 10000000

Timo's is a 10ec:8167 / RTL8169sc/8110sc / XID 18000000. He only tested
the MAC[04] part.

Old stuff.

> I was hesitant to make changes because of the huge number of variants.

Things will be amended if they break and reverted if they get out of
control.

-- 
Ueimor

Re: [PATCH] r8169: fix broken register writes

[Al Viro]

On Sun, Mar 28, 2010 at 11:28:58PM +0200, Fran?ois Romieu wrote:
> Ben Hutchings <ben@decadent.org.uk> :
> [...]
> > Thanks Fran?ois.  Which hardware have you tested this on so far ?
> 
> 10ec:8169 (rev 10) / RTL8169sb/8110sb / XID 10000000
> 
> Timo's is a 10ec:8167 / RTL8169sc/8110sc / XID 18000000. He only tested
> the MAC[04] part.

FWIW, XID18000000 here (J7F4) loses MAC4 on shutdown; hadn't tested the patch
yet.  2.6.26 (on that box) and 2.6.31 (on identical mb) work, 2.6.33 doesn't.
I suspect that bisect would lead to commit cc098dc70 (i.e. the place where we
started to set address on shutdown).  One more data point: ifconfig hw ether
done under 2.6.26 did restore the address.  And that's the same function,
isn't it?

Another interesting bit: unlike the older kernel, grep for eth0 in .33 dmesg

eth0: RTL8169sc/8110sc at 0xf87fc000, 00:30:18:a4:65:89, XID 18000000 IRQ 18
r8169: eth0: link down
ADDRCONF(NETDEV_UP): eth0: link is not ready
r8169: eth0: link up
ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready

where with .26 (with the same userland) had

eth0: RTL8169sc/8110sc at 0xf87fc000, 00:30:18:a4:65:89, XID 18000000 IRQ 18
r8169: eth0: link up
ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready

Same for .31 on another box, modulo different address there...

Re: [PATCH] r8169: fix broken register writes

[Al Viro]

On Sun, Mar 28, 2010 at 11:19:24PM +0100, Al Viro wrote:

> > > Thanks Fran?ois.  Which hardware have you tested this on so far ?
> > 
> > 10ec:8169 (rev 10) / RTL8169sb/8110sb / XID 10000000
> > 
> > Timo's is a 10ec:8167 / RTL8169sc/8110sc / XID 18000000. He only tested
> > the MAC[04] part.
> 
> FWIW, XID18000000 here (J7F4) loses MAC4 on shutdown; hadn't tested the patch
> yet.  2.6.26 (on that box) and 2.6.31 (on identical mb) work, 2.6.33 doesn't.
> I suspect that bisect would lead to commit cc098dc70 (i.e. the place where we
> started to set address on shutdown).  One more data point: ifconfig hw ether
> done under 2.6.26 did restore the address.  And that's the same function,
> isn't it?

As the matter of fact, ifconfig eth0 hw ether .... ends up zeroing upper
32 bits on old kernels once in a while.

What orders accesses as seen by PCI bus in
        RTL_W8(Cfg9346, Cfg9346_Unlock);
        RTL_W32(MAC0, low);
        RTL_W32(MAC4, high);
        RTL_W8(Cfg9346, Cfg9346_Lock);
anyway, and don't we need mmiowb() or two in there?

Re: [PATCH] r8169: fix broken register writes

[Al Viro]

On Mon, Mar 29, 2010 at 02:03:46AM +0100, Al Viro wrote:
> On Sun, Mar 28, 2010 at 11:19:24PM +0100, Al Viro wrote:
> 
> > > > Thanks Fran?ois.  Which hardware have you tested this on so far ?
> > > 
> > > 10ec:8169 (rev 10) / RTL8169sb/8110sb / XID 10000000
> > > 
> > > Timo's is a 10ec:8167 / RTL8169sc/8110sc / XID 18000000. He only tested
> > > the MAC[04] part.
> > 
> > FWIW, XID18000000 here (J7F4) loses MAC4 on shutdown; hadn't tested the patch
> > yet.  2.6.26 (on that box) and 2.6.31 (on identical mb) work, 2.6.33 doesn't.
> > I suspect that bisect would lead to commit cc098dc70 (i.e. the place where we
> > started to set address on shutdown).  One more data point: ifconfig hw ether
> > done under 2.6.26 did restore the address.  And that's the same function,
> > isn't it?
> 
> As the matter of fact, ifconfig eth0 hw ether .... ends up zeroing upper
> 32 bits on old kernels once in a while.

BTW, patch from upthread doesn't help on that box, and neither does
simple reordering of MAC0 and MAC4 writes.  Adding reads of MAC0 and
MAC4 after corresponding writes seems to work.

Re: [PATCH] r8169: fix broken register writes

[=?unknown-8bit?B?RnJhbsOnb2lz?= Romieu]

Al Viro <viro@ZenIV.linux.org.uk> :
[...]
> BTW, patch from upthread doesn't help on that box, and neither does
> simple reordering of MAC0 and MAC4 writes.  Adding reads of MAC0 and
> MAC4 after corresponding writes seems to work.

Does something like the patch below work too ?

I'd like to factor out the posted PCI write commits.

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 9674005..8d1ab0f 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -542,6 +542,11 @@ static int rtl8169_poll(struct napi_struct *napi, int budget);
 static const unsigned int rtl8169_rx_config =
 	(RX_FIFO_THRESH << RxCfgFIFOShift) | (RX_DMA_BURST << RxCfgDMAShift);
 
+static void r816x_pci_write_commit(void __iomem *ioaddr)
+{
+	RTL_R8(ChipCmd);
+}
+
 static void mdio_write(void __iomem *ioaddr, int reg_addr, int value)
 {
 	int i;
@@ -2825,8 +2830,13 @@ static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
 	spin_lock_irq(&tp->lock);
 
 	RTL_W8(Cfg9346, Cfg9346_Unlock);
+
 	RTL_W32(MAC4, high);
+	r816x_pci_write_commit(ioaddr);
+
 	RTL_W32(MAC0, low);
+	r816x_pci_write_commit(ioaddr);
+
 	RTL_W8(Cfg9346, Cfg9346_Lock);
 
 	spin_unlock_irq(&tp->lock);

Re: [PATCH] r8169: fix broken register writes

[François Romieu]

Al Viro <viro@ZenIV.linux.org.uk> :
[...]
> FWIW, XID18000000 here (J7F4) loses MAC4 on shutdown; hadn't tested the patch
> yet.  2.6.26 (on that box) and 2.6.31 (on identical mb) work, 2.6.33 doesn't.
> I suspect that bisect would lead to commit cc098dc70 (i.e. the place where we
> started to set address on shutdown).  One more data point: ifconfig hw ether
> done under 2.6.26 did restore the address.  And that's the same function,
> isn't it?

You are right.

> Another interesting bit: unlike the older kernel, grep for eth0 in .33 dmesg
> 
> eth0: RTL8169sc/8110sc at 0xf87fc000, 00:30:18:a4:65:89, XID 18000000 IRQ 18
> r8169: eth0: link down
> ADDRCONF(NETDEV_UP): eth0: link is not ready
> r8169: eth0: link up
> ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready

Remove rtl8169scd_hw_phy_config ?

[...]
> Same for .31 on another box, modulo different address there...

No, keep it.


At first sight it looks like rtl8169_open::rtl8169_check_link_status and
a LinkChg interrupt (order eventually reversed) while 2.6.26 did not
notice the interrupt.

-- 
Ueimor

Re: [PATCH] r8169: fix broken register writes

[David Miller]

From: François Romieu <romieu@fr.zoreil.com>
Date: Sun, 28 Mar 2010 01:31:43 +0100

> This is quite similar to b39fe41f481d20c201012e4483e76c203802dda7
> though said registers are not even documented as 64-bit registers
> - as opposed to the initial TxDescStartAddress ones - but as single
> bytes which must be combined into 32 bits at the MMIO read/write
> level before being merged into a 64 bit logical entity.
> 
> Credits go to Ben Hutchings <ben@decadent.org.uk> for the MAR
> registers (aka "multicast is broken for ages on ARM) and to
> Timo Teräs <timo.teras@iki.fi> for the MAC registers.
> 
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>

Applied, thanks Francois.

Probably the rest of the driver should be audited for other
areas where we may end up having this problem.

Or, we should create readq/writeq macros (like other drivers do on
32-bit platforms, f.e. see drivers/net/niu.c) which write the two
32-bit parts in this required order.  Then access the registers using
readq/writeq entities throughout the driver.

This would have two benefits:

1) Coverage for all possible bug cases.

2) Real 64-bit accesses on 64-bit platforms.

Just some suggestions.

Thanks.

Re: [PATCH] r8169: fix broken register writes

[François Romieu]

David Miller <davem@davemloft.net> :
[...]
> Probably the rest of the driver should be audited for other
> areas where we may end up having this problem.

Those look safe
- MAC0
- MAC4
- MAR0
- CounterAddrLow
- CounterAddrHigh
- TxDescStartAddrLow
- TxDescStartAddrHigh
- RxDescAddrLow
- RxDescAddrHigh

The other candidates are not used yet.

[...]
> 1) Coverage for all possible bug cases.
>
> 2) Real 64-bit accesses on 64-bit platforms.

I see your point but the MACx and MARx registers are nowhere documented
as 64 bit : MAC0 and MAC4 (0x00 to 0x07) span IDR[0..5] + two reserved
bytes while MARx are documented as MAR[7..0] (0x08 to 0x0f). It would
be nice if it happened to work though.

-- 
Ueimor