Messages in this thread | | | Date | Mon, 5 Jun 2023 15:04:02 -0700 | Subject | Re: [PATCH net-next v6 3/6] net: bcmasp: Add support for ASP2.0 Ethernet controller | From | Florian Fainelli <> |
| |
On 6/2/2023 11:58 PM, Jakub Kicinski wrote: > On Thu, 1 Jun 2023 15:12:28 -0700 Justin Chen wrote: >> + /* general stats */ >> + STAT_NETDEV(rx_packets), >> + STAT_NETDEV(tx_packets), >> + STAT_NETDEV(rx_bytes), >> + STAT_NETDEV(tx_bytes), >> + STAT_NETDEV(rx_errors), >> + STAT_NETDEV(tx_errors), >> + STAT_NETDEV(rx_dropped), >> + STAT_NETDEV(tx_dropped), >> + STAT_NETDEV(multicast), > > please don't report standard interface stats in ethtool -S > >> + /* UniMAC RSV counters */ >> + STAT_BCMASP_MIB_RX("rx_64_octets", mib.rx.pkt_cnt.cnt_64), >> + STAT_BCMASP_MIB_RX("rx_65_127_oct", mib.rx.pkt_cnt.cnt_127), >> + STAT_BCMASP_MIB_RX("rx_128_255_oct", mib.rx.pkt_cnt.cnt_255), >> + STAT_BCMASP_MIB_RX("rx_256_511_oct", mib.rx.pkt_cnt.cnt_511), >> + STAT_BCMASP_MIB_RX("rx_512_1023_oct", mib.rx.pkt_cnt.cnt_1023), >> + STAT_BCMASP_MIB_RX("rx_1024_1518_oct", mib.rx.pkt_cnt.cnt_1518), >> + STAT_BCMASP_MIB_RX("rx_vlan_1519_1522_oct", mib.rx.pkt_cnt.cnt_mgv), >> + STAT_BCMASP_MIB_RX("rx_1522_2047_oct", mib.rx.pkt_cnt.cnt_2047), >> + STAT_BCMASP_MIB_RX("rx_2048_4095_oct", mib.rx.pkt_cnt.cnt_4095), >> + STAT_BCMASP_MIB_RX("rx_4096_9216_oct", mib.rx.pkt_cnt.cnt_9216), > > these should also be removed, and you should implement @get_rmon_stats. > >> + STAT_BCMASP_MIB_RX("rx_pkts", mib.rx.pkt), >> + STAT_BCMASP_MIB_RX("rx_bytes", mib.rx.bytes), >> + STAT_BCMASP_MIB_RX("rx_multicast", mib.rx.mca), >> + STAT_BCMASP_MIB_RX("rx_broadcast", mib.rx.bca), >> + STAT_BCMASP_MIB_RX("rx_fcs", mib.rx.fcs), > > there's a FCS error statistic in the standard stats, no need to > duplicate > >> + STAT_BCMASP_MIB_RX("rx_control", mib.rx.cf), >> + STAT_BCMASP_MIB_RX("rx_pause", mib.rx.pf), > > @get_pause_stats > >> + STAT_BCMASP_MIB_RX("rx_unknown", mib.rx.uo), >> + STAT_BCMASP_MIB_RX("rx_align", mib.rx.aln), >> + STAT_BCMASP_MIB_RX("rx_outrange", mib.rx.flr), >> + STAT_BCMASP_MIB_RX("rx_code", mib.rx.cde), >> + STAT_BCMASP_MIB_RX("rx_carrier", mib.rx.fcr), >> + STAT_BCMASP_MIB_RX("rx_oversize", mib.rx.ovr), >> + STAT_BCMASP_MIB_RX("rx_jabber", mib.rx.jbr), > > these look like candidates from standard stats, too. > Please read thru: > > https://docs.kernel.org/next/networking/statistics.html > >> + STAT_BCMASP_MIB_RX("rx_mtu_err", mib.rx.mtue), >> + STAT_BCMASP_MIB_RX("rx_good_pkts", mib.rx.pok), >> + STAT_BCMASP_MIB_RX("rx_unicast", mib.rx.uc), >> + STAT_BCMASP_MIB_RX("rx_ppp", mib.rx.ppp), >> + STAT_BCMASP_MIB_RX("rx_crc", mib.rx.rcrc), > > hm, what's the difference between rx_crc and rx_fcs ?
Since you are going to respin to address Jakub's feedback, we should also consider using the shared unimac.h header file. We could even make a library out of it for standardized statistics once this driver gets accepted. -- Florian [unhandled content-type:application/pkcs7-signature]
| |