by Roger Quadros

[permalink] [raw]

Subject: Re: [PATCH v8 net-next 02/11] selftests: forwarding: ethtool_mm: fall back to aggregate if device does not report pMAC stats

On 14/12/2023 16:16, Vladimir Oltean wrote:
> On Wed, Dec 13, 2023 at 01:07:12PM +0200, Roger Quadros wrote:
>> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
>> index 8f6ca458af9a..763c262a3453 100755
>> --- a/tools/testing/selftests/net/forwarding/lib.sh
>> +++ b/tools/testing/selftests/net/forwarding/lib.sh
>> @@ -146,6 +146,15 @@ check_ethtool_mm_support()
>> fi
>> }
>>
>> +check_ethtool_pmac_std_stats_support()
>> +{
>> + local dev=$1; shift
>> + local grp=$1; shift
>> +
>> + [ 0 -ne $(ethtool --json -S $dev --all-groups --src pmac 2>/dev/null \
>> + | jq '.[]."$grp" | length') ]
>> +}
>> +
>> check_locked_port_support()
>> {
>> if ! bridge -d link show | grep -q " locked"; then
>> --
>> 2.34.1
>>
>
> FYI, there's another submitted patch that touches the exact same spot,
> and it looks like it has a good chance of getting merged.
> https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/
>
> You need to pay attention to merge conflicts, so you don't waste a patch
> iteration just because of that one thing.
>
> I guess you might be able to wing it, because the other patch does this:
>
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index 8f6ca458af9a..e3740163c384 100755
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -146,6 +146,15 @@ check_ethtool_mm_support()
> fi
> }
>
> +check_ethtool_counter_group_support()
> +{
> + ethtool --help 2>&1| grep -- '--all-groups' &> /dev/null
> + if [[ $? -ne 0 ]]; then
> + echo "SKIP: ethtool too old; it is missing standard counter group support"
> + exit $ksft_skip
> + fi
> +}
> +
> check_locked_port_support()
> {
> if ! bridge -d link show | grep -q " locked"; then
>
> which quite coincidentally does not change what your patch sees in its
> upper context, aka 3 lines like this:
>
> ----
> fi
> }
>
> ----
>
> You can check if your patch set applies on top of Tobias', by formatting
> it as patch files on top of net-next/main, resetting HEAD to net-next,
> applying Tobias' series and then your patches.

Thanks for the heads up :)

--
cheers,
-roger

2023-12-15 11:31:31

by Roger Quadros

[permalink] [raw]

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

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 <[email protected]>
>>> ---
>>> 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 ;).

--
cheers,
-roger