netdev - Re: [PATCH 3/3] ag71xx: Run ag71xx_link

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: <b980f225-66ca-1ae1-23cf-eff06810b050@gmail.com>
Date:   Tue, 18 Feb 2020 08:02:11 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Hauke Mehrtens <hauke@...ke-m.de>, davem@...emloft.net,
        linux@...pel-privat.de
Cc:     netdev@...r.kernel.org, jcliburn@...il.com, chris.snook@...il.com
Subject: Re: [PATCH 3/3] ag71xx: Run ag71xx_link_adjust() only when needed

On 18.02.2020 00:35, Hauke Mehrtens wrote:
> My system printed this line every second:
>   ag71xx 19000000.eth eth0: Link is Up - 1Gbps/Full - flow control off
> The function ag71xx_phy_link_adjust() was called by the PHY layer every
> second even when nothing changed.
> 
With current phylib code this shouldn't happen, see phy_check_link_status().
On which kernel version do you observe this behavior?

> With this patch the old status is stored and the real the
> ag71xx_link_adjust() function is only called when something really
> changed. This way the update and also this print is only done once any
> more.
> 
> Signed-off-by: Hauke Mehrtens <hauke@...ke-m.de>
> Fixes: d51b6ce441d3 ("net: ethernet: add ag71xx driver")
> ---
>  drivers/net/ethernet/atheros/ag71xx.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
> index 7d3fec009030..12eaf6d2518d 100644
> --- a/drivers/net/ethernet/atheros/ag71xx.c
> +++ b/drivers/net/ethernet/atheros/ag71xx.c
> @@ -307,6 +307,10 @@ struct ag71xx {
>  	u32 msg_enable;
>  	const struct ag71xx_dcfg *dcfg;
>  
> +	unsigned int		link;
> +	unsigned int		speed;
> +	int			duplex;
> +
>  	/* From this point onwards we're not looking at per-packet fields. */
>  	void __iomem *mac_base;
>  
> @@ -854,6 +858,7 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool update)
>  
>  	if (!phydev->link && update) {
>  		ag71xx_hw_stop(ag);
> +		phy_print_status(phydev);
>  		return;
>  	}
>  
> @@ -907,8 +912,25 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool update)
>  static void ag71xx_phy_link_adjust(struct net_device *ndev)
>  {
>  	struct ag71xx *ag = netdev_priv(ndev);
> +	struct phy_device *phydev = ndev->phydev;
> +	int status_change = 0;
> +
> +	if (phydev->link) {
> +		if (ag->duplex != phydev->duplex ||
> +		    ag->speed != phydev->speed) {
> +			status_change = 1;
> +		}
> +	}
> +
> +	if (phydev->link != ag->link)
> +		status_change = 1;
> +
> +	ag->link = phydev->link;
> +	ag->duplex = phydev->duplex;
> +	ag->speed = phydev->speed;
>  
> -	ag71xx_link_adjust(ag, true);
> +	if (status_change)
> +		ag71xx_link_adjust(ag, true);
>  }
>  
>  static int ag71xx_phy_connect(struct ag71xx *ag)
>