r8169: fix broken register writes

Submitted by françois romieu on March 28, 2010, 12:31 a.m.

Details

Message ID 20100328003143.GA8501@electric-eye.fr.zoreil.com
State Accepted
Delegated to: David Miller
Headers show

Commit Message

françois romieu March 28, 2010, 12:31 a.m.
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(-)

Comments

Ben Hutchings March 28, 2010, 12:47 a.m.
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.
David Miller March 28, 2010, 2:38 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
françois romieu March 28, 2010, 9:28 p.m.
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.
françois romieu March 28, 2010, 10:04 p.m.
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.
Al Viro March 28, 2010, 10:19 p.m.
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...
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro March 29, 2010, 1:03 a.m.
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?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Al Viro March 29, 2010, 2:17 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
françois romieu March 29, 2010, 9:11 p.m.
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.

Patch hide | download patch | download mbox

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);