Jeff Garzik <jgarzik@xxxxxxxxx> :
[...]
> >+static void rtl8169_get_ethtool_stats(struct net_device *netdev,
> >+ struct ethtool_stats *stats, u64 *data)
> >+{
> >+ struct rtl8169_private *tp = netdev_priv(netdev);
> >+ void __iomem *ioaddr = tp->mmio_addr;
> >+ int work = 100;
> >+ int i;
> >+
> >+ /* begin NIC statistics dump */
> >+ RTL_W32(StatsAddrHigh, tp->nic_stats_addr >> 32);
> >+ RTL_W32(StatsAddrLow, (tp->nic_stats_addr & 0xffffffff) |
> >DumpStats);
> >+ RTL_R32(StatsAddrLow);
> >+
> >+ while (work-- > 0) {
> >+ if ((RTL_R32(StatsAddrLow) & DumpStats) == 0)
> >+ break;
> >+ cpu_relax();
> >+ }
> >+
> >+ if (RTL_R32(StatsAddrLow) & DumpStats)
> >+ return; /* no stats - took too long */
> >+
> >+ i = 0;
> >+ data[i++] = le64_to_cpu(tp->nic_stats->tx_ok);
[...]
> >+ data[i++] = le16_to_cpu(tp->nic_stats->tx_underrun);
> >+ if (i != RTL8169_STATS_LEN)
> >+ BUG();
> >+}
> >+
> >
> >It seems to me that 'i' could be re-used, instead of having both 'i' and
> >'work'.
>
> No. That's a completely useless pseudo-optimization. Write readable
> code, and let the compiler do the rest.
Btw I'd simply remove the 'work' variable and schedule in an interruptible
way until the dump is done.
BUG() is a bit exagerated imho.
--
Ueimor
|