Re: [PATCH net-next v7 2/5] ethtool: netlink: add ETHTOOL_MSG_MSE_GET and wire up PHY MSE access

[Paolo Abeni <pabeni@redhat.com>]

On 10/20/25 12:31 PM, Oleksij Rempel wrote:
> diff --git a/net/ethtool/mse.c b/net/ethtool/mse.c
> new file mode 100644
> index 000000000000..89365bdb1109
> --- /dev/null
> +++ b/net/ethtool/mse.c
> @@ -0,0 +1,411 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/ethtool.h>
> +#include <linux/phy.h>
> +#include <linux/slab.h>
> +
> +#include "netlink.h"
> +#include "common.h"
> +#include "bitset.h"
> +
> +#define PHY_MSE_CHANNEL_COUNT 4
> +
> +struct mse_req_info {
> +	struct ethnl_req_info base;
> +};
> +
> +struct mse_snapshot_entry {
> +	struct phy_mse_snapshot snapshot;
> +	int channel;
> +};
> +
> +struct mse_reply_data {
> +	struct ethnl_reply_data base;
> +	struct phy_mse_capability capability;
> +	struct mse_snapshot_entry *snapshots;
> +	unsigned int num_snapshots;
> +};
> +
> +static inline struct mse_reply_data *
> +mse_repdata(const struct ethnl_reply_data *reply_base)

Please don't use inline in 'c' files, either move to an header or drop
the 'inline' keyword. A few more occurences below.

> +static int mse_get_one_channel(struct phy_device *phydev,
> +			       struct mse_reply_data *data, int channel)
> +{
> +	u32 cap_bit = 0;
> +	int ret;
> +
> +	switch (channel) {
> +	case PHY_MSE_CHANNEL_A:
> +		cap_bit = PHY_MSE_CAP_CHANNEL_A;
> +		break;
> +	case PHY_MSE_CHANNEL_B:
> +		cap_bit = PHY_MSE_CAP_CHANNEL_B;
> +		break;
> +	case PHY_MSE_CHANNEL_C:
> +		cap_bit = PHY_MSE_CAP_CHANNEL_C;
> +		break;
> +	case PHY_MSE_CHANNEL_D:
> +		cap_bit = PHY_MSE_CAP_CHANNEL_D;
> +		break;
> +	case PHY_MSE_CHANNEL_WORST:
> +		cap_bit = PHY_MSE_CAP_WORST_CHANNEL;
> +		break;
> +	case PHY_MSE_CHANNEL_LINK:
> +		cap_bit = PHY_MSE_CAP_LINK;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (!(data->capability.supported_caps & cap_bit))
> +		return -EOPNOTSUPP;
> +
> +	data->snapshots = kzalloc(sizeof(*data->snapshots), GFP_KERNEL);
> +	if (!data->snapshots)
> +		return -ENOMEM;
> +
> +	ret = phydev->drv->get_mse_snapshot(phydev, channel,
> +					    &data->snapshots[0].snapshot);
> +	if (ret)
> +		return ret;
> +
> +	data->snapshots[0].channel = channel;

Minor nit: it looks like you could de-dup some code above reusing the
get_snapshot_if_supported() helper. I see the code could end-up
allocating the structure just to free it in case of unsupported channel,
but I think it still would be better.

[...]> +static int mse_fill_reply(struct sk_buff *skb,
> +			  const struct ethnl_req_info *req_base,
> +			  const struct ethnl_reply_data *reply_base)
> +{
> +	const struct mse_reply_data *data = mse_repdata(reply_base);
> +	struct nlattr *cap_nest, *snap_nest;
> +	unsigned int i;
> +	int ret;
> +
> +	cap_nest = nla_nest_start(skb, ETHTOOL_A_MSE_CAPABILITIES);
> +	if (!cap_nest)
> +		return -EMSGSIZE;
> +
> +	if (data->capability.supported_caps & PHY_MSE_CAP_AVG) {
> +		ret = nla_put_uint(skb,
> +				   ETHTOOL_A_MSE_CAPABILITIES_MAX_AVERAGE_MSE,
> +				   data->capability.max_average_mse);
> +		if (ret < 0)
> +			goto nla_put_cap_failure;
> +	}
> +
> +	if (data->capability.supported_caps & (PHY_MSE_CAP_PEAK |
> +					       PHY_MSE_CAP_WORST_PEAK)) {
> +		ret = nla_put_uint(skb, ETHTOOL_A_MSE_CAPABILITIES_MAX_PEAK_MSE,
> +				   data->capability.max_peak_mse);
> +		if (ret < 0)
> +			goto nla_put_cap_failure;
> +	}
> +
> +	ret = nla_put_uint(skb, ETHTOOL_A_MSE_CAPABILITIES_REFRESH_RATE_PS,
> +			   data->capability.refresh_rate_ps);
> +	if (ret < 0)
> +		goto nla_put_cap_failure;
> +
> +	ret = nla_put_uint(skb, ETHTOOL_A_MSE_CAPABILITIES_NUM_SYMBOLS,
> +			   data->capability.num_symbols);
> +	if (ret < 0)
> +		goto nla_put_cap_failure;
> +
> +	ret = mse_caps_put(skb, ETHTOOL_A_MSE_CAPABILITIES_SUPPORTED_CAPS,
> +			   data->capability.supported_caps,
> +			   req_base->flags & ETHTOOL_FLAG_COMPACT_BITSETS);
> +	if (ret < 0)
> +		goto nla_put_cap_failure;
> +
> +	nla_nest_end(skb, cap_nest);
> +
> +	for (i = 0; i < data->num_snapshots; i++) {
> +		const struct mse_snapshot_entry *s = &data->snapshots[i];
> +		int chan_attr;
> +
> +		switch (s->channel) {
> +		case PHY_MSE_CHANNEL_A:
> +			chan_attr = ETHTOOL_A_MSE_CHANNEL_A;
> +			break;
> +		case PHY_MSE_CHANNEL_B:
> +			chan_attr = ETHTOOL_A_MSE_CHANNEL_B;
> +			break;
> +		case PHY_MSE_CHANNEL_C:
> +			chan_attr = ETHTOOL_A_MSE_CHANNEL_C;
> +			break;
> +		case PHY_MSE_CHANNEL_D:
> +			chan_attr = ETHTOOL_A_MSE_CHANNEL_D;
> +			break;
> +		case PHY_MSE_CHANNEL_WORST:
> +			chan_attr = ETHTOOL_A_MSE_WORST_CHANNEL;
> +			break;
> +		case PHY_MSE_CHANNEL_LINK:
> +			chan_attr = ETHTOOL_A_MSE_LINK;
> +			break;
> +		default:
> +			return -EINVAL;

It looks like the same largish switch is present in
mse_get_one_channel(), I think it would be better to factor it out in a
common helper.

> +		}
> +
> +		snap_nest = nla_nest_start(skb, chan_attr);
> +		if (!snap_nest)
> +			return -EMSGSIZE;
> +
> +		if (data->capability.supported_caps & PHY_MSE_CAP_AVG) {
> +			ret = nla_put_uint(skb,
> +					   ETHTOOL_A_MSE_SNAPSHOT_AVERAGE_MSE,
> +					   s->snapshot.average_mse);
> +			if (ret)
> +				goto nla_put_snap_failure;
> +		}
> +		if (data->capability.supported_caps & PHY_MSE_CAP_PEAK) {
> +			ret = nla_put_uint(skb, ETHTOOL_A_MSE_SNAPSHOT_PEAK_MSE,
> +					   s->snapshot.peak_mse);
> +			if (ret)
> +				goto nla_put_snap_failure;
> +		}
> +		if (data->capability.supported_caps & PHY_MSE_CAP_WORST_PEAK) {
> +			ret = nla_put_uint(skb,
> +					   ETHTOOL_A_MSE_SNAPSHOT_WORST_PEAK_MSE,
> +					   s->snapshot.worst_peak_mse);
> +			if (ret)
> +				goto nla_put_snap_failure;
> +		}
> +
> +		nla_nest_end(skb, snap_nest);
> +	}
> +
> +	return 0;
> +
> +nla_put_cap_failure:
> +	nla_nest_cancel(skb, cap_nest);
> +	return ret;
> +
> +nla_put_snap_failure:
> +	nla_nest_cancel(skb, snap_nest);
> +	return -EMSGSIZE;

Minor: possibly return 'ret' instead? Also you could possibly
consolidate the 2 error path using a single nest variable.

/P