Message ID | 20150907144412.GA26524@calimero.vinschen.de |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
Corinna Vinschen <vinschen@redhat.com> : [...] > I have a bit of a problem with this patch. It crashes when loading the > driver manually w/ modprobe. For some reason dev_get_stats is called > during rtl_init_one and at that time the counters pointer is NULL, so > the kernel gets a SEGV. > > After I worked around that I got a SEGV in > rtl_remove_one, which I still have to find out why. I didn't test with > the latest kernel, though, so I still have to check if that's the reason > for the crashes. That takes a bit longer, I just wanted to let you > know. Call me stupid: I forgot that it's perfectly fine to request the stats of a down interface. :o/ Updated patch is on the way. > There's also a problem with rtl8169_cmd_counters: It always calls > rtl_udelay_loop_wait_low w/ rtl_reset_counters_cond, even when called > from rtl8169_update_counters, where it should call > rtl_udelay_loop_wait_low w/ rtl_counters_cond to check for the > CounterDump flag, rather than for the CounterReset flag. > > For now I applied the below patch, but I think it's a bit ugly to > conditionalize the rtl_cond struct by the incoming flag value. <captain obvious> Let's check both at once: return RTL_R32(CounterAddrLow) & (CounterDump | CounterReset); </captain obvious> Thanks for reviewing.
On Sep 7 23:52, Francois Romieu wrote: > Corinna Vinschen <vinschen@redhat.com> : > [...] > > I have a bit of a problem with this patch. It crashes when loading the > > driver manually w/ modprobe. For some reason dev_get_stats is called > > during rtl_init_one and at that time the counters pointer is NULL, so > > the kernel gets a SEGV. > > > > After I worked around that I got a SEGV in > > rtl_remove_one, which I still have to find out why. I didn't test with > > the latest kernel, though, so I still have to check if that's the reason > > for the crashes. That takes a bit longer, I just wanted to let you > > know. > > Call me stupid: I forgot that it's perfectly fine to request the stats > of a down interface. :o/ > > Updated patch is on the way. > > > There's also a problem with rtl8169_cmd_counters: It always calls > > rtl_udelay_loop_wait_low w/ rtl_reset_counters_cond, even when called > > from rtl8169_update_counters, where it should call > > rtl_udelay_loop_wait_low w/ rtl_counters_cond to check for the > > CounterDump flag, rather than for the CounterReset flag. > > > > For now I applied the below patch, but I think it's a bit ugly to > > conditionalize the rtl_cond struct by the incoming flag value. > > <captain obvious> > > Let's check both at once: > > return RTL_R32(CounterAddrLow) & (CounterDump | CounterReset); If you're sure that's valid, then why not? It's certainly cleaner code. Corinna
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index ae07567..cd7adbf 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -2200,6 +2200,13 @@ DECLARE_RTL_COND(rtl_reset_counters_cond) return RTL_R32(CounterAddrLow) & CounterReset; } +DECLARE_RTL_COND(rtl_counters_cond) +{ + void __iomem *ioaddr = tp->mmio_addr; + + return RTL_R32(CounterAddrLow) & CounterDump; +} + static int rtl8169_cmd_counters(struct net_device *dev, u32 counter_cmd) { struct rtl8169_private *tp = netdev_priv(dev); @@ -2212,7 +2219,10 @@ static int rtl8169_cmd_counters(struct net_device *dev, u32 counter_cmd) RTL_W32(CounterAddrLow, cmd); RTL_W32(CounterAddrLow, cmd | counter_cmd); - return rtl_udelay_loop_wait_low(tp, &rtl_reset_counters_cond, 10, 1000); + return rtl_udelay_loop_wait_low(tp, + counter_cmd == CounterDump ? &rtl_counters_cond : + &rtl_reset_counters_cond, + 10, 1000); } static int rtl8169_reset_counters(struct net_device *dev) @@ -2229,13 +2239,6 @@ static int rtl8169_reset_counters(struct net_device *dev) return rtl8169_cmd_counters(dev, CounterReset); } -DECLARE_RTL_COND(rtl_counters_cond) -{ - void __iomem *ioaddr = tp->mmio_addr; - - return RTL_R32(CounterAddrLow) & CounterDump; -} - static int rtl8169_update_counters(struct net_device *dev) { struct rtl8169_private *tp = netdev_priv(dev); @@ -7800,8 +7803,11 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats) /* * Fetch additonal counter values missing in stats collected by driver - * from tally counters. + * from tally counters, but only if we're already initialized. */ + if (!counters) + return stats; + rtl8169_update_counters(dev); /*