netdev - Re: [PATCH net-next 2/7] net: phylink: add pcs

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: <0d7361a9-ea74-ce75-b5e0-904596fbefd1@seco.com>
Date:   Tue, 14 Dec 2021 14:49:13 -0500
From:   Sean Anderson <sean.anderson@...o.com>
To:     "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Marcin Wojtas <mw@...ihalf.com>, netdev@...r.kernel.org,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH net-next 2/7] net: phylink: add pcs_validate() method

Hi Russell,

On 12/14/21 9:48 AM, Russell King (Oracle) wrote:
> Add a hook for PCS to validate the link parameters. This avoids MAC
> drivers having to have knowledge of their PCS in their validate()
> method, thereby allowing several MAC drivers to be simplfied.
>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> ---
>   drivers/net/phy/phylink.c | 31 +++++++++++++++++++++++++++++++
>   include/linux/phylink.h   | 20 ++++++++++++++++++++
>   2 files changed, 51 insertions(+)
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index c7035d65e159..420201858564 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -424,13 +424,44 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
>   					struct phylink_link_state *state)
>   {
>   	struct phylink_pcs *pcs;
> +	int ret;
>
> +	/* Get the PCS for this interface mode */
>   	if (pl->mac_ops->mac_select_pcs) {
>   		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
>   		if (IS_ERR(pcs))
>   			return PTR_ERR(pcs);
> +	} else {
> +		pcs = pl->pcs;
> +	}
> +
> +	if (pcs) {
> +		/* The PCS, if present, must be setup before phylink_create()
> +		 * has been called. If the ops is not initialised, print an
> +		 * error and backtrace rather than oopsing the kernel.
> +		 */
> +		if (!pcs->ops) {
> +			phylink_err(pl, "interface %s: uninitialised PCS\n",
> +				    phy_modes(state->interface));
> +			dump_stack();
> +			return -EINVAL;
> +		}
> +
> +		/* Validate the link parameters with the PCS */
> +		if (pcs->ops->pcs_validate) {
> +			ret = pcs->ops->pcs_validate(pcs, supported, state);

I wonder if we can add a pcs->supported_interfaces. That would let me
write something like

static int xilinx_pcs_validate(struct phylink_pcs *pcs,
			       unsigned long *supported,
			       struct phylink_link_state *state)
{
	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };

	phylink_set_port_modes(mask);
	phylink_set(mask, Autoneg);
	phylink_get_linkmodes(mask, state->interface,
			      MAC_10FD | MAC_100FD | MAC_1000FD);

	linkmode_and(supported, supported, mask);
}

And of course, the above could become phylink_pcs_validate_generic with
the addition of a pcs->pcs_capabilities member.

The only wrinkle is that we need to handle PHY_INTERFACE_MODE_NA,
because of the pcs = pl->pcs assignment above. This would require doing
the phylink_validate_any dance again.

Maybe the best way is to stick

	if (state->interface == PHY_INTERFACE_MODE_NA)
		return -EINVAL;

at the top of phylink_pcs_validate_generic (perhaps with a warning).
That would catch any MACs who use a PCS which wants the MAC to have
supported_interfaces.

> +			if (ret < 0 || phylink_is_empty_linkmode(supported))
> +				return -EINVAL;
> +
> +			/* Ensure the advertising mask is a subset of the
> +			 * supported mask.
> +			 */
> +			linkmode_and(state->advertising, state->advertising,
> +				     supported);
> +		}
>   	}
>
> +	/* Then validate the link parameters with the MAC */
>   	pl->mac_ops->validate(pl->config, supported, state);

Shouldn't the PCS stuff happen here? Later in the series, you do things
like

	if (phy_interface_mode_is_8023z(state->interface) &&
	    !phylink_test(state->advertising, Autoneg))
		return -EINVAL;

but there's nothing to stop a mac validate from coming along and saying
"we don't support autonegotiation".

--Sean