linux-kernel - 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: <e637fe93-692d-48b1-9cb9-11719f8c90c3@kernel.org>
Date:   Thu, 14 Dec 2023 12:04:35 +0200
From:   Roger Quadros <rogerq@...nel.org>
To:     Vladimir Oltean <vladimir.oltean@....com>
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 13/12/2023 15:54, Vladimir Oltean wrote:
> 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;
> 	}

Got it.

> 
> 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.

OK. I take care of this in next spin. Thanks.
> 
> --
> pw-bot: changes-requested

-- 
cheers,
-roger