| Message ID | 20100328003143.GA8501@electric-eye.fr.zoreil.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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
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.
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.
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
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
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
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.
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);
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(-)