From: Heiner Kallweit <hkallweit1@gmail.com>
To: Antonio Borneo <antonio.borneo@st.com>,
Russell King - ARM Linux admin <linux@armlinux.org.uk>
Cc: Andrew Lunn <andrew@lunn.ch>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
netdev@vger.kernel.org, Yonglong Liu <liuyonglong@huawei.com>,
stable@vger.kernel.org, linuxarm@huawei.com,
Salil Mehta <salil.mehta@huawei.com>,
linux-stm32@st-md-mailman.stormreply.com,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: phy: fix auto-negotiation in case of 'down-shift'
Date: Tue, 24 Nov 2020 16:26:43 +0100 [thread overview]
Message-ID: <2dc7ad93-5719-dd8a-44a9-8667a22a3b19@gmail.com> (raw)
In-Reply-To: <bd83b9c15f6cfed5df90da4f6b50d1a3f479b831.camel@st.com>
Am 24.11.2020 um 16:17 schrieb Antonio Borneo:
> On Tue, 2020-11-24 at 14:56 +0000, Russell King - ARM Linux admin wrote:
>> On Tue, Nov 24, 2020 at 03:38:48PM +0100, Antonio Borneo wrote:
>>> If the auto-negotiation fails to establish a gigabit link, the phy
>>> can try to 'down-shift': it resets the bits in MII_CTRL1000 to
>>> stop advertising 1Gbps and retries the negotiation at 100Mbps.
>>>
>>> From commit 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode
>>> in genphy_read_status") the content of MII_CTRL1000 is not checked
>>> anymore at the end of the negotiation, preventing the detection of
>>> phy 'down-shift'.
>>> In case of 'down-shift' phydev->advertising gets out-of-sync wrt
>>> MII_CTRL1000 and still includes modes that the phy have already
>>> dropped. The link partner could still advertise higher speeds,
>>> while the link is established at one of the common lower speeds.
>>> The logic 'and' in phy_resolve_aneg_linkmode() between
>>> phydev->advertising and phydev->lp_advertising will report an
>>> incorrect mode.
>>>
>>> Issue detected with a local phy rtl8211f connected with a gigabit
>>> capable router through a two-pairs network cable.
>>>
>>> After auto-negotiation, read back MII_CTRL1000 and mask-out from
>>> phydev->advertising the modes that have been eventually discarded
>>> due to the 'down-shift'.
>>
>> Sorry, but no. While your solution will appear to work, in
>> introduces unexpected changes to the user visible APIs.
>>
>>> if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
>>> + if (phydev->is_gigabit_capable) {
>>> + adv = phy_read(phydev, MII_CTRL1000);
>>> + if (adv < 0)
>>> + return adv;
>>> + /* update advertising in case of 'down-shift' */
>>> + mii_ctrl1000_mod_linkmode_adv_t(phydev->advertising,
>>> + adv);
>>
>> If a down-shift occurs, this will cause the configured advertising
>> mask to lose the 1G speed, which will be visible to userspace.
>
> You are right, it gets propagated to user that 1Gbps is not advertised
>
>> Userspace doesn't expect the advertising mask to change beneath it.
>> Since updates from userspace are done using a read-modify-write of
>> the ksettings, this can have the undesired effect of removing 1G
>> from the configured advertising mask.
>>
>> We've had other PHYs have this behaviour; the correct solution is for
>> the PHY driver to implement reading the resolution from the PHY rather
>> than relying on the generic implementation if it can down-shift
>
> If it's already upstream, could you please point to one of the phy driver
> that already implements this properly?
>
See e.g. aqr107_read_rate(), used by aqr107_read_status().
> Thanks
> Antonio
>
next prev parent reply other threads:[~2020-11-24 15:26 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-24 14:38 [PATCH] net: phy: fix auto-negotiation in case of 'down-shift' Antonio Borneo
2020-11-24 14:56 ` Russell King - ARM Linux admin
2020-11-24 15:17 ` Antonio Borneo
2020-11-24 15:26 ` Heiner Kallweit [this message]
2020-11-24 15:37 ` Russell King - ARM Linux admin
2020-11-24 17:00 ` Antonio Borneo
2020-11-24 15:03 ` Heiner Kallweit
2020-11-24 15:17 ` Russell King - ARM Linux admin
2020-11-24 15:31 ` Antonio Borneo
2020-11-24 15:46 ` David Laight
2020-11-24 21:59 ` [PATCH v2] net: phy: realtek: read actual speed on rtl8211f to detect downshift Antonio Borneo
2020-11-24 22:22 ` Heiner Kallweit
2020-11-24 22:33 ` Antonio Borneo
2020-11-24 22:41 ` Heiner Kallweit
2020-11-24 23:07 ` [PATCH v3 net-next] " Antonio Borneo
2020-11-25 15:03 ` Yonglong Liu
2020-11-25 16:57 ` Yonglong Liu
2020-11-25 17:07 ` Russell King - ARM Linux admin
2020-11-26 1:15 ` Yonglong Liu
2020-11-25 20:30 ` Jakub Kicinski
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=2dc7ad93-5719-dd8a-44a9-8667a22a3b19@gmail.com \
--to=hkallweit1@gmail.com \
--cc=andrew@lunn.ch \
--cc=antonio.borneo@st.com \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-stm32@st-md-mailman.stormreply.com \
--cc=linux@armlinux.org.uk \
--cc=linuxarm@huawei.com \
--cc=liuyonglong@huawei.com \
--cc=netdev@vger.kernel.org \
--cc=salil.mehta@huawei.com \
--cc=stable@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).