netdev - Re: [PATCH v8 net-next 11/11] net: ethernet: ti: am65-cpsw: Fix get_eth_mac

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: <20231213135435.czao6wjighpskcvz@skbuf>
Date: Wed, 13 Dec 2023 15:54:35 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Roger Quadros <rogerq@...nel.org>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, shuah@...nel.org, s-vadapalli@...com,
	r-gunasekaran@...com, vigneshr@...com, srk@...com, horms@...nel.org,
	p-varis@...com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 net-next 11/11] net: ethernet: ti: am65-cpsw: Fix
 get_eth_mac_stats

On Wed, Dec 13, 2023 at 01:07:21PM +0200, Roger Quadros wrote:
> We do not support individual stats for PMAC and EMAC so
> report only aggregate stats.
> 
> Fixes: 67372d7a85fc ("net: ethernet: am65-cpsw: Add standard Ethernet MAC stats to ethtool")
> Signed-off-by: Roger Quadros <rogerq@...nel.org>
> ---
>  drivers/net/ethernet/ti/am65-cpsw-ethtool.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> Changelog:
> 
> v8: initial commit
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-ethtool.c b/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
> index d2baffb05d55..35e318458b0c 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
> @@ -671,6 +671,9 @@ static void am65_cpsw_get_eth_mac_stats(struct net_device *ndev,
>  
>  	stats = port->stat_base;
>  
> +	if (s->src != ETHTOOL_MAC_STATS_SRC_AGGREGATE)
> +		return;
> +
>  	s->FramesTransmittedOK = readl_relaxed(&stats->tx_good_frames);
>  	s->SingleCollisionFrames = readl_relaxed(&stats->tx_single_coll_frames);
>  	s->MultipleCollisionFrames = readl_relaxed(&stats->tx_mult_coll_frames);
> -- 
> 2.34.1
>

Fixes are only fixes if they address a visible issue. And the blamed
commit is the one that made the issue visible - the same one that
"git bisect" would lead to - not necessarily the commit that introduced
the code being changed.

If you look at net/ethtool/stats.c, it will only accept ETHTOOL_MAC_STATS_SRC_AGGREGATE
for drivers that don't support the MAC Merge layer.

	if ((src == ETHTOOL_MAC_STATS_SRC_EMAC ||
	     src == ETHTOOL_MAC_STATS_SRC_PMAC) &&
	    !__ethtool_dev_mm_supported(dev)) {
		NL_SET_ERR_MSG_MOD(info->extack,
				   "Device does not support MAC merge layer");
		ethnl_ops_complete(dev);
		return -EOPNOTSUPP;
	}

So, there was nothing broken in commit 67372d7a85fc ("net: ethernet:
am65-cpsw: Add standard Ethernet MAC stats to ethtool").

The first broken commit is when you add support for get_mm(), such that
__ethtool_dev_mm_supported() returns true.

And because you don't add bugs in the code just to fix them later in the
series, you need to order the patches such that all the dependencies for
get_mm() are in place before the get_mm() support is added.

Translated to your case, this patch must not be 11/11, and it must have
the Fixes: tag dropped, and it must explain in the commit message that
it is preparatory work.

--
pw-bot: changes-requested