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

Thanks Leon for your review.
Please, see my comments below.

On Thu, Jan 05, 2023 at 10:55:14AM +0200, Leon Romanovsky wrote:

On Wed, Jan 04, 2023 at 03:05:44PM +0100, Piergiorgio Beruto wrote:

quoted

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 | 138 ++++++++++
 MAINTAINERS                                  |   6 +
 include/linux/ethtool.h                      |  12 +
 include/linux/phy.h                          |  57 ++++
 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                           | 276 +++++++++++++++++++
 9 files changed, 550 insertions(+), 1 deletion(-)
 create mode 100644 net/ethtool/plca.c

<...>

quoted

--- /dev/null
+++ b/net/ethtool/plca.c

@@ -0,0 +1,276 @@
+// 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;
+};
+
+// 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)

No inline function in *.c files.

Fixed, thanks.

quoted

+{
+	if (attr) {
+		*dst = nla_get_u32(attr);
+		*mod = true;
+	}

Success oriented approach, please
if (!attr)
  return;

Fixed.

quoted

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

I see that many places in the code used this ret > 0 check, but it looks
like the right check is if (ret).

Thanks. I've fixed those, although I copied this code from similar files
(like cable test). Maybe we should check these out as well?

Thanks

Thanks!
Piergiorgio