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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221205102209.GA17619@pengutronix.de>
Date:   Mon, 5 Dec 2022 11:22:09 +0100
From:   Oleksij Rempel <o.rempel@...gutronix.de>
To:     Piergiorgio Beruto <piergiorgio.beruto@...il.com>
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
Subject: Re: [PATCH v2 net-next 1/4] net/ethtool: add netlink interface for
 the PLCA RS

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 |