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@xxxxxxxxx> > > 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
