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

[Oleksij Rempel <o.rempel@pengutronix.de>]

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 |