* [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-07 0:00 [PATCH v5 net-next 0/5] add PLCA RS support and onsemi NCN26000 Piergiorgio Beruto
@ 2022-12-07 0:01 ` Piergiorgio Beruto
2022-12-07 3:50 ` Jakub Kicinski
2022-12-07 0:01 ` [PATCH v5 net-next 2/5] drivers/net/phy: add the link modes for the 10BASE-T1S Ethernet PHY Piergiorgio Beruto
` (4 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Piergiorgio Beruto @ 2022-12-07 0:01 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: linux-kernel, netdev, Oleksij Rempel
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 <piergiorgio.beruto@gmail.com>
---
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
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 <piergiorgio.beruto@gmail.com>
+L: netdev@vger.kernel.org
+S: Maintained
+F: net/ethtool/plca.c
+
PLDMFW LIBRARY
M: Jacob Keller <jacob.e.keller@intel.com>
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] = ðnl_module_request_ops,
[ETHTOOL_MSG_PSE_GET] = ðnl_pse_request_ops,
[ETHTOOL_MSG_RSS_GET] = ðnl_rss_request_ops,
+ [ETHTOOL_MSG_PLCA_GET_CFG] = ðnl_plca_cfg_request_ops,
+ [ETHTOOL_MSG_PLCA_GET_STATUS] = ðnl_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] = ðnl_eee_request_ops,
[ETHTOOL_MSG_FEC_NTF] = ðnl_fec_request_ops,
[ETHTOOL_MSG_MODULE_NTF] = ðnl_module_request_ops,
+ [ETHTOOL_MSG_PLCA_NTF] = ðnl_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,
+};
--
2.35.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-07 0:01 ` [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS Piergiorgio Beruto
@ 2022-12-07 3:50 ` Jakub Kicinski
2022-12-07 13:08 ` Piergiorgio Beruto
0 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2022-12-07 3:50 UTC (permalink / raw)
To: Piergiorgio Beruto
Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
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;
> +}
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-07 3:50 ` Jakub Kicinski
@ 2022-12-07 13:08 ` Piergiorgio Beruto
2022-12-07 14:16 ` Andrew Lunn
2022-12-08 2:10 ` Jakub Kicinski
0 siblings, 2 replies; 23+ messages in thread
From: Piergiorgio Beruto @ 2022-12-07 13:08 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
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;
> > +}
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-07 13:08 ` Piergiorgio Beruto
@ 2022-12-07 14:16 ` Andrew Lunn
2022-12-07 15:42 ` Piergiorgio Beruto
2022-12-07 15:49 ` Piergiorgio Beruto
2022-12-08 2:10 ` Jakub Kicinski
1 sibling, 2 replies; 23+ messages in thread
From: Andrew Lunn @ 2022-12-07 14:16 UTC (permalink / raw)
To: Piergiorgio Beruto
Cc: Jakub Kicinski, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
> > 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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-07 14:16 ` Andrew Lunn
@ 2022-12-07 15:42 ` Piergiorgio Beruto
2022-12-07 16:06 ` Andrew Lunn
2022-12-07 15:49 ` Piergiorgio Beruto
1 sibling, 1 reply; 23+ messages in thread
From: Piergiorgio Beruto @ 2022-12-07 15:42 UTC (permalink / raw)
To: Andrew Lunn
Cc: Jakub Kicinski, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-07 15:42 ` Piergiorgio Beruto
@ 2022-12-07 16:06 ` Andrew Lunn
2022-12-07 16:17 ` Piergiorgio Beruto
0 siblings, 1 reply; 23+ messages in thread
From: Andrew Lunn @ 2022-12-07 16:06 UTC (permalink / raw)
To: Piergiorgio Beruto
Cc: Jakub Kicinski, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-07 16:06 ` Andrew Lunn
@ 2022-12-07 16:17 ` Piergiorgio Beruto
2022-12-07 17:48 ` Andrew Lunn
0 siblings, 1 reply; 23+ messages in thread
From: Piergiorgio Beruto @ 2022-12-07 16:17 UTC (permalink / raw)
To: Andrew Lunn
Cc: Jakub Kicinski, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
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?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-07 16:17 ` Piergiorgio Beruto
@ 2022-12-07 17:48 ` Andrew Lunn
2022-12-07 18:44 ` Piergiorgio Beruto
0 siblings, 1 reply; 23+ messages in thread
From: Andrew Lunn @ 2022-12-07 17:48 UTC (permalink / raw)
To: Piergiorgio Beruto
Cc: Jakub Kicinski, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
> > 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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-07 17:48 ` Andrew Lunn
@ 2022-12-07 18:44 ` Piergiorgio Beruto
2022-12-07 19:25 ` Andrew Lunn
0 siblings, 1 reply; 23+ messages in thread
From: Piergiorgio Beruto @ 2022-12-07 18:44 UTC (permalink / raw)
To: Andrew Lunn
Cc: Jakub Kicinski, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-07 18:44 ` Piergiorgio Beruto
@ 2022-12-07 19:25 ` Andrew Lunn
2022-12-08 17:03 ` Piergiorgio Beruto
0 siblings, 1 reply; 23+ messages in thread
From: Andrew Lunn @ 2022-12-07 19:25 UTC (permalink / raw)
To: Piergiorgio Beruto
Cc: Jakub Kicinski, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
> 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-07 19:25 ` Andrew Lunn
@ 2022-12-08 17:03 ` Piergiorgio Beruto
2022-12-11 11:12 ` Andrew Lunn
0 siblings, 1 reply; 23+ messages in thread
From: Piergiorgio Beruto @ 2022-12-08 17:03 UTC (permalink / raw)
To: Andrew Lunn
Cc: Jakub Kicinski, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-08 17:03 ` Piergiorgio Beruto
@ 2022-12-11 11:12 ` Andrew Lunn
2022-12-11 11:43 ` Piergiorgio Beruto
0 siblings, 1 reply; 23+ messages in thread
From: Andrew Lunn @ 2022-12-11 11:12 UTC (permalink / raw)
To: Piergiorgio Beruto
Cc: Jakub Kicinski, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
> 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-11 11:12 ` Andrew Lunn
@ 2022-12-11 11:43 ` Piergiorgio Beruto
0 siblings, 0 replies; 23+ messages in thread
From: Piergiorgio Beruto @ 2022-12-11 11:43 UTC (permalink / raw)
To: Andrew Lunn
Cc: Jakub Kicinski, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-07 14:16 ` Andrew Lunn
2022-12-07 15:42 ` Piergiorgio Beruto
@ 2022-12-07 15:49 ` Piergiorgio Beruto
2022-12-08 2:12 ` Jakub Kicinski
1 sibling, 1 reply; 23+ messages in thread
From: Piergiorgio Beruto @ 2022-12-07 15:49 UTC (permalink / raw)
To: Andrew Lunn
Cc: Jakub Kicinski, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
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;
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-07 15:49 ` Piergiorgio Beruto
@ 2022-12-08 2:12 ` Jakub Kicinski
0 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2022-12-08 2:12 UTC (permalink / raw)
To: Piergiorgio Beruto
Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
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!
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-07 13:08 ` Piergiorgio Beruto
2022-12-07 14:16 ` Andrew Lunn
@ 2022-12-08 2:10 ` Jakub Kicinski
2022-12-08 14:48 ` Piergiorgio Beruto
1 sibling, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2022-12-08 2:10 UTC (permalink / raw)
To: Piergiorgio Beruto
Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS
2022-12-08 2:10 ` Jakub Kicinski
@ 2022-12-08 14:48 ` Piergiorgio Beruto
0 siblings, 0 replies; 23+ messages in thread
From: Piergiorgio Beruto @ 2022-12-08 14:48 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
> > >
> > > 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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 net-next 2/5] drivers/net/phy: add the link modes for the 10BASE-T1S Ethernet PHY
2022-12-07 0:00 [PATCH v5 net-next 0/5] add PLCA RS support and onsemi NCN26000 Piergiorgio Beruto
2022-12-07 0:01 ` [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS Piergiorgio Beruto
@ 2022-12-07 0:01 ` Piergiorgio Beruto
2022-12-07 0:02 ` [PATCH v5 net-next 3/5] drivers/net/phy: add connection between ethtool and phylib for PLCA Piergiorgio Beruto
` (3 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Piergiorgio Beruto @ 2022-12-07 0:01 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: linux-kernel, netdev, Oleksij Rempel
This patch adds the link modes for the IEEE 802.3cg Clause 147 10BASE-T1S
Ethernet PHY. According to the specifications, the 10BASE-T1S supports
Point-To-Point Full-Duplex, Point-To-Point Half-Duplex and/or
Point-To-Multipoint (AKA Multi-Drop) Half-Duplex operations.
Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
---
drivers/net/phy/phy-core.c | 5 ++++-
drivers/net/phy/phylink.c | 6 +++++-
include/linux/phy.h | 11 +++++++++++
include/uapi/linux/ethtool.h | 3 +++
net/ethtool/common.c | 8 ++++++++
5 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 5d08c627a516..a64186dc53f8 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -13,7 +13,7 @@
*/
const char *phy_speed_to_str(int speed)
{
- BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 99,
+ BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 102,
"Enum ethtool_link_mode_bit_indices and phylib are out of sync. "
"If a speed or mode has been added please update phy_speed_to_str "
"and the PHY settings array.\n");
@@ -260,6 +260,9 @@ static const struct phy_setting settings[] = {
PHY_SETTING( 10, FULL, 10baseT_Full ),
PHY_SETTING( 10, HALF, 10baseT_Half ),
PHY_SETTING( 10, FULL, 10baseT1L_Full ),
+ PHY_SETTING( 10, FULL, 10baseT1S_Full ),
+ PHY_SETTING( 10, HALF, 10baseT1S_Half ),
+ PHY_SETTING( 10, HALF, 10baseT1S_P2MP_Half ),
};
#undef PHY_SETTING
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 09cc65c0da93..319790221d7f 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -241,12 +241,16 @@ void phylink_caps_to_linkmodes(unsigned long *linkmodes, unsigned long caps)
if (caps & MAC_ASYM_PAUSE)
__set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, linkmodes);
- if (caps & MAC_10HD)
+ if (caps & MAC_10HD) {
__set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, linkmodes);
+ __set_bit(ETHTOOL_LINK_MODE_10baseT1S_Half_BIT, linkmodes);
+ __set_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT, linkmodes);
+ }
if (caps & MAC_10FD) {
__set_bit(ETHTOOL_LINK_MODE_10baseT_Full_BIT, linkmodes);
__set_bit(ETHTOOL_LINK_MODE_10baseT1L_Full_BIT, linkmodes);
+ __set_bit(ETHTOOL_LINK_MODE_10baseT1S_Full_BIT, linkmodes);
}
if (caps & MAC_100HD) {
diff --git a/include/linux/phy.h b/include/linux/phy.h
index f3ecc9a86e67..49d0488bf480 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1036,6 +1036,17 @@ struct phy_driver {
int (*get_sqi)(struct phy_device *dev);
/** @get_sqi_max: Get the maximum signal quality indication */
int (*get_sqi_max)(struct phy_device *dev);
+
+ /* PLCA RS interface */
+ /** @get_plca_cfg: Return the current PLCA configuration */
+ int (*get_plca_cfg)(struct phy_device *dev,
+ struct phy_plca_cfg *plca_cfg);
+ /** @set_plca_cfg: Set the PLCA configuration */
+ int (*set_plca_cfg)(struct phy_device *dev,
+ const struct phy_plca_cfg *plca_cfg);
+ /** @get_plca_status: Return the current PLCA status info */
+ int (*get_plca_status)(struct phy_device *dev,
+ struct phy_plca_status *plca_st);
};
#define to_phy_driver(d) container_of(to_mdio_common_driver(d), \
struct phy_driver, mdiodrv)
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 58e587ba0450..5f414deacf23 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1741,6 +1741,9 @@ enum ethtool_link_mode_bit_indices {
ETHTOOL_LINK_MODE_800000baseDR8_2_Full_BIT = 96,
ETHTOOL_LINK_MODE_800000baseSR8_Full_BIT = 97,
ETHTOOL_LINK_MODE_800000baseVR8_Full_BIT = 98,
+ ETHTOOL_LINK_MODE_10baseT1S_Full_BIT = 99,
+ ETHTOOL_LINK_MODE_10baseT1S_Half_BIT = 100,
+ ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT = 101,
/* must be last entry */
__ETHTOOL_LINK_MODE_MASK_NBITS
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 21cfe8557205..c586db0c5e68 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -208,6 +208,9 @@ const char link_mode_names[][ETH_GSTRING_LEN] = {
__DEFINE_LINK_MODE_NAME(800000, DR8_2, Full),
__DEFINE_LINK_MODE_NAME(800000, SR8, Full),
__DEFINE_LINK_MODE_NAME(800000, VR8, Full),
+ __DEFINE_LINK_MODE_NAME(10, T1S, Full),
+ __DEFINE_LINK_MODE_NAME(10, T1S, Half),
+ __DEFINE_LINK_MODE_NAME(10, T1S_P2MP, Half),
};
static_assert(ARRAY_SIZE(link_mode_names) == __ETHTOOL_LINK_MODE_MASK_NBITS);
@@ -244,6 +247,8 @@ static_assert(ARRAY_SIZE(link_mode_names) == __ETHTOOL_LINK_MODE_MASK_NBITS);
#define __LINK_MODE_LANES_X 1
#define __LINK_MODE_LANES_FX 1
#define __LINK_MODE_LANES_T1L 1
+#define __LINK_MODE_LANES_T1S 1
+#define __LINK_MODE_LANES_T1S_P2MP 1
#define __LINK_MODE_LANES_VR8 8
#define __LINK_MODE_LANES_DR8_2 8
@@ -366,6 +371,9 @@ const struct link_mode_info link_mode_params[] = {
__DEFINE_LINK_MODE_PARAMS(800000, DR8_2, Full),
__DEFINE_LINK_MODE_PARAMS(800000, SR8, Full),
__DEFINE_LINK_MODE_PARAMS(800000, VR8, Full),
+ __DEFINE_LINK_MODE_PARAMS(10, T1S, Full),
+ __DEFINE_LINK_MODE_PARAMS(10, T1S, Half),
+ __DEFINE_LINK_MODE_PARAMS(10, T1S_P2MP, Half),
};
static_assert(ARRAY_SIZE(link_mode_params) == __ETHTOOL_LINK_MODE_MASK_NBITS);
--
2.35.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v5 net-next 3/5] drivers/net/phy: add connection between ethtool and phylib for PLCA
2022-12-07 0:00 [PATCH v5 net-next 0/5] add PLCA RS support and onsemi NCN26000 Piergiorgio Beruto
2022-12-07 0:01 ` [PATCH v5 net-next 1/5] net/ethtool: add netlink interface for the PLCA RS Piergiorgio Beruto
2022-12-07 0:01 ` [PATCH v5 net-next 2/5] drivers/net/phy: add the link modes for the 10BASE-T1S Ethernet PHY Piergiorgio Beruto
@ 2022-12-07 0:02 ` Piergiorgio Beruto
2022-12-07 0:02 ` [PATCH v5 net-next 4/5] drivers/net/phy: add helpers to get/set PLCA configuration Piergiorgio Beruto
` (2 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Piergiorgio Beruto @ 2022-12-07 0:02 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: linux-kernel, netdev, Oleksij Rempel
This patch adds the required connection between netlink ethtool and
phylib to resolve PLCA get/set config and get status messages.
Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
---
drivers/net/phy/phy.c | 175 +++++++++++++++++++++++++++++++++++
drivers/net/phy/phy_device.c | 3 +
2 files changed, 178 insertions(+)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e5b6cb1a77f9..3fc251f5de26 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -543,6 +543,181 @@ int phy_ethtool_get_stats(struct phy_device *phydev,
}
EXPORT_SYMBOL(phy_ethtool_get_stats);
+/**
+ * phy_ethtool_get_plca_cfg - Get PLCA RS configuration
+ *
+ * @phydev: the phy_device struct
+ * @plca_cfg: where to store the retrieved configuration
+ */
+int phy_ethtool_get_plca_cfg(struct phy_device *phydev,
+ struct phy_plca_cfg *plca_cfg)
+{
+ int ret;
+
+ if (!phydev->drv) {
+ ret = -EIO;
+ goto out;
+ }
+
+ if (!phydev->drv->get_plca_cfg) {
+ ret = -EOPNOTSUPP;
+ goto out;
+ }
+
+ memset(plca_cfg, 0xFF, sizeof(*plca_cfg));
+
+ mutex_lock(&phydev->lock);
+ ret = phydev->drv->get_plca_cfg(phydev, plca_cfg);
+
+ if (ret)
+ goto out_drv;
+
+out_drv:
+ mutex_unlock(&phydev->lock);
+out:
+ return ret;
+}
+
+/**
+ * phy_ethtool_set_plca_cfg - Set PLCA RS configuration
+ *
+ * @phydev: the phy_device struct
+ * @extack: extack for reporting useful error messages
+ * @plca_cfg: new PLCA configuration to apply
+ */
+int phy_ethtool_set_plca_cfg(struct phy_device *phydev,
+ struct netlink_ext_ack *extack,
+ const struct phy_plca_cfg *plca_cfg)
+{
+ int ret;
+ struct phy_plca_cfg *curr_plca_cfg = 0;
+
+ if (!phydev->drv) {
+ ret = -EIO;
+ goto out;
+ }
+
+ if (!phydev->drv->set_plca_cfg ||
+ !phydev->drv->get_plca_cfg) {
+ ret = -EOPNOTSUPP;
+ goto out;
+ }
+
+ curr_plca_cfg = kmalloc(sizeof(*curr_plca_cfg), GFP_KERNEL);
+ memset(curr_plca_cfg, 0xFF, sizeof(*curr_plca_cfg));
+
+ mutex_lock(&phydev->lock);
+
+ ret = phydev->drv->get_plca_cfg(phydev, curr_plca_cfg);
+ if (ret)
+ goto out_drv;
+
+ if (curr_plca_cfg->enabled < 0 && plca_cfg->enabled >= 0) {
+ NL_SET_ERR_MSG(extack,
+ "PHY does not support changing the PLCA 'enable' attribute");
+ ret = -EINVAL;
+ goto out_drv;
+ }
+
+ if (curr_plca_cfg->node_id < 0 && plca_cfg->node_id >= 0) {
+ NL_SET_ERR_MSG(extack,
+ "PHY does not support changing the PLCA 'local node ID' attribute");
+ ret = -EINVAL;
+ goto out_drv;
+ }
+
+ if (curr_plca_cfg->node_cnt < 0 && plca_cfg->node_cnt >= 0) {
+ NL_SET_ERR_MSG(extack,
+ "PHY does not support changing the PLCA 'node count' attribute");
+ ret = -EINVAL;
+ goto out_drv;
+ }
+
+ if (curr_plca_cfg->to_tmr < 0 && plca_cfg->to_tmr >= 0) {
+ NL_SET_ERR_MSG(extack,
+ "PHY does not support changing the PLCA 'TO timer' attribute");
+ ret = -EINVAL;
+ goto out_drv;
+ }
+
+ if (curr_plca_cfg->burst_cnt < 0 && plca_cfg->burst_cnt >= 0) {
+ NL_SET_ERR_MSG(extack,
+ "PHY does not support changing the PLCA 'burst count' attribute");
+ ret = -EINVAL;
+ goto out_drv;
+ }
+
+ if (curr_plca_cfg->burst_tmr < 0 && plca_cfg->burst_tmr >= 0) {
+ NL_SET_ERR_MSG(extack,
+ "PHY does not support changing the PLCA 'burst timer' attribute");
+ ret = -EINVAL;
+ goto out_drv;
+ }
+
+ // if enabling PLCA, perform additional sanity checks
+ if (plca_cfg->enabled > 0) {
+ if (!linkmode_test_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT,
+ phydev->advertising)) {
+ ret = -EOPNOTSUPP;
+ NL_SET_ERR_MSG(extack,
+ "Point to Multi-Point mode is not enabled");
+ }
+
+ // allow setting node_id concurrently with enabled
+ if (plca_cfg->node_id >= 0)
+ curr_plca_cfg->node_id = plca_cfg->node_id;
+
+ if (curr_plca_cfg->node_id >= 255) {
+ NL_SET_ERR_MSG(extack, "PLCA node ID is not set");
+ ret = -EINVAL;
+ goto out_drv;
+ }
+ }
+
+ ret = phydev->drv->set_plca_cfg(phydev, plca_cfg);
+ if (ret)
+ goto out_drv;
+
+out_drv:
+ kfree(curr_plca_cfg);
+ mutex_unlock(&phydev->lock);
+out:
+ return ret;
+}
+
+/**
+ * phy_ethtool_get_plca_status - Get PLCA RS status information
+ *
+ * @phydev: the phy_device struct
+ * @plca_st: where to store the retrieved status information
+ */
+int phy_ethtool_get_plca_status(struct phy_device *phydev,
+ struct phy_plca_status *plca_st)
+{
+ int ret;
+
+ if (!phydev->drv) {
+ ret = -EIO;
+ goto out;
+ }
+
+ if (!phydev->drv->get_plca_status) {
+ ret = -EOPNOTSUPP;
+ goto out;
+ }
+
+ mutex_lock(&phydev->lock);
+ ret = phydev->drv->get_plca_status(phydev, plca_st);
+
+ if (ret)
+ goto out_drv;
+
+out_drv:
+ mutex_unlock(&phydev->lock);
+out:
+ return ret;
+}
+
/**
* phy_start_cable_test - Start a cable test
*
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 716870a4499c..f248010c403d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3262,6 +3262,9 @@ static const struct ethtool_phy_ops phy_ethtool_phy_ops = {
.get_sset_count = phy_ethtool_get_sset_count,
.get_strings = phy_ethtool_get_strings,
.get_stats = phy_ethtool_get_stats,
+ .get_plca_cfg = phy_ethtool_get_plca_cfg,
+ .set_plca_cfg = phy_ethtool_set_plca_cfg,
+ .get_plca_status = phy_ethtool_get_plca_status,
.start_cable_test = phy_start_cable_test,
.start_cable_test_tdr = phy_start_cable_test_tdr,
};
--
2.35.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v5 net-next 4/5] drivers/net/phy: add helpers to get/set PLCA configuration
2022-12-07 0:00 [PATCH v5 net-next 0/5] add PLCA RS support and onsemi NCN26000 Piergiorgio Beruto
` (2 preceding siblings ...)
2022-12-07 0:02 ` [PATCH v5 net-next 3/5] drivers/net/phy: add connection between ethtool and phylib for PLCA Piergiorgio Beruto
@ 2022-12-07 0:02 ` Piergiorgio Beruto
2022-12-07 0:02 ` [PATCH v5 net-next 5/5] drivers/net/phy: add driver for the onsemi NCN26000 10BASE-T1S PHY Piergiorgio Beruto
2022-12-07 0:43 ` [PATCH v5 net-next 0/5] add PLCA RS support and onsemi NCN26000 Andrew Lunn
5 siblings, 0 replies; 23+ messages in thread
From: Piergiorgio Beruto @ 2022-12-07 0:02 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: linux-kernel, netdev, Oleksij Rempel
This patch adds support in phylib to read/write PLCA configuration for
Ethernet PHYs that support the OPEN Alliance "10BASE-T1S PLCA
Management Registers" specifications. These can be found at
https://www.opensig.org/about/specifications/
Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
---
MAINTAINERS | 1 +
drivers/net/phy/mdio-open-alliance.h | 47 +++++++
drivers/net/phy/phy-c45.c | 183 +++++++++++++++++++++++++++
include/linux/phy.h | 6 +
4 files changed, 237 insertions(+)
create mode 100644 drivers/net/phy/mdio-open-alliance.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 7952243e4b43..ed626cbdf5af 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16400,6 +16400,7 @@ PLCA RECONCILIATION SUBLAYER (IEEE802.3 Clause 148)
M: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
L: netdev@vger.kernel.org
S: Maintained
+F: drivers/net/phy/mdio-open-alliance.h
F: net/ethtool/plca.c
PLDMFW LIBRARY
diff --git a/drivers/net/phy/mdio-open-alliance.h b/drivers/net/phy/mdio-open-alliance.h
new file mode 100644
index 000000000000..5f64514108b1
--- /dev/null
+++ b/drivers/net/phy/mdio-open-alliance.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * mdio-open-alliance.h - definition of OPEN Alliance SIG standard registers
+ */
+
+#ifndef __MDIO_OPEN_ALLIANCE__
+#define __MDIO_OPEN_ALLIANCE__
+
+#include <linux/mdio.h>
+
+/* NOTE: all OATC14 registers are located in MDIO_MMD_VEND2 */
+
+/* Open Alliance TC14 (10BASE-T1S) registers */
+#define MDIO_OATC14_PLCA_IDVER 0xca00 /* PLCA ID and version */
+#define MDIO_OATC14_PLCA_CTRL0 0xca01 /* PLCA Control register 0 */
+#define MDIO_OATC14_PLCA_CTRL1 0xca02 /* PLCA Control register 1 */
+#define MDIO_OATC14_PLCA_STATUS 0xca03 /* PLCA Status register */
+#define MDIO_OATC14_PLCA_TOTMR 0xca04 /* PLCA TO Timer register */
+#define MDIO_OATC14_PLCA_BURST 0xca05 /* PLCA BURST mode register */
+
+/* Open Alliance TC14 PLCA IDVER register */
+#define MDIO_OATC14_PLCA_IDM 0xff00 /* PLCA MAP ID */
+#define MDIO_OATC14_PLCA_VER 0x00ff /* PLCA MAP version */
+
+/* Open Alliance TC14 PLCA CTRL0 register */
+#define MDIO_OATC14_PLCA_EN BIT(15) /* PLCA enable */
+#define MDIO_OATC14_PLCA_RST BIT(14) /* PLCA reset */
+
+/* Open Alliance TC14 PLCA CTRL1 register */
+#define MDIO_OATC14_PLCA_NCNT 0xff00 /* PLCA node count */
+#define MDIO_OATC14_PLCA_ID 0x00ff /* PLCA local node ID */
+
+/* Open Alliance TC14 PLCA STATUS register */
+#define MDIO_OATC14_PLCA_PST BIT(15) /* PLCA status indication */
+
+/* Open Alliance TC14 PLCA TOTMR register */
+#define MDIO_OATC14_PLCA_TOT 0x00ff
+
+/* Open Alliance TC14 PLCA BURST register */
+#define MDIO_OATC14_PLCA_MAXBC 0xff00
+#define MDIO_OATC14_PLCA_BTMR 0x00ff
+
+/* Version Identifiers */
+#define OATC14_IDM 0x0a00
+
+
+#endif /* __MDIO_OPEN_ALLIANCE__ */
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index a87a4b3ffce4..1a00d7e07817 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -8,6 +8,8 @@
#include <linux/mii.h>
#include <linux/phy.h>
+#include "mdio-open-alliance.h"
+
/**
* genphy_c45_baset1_able - checks if the PMA has BASE-T1 extended abilities
* @phydev: target phy_device struct
@@ -931,6 +933,187 @@ int genphy_c45_fast_retrain(struct phy_device *phydev, bool enable)
}
EXPORT_SYMBOL_GPL(genphy_c45_fast_retrain);
+/**
+ * genphy_c45_plca_get_cfg - get PLCA configuration from standard registers
+ * @phydev: target phy_device struct
+ * @plca_cfg: output structure to store the PLCA configuration
+ *
+ * Description: if the PHY complies to the Open Alliance TC14 10BASE-T1S PLCA
+ * Management Registers specifications, this function can be used to retrieve
+ * the current PLCA configuration from the standard registers in MMD 31.
+ */
+int genphy_c45_plca_get_cfg(struct phy_device *phydev,
+ struct phy_plca_cfg *plca_cfg)
+{
+ int ret;
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_PLCA_IDVER);
+ if (ret < 0)
+ return ret;
+
+ if ((ret & MDIO_OATC14_PLCA_IDM) != OATC14_IDM)
+ return -ENODEV;
+
+ plca_cfg->version = ret;
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_PLCA_CTRL0);
+ if (ret < 0)
+ return ret;
+
+ plca_cfg->enabled = !!(ret & MDIO_OATC14_PLCA_EN);
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_PLCA_CTRL1);
+ if (ret < 0)
+ return ret;
+
+ plca_cfg->node_cnt = (ret & MDIO_OATC14_PLCA_NCNT) >> 8;
+ plca_cfg->node_id = (ret & MDIO_OATC14_PLCA_ID);
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_PLCA_TOTMR);
+ if (ret < 0)
+ return ret;
+
+ plca_cfg->to_tmr = ret & MDIO_OATC14_PLCA_TOT;
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_PLCA_BURST);
+ if (ret < 0)
+ return ret;
+
+ plca_cfg->burst_cnt = (ret & MDIO_OATC14_PLCA_MAXBC) >> 8;
+ plca_cfg->burst_tmr = (ret & MDIO_OATC14_PLCA_BTMR);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(genphy_c45_plca_get_cfg);
+
+/**
+ * genphy_c45_plca_set_cfg - set PLCA configuration using standard registers
+ * @phydev: target phy_device struct
+ * @plca_cfg: structure containing the PLCA configuration. Fields set to -1 are
+ * not to be changed.
+ *
+ * Description: if the PHY complies to the Open Alliance TC14 10BASE-T1S PLCA
+ * Management Registers specifications, this function can be used to modify
+ * the PLCA configuration using the standard registers in MMD 31.
+ */
+int genphy_c45_plca_set_cfg(struct phy_device *phydev,
+ const struct phy_plca_cfg *plca_cfg)
+{
+ int ret;
+ u16 val;
+
+ // PLCA IDVER is read-only
+ if (plca_cfg->version >= 0)
+ return -EINVAL;
+
+ // first of all, disable PLCA if required
+ if (plca_cfg->enabled == 0) {
+ ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2,
+ MDIO_OATC14_PLCA_CTRL0,
+ MDIO_OATC14_PLCA_EN);
+
+ if (ret < 0)
+ return ret;
+ }
+
+ if (plca_cfg->node_cnt >= 0 || plca_cfg->node_id >= 0) {
+ if (plca_cfg->node_cnt < 0 || plca_cfg->node_id < 0) {
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+ MDIO_OATC14_PLCA_CTRL1);
+
+ if (ret < 0)
+ return ret;
+
+ val = ret;
+ }
+
+ if (plca_cfg->node_cnt >= 0)
+ val = (val & ~MDIO_OATC14_PLCA_NCNT) |
+ (plca_cfg->node_cnt << 8);
+
+ if (plca_cfg->node_id >= 0)
+ val = (val & ~MDIO_OATC14_PLCA_ID) |
+ (plca_cfg->node_id);
+
+ ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+ MDIO_OATC14_PLCA_CTRL1, val);
+
+ if (ret < 0)
+ return ret;
+ }
+
+ if (plca_cfg->to_tmr >= 0) {
+ ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+ MDIO_OATC14_PLCA_TOTMR,
+ plca_cfg->to_tmr);
+
+ if (ret < 0)
+ return ret;
+ }
+
+ if (plca_cfg->burst_cnt >= 0 || plca_cfg->burst_tmr >= 0) {
+ if (plca_cfg->burst_cnt < 0 || plca_cfg->burst_tmr < 0) {
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+ MDIO_OATC14_PLCA_BURST);
+
+ if (ret < 0)
+ return ret;
+
+ val = ret;
+ }
+
+ if (plca_cfg->burst_cnt >= 0)
+ val = (val & ~MDIO_OATC14_PLCA_MAXBC) |
+ (plca_cfg->burst_cnt << 8);
+
+ if (plca_cfg->burst_tmr >= 0)
+ val = (val & ~MDIO_OATC14_PLCA_BTMR) |
+ (plca_cfg->burst_tmr);
+
+ ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+ MDIO_OATC14_PLCA_BURST, val);
+
+ if (ret < 0)
+ return ret;
+ }
+
+ // if we need to enable PLCA, do it at the end
+ if (plca_cfg->enabled > 0) {
+ ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2,
+ MDIO_OATC14_PLCA_CTRL0,
+ MDIO_OATC14_PLCA_EN);
+
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(genphy_c45_plca_set_cfg);
+
+/**
+ * genphy_c45_plca_get_status - get PLCA status from standard registers
+ * @phydev: target phy_device struct
+ * @plca_st: output structure to store the PLCA status
+ *
+ * Description: if the PHY complies to the Open Alliance TC14 10BASE-T1S PLCA
+ * Management Registers specifications, this function can be used to retrieve
+ * the current PLCA status information from the standard registers in MMD 31.
+ */
+int genphy_c45_plca_get_status(struct phy_device *phydev,
+ struct phy_plca_status *plca_st)
+{
+ int ret;
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_PLCA_STATUS);
+ if (ret < 0)
+ return ret;
+
+ plca_st->pst = !!(ret & MDIO_OATC14_PLCA_PST);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(genphy_c45_plca_get_status);
+
struct phy_driver genphy_c45_driver = {
.phy_id = 0xffffffff,
.phy_id_mask = 0xffffffff,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 49d0488bf480..4548c8e8f6a9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1745,6 +1745,12 @@ int genphy_c45_loopback(struct phy_device *phydev, bool enable);
int genphy_c45_pma_resume(struct phy_device *phydev);
int genphy_c45_pma_suspend(struct phy_device *phydev);
int genphy_c45_fast_retrain(struct phy_device *phydev, bool enable);
+int genphy_c45_plca_get_cfg(struct phy_device *phydev,
+ struct phy_plca_cfg *plca_cfg);
+int genphy_c45_plca_set_cfg(struct phy_device *phydev,
+ const struct phy_plca_cfg *plca_cfg);
+int genphy_c45_plca_get_status(struct phy_device *phydev,
+ struct phy_plca_status *plca_st);
/* Generic C45 PHY driver */
extern struct phy_driver genphy_c45_driver;
--
2.35.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v5 net-next 5/5] drivers/net/phy: add driver for the onsemi NCN26000 10BASE-T1S PHY
2022-12-07 0:00 [PATCH v5 net-next 0/5] add PLCA RS support and onsemi NCN26000 Piergiorgio Beruto
` (3 preceding siblings ...)
2022-12-07 0:02 ` [PATCH v5 net-next 4/5] drivers/net/phy: add helpers to get/set PLCA configuration Piergiorgio Beruto
@ 2022-12-07 0:02 ` Piergiorgio Beruto
2022-12-07 0:43 ` [PATCH v5 net-next 0/5] add PLCA RS support and onsemi NCN26000 Andrew Lunn
5 siblings, 0 replies; 23+ messages in thread
From: Piergiorgio Beruto @ 2022-12-07 0:02 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit, Russell King, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: linux-kernel, netdev, Oleksij Rempel
This patch adds support for the onsemi NCN26000 10BASE-T1S industrial
Ethernet PHY. The driver supports Point-to-Multipoint operation without
auto-negotiation and with link control handling. The PHY also features
PLCA for improving performance in P2MP mode.
Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
---
MAINTAINERS | 7 ++
drivers/net/phy/Kconfig | 7 ++
drivers/net/phy/Makefile | 1 +
drivers/net/phy/ncn26000.c | 201 +++++++++++++++++++++++++++++++++++++
4 files changed, 216 insertions(+)
create mode 100644 drivers/net/phy/ncn26000.c
diff --git a/MAINTAINERS b/MAINTAINERS
index ed626cbdf5af..09f0bfa3ae64 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15357,6 +15357,13 @@ L: linux-mips@vger.kernel.org
S: Maintained
F: arch/mips/boot/dts/ralink/omega2p.dts
+ONSEMI ETHERNET PHY DRIVERS
+M: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
+L: netdev@vger.kernel.org
+S: Supported
+W: http://www.onsemi.com
+F: drivers/net/phy/ncn*
+
OP-TEE DRIVER
M: Jens Wiklander <jens.wiklander@linaro.org>
L: op-tee@lists.trustedfirmware.org
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index af00cf44cd97..7c466830c611 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -267,6 +267,13 @@ config NATIONAL_PHY
help
Currently supports the DP83865 PHY.
+config NCN26000_PHY
+ tristate "onsemi 10BASE-T1S Ethernet PHY"
+ help
+ Adds support for the onsemi 10BASE-T1S Ethernet PHY.
+ Currently supports the NCN26000 10BASE-T1S Industrial PHY
+ with MII interface.
+
config NXP_C45_TJA11XX_PHY
tristate "NXP C45 TJA11XX PHYs"
depends on PTP_1588_CLOCK_OPTIONAL
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index f7138d3c896b..b5138066ba04 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_MICROCHIP_T1_PHY) += microchip_t1.o
obj-$(CONFIG_MICROSEMI_PHY) += mscc/
obj-$(CONFIG_MOTORCOMM_PHY) += motorcomm.o
obj-$(CONFIG_NATIONAL_PHY) += national.o
+obj-$(CONFIG_NCN26000_PHY) += ncn26000.o
obj-$(CONFIG_NXP_C45_TJA11XX_PHY) += nxp-c45-tja11xx.o
obj-$(CONFIG_NXP_TJA11XX_PHY) += nxp-tja11xx.o
obj-$(CONFIG_QSEMI_PHY) += qsemi.o
diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c
new file mode 100644
index 000000000000..d3c0a7420ea8
--- /dev/null
+++ b/drivers/net/phy/ncn26000.c
@@ -0,0 +1,201 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
+/*
+ * Driver for the onsemi 10BASE-T1S NCN26000 PHYs family.
+ *
+ * Copyright 2022 onsemi
+ */
+#include <linux/kernel.h>
+#include <linux/bitfield.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mii.h>
+#include <linux/phy.h>
+
+#include "mdio-open-alliance.h"
+
+#define PHY_ID_NCN26000 0x180FF5A1
+
+#define NCN26000_REG_IRQ_CTL 16
+#define NCN26000_REG_IRQ_STATUS 17
+
+// the NCN26000 maps link_ctrl to BMCR_ANENABLE
+#define NCN26000_BCMR_LINK_CTRL_BIT BMCR_ANENABLE
+
+// the NCN26000 maps link_status to BMSR_ANEGCOMPLETE
+#define NCN26000_BMSR_LINK_STATUS_BIT BMSR_ANEGCOMPLETE
+
+#define NCN26000_IRQ_LINKST_BIT BIT(0)
+#define NCN26000_IRQ_PLCAST_BIT BIT(1)
+#define NCN26000_IRQ_LJABBER_BIT BIT(2)
+#define NCN26000_IRQ_RJABBER_BIT BIT(3)
+#define NCN26000_IRQ_PLCAREC_BIT BIT(4)
+#define NCN26000_IRQ_PHYSCOL_BIT BIT(5)
+
+#define TO_TMR_DEFAULT 32
+
+struct ncn26000_priv {
+ u16 enabled_irqs;
+};
+
+// driver callbacks
+
+static int ncn26000_config_init(struct phy_device *phydev)
+{
+ /* HW bug workaround: the default value of the PLCA TO_TIMER should be
+ * 32, where the current version of NCN26000 reports 24. This will be
+ * fixed in future PHY versions. For the time being, we force the
+ * correct default here.
+ */
+ return phy_write_mmd(phydev, MDIO_MMD_VEND2, MDIO_OATC14_PLCA_TOTMR,
+ TO_TMR_DEFAULT);
+}
+
+static int ncn26000_config_aneg(struct phy_device *phydev)
+{
+ // Note: the NCN26000 supports only P2MP link mode. Therefore, AN is not
+ // supported. However, this function is invoked by phylib to enable the
+ // PHY, regardless of the AN support.
+ phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+ phydev->mdix = ETH_TP_MDI;
+
+ // bring up the link
+ return phy_write(phydev, MII_BMCR, NCN26000_BCMR_LINK_CTRL_BIT);
+}
+
+static int ncn26000_get_features(struct phy_device *phydev)
+{
+ linkmode_zero(phydev->supported);
+ linkmode_set_bit(ETHTOOL_LINK_MODE_MII_BIT, phydev->supported);
+
+ linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT,
+ phydev->supported);
+
+ linkmode_copy(phydev->advertising, phydev->supported);
+ return 0;
+}
+
+static int ncn26000_read_status(struct phy_device *phydev)
+{
+ // The NCN26000 reports NCN26000_LINK_STATUS_BIT if the link status of
+ // the PHY is up. It further reports the logical AND of the link status
+ // and the PLCA status in the BMSR_LSTATUS bit.
+ int ret;
+
+ /* The link state is latched low so that momentary link
+ * drops can be detected. Do not double-read the status
+ * in polling mode to detect such short link drops except
+ * the link was already down.
+ */
+ if (!phy_polling_mode(phydev) || !phydev->link) {
+ ret = phy_read(phydev, MII_BMSR);
+ if (ret < 0)
+ return ret;
+ else if (ret & NCN26000_BMSR_LINK_STATUS_BIT)
+ goto upd_link;
+ }
+
+ ret = phy_read(phydev, MII_BMSR);
+ if (unlikely(ret < 0))
+ return ret;
+
+upd_link:
+ // update link status
+ if (ret & NCN26000_BMSR_LINK_STATUS_BIT) {
+ phydev->link = 1;
+ phydev->pause = 0;
+ phydev->duplex = DUPLEX_HALF;
+ phydev->speed = SPEED_10;
+ } else {
+ phydev->link = 0;
+ phydev->duplex = DUPLEX_UNKNOWN;
+ phydev->speed = SPEED_UNKNOWN;
+ }
+
+ return 0;
+}
+
+static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
+{
+ const struct ncn26000_priv *const priv = phydev->priv;
+ int ret;
+
+ // read and aknowledge the IRQ status register
+ ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
+
+ if (unlikely(ret < 0) || (ret & priv->enabled_irqs) == 0)
+ return IRQ_NONE;
+
+ phy_trigger_machine(phydev);
+ return IRQ_HANDLED;
+}
+
+static int ncn26000_config_intr(struct phy_device *phydev)
+{
+ int ret;
+ struct ncn26000_priv *priv = phydev->priv;
+
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+ // acknowledge IRQs
+ ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
+ if (ret < 0)
+ return ret;
+
+ // get link status notifications
+ priv->enabled_irqs = NCN26000_IRQ_LINKST_BIT;
+ } else {
+ // disable all IRQs
+ priv->enabled_irqs = 0;
+ }
+
+ ret = phy_write(phydev, NCN26000_REG_IRQ_CTL, priv->enabled_irqs);
+ if (ret != 0)
+ return ret;
+
+ return 0;
+}
+
+static int ncn26000_probe(struct phy_device *phydev)
+{
+ struct device *dev = &phydev->mdio.dev;
+ struct ncn26000_priv *priv;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ phydev->priv = priv;
+
+ return 0;
+}
+
+static struct phy_driver ncn26000_driver[] = {
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_NCN26000),
+ .name = "NCN26000",
+ .probe = ncn26000_probe,
+ .get_features = ncn26000_get_features,
+ .config_init = ncn26000_config_init,
+ .config_intr = ncn26000_config_intr,
+ .config_aneg = ncn26000_config_aneg,
+ .read_status = ncn26000_read_status,
+ .handle_interrupt = ncn26000_handle_interrupt,
+ .get_plca_cfg = genphy_c45_plca_get_cfg,
+ .set_plca_cfg = genphy_c45_plca_set_cfg,
+ .get_plca_status = genphy_c45_plca_get_status,
+ .soft_reset = genphy_soft_reset,
+ },
+};
+
+module_phy_driver(ncn26000_driver);
+
+static struct mdio_device_id __maybe_unused ncn26000_tbl[] = {
+ { PHY_ID_MATCH_MODEL(PHY_ID_NCN26000) },
+ { }
+};
+
+MODULE_DEVICE_TABLE(mdio, ncn26000_tbl);
+
+MODULE_AUTHOR("Piergiorgio Beruto");
+MODULE_DESCRIPTION("onsemi 10BASE-T1S PHY driver");
+MODULE_LICENSE("Dual BSD/GPL");
--
2.35.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v5 net-next 0/5] add PLCA RS support and onsemi NCN26000
2022-12-07 0:00 [PATCH v5 net-next 0/5] add PLCA RS support and onsemi NCN26000 Piergiorgio Beruto
` (4 preceding siblings ...)
2022-12-07 0:02 ` [PATCH v5 net-next 5/5] drivers/net/phy: add driver for the onsemi NCN26000 10BASE-T1S PHY Piergiorgio Beruto
@ 2022-12-07 0:43 ` Andrew Lunn
5 siblings, 0 replies; 23+ messages in thread
From: Andrew Lunn @ 2022-12-07 0:43 UTC (permalink / raw)
To: Piergiorgio Beruto
Cc: Heiner Kallweit, Russell King, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, linux-kernel, netdev, Oleksij Rempel
On Wed, Dec 07, 2022 at 01:00:58AM +0100, Piergiorgio Beruto wrote:
> This patchset adds support for getting/setting the Physical Layer
> Collision Avoidace (PLCA) Reconciliation Sublayer (RS) configuration and
> status on Ethernet PHYs that supports it.
>
> PLCA is a feature that provides improved media-access performance in terms
> of throughput, latency and fairness for multi-drop (P2MP) half-duplex PHYs.
> PLCA is defined in Clause 148 of the IEEE802.3 specifications as amended
> by 802.3cg-2019. Currently, PLCA is supported by the 10BASE-T1S single-pair
> Ethernet PHY defined in the same standard and related amendments. The OPEN
> Alliance SIG TC14 defines additional specifications for the 10BASE-T1S PHY,
> including a standard register map for PHYs that embeds the PLCA RS (see
> PLCA management registers at https://www.opensig.org/about/specifications/).
>
> The changes proposed herein add the appropriate ethtool netlink interface
> for configuring the PLCA RS on PHYs that supports it. A separate patchset
> further modifies the ethtool userspace program to show and modify the
> configuration/status of the PLCA RS.
>
> Additionally, this patchset adds support for the onsemi NCN26000
> Industrial Ethernet 10BASE-T1S PHY that uses the newly added PLCA
> infrastructure.
You should be listing what has changed since the previous
version. Either here, or in each patch, below the --- marker. It helps
reviewers know their comments have been acted up.
Andrew
^ permalink raw reply [flat|nested] 23+ messages in thread