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]
Date:   Mon, 5 Dec 2022 11:03:58 +0100
From:   Piergiorgio Beruto <piergiorgio.beruto@...il.com>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
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

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

Powered by blists - more mailing lists