| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y+lqw1NPWdlMexNZ@gvm01>
Date: Sun, 12 Feb 2023 23:40:03 +0100
From: Piergiorgio Beruto <piergiorgio.beruto@...il.com>
To: Michal Kubecek <mkubecek@...e.cz>
Cc: Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, Oleksij Rempel <o.rempel@...gutronix.de>,
mailhol.vincent@...adoo.fr, sudheer.mogilappagari@...el.com,
sbhatta@...vell.com, linux-doc@...r.kernel.org,
wangjie125@...wei.com, corbet@....net, lkp@...el.com,
gal@...dia.com, gustavoars@...nel.org, bagasdotme@...il.com
Subject: Re: [PATCH v5 ethtool-next 1/1] add support for IEEE 802.3cg-2019
Clause 148
On Sun, Feb 12, 2023 at 08:57:23PM +0100, Michal Kubecek wrote:
> On Thu, Feb 02, 2023 at 09:53:15AM +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@...il.com>
>
> Sorry for the delay, I missed the patch in the mailing list and did not
> get it directly into my inbox. Thankfully I noticed it in patchwork.
No worries, thanks for taking time to review this :-)
>
> > ---
> > Makefile.am | 1 +
> > ethtool.8.in | 83 ++++++++++++-
> > ethtool.c | 21 ++++
> > netlink/extapi.h | 6 +
> > netlink/plca.c | 296 +++++++++++++++++++++++++++++++++++++++++++++
> > netlink/settings.c | 82 ++++++++++++-
> > 6 files changed, 486 insertions(+), 3 deletions(-)
> > create mode 100644 netlink/plca.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 999e7691e81c..cbc1f4f5fdf2 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -42,6 +42,7 @@ ethtool_SOURCES += \
> > netlink/desc-ethtool.c netlink/desc-genlctrl.c \
> > netlink/module-eeprom.c netlink/module.c netlink/rss.c \
> > netlink/desc-rtnl.c netlink/cable_test.c netlink/tunnels.c \
> > + netlink/plca.c \
> > uapi/linux/ethtool_netlink.h \
> > uapi/linux/netlink.h uapi/linux/genetlink.h \
> > uapi/linux/rtnetlink.h uapi/linux/if_link.h \
> > diff --git a/ethtool.8.in b/ethtool.8.in
> > index eaf6c55a84bf..c43c6d8b5263 100644
> > --- a/ethtool.8.in
> > +++ b/ethtool.8.in
> [...]
> > +.TP
> > +.BI node\-cnt \ N
> > +The node-cnt [1 .. 255] should be set after the maximum number of nodes that
> > +can be plugged to the multi-drop network. This parameter regulates the minimum
> > +length of the PLCA cycle. Therefore, it is only meaningful for the coordinator
> > +node (\fBnod-id\fR = 0). Setting this parameter on a follower node has no
> > +effect. The \fBnode\-cnt\fR parameter maps to IEEE 802.3cg-2019 clause
> > +30.16.1.1.3 (aPLCANodeCount).
> > +.TP
> > +.BI to\-tmr \ N
> > +The TO timer parameter sets the value of the transmit opportunity timer in
> > +bit-times, and shall be set equal across all the nodes sharing the same
> > +medium for PLCA to work. The default value of 32 is enough to cover a link of
> > +roughly 50 mt. This parameter maps to IEEE 802.3cg-2019 clause 30.16.1.1.5
> > +(aPLCATransmitOpportunityTimer).
> > +.TP
> > +.BI burst\-cnt \ N
> > +The \fBburst\-cnt\fR parameter [0 .. 255] indicates the extra number of packets
> > +that the node is allowed to send during a single transmit opportunity.
> > +By default, this attribute is 0, meaning that the node can 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 \fBburst\-tmr\fR 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 parameter maps to IEEE 802.3cg-2019 clause 30.16.1.1.6 (aPLCAMaxBurstCount).
> > +.TP
> > +.BI burst\-tmr \ N
> > +The \fBburst\-tmr\fR parameter [0 .. 255] sets how many bit-times the PLCA RS
> > +waits for the MAC to initiate a new transmission when \fBburst\-cnt\fR 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 parameter maps to IEEE 802.3cg-2019 clause
> > +30.16.1.1.7 (aPLCABurstTimer). The value of \fBburst\-tmr\fR 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.
>
> There are some trailing spaces in these paragraphs.
Uh, sorry, thanks for noticing!
>
> > +.RE
> > +.TP
> > +.B \-\-get\-plca\-status
> > +Show the current PLCA status for the given interface. If \fBon\fR, the PHY is
> > +successfully receiving or generating the BEACON signal. If \fBoff\fR, the PLCA
> > +function is temporarily disabled and the PHY is operating in plain CSMA/CD mode.
> > .SH BUGS
> > Not supported (in part or whole) on all network drivers.
> > .SH AUTHOR
> > @@ -1532,7 +1610,8 @@ Alexander Duyck,
> > Sucheta Chakraborty,
> > Jesse Brandeburg,
> > Ben Hutchings,
> > -Scott Branden.
> > +Scott Branden,
> > +Piergiorgio Beruto.
> > .SH AVAILABILITY
> > .B ethtool
> > is available from
>
> Do you have a strong reason to add your name here? In general, I rather
> see lists like these as a historical relic. In this case, the list has
> not been updated since 2017 and even before that, I'm pretty sure it is
> only a small fraction of contributors. IMHO the git log serves the
> purpose much better today.
Ah, not really. I just read through the doc and finding this I
assumed it was a list to be populated with contributors! No real
reasons.
>
> [...]
> > diff --git a/netlink/settings.c b/netlink/settings.c
> > index 1107082167d4..a349e270dff9 100644
> > --- a/netlink/settings.c
> > +++ b/netlink/settings.c
> [...]
> > @@ -923,7 +987,10 @@ int nl_gset(struct cmd_context *ctx)
> > netlink_cmd_check(ctx, ETHTOOL_MSG_LINKINFO_GET, true) ||
> > netlink_cmd_check(ctx, ETHTOOL_MSG_WOL_GET, true) ||
> > netlink_cmd_check(ctx, ETHTOOL_MSG_DEBUG_GET, true) ||
> > - netlink_cmd_check(ctx, ETHTOOL_MSG_LINKSTATE_GET, true))
> > + netlink_cmd_check(ctx, ETHTOOL_MSG_LINKSTATE_GET, true) ||
> > + netlink_cmd_check(ctx, ETHTOOL_MSG_LINKSTATE_GET, true) ||
>
> You accidentally duplicated the line here.
Thanks for noticing.
>
> > + netlink_cmd_check(ctx, ETHTOOL_MSG_PLCA_GET_CFG, true) ||
> > + netlink_cmd_check(ctx, ETHTOOL_MSG_PLCA_GET_STATUS, true))
> > return -EOPNOTSUPP;
> >
> > nlctx->suppress_nlerr = 1;
> > @@ -943,6 +1010,12 @@ int nl_gset(struct cmd_context *ctx)
> > if (ret == -ENODEV)
> > return ret;
> >
> > + ret = gset_request(nlctx, ETHTOOL_MSG_PLCA_GET_CFG,
> > + ETHTOOL_A_PLCA_HEADER, plca_cfg_reply_cb);
> > +
> > + if (ret == -ENODEV)
> > + return ret;
> > +
> > ret = gset_request(nlctx, ETHTOOL_MSG_DEBUG_GET, ETHTOOL_A_DEBUG_HEADER,
> > debug_reply_cb);
> > if (ret == -ENODEV)
> > @@ -950,6 +1023,13 @@ int nl_gset(struct cmd_context *ctx)
> >
> > ret = gset_request(nlctx, ETHTOOL_MSG_LINKSTATE_GET,
> > ETHTOOL_A_LINKSTATE_HEADER, linkstate_reply_cb);
> > +
> > + if (ret == -ENODEV)
> > + return ret;
> > +
> > +
> > + ret = gset_request(nlctx, ETHTOOL_MSG_PLCA_GET_STATUS,
> > + ETHTOOL_A_PLCA_HEADER, plca_status_reply_cb);
> > if (ret == -ENODEV)
> > return ret;
>
> Please make the whitespace consistent with existing code, i.e. no empty
> line between gset_request() call and the test of ret and no double empty
> line.
Got it!
>
> As I have no actual objections, I can adjust the whitespace issues here
> and in ethtool.8.in if you would prefer to avoid v6 before moving on to
> the MAC merge series.
Oh, thank you! I really appreciate this.
--Piergiorgio
>
> Michal
Powered by blists - more mailing lists