| 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
| ||
|
Message-ID: <ee3d9a4d-641b-413d-997b-3ce37aeb9279@redhat.com>
Date: Thu, 23 Oct 2025 11:10:54 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>, Andrew Lunn <andrew@...n.ch>,
Jakub Kicinski <kuba@...nel.org>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Simon Horman <horms@...nel.org>,
Donald Hunter <donald.hunter@...il.com>, Jonathan Corbet <corbet@....net>,
Heiner Kallweit <hkallweit1@...il.com>, Russell King
<linux@...linux.org.uk>, Kory Maincent <kory.maincent@...tlin.com>,
Maxime Chevallier <maxime.chevallier@...tlin.com>, Nishanth Menon <nm@...com>
Cc: kernel@...gutronix.de, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, UNGLinuxDriver@...rochip.com,
linux-doc@...r.kernel.org, Michal Kubecek <mkubecek@...e.cz>,
Roan van Dijk <roan@...tonic.nl>
Subject: Re: [PATCH net-next v7 2/5] ethtool: netlink: add ETHTOOL_MSG_MSE_GET
and wire up PHY MSE access
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
Powered by blists - more mailing lists