diff mbox series

[v8,net-next,11/11] net: ethernet: ti: am65-cpsw: Fix get_eth_mac_stats

Message ID 20231213110721.69154-12-rogerq@kernel.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: ethernet: am65-cpsw: Add mqprio, frame pre-emption & coalescing | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers fail 1 blamed authors not CCed: andrew@lunn.ch; 1 maintainers not CCed: andrew@lunn.ch
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 9 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Roger Quadros Dec. 13, 2023, 11:07 a.m. UTC 
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@kernel.org>
---
 drivers/net/ethernet/ti/am65-cpsw-ethtool.c | 3 +++
 1 file changed, 3 insertions(+)

Changelog:

v8: initial commit

Comments

Vladimir Oltean Dec. 13, 2023, 1:54 p.m. UTC | #1 
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@kernel.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
Roger Quadros Dec. 14, 2023, 10:04 a.m. UTC | #2 
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@kernel.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
Roger Quadros Dec. 15, 2023, 11:31 a.m. UTC | #3 
On 14/12/2023 12:04, Roger Quadros wrote:
> 
> 
> 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@kernel.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.

Just a heads up. I have decided to merge this patch with mac-merge patch.
One less patch to deal with ;).
diff mbox series

Patch

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);