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]

Date:   Tue, 18 Feb 2020 11:30:44 +0100
From:   Oleksij Rempel <linux@...pel-privat.de>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Hauke Mehrtens <hauke@...ke-m.de>, davem@...emloft.net
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

Hi,

current kernel still missing following patch:
https://github.com/olerem/linux-2.6/commit/9a9a531bbad6d00c6221f393fd85628475e1f121

@Hauke, can you please test, rebase your changes (if needed) on top of this patch?

Am 18.02.20 um 08:02 schrieb Heiner Kallweit:
> 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)
>>
>


--
Regards,
Oleksij