netdev - Re: [PATCH net-next 01/15] net: phylink: add PCS negotiation mode

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: <ZJHJAtkEvQ2dAXux@shell.armlinux.org.uk>
Date: Tue, 20 Jun 2023 16:42:58 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Alexander Couzens <lynxis@...0.eu>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Cc@....codeaurora.org:Claudiu Beznea <claudiu.beznea@...rochip.com>,
	Daniel Golle <daniel@...rotopia.org>,
	Daniel Machon <daniel.machon@...rochip.com>,
	"David S. Miller" <davem@...emloft.net>,
	DENG Qingfang <dqfext@...il.com>, Eric Dumazet <edumazet@...gle.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Horatiu Vultur <horatiu.vultur@...rochip.com>,
	Ioana Ciornei <ioana.ciornei@....com>,
	Jakub Kicinski <kuba@...nel.org>,
	Jose Abreu <Jose.Abreu@...opsys.com>,
	Landen Chao <Landen.Chao@...iatek.com>,
	Lars Povlsen <lars.povlsen@...rochip.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org,
	Madalin Bucur <madalin.bucur@....com>,
	Marcin Wojtas <mw@...ihalf.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Michal Simek <michal.simek@....com>, netdev@...r.kernel.org,
	Nicolas Ferre <nicolas.ferre@...rochip.com>,
	Paolo Abeni <pabeni@...hat.com>,
	Radhey Shyam Pandey <radhey.shyam.pandey@...inx.com>,
	Sean Anderson <sean.anderson@...o.com>,
	Sean Wang <sean.wang@...iatek.com>,
	Steen Hegelund <Steen.Hegelund@...rochip.com>,
	Taras Chornyi <taras.chornyi@...ision.eu>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next 01/15] net: phylink: add PCS negotiation mode

On Tue, Jun 20, 2023 at 02:34:29PM +0300, Vladimir Oltean wrote:
> On Fri, Jun 16, 2023 at 01:06:22PM +0100, Russell King (Oracle) wrote:
> > PCS have to work out whether they should enable PCS negotiation by
> > looking at the "mode" and "interface" arguments, and the Autoneg bit
> > in the advertising mask.
> > 
> > This leads to some complex logic, so lets pull that out into phylink
> > and instead pass a "neg_mode" argument to the PCS configuration and
> > link up methods, instead of the "mode" argument.
> > 
> > In order to transition drivers, add a "neg_mode" flag to the phylink
> > PCS structure to PCS can indicate whether they want to be passed the
> > neg_mode or the old mode argument.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> > ---
> > diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> > index 0cf07d7d11b8..2b322d7fa51a 100644
> > --- a/include/linux/phylink.h
> > +++ b/include/linux/phylink.h
> > @@ -21,6 +21,24 @@ enum {
> >  	MLO_AN_FIXED,	/* Fixed-link mode */
> >  	MLO_AN_INBAND,	/* In-band protocol */
> >  
> > +	/* PCS "negotiation" mode.
> > +	 *  PHYLINK_PCS_NEG_NONE - protocol has no inband capability
> > +	 *  PHYLINK_PCS_NEG_OUTBAND - some out of band or fixed link setting
> 
> Would OUTBAND be more clearly named as FORCED maybe?

I don't see how FORCED would improve anything.

> > +	 *  PHYLINK_PCS_NEG_INBAND_DISABLED - inband mode disabled, e.g.
> > +	 *				      1000base-X with autoneg off
> > +	 *  PHYLINK_PCS_NEG_INBAND_ENABLED - inband mode enabled
> > +	 * Additionally, this can be tested using bitmasks:
> > +	 *  PHYLINK_PCS_NEG_INBAND - inband mode selected
> > +	 *  PHYLINK_PCS_NEG_ENABLED - negotiation mode enabled
> > +	 */
> > +	PHYLINK_PCS_NEG_NONE = 0,
> > +	PHYLINK_PCS_NEG_ENABLED = BIT(4),
> 
> Why do we start the enum values from BIT(4)? What are we colliding with,
> in the range of lower values?

I want to make sure that they can't be confused with MLO_AN_* values,
especially since we pass them through the same interface that MLO_AN_*
was using.

> > @@ -79,6 +97,70 @@ static inline bool phylink_autoneg_inband(unsigned int mode)
> >  	return mode == MLO_AN_INBAND;
> >  }
> >  
> > +/**
> > + * phylink_pcs_neg_mode() - helper to determine PCS inband mode
> 
> I think you are naming it "neg_mode" rather than "aneg_mode" because
> with OUTBAND/NONE, there's nothing "automatic" about the negotiation.
> However, "neg" rather than "aneg" sounds more like a shorthand form of
> "negation" or "negative". Would you oppose renaming it to "aneg_mode"?

I want to try to get away from "auto-negotiation" for this because
that has connotations of there being some kind of agreement reached
between both ends, like what happens with baseT links. However, that
is not the case with the SGMII family, which are more a form of
inband signalling.

I can't find any other term for the inband signalling that covers
all cases. "Negotiation" on its own covers both "negotiation result"
for SGMII as well as in-band negotiation for base-X cases. I don't
think "auto-negotiation" covers SGMII, and "in-band signalling"
wouldn't really cover base-X.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!