netdev - Re: [PATCH net-next 3/3] net: phy: move handling latched link-down to phylib state machine

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:   Tue, 28 May 2019 20:22:35 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 3/3] net: phy: move handling latched link-down to
 phylib state machine

On 28.05.2019 15:15, Russell King - ARM Linux admin wrote:
> On Mon, May 27, 2019 at 08:29:45PM +0200, Heiner Kallweit wrote:
>> Especially with fibre links there may be very short link drops. And if
>> interrupt handling is slow we may miss such a link drop. To deal with
>> this we remove the double link status read from the generic link status
>> read functions, and call the state machine twice instead.
>> The flag for double-reading link status can be set by phy_mac_interrupt
>> from hard irq context, therefore we have to use an atomic operation.
> 
> I came up with a different solution to this - I haven't extensively
> tested it yet though:
> 
>  drivers/net/phy/phy-c45.c    | 12 ------------
>  drivers/net/phy/phy.c        | 29 +++++++++++++++++++----------
>  drivers/net/phy/phy_device.c | 14 --------------
>  3 files changed, 19 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index 9e24d9569424..756d7711cbc5 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -222,18 +222,6 @@ int genphy_c45_read_link(struct phy_device *phydev)
>  		devad = __ffs(mmd_mask);
>  		mmd_mask &= ~BIT(devad);
>  
> -		/* The link state is latched low so that momentary link
> -		 * drops can be detected. Do not double-read the status
> -		 * in polling mode to detect such short link drops.
> -		 */
> -		if (!phy_polling_mode(phydev)) {
> -			val = phy_read_mmd(phydev, devad, MDIO_STAT1);
> -			if (val < 0)
> -				return val;
> -			else if (val & MDIO_STAT1_LSTATUS)
> -				continue;
> -		}
> -
>  		val = phy_read_mmd(phydev, devad, MDIO_STAT1);
>  		if (val < 0)
>  			return val;
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 7b3c5eec0129..2e7f0428e8fa 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -507,20 +507,29 @@ static int phy_config_aneg(struct phy_device *phydev)
>   */
>  static int phy_check_link_status(struct phy_device *phydev)
>  {
> -	int err;
> +	int err, i;
>  
>  	WARN_ON(!mutex_is_locked(&phydev->lock));
>  
> -	err = phy_read_status(phydev);
> -	if (err)
> -		return err;
> +	/* The link state is latched low so that momentary link drops can
> +	 * be detected. If the link has failed, re-read the link status
> +	 * to ensure that we are up to date with the current link state,
> +	 * while notifying that the link status has changed.
> +	 */
> +	for (i = 0; i < 2; i++) {
> +		err = phy_read_status(phydev);
> +		if (err)
> +			return err;
>  
> -	if (phydev->link && phydev->state != PHY_RUNNING) {
> -		phydev->state = PHY_RUNNING;
> -		phy_link_up(phydev);
> -	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
> -		phydev->state = PHY_NOLINK;
> -		phy_link_down(phydev, true);
> +		if (phydev->link && phydev->state != PHY_RUNNING) {
> +			phydev->state = PHY_RUNNING;
> +			phy_link_up(phydev);
> +		} else if (!phydev->link && phydev->state != PHY_NOLINK) {
> +			phydev->state = PHY_NOLINK;
> +			phy_link_down(phydev, true);
> +		}
> +		if (phydev->link)
> +			break;

One drawback of this approach may be that if we have an up-down-up
cycle then callback link_change_notify isn't called for either
transition. In most cases this shouldn't be an issue, but it's not
the expected behavior.

>  	}
>  
>  	return 0;
> [...]