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]

Date:   Mon, 13 Jun 2022 23:44:35 +0000
From:   "Ong, Boon Leong" <boon.leong.ong@...el.com>
To:     Russell King <linux@...linux.org.uk>
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" <netdev@...r.kernel.org>,
        "linux-stm32@...md-mailman.stormreply.com" 
        <linux-stm32@...md-mailman.stormreply.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Riva, Emilio" <emilio.riva@...csson.com>
Subject: RE: [PATCH net-next v2 3/6] net: pcs: xpcs: add CL37 1000BASE-X AN
 support

>> +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.
Thank for the suggestion. I will change it accordingly to use
phylink_mii_c22_pcs_decode_state()

>
>> +
>> +		/* 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.
Thanks for pointing it. Ya, it is unnecessary.

>
>> +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?
Thanks. Ya, it does not need. Will fix. 


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