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

[Piergiorgio Beruto]

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.

> > +
> > +	/* 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?

Thanks,
Piergiorgio