LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: netdev@vger.kernel.org, Joao Pinto <Joao.Pinto@synopsys.com>,
	Giuseppe Cavallaro <peppe.cavallaro@st.com>,
	Alexandre Torgue <alexandre.torgue@st.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 7/8] net: phy: Add Synopsys DesignWare XPCS MDIO module
Date: Fri, 5 Jun 2020 18:10:34 +0100	[thread overview]
Message-ID: <20200605171034.GF1605@shell.armlinux.org.uk> (raw)
In-Reply-To: <7d9880643585e4347027538df2a722dde54156cf.1583742616.git.Jose.Abreu@synopsys.com>

Hi Jose,

I just tripped over a bug while grepping for something else and
reading a bit of this driver:

On Mon, Mar 09, 2020 at 09:36:26AM +0100, Jose Abreu wrote:
> +static int xpcs_read_lpa(struct mdio_xpcs_args *xpcs,
> +			 struct phylink_link_state *state)
> +{
> +	int ret;
> +
> +	ret = xpcs_read(xpcs, MDIO_MMD_AN, MDIO_STAT1);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!(ret & MDIO_AN_STAT1_LPABLE)) {
> +		phylink_clear(state->lp_advertising, Autoneg);
> +		return 0;
> +	}
> +
> +	phylink_set(state->lp_advertising, Autoneg);
> +
> +	/* Clause 73 outcome */
> +	ret = xpcs_read(xpcs, MDIO_MMD_AN, DW_SR_AN_LP_ABL3);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & DW_C73_2500KX)
> +		phylink_set(state->lp_advertising, 2500baseX_Full);
> +
> +	ret = xpcs_read(xpcs, MDIO_MMD_AN, DW_SR_AN_LP_ABL2);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & DW_C73_1000KX)
> +		phylink_set(state->lp_advertising, 1000baseKX_Full);
> +	if (ret & DW_C73_10000KX4)
> +		phylink_set(state->lp_advertising, 10000baseKX4_Full);
> +	if (ret & DW_C73_10000KR)
> +		phylink_set(state->lp_advertising, 10000baseKR_Full);
> +
> +	ret = xpcs_read(xpcs, MDIO_MMD_AN, DW_SR_AN_LP_ABL1);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & DW_C73_PAUSE)
> +		phylink_set(state->lp_advertising, Pause);
> +	if (ret & DW_C73_ASYM_PAUSE)
> +		phylink_set(state->lp_advertising, Asym_Pause);
> +
> +	linkmode_and(state->lp_advertising, state->lp_advertising,
> +		     state->advertising);

This is incorrect - you should not mask the link partner's advertisement
with our advertisement like this; consider the table in 802.3 for
resolving the pause modes, where simply doing a bitwise-and operation
between the two advertisements would severely restrict the resulting
resolution to either symmetric pause or nothing at all.

You want to do this when you resolve the speed, but only _temporarily_
in order to resolve the speed - you do not want to write back the
result to state->lp_advertising.

You may wish to fix that.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

  reply	other threads:[~2020-06-05 17:10 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-09  8:36 [PATCH net-next 0/8] net: Add support for Synopsys DesignWare XPCS Jose Abreu
2020-03-09  8:36 ` [PATCH net-next 1/8] net: stmmac: selftests: Do not fail if PHY is not attached Jose Abreu
2020-03-09  8:36 ` [PATCH net-next 2/8] net: stmmac: Switch to linkmode_and()/linkmode_andnot() Jose Abreu
2020-03-09  8:36 ` [PATCH net-next 3/8] net: stmmac: Fallback to dev_fwnode() if needed Jose Abreu
2020-03-09  8:36 ` [PATCH net-next 4/8] net: stmmac: Use resolved link config in mac_link_up() Jose Abreu
2020-03-09  8:36 ` [PATCH net-next 5/8] net: phylink: Add missing Backplane speeds Jose Abreu
2020-03-09  8:36 ` [PATCH net-next 6/8] net: phylink: Test if MAC/PCS support Autoneg Jose Abreu
2020-03-09  8:36 ` [PATCH net-next 7/8] net: phy: Add Synopsys DesignWare XPCS MDIO module Jose Abreu
2020-06-05 17:10   ` Russell King - ARM Linux admin [this message]
2020-06-09 14:24     ` Jose Abreu
2020-03-09  8:36 ` [PATCH net-next 8/8] net: stmmac: Integrate it with DesignWare XPCS Jose Abreu
2020-03-10  3:13 ` [PATCH net-next 0/8] net: Add support for Synopsys " David Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200605171034.GF1605@shell.armlinux.org.uk \
    --to=linux@armlinux.org.uk \
    --cc=Joao.Pinto@synopsys.com \
    --cc=Jose.Abreu@synopsys.com \
    --cc=alexandre.torgue@st.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-stm32@st-md-mailman.stormreply.com \
    --cc=mcoquelin.stm32@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=peppe.cavallaro@st.com \
    --subject='Re: [PATCH net-next 7/8] net: phy: Add Synopsys DesignWare XPCS MDIO module' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).