RE: [PATCH V2 net-next 7/8] net: hns3: Add Ethtool support to HNS3 driver

[Salil Mehta]

Hi Stephen

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@xxxxxxxxxxxxxxxxxx]
> Sent: Wednesday, June 14, 2017 12:55 AM
> To: Salil Mehta
> Cc: davem@xxxxxxxxxxxxx; Zhuangyuzeng (Yisen); huangdaode; lipeng (Y);
> mehta.salil.lnk@xxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Linuxarm
> Subject: Re: [PATCH V2 net-next 7/8] net: hns3: Add Ethtool support to
> HNS3 driver
> 
> On Wed, 14 Jun 2017 00:10:34 +0100
> Salil Mehta <salil.mehta@xxxxxxxxxx> wrote:
> 
> > +/* netdev related stats */
> > +#define HNS3_NETDEV_STAT(_string, _member)			\
> > +	{ _string,						\
> > +	  FIELD_SIZEOF(struct rtnl_link_stats64, _member),	\
> > +	  offsetof(struct rtnl_link_stats64, _member),		\
> > +	}
> > +
> > +static const struct hns3_stats hns3_netdev_stats[] = {
> > +	/* misc. Rx/Tx statistics */
> > +	HNS3_NETDEV_STAT("rx_packets", rx_packets),
> > +	HNS3_NETDEV_STAT("tx_packets", tx_packets),
> > +	HNS3_NETDEV_STAT("rx_bytes", rx_bytes),
> > +	HNS3_NETDEV_STAT("tx_bytes", tx_bytes),
> > +	HNS3_NETDEV_STAT("rx_errors", rx_errors),
> > +	HNS3_NETDEV_STAT("tx_errors", tx_errors),
> > +	HNS3_NETDEV_STAT("rx_dropped", rx_dropped),
> > +	HNS3_NETDEV_STAT("tx_dropped", tx_dropped),
> > +	HNS3_NETDEV_STAT("multicast", multicast),
> > +	HNS3_NETDEV_STAT("collisions", collisions),
> > +
> > +	/* detailed Rx errors */
> > +	HNS3_NETDEV_STAT("rx_length_errors", rx_length_errors),
> > +	HNS3_NETDEV_STAT("rx_over_errors", rx_over_errors),
> > +	HNS3_NETDEV_STAT("rx_crc_errors", rx_crc_errors),
> > +	HNS3_NETDEV_STAT("rx_frame_errors", rx_frame_errors),
> > +	HNS3_NETDEV_STAT("rx_fifo_errors", rx_fifo_errors),
> > +	HNS3_NETDEV_STAT("rx_missed_errors", rx_missed_errors),
> > +
> > +	/* detailed Tx errors */
> > +	HNS3_NETDEV_STAT("tx_aborted_errors", tx_aborted_errors),
> > +	HNS3_NETDEV_STAT("tx_carrier_errors", tx_carrier_errors),
> > +	HNS3_NETDEV_STAT("tx_fifo_errors", tx_fifo_errors),
> > +	HNS3_NETDEV_STAT("tx_heartbeat_errors", tx_heartbeat_errors),
> > +	HNS3_NETDEV_STAT("tx_window_errors", tx_window_errors),
> > +
> 
> Ethtool statistics should be reserved for device specific values and
> should not be used just to clone statistics that already exist in
> the network device.
We saw an example from some of the existing drivers like netronome, apm,
intel etc. in the mainline and so thought this can be done. 
But your point about ethtool to be used only for device specific stats
definitely makes sense to me. 
Do you think We can live with this for now or do you wish to change it?

Many thanks!

Best regards
Salil