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: <20230620113429.mc4p4y6mny5cm4ih@skbuf>
Date: Tue, 20 Jun 2023 14:34:29 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>
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 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?

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

> +	PHYLINK_PCS_NEG_OUTBAND = BIT(5),
> +	PHYLINK_PCS_NEG_INBAND = BIT(6),
> +	PHYLINK_PCS_NEG_INBAND_DISABLED = PHYLINK_PCS_NEG_INBAND,
> +	PHYLINK_PCS_NEG_INBAND_ENABLED = PHYLINK_PCS_NEG_INBAND |
> +					 PHYLINK_PCS_NEG_ENABLED,
> +
>  	/* MAC_SYM_PAUSE and MAC_ASYM_PAUSE are used when configuring our
>  	 * autonegotiation advertisement. They correspond to the PAUSE and
>  	 * ASM_DIR bits defined by 802.3, respectively.
> @@ -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"?