netdev - Re: [PATCH v3 4/5] alx: add alx_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Tue, 7 Jan 2014 11:02:05 +0100
From:	Sabrina Dubroca <sd@...asysnail.net>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	davem@...emloft.net, johannes@...solutions.net,
	stephen@...workplumber.org, netdev@...r.kernel.org
Subject: Re: [PATCH v3 4/5] alx: add alx_get_stats64 operation

Then the stats in all atheros drivers are a bit broken? They all use
the same formulas for ndo_get_stats.

Other atheros drivers (atl1e/atl1e.h and atl1c/atl1c.h) have the same
stats structure with a comment for each field. I will copy the
comments to alx.h.


2014-01-06, 19:07:33 +0000, Ben Hutchings wrote:
> On Mon, 2014-01-06 at 17:33 +0100, Sabrina Dubroca wrote:
> [...]
> > --- a/drivers/net/ethernet/atheros/alx/main.c
> > +++ b/drivers/net/ethernet/atheros/alx/main.c
> > @@ -1166,10 +1166,54 @@ static void alx_poll_controller(struct net_device *netdev)
> >  }
> >  #endif
> >  
> > +static struct rtnl_link_stats64 *alx_get_stats64(struct net_device *dev,
> > +					struct rtnl_link_stats64 *net_stats)
> > +{
> > +	struct alx_priv *alx = netdev_priv(dev);
> > +	struct alx_hw_stats *hw_stats = &alx->hw.stats;
> > +
> > +	spin_lock(&alx->stats_lock);
> > +
> > +	alx_update_hw_stats(&alx->hw);
> > +
> > +	net_stats->tx_packets = hw_stats->tx_ok;
> 
> I think this should be set to hw_stats->tx_ok + net_stats->tx_errors
> (after you set tx_errors).
> 
> > +	net_stats->tx_bytes   = hw_stats->tx_byte_cnt;
> > +	net_stats->rx_packets = hw_stats->rx_ok;
> 
> Similarly, I think this should be hw_stats->rx_ok +
> net_stats->rx_errors.
> 
> > +	net_stats->rx_bytes   = hw_stats->rx_byte_cnt;
> > +	net_stats->multicast  = hw_stats->rx_mcast;
> > +	net_stats->collisions = hw_stats->tx_single_col +
> > +				hw_stats->tx_multi_col * 2 +
> 
> I would expect this to count the number of packets that had collisions
> rather than total number of collisions (which you're only guessing at by
> using * 2).

Okay, all changed.


> > +				hw_stats->tx_late_col + hw_stats->tx_abort_col;
> > +
> > +	net_stats->rx_errors  = hw_stats->rx_frag + hw_stats->rx_fcs_err +
> > +				hw_stats->rx_len_err + hw_stats->rx_ov_sz +
> > +				hw_stats->rx_ov_rrd + hw_stats->rx_align_err;
> > +
> > +	net_stats->rx_fifo_errors   = hw_stats->rx_ov_rxf;
> > +	net_stats->rx_length_errors = hw_stats->rx_len_err;
> > +	net_stats->rx_crc_errors    = hw_stats->rx_fcs_err;
> > +	net_stats->rx_frame_errors  = hw_stats->rx_align_err;
> > +	net_stats->rx_over_errors   = hw_stats->rx_ov_rrd + hw_stats->rx_ov_rxf;
> 
> rx_over_errors is commented as 'receiver ring buff overflow' and
> ifconfig includes it in the 'frame' error count.  I think it is intended
> to count frames which are too large for on-chip RX buffers and should
> always be 0 for devices that do RX DMA.
> 
> Each error should contribute to at most one specific error stat, so
> don't count rx_ov_rxf in both rx_fifo_errors and rx_over_errors.  I
> would guess rx_fifo_errors is the right counter.
>
> I don't know what rx_ov_rrd represents, but if it's the number of
> packets dropped because the RX descriptor ring was empty then it should
> be counted in rx_dropped not rx_over_errors.

in atl1e/atl1e.h:
rx_ov_rxf: The number of frame dropped due to occurrence of RX FIFO overflow.
rx_ov_rrd: The number of frame dropped due to occurrence of RRD overflow.

I'm not sure which counter fits these best.


> > +	net_stats->rx_missed_errors = hw_stats->rx_ov_rrd + hw_stats->rx_ov_rxf;
> [...]
> 
> This counter is commented as 'receiver missed packet'.  I think this
> means the MAC detected SOF but was somehow too busy to receive the
> packet, but I'm not sure.  I don't think these hardware counters match
> the description, and certainly they shouldn't be counted here as well as
> the other specific error stats.

Okay.


Thanks,

-- 
Sabrina
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html