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

Commit Message

Piergiorgio Beruto Dec. 7, 2022, 12:01 a.m. UTC

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.

Signed-off-by: Piergiorgio Beruto <[e-mail]>
---
 Documentation/networking/ethtool-netlink.rst | 133 +++++++++
 MAINTAINERS                                  |   6 +
 include/linux/ethtool.h                      |  11 +
 include/linux/phy.h                          |  64 ++++
 include/uapi/linux/ethtool_netlink.h         |  25 ++
 net/ethtool/Makefile                         |   2 +-
 net/ethtool/netlink.c                        |  29 ++
 net/ethtool/netlink.h                        |   6 +
 net/ethtool/plca.c                           | 290 +++++++++++++++++++
 9 files changed, 565 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/plca.c

Comments

Jakub Kicinski Dec. 7, 2022, 3:50 a.m. UTC | #1

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;
> +}

Piergiorgio Beruto Dec. 7, 2022, 1:08 p.m. UTC | #2

Hi Jakub,
thank you very much for your thorough review. Please see my answers
interleaved.

On Tue, Dec 06, 2022 at 07:50:14PM -0800, Jakub Kicinski wrote:
> > 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.
Fixed. New sentence is "Gets the IEEE 802.3cg-2019 Clause 148 Physical
Layer Collision Avoidance (PLCA) Reconciliation Sublayer (RS) attributes."
 
> > +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
Got it, thanks.

> > +                                                  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?
This is indeed aPLCANodeCount. What standard are you referring to
exactly? This is the excerpt from IEEE802.3cg-2019

"
30.16.1.1.3 aPLCANodeCount
ATTRIBUTE
APPROPRIATE SYNTAX:
INTEGER
BEHAVIOUR DEFINED AS:
This value is assigned to define the number of nodes getting a transmit opportunity before a new
BEACON is generated. Valid range is 0 to 255, inclusive. The default value is 8.;
"

This is what I can read from Clause 148.4.4.1:
"
plca_node_count

Maximum number of PLCA nodes on the mixing segment receiving transmit
opportunities before the node with local_nodeID = 0 generates a new 
BEACON, reflecting the value of aPLCANodeCount. This parameter is 
meaningful only for the node with local_nodeID = 0; otherwise, it is
ignored.

Values: integer number from 0 to 255
"

And this is what I can read in the OPEN Alliance documentation:

"
4.3.1 NCNT
This field sets the maximum number of PLCA nodes supported on the multidrop
network. On the node with PLCA ID = 0 (see 4.3.2), this value must be set at
least to the number of nodes that may be plugged to the network in order for
PLCA to operate properly. This bit maps to the aPLCANodeCount object in [1]
Clause 30.
"

So the valid range is actually 1..255. A value of 0 does not really mean
anything. PHYs would just clamp this to 1. So maybe we should set a
minimum limit in the kernel?

Please, feel free to ask more questions here, it is important that we
fully understand what this is. Fortunately, I am the guy who invented
PLCA and wrote the specs, so I should be able to answer these questions :-D.

> 
> > +  ``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())
Ok, modifed according to your indication. I honestly hardly believe it
would make any sense to expand those variables in the future, PLCA works
well for a limited number of nodes and for small TO_TIMER values. Above
128, CSMA/CD starts to be competitive and above 255 there is no question
that CSMA/CD is better. But nevertheless, I'm ok with this change.


> > +  ======================================  ======  =============================
> > +
> > +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
Got it, thanks.

> 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.
How about this: "When this standard is supported, the upper byte of
``ETHTOOL_A_PLCA_VERSION`` shall be 0x0A (see Table A.1.0 — IDVER 
bits assignment).

> > +
> > +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)
Are you referring to this?

"
45.2.3.68f.1 CorruptedTxCnt (3.2294.15:0)
Bits 3.2294.15:0 count up at each positive edge of the MII signal COL.
When the maximum allowed value (65 535) is reached, the count stops until
this register is cleared by a read operation.
"

This is the only one statistic counter I can think of. Although, it is a
10BASE-T1S PHY related register, it is not specific to PLCA, even if its
main purpose is to help the user distinguish between logical and
physical collisions.

I would be inclined to add this as a separate feature unrelated to PLCA.
Please, let me know what you think.

> >   * 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
Fixed. Good catch.

> 
> >   * @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
I actually copied from the cable_test_tdr callback below. Do you still
want me to change the order? Should we do this for cable test as well?
(as a different patch maybe?). Please, let me know.

> 
> > +	int (*get_plca_status)(struct phy_device *dev,
> > +			       struct phy_plca_status *plca_st);
> 
> get status doesn't need exact? I guess..
This is my assumption. I would say it is similar to get_config, and I
would say it is mandatory. I can hardly think of a system that does not
implement get_status when supporting PLCA.
Thoughts?

> 
> >  	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? :)
Ah! It's obviously -1. I just forgot to update the comment... Thanks for
noticing!

> 
> > + * @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
Ok, done.

> > +	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..
Oops. Fixed it.


> > +	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..
Removed it. Sorry, In the past I used to work in an environment where
thay wanted -unnecessary- casts everywhere. I just need to get used to
this...

> 
> > +		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)
Oh, yes, it is a bool. Fixed.

> 
> > +	[ETHTOOL_A_PLCA_NODE_ID]	= { .type = NLA_U8 },
> 
> Does this one also need check against 255 or is 255 allowed?
Good question. 255 has a special meaning --> unconfigured.
So the question is, do we allow the user to set nodeID back to
unconfigured? My opinion is yes, I don't really see a reson why not and
I can see cases where I actually want to do this.
Would you agree?

> 
> > +	[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
Fixed

> 
> > +	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?
Fixed

> 
> > +	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)?
I put the helper locally. We can always promote them later if more
features follow this 'new' approach suggested by Andrew. Makes sense?

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

> 
> > +	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
Copy & paste error... Fixed again! Thanks!

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

Andrew Lunn Dec. 7, 2022, 2:16 p.m. UTC | #3

> > 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.
> How about this: "When this standard is supported, the upper byte of
> ``ETHTOOL_A_PLCA_VERSION`` shall be 0x0A (see Table A.1.0 — IDVER 
> bits assignment).

I think the 0x0A is pointless and should not be included here. If the
register does not contain 0x0A, the device does not follow the open
alliance standard, and hence the lower part of the register is
meaningless.

This is why i suggested -ENODEV should actually be returned on invalid
values in this register.

> > >   * 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
> Fixed. Good catch.

Building with W=1 C=2 will tell you about kerneldoc issues. Ideally we
want all network code to be clean with these two options.

     Andrew

Piergiorgio Beruto Dec. 7, 2022, 3:42 p.m. UTC | #4

On Wed, Dec 07, 2022 at 03:16:00PM +0100, Andrew Lunn wrote:
> > > 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.
> > How about this: "When this standard is supported, the upper byte of
> > ``ETHTOOL_A_PLCA_VERSION`` shall be 0x0A (see Table A.1.0 — IDVER 
> > bits assignment).
> 
> I think the 0x0A is pointless and should not be included here. If the
> register does not contain 0x0A, the device does not follow the open
> alliance standard, and hence the lower part of the register is
> meaningless.
> 
> This is why i suggested -ENODEV should actually be returned on invalid
> values in this register.
I already integrated this change in v5 (returning -ENODEV). Give what you're
saying, I can just remove that sentence from the documentations. Agreed?

> 
> > > >   * 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
> > Fixed. Good catch.
> 
> Building with W=1 C=2 will tell you about kerneldoc issues. Ideally we
> want all network code to be clean with these two options.
Ok thanks. I probably need to upgrade my machine to achieve this. Will
do.

Piergiorgio Beruto Dec. 7, 2022, 3:49 p.m. UTC | #5

Andrew, Jakub,
I believe this should address your comments for this patch?
It is a diff WRT patch v5.

Thanks,
Piergiorgio

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index fe4847611299..c59b542eb693 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -1719,7 +1719,8 @@ indicates queue number.
 PLCA_GET_CFG
 ============

-Gets PLCA RS attributes.
+Gets the IEEE 802.3cg-2019 Clause 148 Physical Layer Collision Avoidance
+(PLCA) Reconciliation Sublayer (RS) attributes.

 Request contents:

@@ -1734,16 +1735,16 @@ Kernel response contents:
   ``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
+  ``ETHTOOL_A_PLCA_NODE_ID``              u32     PLCA unique local node ID
+  ``ETHTOOL_A_PLCA_NODE_CNT``             u32     Number of PLCA nodes on the
+                                                  network, including the
                                                   coordinator
-  ``ETHTOOL_A_PLCA_TO_TMR``               u8      Transmit Opportunity Timer
+  ``ETHTOOL_A_PLCA_TO_TMR``               u32     Transmit Opportunity Timer
                                                   value in bit-times (BT)
-  ``ETHTOOL_A_PLCA_BURST_CNT``            u8      Number of additional packets
+  ``ETHTOOL_A_PLCA_BURST_CNT``            u32     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
+  ``ETHTOOL_A_PLCA_BURST_TMR``            u32     Time to wait for the MAC to
                                                   transmit a new frame before
                                                   terminating the burst
   ======================================  ======  =============================
@@ -1753,9 +1754,7 @@ 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
-map version (see Table A.1.0 — IDVER bits assignment).
+Registers" at https://www.opensig.org/about/specifications/.

 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"
@@ -1765,7 +1764,8 @@ 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.
+corresponding to ``IEEE 802.3cg-2019`` 30.16.1.1.4 aPLCALocalNodeID. The valid
+range for this attribute is [0 .. 255] where 255 means "not configured".

 When set, the optional ``ETHTOOL_A_PLCA_NODE_CNT`` attribute indicates the
 configured maximum number of PLCA nodes on the mixing-segment. This number
@@ -1773,13 +1773,14 @@ 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.
+aPLCANodeCount. The valid range for this attribute is [1 .. 255].

 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.
+aPLCATransmitOpportunityTimer. The valid range for this attribute is
+[0 .. 255].

 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
@@ -1789,14 +1790,18 @@ 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.
+``IEEE 802.3cg-2019`` 30.16.1.1.6 aPLCAMaxBurstCount. The valid range for this
+attribute is [0 .. 255].

 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.
+is corresponding to ``IEEE 802.3cg-2019`` 30.16.1.1.7 aPLCABurstTimer. The
+valid range for this attribute is [0 .. 255]. Although, the value should be
+set greater than the Inter-Frame-Gap (IFG) time of the MAC (plus some margin)
+for PLCA burst mode to work as intended.

 PLCA_SET_CFG
 ============
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 4bfe95ec1f0a..8b1a27210589 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -812,6 +812,7 @@ struct phy_plca_status;
  * @get_stats: Return extended statistics about the PHY device.
  * @get_plca_cfg: Return PLCA configuration.
  * @set_plca_cfg: Set PLCA configuration.
+ * @get_plca_status: Get PLCA configuration.
  * @start_cable_test: Start a cable test
  * @start_cable_test_tdr: Start a Time Domain Reflectometry cable test
  *
diff --git a/include/linux/phy.h b/include/linux/phy.h
index f3ecc9a86e67..d23951cf76ca 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -769,7 +769,7 @@ struct phy_tdr_config {
  * 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
+ * @version: read-only PLCA register map version. -1 = not available. Ignored
  *   when setting the configuration. Format is the same as reported by the PLCA
  *   IDVER register (31.CA00). -1 = not available.
  * @enabled: PLCA configured mode (enabled/disabled). -1 = not available / don't
@@ -798,13 +798,13 @@ struct phy_tdr_config {
  * but should report what is actually used.
  */
 struct phy_plca_cfg {
-	s32 version;
-	s16 enabled;
-	s16 node_id;
-	s16 node_cnt;
-	s16 to_tmr;
-	s16 burst_cnt;
-	s16 burst_tmr;
+	int version;
+	int enabled;
+	int node_id;
+	int node_cnt;
+	int to_tmr;
+	int burst_cnt;
+	int burst_tmr;
 };

 /**
diff --git a/net/ethtool/plca.c b/net/ethtool/plca.c
index 0282acab1c4d..eec77cb94848 100644
--- a/net/ethtool/plca.c
+++ b/net/ethtool/plca.c
@@ -16,9 +16,23 @@ struct plca_reply_data {
 	struct phy_plca_status		plca_st;
 };

+
+// Helpers ------------------------------------------------------------------ //
+
 #define PLCA_REPDATA(__reply_base) \
 	container_of(__reply_base, struct plca_reply_data, base)

+static inline void plca_update_sint(int *dst, const struct nlattr *attr,
+				    bool *mod)
+{
+	if (!attr)
+		*dst = -1;
+	else {
+		*dst = nla_get_u32(attr);
+		*mod = true;
+	}
+}
+
 // PLCA get configuration message ------------------------------------------- //

 const struct nla_policy ethnl_plca_get_cfg_policy[] = {
@@ -53,9 +67,6 @@ static int plca_get_cfg_prepare_data(const struct ethnl_req_info *req_base,
 		goto out;

 	ret = ops->get_plca_cfg(dev->phydev, &data->plca_cfg);
-	if (ret < 0)
-		goto out;
-
 	ethnl_ops_complete(dev);

 out:
@@ -83,19 +94,19 @@ static int plca_get_cfg_fill_reply(struct sk_buff *skb,
 	const struct phy_plca_cfg *plca = &data->plca_cfg;

 	if ((plca->version >= 0 &&
-	     nla_put_u16(skb, ETHTOOL_A_PLCA_VERSION, (u16)plca->version)) ||
+	     nla_put_u16(skb, ETHTOOL_A_PLCA_VERSION, 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)) ||
+	     nla_put_u32(skb, ETHTOOL_A_PLCA_NODE_ID, plca->node_id)) ||
 	    (plca->node_cnt >= 0 &&
-	     nla_put_u8(skb, ETHTOOL_A_PLCA_NODE_CNT, (u8)plca->node_cnt)) ||
+	     nla_put_u32(skb, ETHTOOL_A_PLCA_NODE_CNT, plca->node_cnt)) ||
 	    (plca->to_tmr >= 0 &&
-	     nla_put_u8(skb, ETHTOOL_A_PLCA_TO_TMR, (u8)plca->to_tmr)) ||
+	     nla_put_u32(skb, ETHTOOL_A_PLCA_TO_TMR, plca->to_tmr)) ||
 	    (plca->burst_cnt >= 0 &&
-	     nla_put_u8(skb, ETHTOOL_A_PLCA_BURST_CNT, (u8)plca->burst_cnt)) ||
+	     nla_put_u32(skb, ETHTOOL_A_PLCA_BURST_CNT, plca->burst_cnt)) ||
 	    (plca->burst_tmr >= 0 &&
-	     nla_put_u8(skb, ETHTOOL_A_PLCA_BURST_TMR, (u8)plca->burst_tmr)))
+	     nla_put_u32(skb, ETHTOOL_A_PLCA_BURST_TMR, plca->burst_tmr)))
 		return -EMSGSIZE;

 	return 0;
@@ -118,12 +129,12 @@ const struct ethnl_request_ops ethnl_plca_cfg_request_ops = {
 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 },
-	[ETHTOOL_A_PLCA_NODE_ID]	= { .type = NLA_U8 },
-	[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 },
+	[ETHTOOL_A_PLCA_ENABLED]	= NLA_POLICY_MAX(NLA_U8, 1),
+	[ETHTOOL_A_PLCA_NODE_ID]	= NLA_POLICY_MAX(NLA_U32, 255),
+	[ETHTOOL_A_PLCA_NODE_CNT]	= NLA_POLICY_RANGE(NLA_U32, 1, 255),
+	[ETHTOOL_A_PLCA_TO_TMR]		= NLA_POLICY_MAX(NLA_U32, 255),
+	[ETHTOOL_A_PLCA_BURST_CNT]	= NLA_POLICY_MAX(NLA_U32, 255),
+	[ETHTOOL_A_PLCA_BURST_TMR]	= NLA_POLICY_MAX(NLA_U32, 255),
 };

 int ethnl_set_plca_cfg(struct sk_buff *skb, struct genl_info *info)
@@ -133,7 +144,6 @@ int ethnl_set_plca_cfg(struct sk_buff *skb, struct genl_info *info)
 	const struct ethtool_phy_ops *ops;
 	struct phy_plca_cfg plca_cfg;
 	struct net_device *dev;
-
 	bool mod = false;
 	int ret;

@@ -146,9 +156,9 @@ int ethnl_set_plca_cfg(struct sk_buff *skb, struct genl_info *info)

 	dev = req_info.dev;

-	// check that the PHY device is available and connected
 	rtnl_lock();

+	// check that the PHY device is available and connected
 	if (!dev->phydev) {
 		ret = -EOPNOTSUPP;
 		goto out_rtnl;
@@ -164,44 +174,20 @@ int ethnl_set_plca_cfg(struct sk_buff *skb, struct genl_info *info)
 	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;
-	}
+	plca_update_sint(&plca_cfg.enabled, tb[ETHTOOL_A_PLCA_ENABLED], &mod);
+	plca_update_sint(&plca_cfg.node_id, tb[ETHTOOL_A_PLCA_NODE_ID], &mod);
+	plca_update_sint(&plca_cfg.node_cnt, tb[ETHTOOL_A_PLCA_NODE_CNT], &mod);
+	plca_update_sint(&plca_cfg.to_tmr, tb[ETHTOOL_A_PLCA_TO_TMR], &mod);
+	plca_update_sint(&plca_cfg.burst_cnt, tb[ETHTOOL_A_PLCA_BURST_CNT],
+			 &mod);
+	plca_update_sint(&plca_cfg.burst_tmr, tb[ETHTOOL_A_PLCA_BURST_TMR],
+			 &mod);

 	ret = 0;
 	if (!mod)
 		goto out_ops;

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

@@ -250,9 +236,6 @@ static int plca_get_status_prepare_data(const struct ethnl_req_info *req_base,
 		goto out;

 	ret = ops->get_plca_status(dev->phydev, &data->plca_st);
-	if (ret < 0)
-		goto out;
-
 	ethnl_ops_complete(dev);
 out:
 	return ret;

Andrew Lunn Dec. 7, 2022, 4:06 p.m. UTC | #6

On Wed, Dec 07, 2022 at 04:42:15PM +0100, Piergiorgio Beruto wrote:
> On Wed, Dec 07, 2022 at 03:16:00PM +0100, Andrew Lunn wrote:
> > > > 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.
> > > How about this: "When this standard is supported, the upper byte of
> > > ``ETHTOOL_A_PLCA_VERSION`` shall be 0x0A (see Table A.1.0 — IDVER 
> > > bits assignment).
> > 
> > I think the 0x0A is pointless and should not be included here. If the
> > register does not contain 0x0A, the device does not follow the open
> > alliance standard, and hence the lower part of the register is
> > meaningless.
> > 
> > This is why i suggested -ENODEV should actually be returned on invalid
> > values in this register.
> I already integrated this change in v5 (returning -ENODEV). Give what you're
> saying, I can just remove that sentence from the documentations. Agreed?

And only return the actual version value, not the 0x0A.

    Andrew

Piergiorgio Beruto Dec. 7, 2022, 4:17 p.m. UTC | #7

On Wed, Dec 07, 2022 at 05:06:42PM +0100, Andrew Lunn wrote:
> On Wed, Dec 07, 2022 at 04:42:15PM +0100, Piergiorgio Beruto wrote:
> > On Wed, Dec 07, 2022 at 03:16:00PM +0100, Andrew Lunn wrote:
> > > > > 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.
> > > > How about this: "When this standard is supported, the upper byte of
> > > > ``ETHTOOL_A_PLCA_VERSION`` shall be 0x0A (see Table A.1.0 — IDVER 
> > > > bits assignment).
> > > 
> > > I think the 0x0A is pointless and should not be included here. If the
> > > register does not contain 0x0A, the device does not follow the open
> > > alliance standard, and hence the lower part of the register is
> > > meaningless.
> > > 
> > > This is why i suggested -ENODEV should actually be returned on invalid
> > > values in this register.
> > I already integrated this change in v5 (returning -ENODEV). Give what you're
> > saying, I can just remove that sentence from the documentations. Agreed?
> 
> And only return the actual version value, not the 0x0A.
About this, at the moment I am reporting the 0x0A to allow in the future
possible extensions of the standard. A single byte for the version may
be too limited given this technology is relatively fresh.
What you think of this?

Andrew Lunn Dec. 7, 2022, 5:48 p.m. UTC | #8

> > And only return the actual version value, not the 0x0A.
> About this, at the moment I am reporting the 0x0A to allow in the future
> possible extensions of the standard. A single byte for the version may
> be too limited given this technology is relatively fresh.
> What you think of this?

What does the standards document say about this 0x0A?

     Andrew

Piergiorgio Beruto Dec. 7, 2022, 6:44 p.m. UTC | #9

On Wed, Dec 07, 2022 at 06:48:03PM +0100, Andrew Lunn wrote:
> > > And only return the actual version value, not the 0x0A.
> > About this, at the moment I am reporting the 0x0A to allow in the future
> > possible extensions of the standard. A single byte for the version may
> > be too limited given this technology is relatively fresh.
> > What you think of this?
> 
> What does the standards document say about this 0x0A?
It doesn't actually say much, except that it is the identifier for the
OPEN Alliance mapping.

Excerpt:

"
4.1.1 IDM
Constant field indicating that the address space is defined by this document.
These bits shall read as 0x0A (Open Alliance).

4.1.2 VER
Constant field indicating the version of this document the register map
conforms to. Some registers/bits defined herein may not be available in all
revisions. The management entity can read this register to provide
backward compatibility. For the present revision of this specification,
these bits shall read as indicated in Table A.1.0/Value.
"

Thanks,
Piergiorgio

Andrew Lunn Dec. 7, 2022, 7:25 p.m. UTC | #10

> 4.1.1 IDM
> Constant field indicating that the address space is defined by this document.
> These bits shall read as 0x0A (Open Alliance).

So it is local to this document. It has no global meaning within Open
Alliance, so some other working group could use the same value in the
same location, and it has a totally different meaning.

Also, 'by this document' means any future changes need to be in this
document. Except when they are in another document, and decide to
reuse the value 0x0a because it is local to the document....

So it actually looks like 0x0a does not have much meaning. 

So why return it?

Does Open Alliance have any sort of global registry of magic numbers
which are unique across specifications? Maybe you want to add another
register whos value is not defined by this document, but something
with bigger scope?

   Andrew

Jakub Kicinski Dec. 8, 2022, 2:10 a.m. UTC | #11

On Wed, 7 Dec 2022 14:08:51 +0100 Piergiorgio Beruto wrote:
> > > +  ``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  
> Got it, thanks.
> 
> > > +                                                  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?  
> This is indeed aPLCANodeCount. What standard are you referring to
> exactly? This is the excerpt from IEEE802.3cg-2019
> 
> "
> 30.16.1.1.3 aPLCANodeCount
> ATTRIBUTE
> APPROPRIATE SYNTAX:
> INTEGER
> BEHAVIOUR DEFINED AS:
> This value is assigned to define the number of nodes getting a transmit opportunity before a new
> BEACON is generated. Valid range is 0 to 255, inclusive. The default value is 8.;

FWIW this is what I was referring to. To a layperson this description
reads like a beacon interval. While the name and you documentation
sounds like the define max number of endpoints. 

> And this is what I can read in the OPEN Alliance documentation:
> 
> "
> 4.3.1 NCNT
> This field sets the maximum number of PLCA nodes supported on the multidrop
> network. On the node with PLCA ID = 0 (see 4.3.2), this value must be set at
> least to the number of nodes that may be plugged to the network in order for
> PLCA to operate properly. This bit maps to the aPLCANodeCount object in [1]
> Clause 30.
> "
> 
> So the valid range is actually 1..255. A value of 0 does not really mean
> anything. PHYs would just clamp this to 1. So maybe we should set a
> minimum limit in the kernel?

SG, loosing limits is easy. Introducing them once they are in 
a released kernel is almost impossible.

> Please, feel free to ask more questions here, it is important that we
> fully understand what this is. Fortunately, I am the guy who invented
> PLCA and wrote the specs, so I should be able to answer these questions :-D.

I am a little curious why beacon interval == node count here, but don't
want to take up too much of your time for explaining things I could
likely understand by reading the spec, rather than fuzzy-mapping onto
concepts I comprehend :(

This is completely unrelated to the series - do you know if any of
the new 10BASE stuff can easily run over a DC power rail within 
a server rack? :)

> > > +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)  
> Are you referring to this?
> 
> "
> 45.2.3.68f.1 CorruptedTxCnt (3.2294.15:0)
> Bits 3.2294.15:0 count up at each positive edge of the MII signal COL.
> When the maximum allowed value (65 535) is reached, the count stops until
> this register is cleared by a read operation.
> "
> 
> This is the only one statistic counter I can think of. Although, it is a
> 10BASE-T1S PHY related register, it is not specific to PLCA, even if its
> main purpose is to help the user distinguish between logical and
> physical collisions.
> 
> I would be inclined to add this as a separate feature unrelated to PLCA.
> Please, let me know what you think.

Fair point, I just opened the standard and searched counters.
Indeed outside of the scope here.

> > >   * @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  
> I actually copied from the cable_test_tdr callback below. Do you still
> want me to change the order? Should we do this for cable test as well?
> (as a different patch maybe?). Please, let me know.

Let's not move it in the existing callbacks but yes, cable_test_tdr
is more of an exception than a rule here, I reckon.

> > > +	int (*get_plca_status)(struct phy_device *dev,
> > > +			       struct phy_plca_status *plca_st);  
> > 
> > get status doesn't need exact? I guess..  
> This is my assumption. I would say it is similar to get_config, and I
> would say it is mandatory. I can hardly think of a system that does not
> implement get_status when supporting PLCA.
> Thoughts?

Sounds good.

> > The casts are unnecessary, but if you really really want them they 
> > can stay..  
> Removed it. Sorry, In the past I used to work in an environment where
> thay wanted -unnecessary- casts everywhere. I just need to get used to
> this...

:( Turns out the C language is evolving more than one would have
thought. Or at least what's considered good taste. In the kernel
we don't require casts. Here the helper has type in its name,
so it's very obvious.

> > > +	[ETHTOOL_A_PLCA_NODE_ID]	= { .type = NLA_U8 },  
> > 
> > Does this one also need check against 255 or is 255 allowed?  
> Good question. 255 has a special meaning --> unconfigured.
> So the question is, do we allow the user to set nodeID back to
> unconfigured? My opinion is yes, I don't really see a reson why not and
> I can see cases where I actually want to do this.
> Would you agree?

Just asking because of the tendency to limit inputs where possible
for backward compatibility. I'd think user space should be happy
with just disabling a node, but I defer to your expertise on whether
there's possible cases for giving the access to the full range to 
the user.

> > > +	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)?  
> I put the helper locally. We can always promote them later if more
> features follow this 'new' approach suggested by Andrew. Makes sense?

SG

Jakub Kicinski Dec. 8, 2022, 2:12 a.m. UTC | #12

On Wed, 7 Dec 2022 16:49:57 +0100 Piergiorgio Beruto wrote:
> Andrew, Jakub,
> I believe this should address your comments for this patch?
> It is a diff WRT patch v5.

It does mine, I think, thanks!

Piergiorgio Beruto Dec. 8, 2022, 2:48 p.m. UTC | #13

> > > 
> > > 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?  
> > This is indeed aPLCANodeCount. What standard are you referring to
> > exactly? This is the excerpt from IEEE802.3cg-2019
> > 
> > "
> > 30.16.1.1.3 aPLCANodeCount
> > ATTRIBUTE
> > APPROPRIATE SYNTAX:
> > INTEGER
> > BEHAVIOUR DEFINED AS:
> > This value is assigned to define the number of nodes getting a transmit opportunity before a new
> > BEACON is generated. Valid range is 0 to 255, inclusive. The default value is 8.;
> 
> FWIW this is what I was referring to. To a layperson this description
> reads like a beacon interval. While the name and you documentation
> sounds like the define max number of endpoints.
I agree, the description is not friendly to the average user, it mostly
comes from IEEE jargon. What it really means is that the coordinator
(master) will count up to aPLCANodeCount TOs before generating a new
BEACON. This means that nodes shall have IDs from 1 to plcaNodeCount - 1
to get a TO. ID = 0 is reserved for the coordinator.

> 
> > And this is what I can read in the OPEN Alliance documentation:
> > 
> > "
> > 4.3.1 NCNT
> > This field sets the maximum number of PLCA nodes supported on the multidrop
> > network. On the node with PLCA ID = 0 (see 4.3.2), this value must be set at
> > least to the number of nodes that may be plugged to the network in order for
> > PLCA to operate properly. This bit maps to the aPLCANodeCount object in [1]
> > Clause 30.
> > "
> > 
> > So the valid range is actually 1..255. A value of 0 does not really mean
> > anything. PHYs would just clamp this to 1. So maybe we should set a
> > minimum limit in the kernel?
> 
> SG, loosing limits is easy. Introducing them once they are in 
> a released kernel is almost impossible.
Ok, so let's set the lower limit to 1.

> 
> > Please, feel free to ask more questions here, it is important that we
> > fully understand what this is. Fortunately, I am the guy who invented
> > PLCA and wrote the specs, so I should be able to answer these questions :-D.
> 
> I am a little curious why beacon interval == node count here, but don't
> want to take up too much of your time for explaining things I could
> likely understand by reading the spec, rather than fuzzy-mapping onto
> concepts I comprehend :(
See also my answer above. The basic idea is that you have a concept of
'cycle', similar to a TDMA system, and each node has a chance to
transmit in turn (round-robin). The difference with a TDMA system is that
the TO is only TO_TIMER bits by default, but once a node "commits" to
the TO by initiating a transmission, the TO itself can be extended up to
the MTU multiplied by the configured burst counter (which by default is
0, meaning 1 as it is 0-based). In other words, the end of a TO is
determined by the detection of the end-of-frame. Note that PLCA is a
fully distributed system, meaning that each node counts the TOs on its
own and no PLCA information is ever shared over the media. It is a full
Layer 1 protocol.

So the coordinator node is the one periodically sending a BEACON *which
is a layer 1 signal) which indicates the start of a cycle, and its
coding is made as such that it is easy to detect in noisy environments,
and you can detect the end of it with a high precision.

The node count is the only parameter that the coordinat shall know in
order to send a new BEACON periodically (tracking the TOs based on what
nodes transmit ofc).

I am writing a wikipedia article as well to better explain all of these
concepts. I will keep you posted if you're interested.

In the meantime, you can check this for a general overview:
https://www.dropbox.com/s/ao9m23s6fd7ytns/AEC2019%2010BASE-T1S%20Highlights%20on%20Key%20Figures%20of%20the%2010BASE-T1S%20Multidrop%20PHY.pptx?dl=0

>
> This is completely unrelated to the series - do you know if any of
> the new 10BASE stuff can easily run over a DC power rail within
> a server rack? :)
Well, the 10BASE-T1S does.
I recall DELL presenting exactly this use-case in the IEEE.

All xBASE-T1* PHYs support PoDL (power over data line).


> > Are you referring to this?
> > 
> > "
> > 45.2.3.68f.1 CorruptedTxCnt (3.2294.15:0)
> > Bits 3.2294.15:0 count up at each positive edge of the MII signal COL.
> > When the maximum allowed value (65 535) is reached, the count stops until
> > this register is cleared by a read operation.
> > "
> > 
> > This is the only one statistic counter I can think of. Although, it is a
> > 10BASE-T1S PHY related register, it is not specific to PLCA, even if its
> > main purpose is to help the user distinguish between logical and
> > physical collisions.
> > 
> > I would be inclined to add this as a separate feature unrelated to PLCA.
> > Please, let me know what you think.
> 
> Fair point, I just opened the standard and searched counters.
> Indeed outside of the scope here.
Ok, but still a thing to keep in mind, thanks for pointing this out.

> > > extack is usually the last argument  
> > I actually copied from the cable_test_tdr callback below. Do you still
> > want me to change the order? Should we do this for cable test as well?
> > (as a different patch maybe?). Please, let me know.
> 
> Let's not move it in the existing callbacks but yes, cable_test_tdr
> is more of an exception than a rule here, I reckon.
Ok, so I will change the order for the plca functions.

Piergiorgio Beruto Dec. 8, 2022, 5:03 p.m. UTC | #14

On Wed, Dec 07, 2022 at 08:25:04PM +0100, Andrew Lunn wrote:
> > 4.1.1 IDM
> > Constant field indicating that the address space is defined by this document.
> > These bits shall read as 0x0A (Open Alliance).
> 
> So it is local to this document. It has no global meaning within Open
> Alliance, so some other working group could use the same value in the
> same location, and it has a totally different meaning.
Actually, we are sharing an excel file with all register addresses. This file
is internal to the OPEN Alliance, but global across the various TCs. I
understand it is not a strong guarantee, but the OPEN review process should
check that no one else re-uses the same addresses for other purposes.

> 
> Also, 'by this document' means any future changes need to be in this
> document. Except when they are in another document, and decide to
> reuse the value 0x0a because it is local to the document....
No, that cannto happen (see above). Not within the OPEN at least.
Unfortunately, this global excel sheet for registers was introdiced
AFTER the release date of this document, therefore you see this
statement.

> So it actually looks like 0x0a does not have much meaning. 
> 
> So why return it?
> 
> Does Open Alliance have any sort of global registry of magic numbers
> which are unique across specifications? Maybe you want to add another
> register whos value is not defined by this document, but something
> with bigger scope?
AT the moment, only TC14 (T1S) is using the excel sheet I mentioned, but
we're pushing to make this a global registry across all groups.

Given what I just said, what would you suggest to do?

Thanks,
Piergiorgio

Andrew Lunn Dec. 11, 2022, 11:12 a.m. UTC | #15

> Given what I just said, what would you suggest to do?

I would return just the version part, not the whole register contents.

The nice thing about netlink is you can add extra attributes without
breaking the ABI. If that 0xA ever gains a meaning and more values, an
attribute for it can be added.

   Andrew

Piergiorgio Beruto Dec. 11, 2022, 11:43 a.m. UTC | #16

On Sun, Dec 11, 2022 at 12:12:23PM +0100, Andrew Lunn wrote:
> > Given what I just said, what would you suggest to do?
> 
> I would return just the version part, not the whole register contents.
> 
> The nice thing about netlink is you can add extra attributes without
> breaking the ABI. If that 0xA ever gains a meaning and more values, an
> attribute for it can be added.
TBFH, this time I cannot say I fully agree with that. However, if this
is something you require to approve the changes, would you like me to
change the attribute VERSION down to an u8 or just masking the 0x0A?

Thanks,
Piergiorgio

Patch

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.
+
+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
+                                                  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
+  ======================================  ======  =============================
+
+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
+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.
+
 Request translation
 ===================
 
@@ -1817,4 +1947,7 @@  are netlink only.
   n/a                                 ``ETHTOOL_MSG_PHC_VCLOCKS_GET``
   n/a                                 ``ETHTOOL_MSG_MODULE_GET``
   n/a                                 ``ETHTOOL_MSG_MODULE_SET``
+  n/a                                 ``ETHTOOL_MSG_PLCA_GET_CFG``
+  n/a                                 ``ETHTOOL_MSG_PLCA_SET_CFG``
+  n/a                                 ``ETHTOOL_MSG_PLCA_GET_STATUS``
   =================================== =====================================
diff --git a/MAINTAINERS b/MAINTAINERS
index 955c1be1efb2..7952243e4b43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16396,6 +16396,12 @@  S:	Maintained
 F:	Documentation/devicetree/bindings/iio/chemical/plantower,pms7003.yaml
 F:	drivers/iio/chemical/pms7003.c
 
+PLCA RECONCILIATION SUBLAYER (IEEE802.3 Clause 148)
+M:	Piergiorgio Beruto <[e-mail]>
+L:	[e-mail]
+S:	Maintained
+F:	net/ethtool/plca.c
+
 PLDMFW LIBRARY
 M:	Jacob Keller <[e-mail]>
 S:	Maintained
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 9e0a76fc7de9..4bfe95ec1f0a 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -802,12 +802,16 @@  int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
 
 struct phy_device;
 struct phy_tdr_config;
+struct phy_plca_cfg;
+struct phy_plca_status;
 
 /**
  * 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.
  * @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);
+	int (*get_plca_status)(struct phy_device *dev,
+			       struct phy_plca_status *plca_st);
 	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.
+ * @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].
+ *
+ * A structure containing configuration parameters for setting/getting the PLCA
+ * RS configuration. The driver does not need to implement all the parameters,
+ * but should report what is actually used.
+ */
+struct phy_plca_cfg {
+	s32 version;
+	s16 enabled;
+	s16 node_id;
+	s16 node_cnt;
+	s16 to_tmr;
+	s16 burst_cnt;
+	s16 burst_tmr;
+};
+
+/**
+ * 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;
+};
+
 /**
  * struct phy_driver - Driver structure for a particular PHY type
  *
@@ -1775,6 +1832,13 @@  int phy_ethtool_get_strings(struct phy_device *phydev, u8 *data);
 int phy_ethtool_get_sset_count(struct phy_device *phydev);
 int phy_ethtool_get_stats(struct phy_device *phydev,
 			  struct ethtool_stats *stats, u64 *data);
+int phy_ethtool_get_plca_cfg(struct phy_device *phydev,
+			     struct phy_plca_cfg *plca_cfg);
+int phy_ethtool_set_plca_cfg(struct phy_device *phydev,
+			     struct netlink_ext_ack *extack,
+			     const struct phy_plca_cfg *plca_cfg);
+int phy_ethtool_get_plca_status(struct phy_device *phydev,
+				struct phy_plca_status *plca_st);
 
 static inline int phy_package_read(struct phy_device *phydev, u32 regnum)
 {
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 5799a9db034e..7ba98ecc811c 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -52,6 +52,9 @@  enum {
 	ETHTOOL_MSG_PSE_GET,
 	ETHTOOL_MSG_PSE_SET,
 	ETHTOOL_MSG_RSS_GET,
+	ETHTOOL_MSG_PLCA_GET_CFG,
+	ETHTOOL_MSG_PLCA_SET_CFG,
+	ETHTOOL_MSG_PLCA_GET_STATUS,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -99,6 +102,9 @@  enum {
 	ETHTOOL_MSG_MODULE_NTF,
 	ETHTOOL_MSG_PSE_GET_REPLY,
 	ETHTOOL_MSG_RSS_GET_REPLY,
+	ETHTOOL_MSG_PLCA_GET_CFG_REPLY,
+	ETHTOOL_MSG_PLCA_GET_STATUS_REPLY,
+	ETHTOOL_MSG_PLCA_NTF,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -894,6 +900,25 @@  enum {
 	ETHTOOL_A_RSS_MAX = (__ETHTOOL_A_RSS_CNT - 1),
 };
 
+/* PLCA */
+
+enum {
+	ETHTOOL_A_PLCA_UNSPEC,
+	ETHTOOL_A_PLCA_HEADER,			/* nest - _A_HEADER_* */
+	ETHTOOL_A_PLCA_VERSION,			/* u16 */
+	ETHTOOL_A_PLCA_ENABLED,			/* u8 */
+	ETHTOOL_A_PLCA_STATUS,			/* u8 */
+	ETHTOOL_A_PLCA_NODE_CNT,		/* u8 */
+	ETHTOOL_A_PLCA_NODE_ID,			/* u8 */
+	ETHTOOL_A_PLCA_TO_TMR,			/* u8 */
+	ETHTOOL_A_PLCA_BURST_CNT,		/* u8 */
+	ETHTOOL_A_PLCA_BURST_TMR,		/* u8 */
+
+	/* add new constants above here */
+	__ETHTOOL_A_PLCA_CNT,
+	ETHTOOL_A_PLCA_MAX = (__ETHTOOL_A_PLCA_CNT - 1)
+};
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 228f13df2e18..563864c1bf5a 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -8,4 +8,4 @@  ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o rss.o \
 		   linkstate.o debug.o wol.o features.o privflags.o rings.o \
 		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o \
 		   tunnels.o fec.o eeprom.o stats.o phc_vclocks.o module.o \
-		   pse-pd.o
+		   pse-pd.o plca.o
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index aee98be6237f..9f924875bba9 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -288,6 +288,8 @@  ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_MODULE_GET]	= &ethnl_module_request_ops,
 	[ETHTOOL_MSG_PSE_GET]		= &ethnl_pse_request_ops,
 	[ETHTOOL_MSG_RSS_GET]		= &ethnl_rss_request_ops,
+	[ETHTOOL_MSG_PLCA_GET_CFG]	= &ethnl_plca_cfg_request_ops,
+	[ETHTOOL_MSG_PLCA_GET_STATUS]	= &ethnl_plca_status_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -603,6 +605,7 @@  ethnl_default_notify_ops[ETHTOOL_MSG_KERNEL_MAX + 1] = {
 	[ETHTOOL_MSG_EEE_NTF]		= &ethnl_eee_request_ops,
 	[ETHTOOL_MSG_FEC_NTF]		= &ethnl_fec_request_ops,
 	[ETHTOOL_MSG_MODULE_NTF]	= &ethnl_module_request_ops,
+	[ETHTOOL_MSG_PLCA_NTF]		= &ethnl_plca_cfg_request_ops,
 };
 
 /* default notification handler */
@@ -696,6 +699,7 @@  static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
 	[ETHTOOL_MSG_EEE_NTF]		= ethnl_default_notify,
 	[ETHTOOL_MSG_FEC_NTF]		= ethnl_default_notify,
 	[ETHTOOL_MSG_MODULE_NTF]	= ethnl_default_notify,
+	[ETHTOOL_MSG_PLCA_NTF]		= ethnl_default_notify,
 };
 
 void ethtool_notify(struct net_device *dev, unsigned int cmd, const void *data)
@@ -1047,6 +1051,31 @@  static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_rss_get_policy,
 		.maxattr = ARRAY_SIZE(ethnl_rss_get_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_PLCA_GET_CFG,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_plca_get_cfg_policy,
+		.maxattr = ARRAY_SIZE(ethnl_plca_get_cfg_policy) - 1,
+	},
+	{
+		.cmd	= ETHTOOL_MSG_PLCA_SET_CFG,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_set_plca_cfg,
+		.policy = ethnl_plca_set_cfg_policy,
+		.maxattr = ARRAY_SIZE(ethnl_plca_set_cfg_policy) - 1,
+	},
+	{
+		.cmd	= ETHTOOL_MSG_PLCA_GET_STATUS,
+		.doit	= ethnl_default_doit,
+		.start	= ethnl_default_start,
+		.dumpit	= ethnl_default_dumpit,
+		.done	= ethnl_default_done,
+		.policy = ethnl_plca_get_status_policy,
+		.maxattr = ARRAY_SIZE(ethnl_plca_get_status_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 3753787ba233..f271266f6e28 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -347,6 +347,8 @@  extern const struct ethnl_request_ops ethnl_phc_vclocks_request_ops;
 extern const struct ethnl_request_ops ethnl_module_request_ops;
 extern const struct ethnl_request_ops ethnl_pse_request_ops;
 extern const struct ethnl_request_ops ethnl_rss_request_ops;
+extern const struct ethnl_request_ops ethnl_plca_cfg_request_ops;
+extern const struct ethnl_request_ops ethnl_plca_status_request_ops;
 
 extern const struct nla_policy ethnl_header_policy[ETHTOOL_A_HEADER_FLAGS + 1];
 extern const struct nla_policy ethnl_header_policy_stats[ETHTOOL_A_HEADER_FLAGS + 1];
@@ -388,6 +390,9 @@  extern const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MO
 extern const struct nla_policy ethnl_pse_get_policy[ETHTOOL_A_PSE_HEADER + 1];
 extern const struct nla_policy ethnl_pse_set_policy[ETHTOOL_A_PSE_MAX + 1];
 extern const struct nla_policy ethnl_rss_get_policy[ETHTOOL_A_RSS_CONTEXT + 1];
+extern const struct nla_policy ethnl_plca_get_cfg_policy[ETHTOOL_A_PLCA_HEADER + 1];
+extern const struct nla_policy ethnl_plca_set_cfg_policy[ETHTOOL_A_PLCA_MAX + 1];
+extern const struct nla_policy ethnl_plca_get_status_policy[ETHTOOL_A_PLCA_HEADER + 1];
 
 int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info);
@@ -408,6 +413,7 @@  int ethnl_tunnel_info_dumpit(struct sk_buff *skb, struct netlink_callback *cb);
 int ethnl_set_fec(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_module(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_pse(struct sk_buff *skb, struct genl_info *info);
+int ethnl_set_plca_cfg(struct sk_buff *skb, struct genl_info *info);
 
 extern const char stats_std_names[__ETHTOOL_STATS_CNT][ETH_GSTRING_LEN];
 extern const char stats_eth_phy_names[__ETHTOOL_A_STATS_ETH_PHY_CNT][ETH_GSTRING_LEN];
diff --git a/net/ethtool/plca.c b/net/ethtool/plca.c
new file mode 100644
index 000000000000..0282acab1c4d
--- /dev/null
+++ b/net/ethtool/plca.c
@@ -0,0 +1,290 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+
+#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;
+
+	ethnl_ops_complete(dev);
+
+out:
+	return ret;
+}
+
+static int plca_get_cfg_reply_size(const struct ethnl_req_info *req_base,
+				   const struct ethnl_reply_data *reply_base)
+{
+	return nla_total_size(sizeof(u16)) +	/* _VERSION */
+	       nla_total_size(sizeof(u8)) +	/* _ENABLED */
+	       nla_total_size(sizeof(u8)) +	/* _STATUS  */
+	       nla_total_size(sizeof(u8)) +	/* _NODE_CNT */
+	       nla_total_size(sizeof(u8)) +	/* _NODE_ID */
+	       nla_total_size(sizeof(u8)) +	/* _TO_TIMER */
+	       nla_total_size(sizeof(u8)) +	/* _BURST_COUNT */
+	       nla_total_size(sizeof(u8));	/* _BURST_TIMER */
+}
+
+static int plca_get_cfg_fill_reply(struct sk_buff *skb,
+				   const struct ethnl_req_info *req_base,
+				   const struct ethnl_reply_data *reply_base)
+{
+	const struct plca_reply_data *data = PLCA_REPDATA(reply_base);
+	const struct phy_plca_cfg *plca = &data->plca_cfg;
+
+	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)))
+		return -EMSGSIZE;
+
+	return 0;
+};
+
+const struct ethnl_request_ops ethnl_plca_cfg_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_PLCA_GET_CFG,
+	.reply_cmd		= ETHTOOL_MSG_PLCA_GET_CFG_REPLY,
+	.hdr_attr		= ETHTOOL_A_PLCA_HEADER,
+	.req_info_size		= sizeof(struct plca_req_info),
+	.reply_data_size	= sizeof(struct plca_reply_data),
+
+	.prepare_data		= plca_get_cfg_prepare_data,
+	.reply_size		= plca_get_cfg_reply_size,
+	.fill_reply		= plca_get_cfg_fill_reply,
+};
+
+// PLCA set configuration message ------------------------------------------- //
+
+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 },
+	[ETHTOOL_A_PLCA_NODE_ID]	= { .type = NLA_U8 },
+	[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;
+
+	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
+	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;
+	}
+
+	ret = 0;
+	if (!mod)
+		goto out_ops;
+
+	ret = ops->set_plca_cfg(dev->phydev, info->extack, &plca_cfg);
+
+	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;
+
+	ethnl_ops_complete(dev);
+out:
+	return ret;
+}
+
+static int plca_get_status_reply_size(const struct ethnl_req_info *req_base,
+				      const struct ethnl_reply_data *reply_base)
+{
+	return nla_total_size(sizeof(u8));	/* _STATUS */
+}
+
+static int plca_get_status_fill_reply(struct sk_buff *skb,
+				      const struct ethnl_req_info *req_base,
+				      const struct ethnl_reply_data *reply_base)
+{
+	const struct plca_reply_data *data = PLCA_REPDATA(reply_base);
+	const u8 status = data->plca_st.pst;
+
+	if (nla_put_u8(skb, ETHTOOL_A_PLCA_STATUS, !!status))
+		return -EMSGSIZE;
+
+	return 0;
+};
+
+const struct ethnl_request_ops ethnl_plca_status_request_ops = {
+	.request_cmd		= ETHTOOL_MSG_PLCA_GET_STATUS,
+	.reply_cmd		= ETHTOOL_MSG_PLCA_GET_STATUS_REPLY,
+	.hdr_attr		= ETHTOOL_A_PLCA_HEADER,
+	.req_info_size		= sizeof(struct plca_req_info),
+	.reply_data_size	= sizeof(struct plca_reply_data),
+
+	.prepare_data		= plca_get_status_prepare_data,
+	.reply_size		= plca_get_status_reply_size,
+	.fill_reply		= plca_get_status_fill_reply,
+};