Re: [PATCH]: r8169: Expose hardware stats via ethtool

[Richard Dawe <rich@phekda.gotadsl.co.uk>]

Hello.

Jeff Garzik wrote:
> Richard Dawe wrote:
[snip]
>> +static const char rtl8169_gstrings_stats[][ETH_GSTRING_LEN] = {
>> +    "tx_ok", "rx_ok", "tx_err", "rx_err",
>> +    "rx_fifo", "frame_align", "tx_ok_1col", "tx_ok_mcol",
>> +    "rx_ok_phys", "rx_ok_bcast", "rx_ok_mcast", "tx_abort",
>> +    "tx_underrun",
>> +};
> 
> 
> Don't needlessly reformat copied code.  It's one-string-per-line 
> intentionally, for ease of maintenance and ease of adding new strings.

OK, I'll fix that.

> Also, I don't see why you renamed this from ethtool_stats_keys[].

I didn't copy it directly. I started off with something that looked like 
the ethtool stats code from the e100 driver. Then I noticed that 8139cp 
did the right thing for r8169.

I'll rename it.

>> +    /* begin NIC statistics dump */
>> +    RTL_W32(StatsAddrHigh, tp->nic_stats_addr >> 32);
>> +    RTL_W32(StatsAddrLow, (tp->nic_stats_addr & 0xffffffff) | 
>> DumpStats);
>> +    RTL_R32(StatsAddrLow);
> 
> 
> This last RTL_R32() can be removed [from 8139cp too], because a flush 
> immediately follows anyway:
[snip]

OK, will do.

Thanks, bye, Rich =]

-- 
Richard Dawe [ http://homepages.nildram.co.uk/~phekda/richdawe/ ]

"You can't evaluate a man by logic alone."
   -- McCoy, "I, Mudd", Star Trek