linux-kernel - Re: [PATCH net-next v2 3/6] net: pcs: xpcs: add CL37 1000BASE-X AN support

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: <YqLyP6ezO3C9Fe4t@shell.armlinux.org.uk>
Date:   Fri, 10 Jun 2022 08:26:55 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Ong Boon Leong <boon.leong.ong@...el.com>
Cc:     Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu <Jose.Abreu@...opsys.com>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Paolo Abeni <pabeni@...hat.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Vladimir Oltean <olteanv@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Emilio Riva <emilio.riva@...csson.com>
Subject: Re: [PATCH net-next v2 3/6] net: pcs: xpcs: add CL37 1000BASE-X AN
 support

On Fri, Jun 10, 2022 at 11:29:38AM +0800, Ong Boon Leong wrote:
> +static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs,
> +					struct phylink_link_state *state)
> +{
> +	int lpa, adv;
> +	int ret;
> +
> +	if (state->an_enabled) {
> +		/* Reset link state */
> +		state->link = false;
> +
> +		lpa = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_LPA);
> +		if (lpa < 0 || lpa & LPA_RFAULT)
> +			return lpa;
> +
> +		adv = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_ADVERTISE);
> +		if (adv < 0)
> +			return adv;
> +
> +		if (lpa & ADVERTISE_1000XFULL &&
> +		    adv & ADVERTISE_1000XFULL) {
> +			state->link = true;
> +			state->speed = SPEED_1000;
> +			state->duplex = DUPLEX_FULL;
> +		}

phylink_mii_c22_pcs_decode_state() is your friend here, will implement
this correctly, and will set lp_advertising correctly as well.

> +
> +		/* Clear CL37 AN complete status */
> +		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> +		if (ret < 0)
> +			return ret;

Why do you need to clear the interrupt status here? This function will
be called from a work queue sometime later after an interrupt has fired.
It will also be called at random times when userspace enquires what the
link parameters are, so clearing the interrupt here can result in lost
link changes.

> +static void xpcs_link_up_1000basex(struct dw_xpcs *xpcs, int speed,
> +				   int duplex)
> +{
> +	int val, ret;
> +
> +	switch (speed) {
> +	case SPEED_1000:
> +		val = BMCR_SPEED1000;
> +		break;
> +	case SPEED_100:
> +	case SPEED_10:
> +	default:
> +		pr_err("%s: speed = %d\n", __func__, speed);
> +		return;
> +	}
> +
> +	if (duplex == DUPLEX_FULL)
> +		val |= BMCR_FULLDPLX;
> +	else
> +		pr_err("%s: half duplex not supported\n", __func__);
> +
> +	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, val);
> +	if (ret)
> +		pr_err("%s: xpcs_write returned %pe\n", __func__, ERR_PTR(ret));

Does this need to be done even when AN is enabled?

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