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

Oleksij Rempel
2022-12-05T10:22:30Z

On Mon, Dec 05, 2022 at 11:03:58AM +0100, Piergiorgio Beruto wrote:
> Hello Oleksij, and thank you for your review!
> Please see my comments below.
>
> On Mon, Dec 05, 2022 at 07:00:57AM +0100, Oleksij Rempel wrote:
> > > diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> > > index aaf7c6963d61..81e3d7b42d0f 100644
> > > --- a/include/uapi/linux/ethtool_netlink.h
> > > +++ b/include/uapi/linux/ethtool_netlink.h
> > > @@ -51,6 +51,9 @@ enum {
> > > ETHTOOL_MSG_MODULE_SET,
> > > ETHTOOL_MSG_PSE_GET,
> > > ETHTOOL_MSG_PSE_SET,
> > > + ETHTOOL_MSG_PLCA_GET_CFG,
> > > + ETHTOOL_MSG_PLCA_SET_CFG,
> > > + ETHTOOL_MSG_PLCA_GET_STATUS,
> > >
> > > /* add new constants above here */
> > > __ETHTOOL_MSG_USER_CNT,
> > > @@ -97,6 +100,9 @@ enum {
> > > ETHTOOL_MSG_MODULE_GET_REPLY,
> > > ETHTOOL_MSG_MODULE_NTF,
> > > ETHTOOL_MSG_PSE_GET_REPLY,
> > > + ETHTOOL_MSG_PLCA_GET_CFG_REPLY,
> > > + ETHTOOL_MSG_PLCA_GET_STATUS_REPLY,
> > > + ETHTOOL_MSG_PLCA_NTF,
> > >
> > > /* add new constants above here */
> > > __ETHTOOL_MSG_KERNEL_CNT,
> > > @@ -880,6 +886,25 @@ enum {
> > > ETHTOOL_A_PSE_MAX = (__ETHTOOL_A_PSE_CNT - 1)
> > > };
> > >
> > > +/* PLCA */
> > > +
> >
> > Please use names used in the specification as close as possible and
> > document in comments real specification names.
> I was actually following the names in the OPEN Alliance SIG
> specifications which I referenced. Additionally, the OPEN names are more
> similar to those that you can find in Clause 147. As I was trying to
> explain in other threads, the names in Clause 30 were sort of a workaround
> because we were not allowed to add registers in Clause 45.
>
> I can change the names if you really want to, but I'm inclined to keep
> it simple and "user-friendly". People using this technology are more
> used to these names, and they totally ignore Clause 30.
>
> Please, let me know what you think.

A comment about name mapping to specification, spec version and reason
to take one variants instead of other one will be enough Somewhat similar to
what i did for PoDL. See ETHTOOL_A_PODL_* in
Documentation/networking/ethtool-netlink.rst and include/uapi/linux/ethtool.h

It will help people who use spec to review or extend this UAPI.

> > > +
> > > + /* add new constants above here */
> > > + __ETHTOOL_A_PLCA_CNT,
> > > + ETHTOOL_A_PLCA_MAX = (__ETHTOOL_A_PLCA_CNT - 1)
> > > +};
> >
> > Should we have access to 30.16.1.2.2 acPLCAReset in user space?
> I omitted that parameter on purpose. The reason is that again, we were
> "forced" to do this in IEEE802.3cg, but it was a poor choice. I
> understand purity of the specifications, but in the real-world where
> PLCA is implemented in the PHY, resetting the PLCA layer independently
> of the PCS/PMA is all but a good idea: it does more harm than good. As a
> matter of fact, PHY vendors typically map the PLCA reset bit to the PHY
> soft reset bit, or at least to the PCS reset bit.
>
> I'm inclined to keep this as-is and see in the future if and why someone
> would need this feature. What you think?

Ok. Sounds good.

Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |