From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>,
Florian Fainelli <f.fainelli@gmail.com>,
David Miller <davem@davemloft.net>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Subject: Re: [PATCH net-next 1/3] net: phy: add and use phy_check_downshift
Date: Wed, 18 Mar 2020 23:21:59 +0000 [thread overview]
Message-ID: <20200318232159.GA25745@shell.armlinux.org.uk> (raw)
In-Reply-To: <d2822357-4c1e-a072-632e-a902b04eba7c@gmail.com>
On Wed, Mar 18, 2020 at 10:29:01PM +0100, Heiner Kallweit wrote:
> So far PHY drivers have to check whether a downshift occurred to be
> able to notify the user. To make life of drivers authors a little bit
> easier move the downshift notification to phylib. phy_check_downshift()
> compares the highest mutually advertised speed with the actual value
> of phydev->speed (typically read by the PHY driver from a
> vendor-specific register) to detect a downshift.
My personal position on this is that reporting a downshift will be
sporadic at best, even when the link has negotiated slower.
The reason for this is that either end can decide to downshift. If
the remote partner downshifts, then the local side has no idea that
a downshift occurred, and can't report that the link was downshifted.
So, is it actually useful to report these events?
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/phy/phy-core.c | 33 +++++++++++++++++++++++++++++++++
> drivers/net/phy/phy.c | 1 +
> include/linux/phy.h | 1 +
> 3 files changed, 35 insertions(+)
>
> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
> index e083e7a76..8e861be73 100644
> --- a/drivers/net/phy/phy-core.c
> +++ b/drivers/net/phy/phy-core.c
> @@ -329,6 +329,39 @@ void phy_resolve_aneg_linkmode(struct phy_device *phydev)
> }
> EXPORT_SYMBOL_GPL(phy_resolve_aneg_linkmode);
>
> +/**
> + * phy_check_downshift - check whether downshift occurred
> + * @phydev: The phy_device struct
> + *
> + * Check whether a downshift to a lower speed occurred. If this should be the
> + * case warn the user.
> + */
> +bool phy_check_downshift(struct phy_device *phydev)
> +{
> + __ETHTOOL_DECLARE_LINK_MODE_MASK(common);
> + int i, speed = SPEED_UNKNOWN;
> +
> + if (phydev->autoneg == AUTONEG_DISABLE)
> + return false;
> +
> + linkmode_and(common, phydev->lp_advertising, phydev->advertising);
> +
> + for (i = 0; i < ARRAY_SIZE(settings); i++)
> + if (test_bit(settings[i].bit, common)) {
> + speed = settings[i].speed;
> + break;
> + }
> +
> + if (phydev->speed == speed)
> + return false;
> +
> + phydev_warn(phydev, "Downshift occurred from negotiated speed %s to actual speed %s, check cabling!\n",
> + phy_speed_to_str(speed), phy_speed_to_str(phydev->speed));
> +
> + return true;
> +}
> +EXPORT_SYMBOL_GPL(phy_check_downshift);
> +
> static int phy_resolve_min_speed(struct phy_device *phydev, bool fdx_only)
> {
> __ETHTOOL_DECLARE_LINK_MODE_MASK(common);
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d71212a41..067ff5fec 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -507,6 +507,7 @@ static int phy_check_link_status(struct phy_device *phydev)
> return err;
>
> if (phydev->link && phydev->state != PHY_RUNNING) {
> + phy_check_downshift(phydev);
> phydev->state = PHY_RUNNING;
> phy_link_up(phydev);
> } else if (!phydev->link && phydev->state != PHY_NOLINK) {
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index cb5a2182b..4962766b2 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -698,6 +698,7 @@ static inline bool phy_is_started(struct phy_device *phydev)
>
> void phy_resolve_aneg_pause(struct phy_device *phydev);
> void phy_resolve_aneg_linkmode(struct phy_device *phydev);
> +bool phy_check_downshift(struct phy_device *phydev);
>
> /**
> * phy_read - Convenience function for reading a given PHY register
> --
> 2.25.1
>
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up
next prev parent reply other threads:[~2020-03-18 23:22 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-18 21:27 [PATCH net-next 0/3] net: phy: add and use phy_check_downshift Heiner Kallweit
2020-03-18 21:29 ` [PATCH net-next 1/3] " Heiner Kallweit
2020-03-18 23:21 ` Russell King - ARM Linux admin [this message]
2020-03-19 7:30 ` Heiner Kallweit
2020-03-19 11:25 ` Russell King - ARM Linux admin
2020-03-19 13:04 ` Andrew Lunn
2020-03-19 13:58 ` Russell King - ARM Linux admin
2020-03-19 16:36 ` Heiner Kallweit
2020-03-19 16:55 ` Florian Fainelli
2020-03-19 17:04 ` Heiner Kallweit
2020-03-19 17:08 ` Russell King - ARM Linux admin
2020-03-20 8:07 ` Heiner Kallweit
2020-03-19 9:06 ` Andrew Lunn
2020-03-19 9:10 ` Andrew Lunn
2020-03-18 21:29 ` [PATCH net-next 2/3] net: phy: marvell: remove downshift warning now that phylib takes care Heiner Kallweit
2020-03-18 21:31 ` [PATCH net-next 3/3] net: phy: aquantia: " Heiner Kallweit
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=20200318232159.GA25745@shell.armlinux.org.uk \
--to=linux@armlinux.org.uk \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=hkallweit1@gmail.com \
--cc=netdev@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).