netdev - RE: [PATCH net-next 1/2] net: phy: don't stop state machine in case of MDIO error

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]

Message-ID: <VI1PR0402MB3600D0AA64972E36908258BCFFBC0@VI1PR0402MB3600.eurprd04.prod.outlook.com>
Date:   Mon, 17 Dec 2018 02:14:44 +0000
From:   Andy Duan <fugang.duan@....com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH net-next 1/2] net: phy: don't stop state machine in case
 of MDIO error

From: Heiner Kallweit <hkallweit1@...il.com> Sent: 2018年12月16日 0:19
> If we detect a MDIO error, it seems to be a little bit too aggressive to stop the
> state machine and bring down the PHY completely.
> E.g. when polling and we miss one update, then this has no relevant impact.
> And in phy_stop_interrupts() actually interrupts should be disabled already
> because phy_stop() is called before. The only caller of phy_stop_interrupts()
> isn't interested in the return value, so let's change return type to void.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
> ---
>  drivers/net/phy/phy.c | 42 +++++++++---------------------------------
>  include/linux/phy.h   |  2 +-
>  2 files changed, 10 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index
> e24708f1f..f926ec52a 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -714,24 +714,6 @@ void phy_stop_machine(struct phy_device *phydev)
>  	mutex_unlock(&phydev->lock);
>  }
> 
> -/**
> - * phy_error - enter HALTED state for this PHY device
> - * @phydev: target phy_device struct
> - *
> - * Moves the PHY to the HALTED state in response to a read
> - * or write error, and tells the controller the link is down.
> - * Must not be called from interrupt context, or while the
> - * phydev->lock is held.
> - */
> -static void phy_error(struct phy_device *phydev) -{
> -	mutex_lock(&phydev->lock);
> -	phydev->state = PHY_HALTED;
> -	mutex_unlock(&phydev->lock);
> -
> -	phy_trigger_machine(phydev);
> -}
> -
>  /**
>   * phy_disable_interrupts - Disable the PHY interrupts from the PHY side
>   * @phydev: target phy_device struct
> @@ -769,13 +751,12 @@ static irqreturn_t phy_interrupt(int irq, void
> *phy_dat)
>  	/* reschedule state queue work to run as soon as possible */
>  	phy_trigger_machine(phydev);
> 
> -	if (phy_clear_interrupt(phydev))
> -		goto phy_err;
> -	return IRQ_HANDLED;
> +	if (phy_clear_interrupt(phydev)) {
> +		phydev_err(phydev, "failed to ack interrupt\n");
> +		return IRQ_NONE;
> +	}
> 
> -phy_err:
> -	phy_error(phydev);
> -	return IRQ_NONE;
> +	return IRQ_HANDLED;
>  }
> 
>  /**
> @@ -821,16 +802,10 @@ EXPORT_SYMBOL(phy_start_interrupts);
>   * phy_stop_interrupts - disable interrupts from a PHY device
>   * @phydev: target phy_device struct
>   */
> -int phy_stop_interrupts(struct phy_device *phydev)
> +void phy_stop_interrupts(struct phy_device *phydev)
>  {
> -	int err = phy_disable_interrupts(phydev);
> -
> -	if (err)
> -		phy_error(phydev);
> -
> +	phy_disable_interrupts(phydev);
>  	free_irq(phydev->irq, phydev);
> -
> -	return err;
>  }
>  EXPORT_SYMBOL(phy_stop_interrupts);
> 
> @@ -969,7 +944,8 @@ void phy_state_machine(struct work_struct *work)
>  		phy_suspend(phydev);
> 
>  	if (err < 0)
> -		phy_error(phydev);
> +		phydev_err(phydev,
> +			   "error %d executing PHY state machine\n", err);
If to stop state machine when PHY is in polling mode, so we have to return here ?

> 
>  	if (old_state != phydev->state)
>  		phydev_dbg(phydev, "PHY state change %s -> %s\n", diff --git
> a/include/linux/phy.h b/include/linux/phy.h index 8f927246a..9b2235bd5
> 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -932,7 +932,7 @@ int phy_aneg_done(struct phy_device *phydev);  int
> phy_speed_down(struct phy_device *phydev, bool sync);  int
> phy_speed_up(struct phy_device *phydev);
> 
> -int phy_stop_interrupts(struct phy_device *phydev);
> +void phy_stop_interrupts(struct phy_device *phydev);
>  int phy_restart_aneg(struct phy_device *phydev);  int
> phy_reset_after_clk_enable(struct phy_device *phydev);
> 
> --
> 2.20.0
>