netdev - Re: [RFC v2 PATCH 05/10] net: ti: prueth: Adds ethtool support for ICSSM PRUETH Driver

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]
Message-ID: <1332280195.481828.1738417717332.JavaMail.zimbra@couthit.local>
Date: Sat, 1 Feb 2025 19:18:37 +0530 (IST)
From: Basharath Hussain Khaja <basharath@...thit.com>
To: horms <horms@...nel.org>
Cc: basharath <basharath@...thit.com>, danishanwar <danishanwar@...com>, 
	rogerq <rogerq@...nel.org>, andrew+netdev <andrew+netdev@...n.ch>, 
	davem <davem@...emloft.net>, edumazet <edumazet@...gle.com>, 
	kuba <kuba@...nel.org>, pabeni <pabeni@...hat.com>, 
	Rob Herring <robh@...nel.org>, krzk+dt <krzk+dt@...nel.org>, 
	conor+dt <conor+dt@...nel.org>, nm <nm@...com>, 
	ssantosh <ssantosh@...nel.org>, tony <tony@...mide.com>, 
	richardcochran <richardcochran@...il.com>, 
	parvathi <parvathi@...thit.com>, schnelle <schnelle@...ux.ibm.com>, 
	rdunlap <rdunlap@...radead.org>, diogo ivo <diogo.ivo@...mens.com>, 
	m-karicheri2 <m-karicheri2@...com>, 
	jacob e keller <jacob.e.keller@...el.com>, 
	m-malladi <m-malladi@...com>, 
	javier carrasco cruz <javier.carrasco.cruz@...il.com>, 
	afd <afd@...com>, s-anna <s-anna@...com>, 
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>, 
	netdev <netdev@...r.kernel.org>, 
	devicetree <devicetree@...r.kernel.org>, 
	linux-kernel <linux-kernel@...r.kernel.org>, 
	linux-omap <linux-omap@...r.kernel.org>, 
	pratheesh <pratheesh@...com>, prajith <prajith@...com>, 
	vigneshr <vigneshr@...com>, praneeth <praneeth@...com>, 
	srk <srk@...com>, rogerq <rogerq@...com>, 
	krishna <krishna@...thit.com>, pmohan <pmohan@...thit.com>, 
	mohan <mohan@...thit.com>
Subject: Re: [RFC v2 PATCH 05/10] net: ti: prueth: Adds ethtool support for
 ICSSM PRUETH Driver


> On Fri, Jan 24, 2025 at 07:10:51PM +0530, Basharath Hussain Khaja wrote:
>> From: Roger Quadros <rogerq@...com>
>> 
>> Changes for enabling ethtool support for the newly added PRU Ethernet
>> interfaces. Extends the support for statistics collection from PRU internal
>> memory and displays it in the user space. Along with statistics,
>> enable/disable of features, configuring link speed etc.are now supported.
>> 
>> The firmware running on PRU maintains statistics in internal data memory.
>> When requested ethtool collects all the statistics for the specified
>> interface and displays it in the user space.
>> 
>> Makefile is updated to include ethtool support into PRUETH driver.
>> 
>> Signed-off-by: Roger Quadros <rogerq@...com>
>> Signed-off-by: Andrew F. Davis <afd@...com>
>> Signed-off-by: Parvathi Pudi <parvathi@...thit.com>
>> Signed-off-by: Basharath Hussain Khaja <basharath@...thit.com>
> 
> ...
> 
>> diff --git a/drivers/net/ethernet/ti/icssm/icssm_ethtool.c
>> b/drivers/net/ethernet/ti/icssm/icssm_ethtool.c
> 
> ...
> 
>> +static const struct {
>> +	char string[ETH_GSTRING_LEN];
>> +	u32 offset;
>> +} prueth_ethtool_stats[] = {
>> +	{"txBcast", PRUETH_STAT_OFFSET(tx_bcast)},
>> +	{"txMcast", PRUETH_STAT_OFFSET(tx_mcast)},
>> +	{"txUcast", PRUETH_STAT_OFFSET(tx_ucast)},
>> +	{"txOctets", PRUETH_STAT_OFFSET(tx_octets)},
>> +	{"rxBcast", PRUETH_STAT_OFFSET(rx_bcast)},
>> +	{"rxMcast", PRUETH_STAT_OFFSET(rx_mcast)},
>> +	{"rxUcast", PRUETH_STAT_OFFSET(rx_ucast)},
>> +	{"rxOctets", PRUETH_STAT_OFFSET(rx_octets)},
> 
> Hi Roger, Basharath, all,
> 
> There seems to be some overlap between the above and struct rtnl_link_stats64.
> 
> Please implement those stats which are present in struct rtnl_link_stats64
> using ndo_get_stats64 and omit them from your implementation of
> get_ethtool_stats.
> 
> IOW, get_ethtool_stats() is for extended stats, whereas is for standard
> stats ndo_get_stats64().  And standard stats should not be presented to the
> user as extended stats.
> 
> Link:
> https://docs.kernel.org/networking/statistics.html#notes-for-driver-authors
> 

We will address this along with the changes that will be done by using 
rtnl_link_stats64 instead of legacy net_device_stats. 

>> +	{"tx64byte", PRUETH_STAT_OFFSET(tx64byte)},
>> +	{"tx65_127byte", PRUETH_STAT_OFFSET(tx65_127byte)},
>> +	{"tx128_255byte", PRUETH_STAT_OFFSET(tx128_255byte)},
>> +	{"tx256_511byte", PRUETH_STAT_OFFSET(tx256_511byte)},
>> +	{"tx512_1023byte", PRUETH_STAT_OFFSET(tx512_1023byte)},
>> +	{"tx1024byte", PRUETH_STAT_OFFSET(tx1024byte)},
>> +	{"rx64byte", PRUETH_STAT_OFFSET(rx64byte)},
>> +	{"rx65_127byte", PRUETH_STAT_OFFSET(rx65_127byte)},
>> +	{"rx128_255byte", PRUETH_STAT_OFFSET(rx128_255byte)},
>> +	{"rx256_511byte", PRUETH_STAT_OFFSET(rx256_511byte)},
>> +	{"rx512_1023byte", PRUETH_STAT_OFFSET(rx512_1023byte)},
>> +	{"rx1024byte", PRUETH_STAT_OFFSET(rx1024byte)},
> 
> Similarly, the above, along with rxOverSizedFrames and rxUnderSizedFrames
> below seem to be RMON (RFC 2819) statistics. So I think they should
> be handled by implementing get_rmon_stats().
> 

Sure. We will add get_rmon_stats() function and update necessary statistics
in that function.

>> +
>> +	{"lateColl", PRUETH_STAT_OFFSET(late_coll)},
>> +	{"singleColl", PRUETH_STAT_OFFSET(single_coll)},
>> +	{"multiColl", PRUETH_STAT_OFFSET(multi_coll)},
>> +	{"excessColl", PRUETH_STAT_OFFSET(excess_coll)},
> 
> And likewise, the section above and below seem to overlap
> with Basic IEEE 802.3 MAC statistics which I believe
> should be handled by implementing get_eth_mac_stats()
> 

For now we will remove these stats in the next version
and address this in the next series of patches.

>> +
>> +	{"rxMisAlignmentFrames", PRUETH_STAT_OFFSET(rx_misalignment_frames)},
>> +	{"stormPrevCounterBC", PRUETH_STAT_OFFSET(stormprev_counter_bc)},
>> +	{"stormPrevCounterMC", PRUETH_STAT_OFFSET(stormprev_counter_mc)},
>> +	{"stormPrevCounterUC", PRUETH_STAT_OFFSET(stormprev_counter_uc)},
>> +	{"macRxError", PRUETH_STAT_OFFSET(mac_rxerror)},
>> +	{"SFDError", PRUETH_STAT_OFFSET(sfd_error)},
>> +	{"defTx", PRUETH_STAT_OFFSET(def_tx)},
>> +	{"macTxError", PRUETH_STAT_OFFSET(mac_txerror)},
>> +	{"rxOverSizedFrames", PRUETH_STAT_OFFSET(rx_oversized_frames)},
>> +	{"rxUnderSizedFrames", PRUETH_STAT_OFFSET(rx_undersized_frames)},
>> +	{"rxCRCFrames", PRUETH_STAT_OFFSET(rx_crc_frames)},
>> +	{"droppedPackets", PRUETH_STAT_OFFSET(dropped_packets)},
>> +
>> +	{"txHWQOverFlow", PRUETH_STAT_OFFSET(tx_hwq_overflow)},
>> +	{"txHWQUnderFlow", PRUETH_STAT_OFFSET(tx_hwq_underflow)},
>> +	{"vlanDropped", PRUETH_STAT_OFFSET(vlan_dropped)},
>> +	{"multicastDropped", PRUETH_STAT_OFFSET(multicast_dropped)},
>> +};
> 
> ...

Thanks & Best Regards,
Basharath