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: <2a6b9908-44d8-3c81-4b44-be1e5568a567@cogentembedded.com>
Date:   Tue, 18 Feb 2020 12:43:41 +0300
From:   Sergei Shtylyov <sergei.shtylyov@...entembedded.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

Hello!

On 18.02.2020 2: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 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;
> +		}

    Do we really need {} here? There's only 1 statement enclosed...

> +	}
> +
> +	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)

MBR, Sergei