linux-kernel - Re: [PATCH net-next v6 3/6] net: bcmasp: Add support for ASP2.0 Ethernet controller

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <91341500-79f0-b565-98ec-be47cfd488cc@broadcom.com>
Date:   Mon, 5 Jun 2023 15:04:02 -0700
From:   Florian Fainelli <florian.fainelli@...adcom.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Justin Chen <justin.chen@...adcom.com>
Cc:     netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        bcm-kernel-feedback-list@...adcom.com, davem@...emloft.net,
        edumazet@...gle.com, pabeni@...hat.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        opendmb@...il.com, andrew@...n.ch, hkallweit1@...il.com,
        linux@...linux.org.uk, richardcochran@...il.com,
        sumit.semwal@...aro.org, christian.koenig@....com,
        simon.horman@...igine.com
Subject: Re: [PATCH net-next v6 3/6] net: bcmasp: Add support for ASP2.0
 Ethernet controller



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

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4221 bytes)