Patchwork r8169: fix broken register writes

login
register
mail settings
Submitter françois romieu
Date March 28, 2010, 12:31 a.m.
Message ID <20100328003143.GA8501@electric-eye.fr.zoreil.com>
Download mbox | patch
Permalink /patch/48765/
State Accepted
Delegated to: David Miller
Headers show

Comments

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

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