Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS

From: Jakub Kicinski
Date: Tue Dec 06 2022 - 22:50:23 EST

Next message: Zheng Yejian: "[PATCH] tracing/hist: Fix out-of-bound write on 'action_data.var_ref_idx'"
Previous message: Martin K. Petersen: "Re: [PATCH v4 0/4] ufs: core: Always read the descriptors with max length"
In reply to: Piergiorgio Beruto: "[PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS"
Next in thread: Piergiorgio Beruto: "Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, 7 Dec 2022 01:01:23 +0100 Piergiorgio Beruto wrote:
> Add support for configuring the PLCA Reconciliation Sublayer on
> multi-drop PHYs that support IEEE802.3cg-2019 Clause 148 (e.g.,
> 10BASE-T1S). This patch adds the appropriate netlink interface
> to ethtool.

Please LMK if I'm contradicting prior reviewers, I've scanned
the previous versions but may have missed stuff.

> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index f10f8eb44255..fe4847611299 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -1716,6 +1716,136 @@ being used. Current supported options are toeplitz, xor or crc32.
> ETHTOOL_A_RSS_INDIR attribute returns RSS indrection table where each byte
> indicates queue number.
>
> +PLCA_GET_CFG
> +============
> +
> +Gets PLCA RS attributes.

Let's spell out PLCA RS, this is the first use of the term in the doc.

> +Request contents:
> +
> + ===================================== ====== ==========================
> + ``ETHTOOL_A_PLCA_HEADER`` nested request header
> + ===================================== ====== ==========================
> +
> +Kernel response contents:
> +
> + ====================================== ====== =============================
> + ``ETHTOOL_A_PLCA_HEADER`` nested reply header
> + ``ETHTOOL_A_PLCA_VERSION`` u16 Supported PLCA management
> + interface standard/version
> + ``ETHTOOL_A_PLCA_ENABLED`` u8 PLCA Admin State
> + ``ETHTOOL_A_PLCA_NODE_ID`` u8 PLCA unique local node ID
> + ``ETHTOOL_A_PLCA_NODE_CNT`` u8 Number of PLCA nodes on the
> + netkork, including the

netkork -> network

> + coordinator

This is 30.16.1.1.3 aPLCANodeCount ? The phrasing of the help is quite
different than the standard. Pure count should be max node + 1 (IOW max
of 256, which won't fit into u8, hence the question)
Or is node 255 reserved?

> + ``ETHTOOL_A_PLCA_TO_TMR`` u8 Transmit Opportunity Timer
> + value in bit-times (BT)
> + ``ETHTOOL_A_PLCA_BURST_CNT`` u8 Number of additional packets
> + the node is allowed to send
> + within a single TO
> + ``ETHTOOL_A_PLCA_BURST_TMR`` u8 Time to wait for the MAC to
> + transmit a new frame before
> + terminating the burst

Please consider making the fields u16 or u32. Netlink pads all
attributes to 4B, and once we decide the size in the user API
we can never change it. So even if the standard says max is 255
if some vendor somewhere may decide to allow a bigger range we
may be better off using a u32 type and limiting the accepted
range in the netlink policy (grep for NLA_POLICY_MAX())

> + ====================================== ====== =============================
> +
> +When set, the optional ``ETHTOOL_A_PLCA_VERSION`` attribute indicates which
> +standard and version the PLCA management interface complies to. When not set,
> +the interface is vendor-specific and (possibly) supplied by the driver.
> +The OPEN Alliance SIG specifies a standard register map for 10BASE-T1S PHYs
> +embedding the PLCA Reconcialiation Sublayer. See "10BASE-T1S PLCA Management
> +Registers" at https://www.opensig.org/about/specifications/. When this standard
> +is supported, ETHTOOL_A_PLCA_VERSION is reported as 0Axx where 'xx' denotes the

you put backticks around other attr names but not here

TBH I can't parse the "ETHTOOL_A_PLCA_VERSION is reported as 0Axx
where.." sentence. Specifically I'm confused about what the 0A is.

> +map version (see Table A.1.0 — IDVER bits assignment).

> +When set, the optional ``ETHTOOL_A_PLCA_ENABLED`` attribute indicates the
> +administrative state of the PLCA RS. When not set, the node operates in "plain"
> +CSMA/CD mode. This option is corresponding to ``IEEE 802.3cg-2019`` 30.16.1.1.1
> +aPLCAAdminState / 30.16.1.2.1 acPLCAAdminControl.
> +
> +When set, the optional ``ETHTOOL_A_PLCA_NODE_ID`` attribute indicates the
> +configured local node ID of the PHY. This ID determines which transmit
> +opportunity (TO) is reserved for the node to transmit into. This option is
> +corresponding to ``IEEE 802.3cg-2019`` 30.16.1.1.4 aPLCALocalNodeID.
> +
> +When set, the optional ``ETHTOOL_A_PLCA_NODE_CNT`` attribute indicates the
> +configured maximum number of PLCA nodes on the mixing-segment. This number
> +determines the total number of transmit opportunities generated during a
> +PLCA cycle. This attribute is relevant only for the PLCA coordinator, which is
> +the node with aPLCALocalNodeID set to 0. Follower nodes ignore this setting.
> +This option is corresponding to ``IEEE 802.3cg-2019`` 30.16.1.1.3
> +aPLCANodeCount.
> +
> +When set, the optional ``ETHTOOL_A_PLCA_TO_TMR`` attribute indicates the
> +configured value of the transmit opportunity timer in bit-times. This value
> +must be set equal across all nodes sharing the medium for PLCA to work
> +correctly. This option is corresponding to ``IEEE 802.3cg-2019`` 30.16.1.1.5
> +aPLCATransmitOpportunityTimer.
> +
> +When set, the optional ``ETHTOOL_A_PLCA_BURST_CNT`` attribute indicates the
> +configured number of extra packets that the node is allowed to send during a
> +single transmit opportunity. By default, this attribute is 0, meaning that
> +the node can only send a sigle frame per TO. When greater than 0, the PLCA RS
> +keeps the TO after any transmission, waiting for the MAC to send a new frame
> +for up to aPLCABurstTimer BTs. This can only happen a number of times per PLCA
> +cycle up to the value of this parameter. After that, the burst is over and the
> +normal counting of TOs resumes. This option is corresponding to
> +``IEEE 802.3cg-2019`` 30.16.1.1.6 aPLCAMaxBurstCount.
> +
> +When set, the optional ``ETHTOOL_A_PLCA_BURST_TMR`` attribute indicates how
> +many bit-times the PLCA RS waits for the MAC to initiate a new transmission
> +when aPLCAMaxBurstCount is greater than 0. If the MAC fails to send a new
> +frame within this time, the burst ends and the counting of TOs resumes.
> +Otherwise, the new frame is sent as part of the current burst. This option
> +is corresponding to ``IEEE 802.3cg-2019`` 30.16.1.1.7 aPLCABurstTimer.
> +
> +PLCA_SET_CFG
> +============
> +
> +Sets PLCA RS parameters.
> +
> +Request contents:
> +
> + ====================================== ====== =============================
> + ``ETHTOOL_A_PLCA_HEADER`` nested request header
> + ``ETHTOOL_A_PLCA_ENABLED`` u8 PLCA Admin State
> + ``ETHTOOL_A_PLCA_NODE_ID`` u8 PLCA unique local node ID
> + ``ETHTOOL_A_PLCA_NODE_CNT`` u8 Number of PLCA nodes on the
> + netkork, including the
> + coordinator
> + ``ETHTOOL_A_PLCA_TO_TMR`` u8 Transmit Opportunity Timer
> + value in bit-times (BT)
> + ``ETHTOOL_A_PLCA_BURST_CNT`` u8 Number of additional packets
> + the node is allowed to send
> + within a single TO
> + ``ETHTOOL_A_PLCA_BURST_TMR`` u8 Time to wait for the MAC to
> + transmit a new frame before
> + terminating the burst
> + ====================================== ====== =============================
> +
> +For a description of each attribute, see ``PLCA_GET_CFG``.
> +
> +PLCA_GET_STATUS
> +===============
> +
> +Gets PLCA RS status information.
> +
> +Request contents:
> +
> + ===================================== ====== ==========================
> + ``ETHTOOL_A_PLCA_HEADER`` nested request header
> + ===================================== ====== ==========================
> +
> +Kernel response contents:
> +
> + ====================================== ====== =============================
> + ``ETHTOOL_A_PLCA_HEADER`` nested reply header
> + ``ETHTOOL_A_PLCA_STATUS`` u8 PLCA RS operational status
> + ====================================== ====== =============================
> +
> +When set, the ``ETHTOOL_A_PLCA_STATUS`` attribute indicates whether the node is
> +detecting the presence of the BEACON on the network. This flag is
> +corresponding to ``IEEE 802.3cg-2019`` 30.16.1.1.2 aPLCAStatus.

I noticed some count attributes in the spec, are these statistics?
Do any of your devices support them? It'd be good to add support in
a fixed format via net/ethtool/stats.c from the start, so that people
don't start inventing their own ways of reporting them.

(feel free to ask for more guidance, the stats support is a bit spread
out throughout the code)

> * struct ethtool_phy_ops - Optional PHY device options
> * @get_sset_count: Get number of strings that @get_strings will write.
> * @get_strings: Return a set of strings that describe the requested objects
> * @get_stats: Return extended statistics about the PHY device.
> + * @get_plca_cfg: Return PLCA configuration.
> + * @set_plca_cfg: Set PLCA configuration.

missing get status in kdoc

> * @start_cable_test: Start a cable test
> * @start_cable_test_tdr: Start a Time Domain Reflectometry cable test
> *
> @@ -819,6 +823,13 @@ struct ethtool_phy_ops {
> int (*get_strings)(struct phy_device *dev, u8 *data);
> int (*get_stats)(struct phy_device *dev,
> struct ethtool_stats *stats, u64 *data);
> + int (*get_plca_cfg)(struct phy_device *dev,
> + struct phy_plca_cfg *plca_cfg);
> + int (*set_plca_cfg)(struct phy_device *dev,
> + struct netlink_ext_ack *extack,
> + const struct phy_plca_cfg *plca_cfg);

extack is usually the last argument

> + int (*get_plca_status)(struct phy_device *dev,
> + struct phy_plca_status *plca_st);

get status doesn't need exact? I guess..

> int (*start_cable_test)(struct phy_device *phydev,
> struct netlink_ext_ack *extack);
> int (*start_cable_test_tdr)(struct phy_device *phydev,
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 71eeb4e3b1fd..f3ecc9a86e67 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -765,6 +765,63 @@ struct phy_tdr_config {
> };
> #define PHY_PAIR_ALL -1
>
> +/**
> + * struct phy_plca_cfg - Configuration of the PLCA (Physical Layer Collision
> + * Avoidance) Reconciliation Sublayer.
> + *
> + * @version: read-only PLCA register map version. 0 = not available. Ignored

^^^^^^^^^^^^^^^^^^

> + * when setting the configuration. Format is the same as reported by the PLCA
> + * IDVER register (31.CA00). -1 = not available.

^^^^^^^^^^^^^^^^^^^

So is it 0 or -1 that's N/A for this field? :)

> + * @enabled: PLCA configured mode (enabled/disabled). -1 = not available / don't
> + * set. 0 = disabled, anything else = enabled.
> + * @node_id: the PLCA local node identifier. -1 = not available / don't set.
> + * Allowed values [0 .. 254]. 255 = node disabled.
> + * @node_cnt: the PLCA node count (maximum number of nodes having a TO). Only
> + * meaningful for the coordinator (node_id = 0). -1 = not available / don't
> + * set. Allowed values [0 .. 255].
> + * @to_tmr: The value of the PLCA to_timer in bit-times, which determines the
> + * PLCA transmit opportunity window opening. See IEEE802.3 Clause 148 for
> + * more details. The to_timer shall be set equal over all nodes.
> + * -1 = not available / don't set. Allowed values [0 .. 255].
> + * @burst_cnt: controls how many additional frames a node is allowed to send in
> + * single transmit opportunity (TO). The default value of 0 means that the
> + * node is allowed exactly one frame per TO. A value of 1 allows two frames
> + * per TO, and so on. -1 = not available / don't set.
> + * Allowed values [0 .. 255].
> + * @burst_tmr: controls how many bit times to wait for the MAC to send a new
> + * frame before interrupting the burst. This value should be set to a value
> + * greater than the MAC inter-packet gap (which is typically 96 bits).
> + * -1 = not available / don't set. Allowed values [0 .. 255].

> +struct phy_plca_cfg {
> + s32 version;
> + s16 enabled;
> + s16 node_id;
> + s16 node_cnt;
> + s16 to_tmr;
> + s16 burst_cnt;
> + s16 burst_tmr;

make them all int, oddly sized integers are only a source of trouble

> +};
> +
> +/**
> + * struct phy_plca_status - Status of the PLCA (Physical Layer Collision
> + * Avoidance) Reconciliation Sublayer.
> + *
> + * @pst: The PLCA status as reported by the PST bit in the PLCA STATUS
> + * register(31.CA03), indicating BEACON activity.
> + *
> + * A structure containing status information of the PLCA RS configuration.
> + * The driver does not need to implement all the parameters, but should report
> + * what is actually used.
> + */
> +struct phy_plca_status {
> + bool pst;
> +};

> +#include <linux/phy.h>
> +#include <linux/ethtool_netlink.h>
> +
> +#include "netlink.h"
> +#include "common.h"
> +
> +struct plca_req_info {
> + struct ethnl_req_info base;
> +};
> +
> +struct plca_reply_data {
> + struct ethnl_reply_data base;
> + struct phy_plca_cfg plca_cfg;
> + struct phy_plca_status plca_st;
> +};
> +
> +#define PLCA_REPDATA(__reply_base) \
> + container_of(__reply_base, struct plca_reply_data, base)
> +
> +// PLCA get configuration message ------------------------------------------- //
> +
> +const struct nla_policy ethnl_plca_get_cfg_policy[] = {
> + [ETHTOOL_A_PLCA_HEADER] =
> + NLA_POLICY_NESTED(ethnl_header_policy),
> +};
> +
> +static int plca_get_cfg_prepare_data(const struct ethnl_req_info *req_base,
> + struct ethnl_reply_data *reply_base,
> + struct genl_info *info)
> +{
> + struct plca_reply_data *data = PLCA_REPDATA(reply_base);
> + struct net_device *dev = reply_base->dev;
> + const struct ethtool_phy_ops *ops;
> + int ret;
> +
> + // check that the PHY device is available and connected
> + if (!dev->phydev) {
> + ret = -EOPNOTSUPP;
> + goto out;
> + }
> +
> + // note: rtnl_lock is held already by ethnl_default_doit
> + ops = ethtool_phy_ops;
> + if (!ops || !ops->get_plca_cfg) {
> + ret = -EOPNOTSUPP;
> + goto out;
> + }
> +
> + ret = ethnl_ops_begin(dev);
> + if (ret < 0)
> + goto out;
> +
> + ret = ops->get_plca_cfg(dev->phydev, &data->plca_cfg);
> + if (ret < 0)
> + goto out;

You still need to complete the op, no? Don't jump over that..

> + ethnl_ops_complete(dev);
> +
> +out:
> + return ret;
> +}

> + if ((plca->version >= 0 &&
> + nla_put_u16(skb, ETHTOOL_A_PLCA_VERSION, (u16)plca->version)) ||
> + (plca->enabled >= 0 &&
> + nla_put_u8(skb, ETHTOOL_A_PLCA_ENABLED, !!plca->enabled)) ||
> + (plca->node_id >= 0 &&
> + nla_put_u8(skb, ETHTOOL_A_PLCA_NODE_ID, (u8)plca->node_id)) ||
> + (plca->node_cnt >= 0 &&
> + nla_put_u8(skb, ETHTOOL_A_PLCA_NODE_CNT, (u8)plca->node_cnt)) ||
> + (plca->to_tmr >= 0 &&
> + nla_put_u8(skb, ETHTOOL_A_PLCA_TO_TMR, (u8)plca->to_tmr)) ||
> + (plca->burst_cnt >= 0 &&
> + nla_put_u8(skb, ETHTOOL_A_PLCA_BURST_CNT, (u8)plca->burst_cnt)) ||
> + (plca->burst_tmr >= 0 &&
> + nla_put_u8(skb, ETHTOOL_A_PLCA_BURST_TMR, (u8)plca->burst_tmr)))

The casts are unnecessary, but if you really really want them they
can stay..

> + return -EMSGSIZE;
> +
> + return 0;
> +};

> +const struct nla_policy ethnl_plca_set_cfg_policy[] = {
> + [ETHTOOL_A_PLCA_HEADER] =
> + NLA_POLICY_NESTED(ethnl_header_policy),
> + [ETHTOOL_A_PLCA_ENABLED] = { .type = NLA_U8 },

NLA_POLICY_MAX(NLA_U8, 1)

> + [ETHTOOL_A_PLCA_NODE_ID] = { .type = NLA_U8 },

Does this one also need check against 255 or is 255 allowed?

> + [ETHTOOL_A_PLCA_NODE_CNT] = { .type = NLA_U8 },
> + [ETHTOOL_A_PLCA_TO_TMR] = { .type = NLA_U8 },
> + [ETHTOOL_A_PLCA_BURST_CNT] = { .type = NLA_U8 },
> + [ETHTOOL_A_PLCA_BURST_TMR] = { .type = NLA_U8 },

> +int ethnl_set_plca_cfg(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct ethnl_req_info req_info = {};
> + struct nlattr **tb = info->attrs;
> + const struct ethtool_phy_ops *ops;
> + struct phy_plca_cfg plca_cfg;
> + struct net_device *dev;
> +

spurious new line

> + bool mod = false;
> + int ret;
> +
> + ret = ethnl_parse_header_dev_get(&req_info,
> + tb[ETHTOOL_A_PLCA_HEADER],
> + genl_info_net(info), info->extack,
> + true);
> + if (ret < 0)
> + return ret;
> +
> + dev = req_info.dev;
> +
> + // check that the PHY device is available and connected

Comment slightly misplaced now?

> + rtnl_lock();
> +
> + if (!dev->phydev) {
> + ret = -EOPNOTSUPP;
> + goto out_rtnl;
> + }
> +
> + ops = ethtool_phy_ops;
> + if (!ops || !ops->set_plca_cfg) {
> + ret = -EOPNOTSUPP;
> + goto out_rtnl;
> + }
> +
> + ret = ethnl_ops_begin(dev);
> + if (ret < 0)
> + goto out_rtnl;
> +
> + memset(&plca_cfg, 0xFF, sizeof(plca_cfg));
> +
> + if (tb[ETHTOOL_A_PLCA_ENABLED]) {
> + plca_cfg.enabled = !!nla_get_u8(tb[ETHTOOL_A_PLCA_ENABLED]);
> + mod = true;
> + }
> +
> + if (tb[ETHTOOL_A_PLCA_NODE_ID]) {
> + plca_cfg.node_id = nla_get_u8(tb[ETHTOOL_A_PLCA_NODE_ID]);
> + mod = true;
> + }
> +
> + if (tb[ETHTOOL_A_PLCA_NODE_CNT]) {
> + plca_cfg.node_cnt = nla_get_u8(tb[ETHTOOL_A_PLCA_NODE_CNT]);
> + mod = true;
> + }
> +
> + if (tb[ETHTOOL_A_PLCA_TO_TMR]) {
> + plca_cfg.to_tmr = nla_get_u8(tb[ETHTOOL_A_PLCA_TO_TMR]);
> + mod = true;
> + }
> +
> + if (tb[ETHTOOL_A_PLCA_BURST_CNT]) {
> + plca_cfg.burst_cnt = nla_get_u8(tb[ETHTOOL_A_PLCA_BURST_CNT]);
> + mod = true;
> + }
> +
> + if (tb[ETHTOOL_A_PLCA_BURST_TMR]) {
> + plca_cfg.burst_tmr = nla_get_u8(tb[ETHTOOL_A_PLCA_BURST_TMR]);
> + mod = true;
> + }

Could you add a helper for the modifications? A'la ethnl_update_u8, but
accounting for the oddness in types (ergo probably locally in this file
rather that in the global scope)?

> + ret = 0;
> + if (!mod)
> + goto out_ops;
> +
> + ret = ops->set_plca_cfg(dev->phydev, info->extack, &plca_cfg);
> +

spurious new line

> + if (ret < 0)
> + goto out_ops;
> +
> + ethtool_notify(dev, ETHTOOL_MSG_PLCA_NTF, NULL);
> +
> +out_ops:
> + ethnl_ops_complete(dev);
> +out_rtnl:
> + rtnl_unlock();
> + ethnl_parse_header_dev_put(&req_info);
> +
> + return ret;
> +}
> +
> +// PLCA get status message -------------------------------------------------- //
> +
> +const struct nla_policy ethnl_plca_get_status_policy[] = {
> + [ETHTOOL_A_PLCA_HEADER] =
> + NLA_POLICY_NESTED(ethnl_header_policy),
> +};
> +
> +static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
> + struct ethnl_reply_data *reply_base,
> + struct genl_info *info)
> +{
> + struct plca_reply_data *data = PLCA_REPDATA(reply_base);
> + struct net_device *dev = reply_base->dev;
> + const struct ethtool_phy_ops *ops;
> + int ret;
> +
> + // check that the PHY device is available and connected
> + if (!dev->phydev) {
> + ret = -EOPNOTSUPP;
> + goto out;
> + }
> +
> + // note: rtnl_lock is held already by ethnl_default_doit
> + ops = ethtool_phy_ops;
> + if (!ops || !ops->get_plca_status) {
> + ret = -EOPNOTSUPP;
> + goto out;
> + }
> +
> + ret = ethnl_ops_begin(dev);
> + if (ret < 0)
> + goto out;
> +
> + ret = ops->get_plca_status(dev->phydev, &data->plca_st);
> + if (ret < 0)
> + goto out;

don't skip complete

> + ethnl_ops_complete(dev);
> +out:
> + return ret;
> +}

Next message: Zheng Yejian: "[PATCH] tracing/hist: Fix out-of-bound write on 'action_data.var_ref_idx'"
Previous message: Martin K. Petersen: "Re: [PATCH v4 0/4] ufs: core: Always read the descriptors with max length"
In reply to: Piergiorgio Beruto: "[PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS"
Next in thread: Piergiorgio Beruto: "Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]