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

From: Piergiorgio Beruto
Date: Thu Jan 05 2023 - 05:47:55 EST


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:
> > 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@xxxxxxxxx>
> > ---
> > 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
>
> <...>
>
> > --- /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.
>
> > +{
> > + if (attr) {
> > + *dst = nla_get_u32(attr);
> > + *mod = true;
> > + }
>
> Success oriented approach, please
> if (!attr)
> return;
Fixed.
>
> > +}
> > +
> > +// 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