Date:   Thu, 2 Feb 2023 09:55:34 +0100
From:   Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
To:     Michal Kubecek <mkubecek@suse.cz>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, Oleksij Rempel <o.rempel@pengutronix.de>
Subject: Re: [PATCH v4 ethtool-next 2/2] add support for IEEE 802.3cg-2019
 Clause 148 - PLCA RS
Message-ID: <Y9t6hlh2eRHGyq5m@gvm01>
References: <cover.1671236215.git.piergiorgio.beruto@gmail.com>
 <1ddabd3850c3f3aea4b2ce840a053f0e917803ba.1671236216.git.piergiorgio.beruto@gmail.com>
 <20230201212845.t5qriiofktivtzse@lion.mk-sys.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230201212845.t5qriiofktivtzse@lion.mk-sys.cz>
Precedence: bulk

On Wed, Feb 01, 2023 at 10:28:45PM +0100, Michal Kubecek wrote:
> On Sat, Dec 17, 2022 at 01:50:39AM +0100, Piergiorgio Beruto wrote:
> > This patch adds support for the Physical Layer Collision Avoidance
> > Reconciliation Sublayer which was introduced in the IEEE 802.3
> > standard by the 802.3cg working group in 2019.
> > 
> > The ethtool interface has been extended as follows:
> > - show if the device supports PLCA when ethtool is invoked without FLAGS
> >    - additionally show what PLCA version is supported
> >    - show the current PLCA status
> > - add FLAGS for getting and setting the PLCA configuration
> > 
> > Signed-off-by: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
> > ---
> >  Makefile.am        |   1 +
> >  ethtool.c          |  21 ++++
> >  netlink/extapi.h   |   6 +
> >  netlink/plca.c     | 295 +++++++++++++++++++++++++++++++++++++++++++++
> >  netlink/settings.c |  86 ++++++++++++-
> 
> Please update also the manual page (ethtool.8.in), this should be done
> whenever adding a new feature so that documented and implemented
> features don't diverge.
I wrote the required text, please have a look and see it's ok!
> 
> [...]
> > diff --git a/netlink/extapi.h b/netlink/extapi.h
> > index 1bb580a889a8..0add156e644a 100644
> > --- a/netlink/extapi.h
> > +++ b/netlink/extapi.h
> [...]
> > @@ -114,6 +117,9 @@ nl_get_eeprom_page(struct cmd_context *ctx __maybe_unused,
> >  #define nl_getmodule		NULL
> >  #define nl_gmodule		NULL
> >  #define nl_smodule		NULL
> > +#define nl_get_plca_cfg		NULL
> > +#define nl_set_plca_cfg		NULL
> > +#define nl_get_plca_status	NULL
> >  
> >  #endif /* ETHTOOL_ENABLE_NETLINK */
> >  
> 
> The function names are misspelled here so that a build with
> --disable-netlink fails.
Ok, sorry about that. I've fixed the problem and tested with
--disable-netlink. Should be OK now.
> 
> [...]
> > diff --git a/netlink/plca.c b/netlink/plca.c
> > new file mode 100644
> > index 000000000000..f7d7bdbc5c84
> > --- /dev/null
> > +++ b/netlink/plca.c
> > @@ -0,0 +1,295 @@
> > +/*
> > + * plca.c - netlink implementation of plca command
> > + *
> > + * Implementation of "ethtool --show-plca <dev>" and
> > + * "ethtool --set-plca <dev> ..."
> > + */
> > +
> > +#include <errno.h>
> > +#include <string.h>
> > +#include <stdio.h>
> > +
> > +#include "../internal.h"
> > +#include "../common.h"
> > +#include "netlink.h"
> > +#include "bitset.h"
> > +#include "parser.h"
> > +
> > +/* PLCA_GET_CFG */
> [...]
> > +int plca_get_cfg_reply_cb(const struct nlmsghdr *nlhdr, void *data)
> > +{
> > +	const struct nlattr *tb[ETHTOOL_A_PLCA_MAX + 1] = {};
> > +	DECLARE_ATTR_TB_INFO(tb);
> > +	struct nl_context *nlctx = data;
> > +	bool silent;
> > +	int idv, val;
> > +	int err_ret;
> > +	int ret;
> [...]
> > +		// The node count is ignored by follower nodes. However, it can
> > +		// be pre-set to enable fast coordinator role switchover.
> > +		// Therefore, on a follower node we still wanto to show it,
> > +		// indicating it is not currently used.
> > +		if (tb[ETHTOOL_A_PLCA_NODE_ID] && idv != 0)
> > +			printf(" (ignored)");
> 
> The compiler warns that idv may be uninitialized here. While it's in
> wrong, AFAICS, not even gcc13 is smart enough to recognize that it's not
> actually possible so let's initialize the variable to make it happy.
> Also, both idv and val are used to store unsigned values so they should
> be unsigned int.
> 
> Other than these, the patch looks good to me. The next branch already
> has the UAPI updates needed for it so patch 1 won't be needed any more
> if you rebase on top of current next.
> 
> Michal
Thanks,
Piergiorgio