netdev - Re: [PATCH net-next v8 6/6] net: tn40xx: add phylink support

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: <20240603.205455.1265693633847576919.fujita.tomonori@gmail.com>
Date: Mon, 03 Jun 2024 20:54:55 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: linux@...linux.org.uk
Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org, andrew@...n.ch,
 horms@...nel.org, kuba@...nel.org, jiri@...nulli.us, pabeni@...hat.com,
 hfdevel@....net, naveenm@...vell.com, jdamato@...tly.com
Subject: Re: [PATCH net-next v8 6/6] net: tn40xx: add phylink support

On Mon, 3 Jun 2024 10:22:17 +0100
"Russell King (Oracle)" <linux@...linux.org.uk> wrote:

> On Mon, Jun 03, 2024 at 03:49:55PM +0900, FUJITA Tomonori wrote:
>> @@ -1374,6 +1375,10 @@ static void tn40_stop(struct tn40_priv *priv)
>>  static int tn40_close(struct net_device *ndev)
>>  {
>>  	struct tn40_priv *priv = netdev_priv(ndev);
>> +
>> +	phylink_stop(priv->phylink);
>> +	phylink_disconnect_phy(priv->phylink);
> 
> There is no need to pair both of these together - you can disconnect
> from the PHY later if it's more convenient.

I see. Seems that there is no reason to call phylink_disconnect_phy()
later so I leave this alone.

>> +
>>  	napi_disable(&priv->napi);
>>  	netif_napi_del(&priv->napi);
>>  	tn40_stop(priv);
>> @@ -1392,6 +1397,14 @@ static int tn40_open(struct net_device *dev)
>>  		return ret;
>>  	}
>>  	napi_enable(&priv->napi);
>> +	ret = phylink_connect_phy(priv->phylink, priv->phydev);
>> +	if (ret) {
>> +		napi_disable(&priv->napi);
>> +		tn40_stop(priv);
>> +		netdev_err(dev, "failed to connect to phy %d\n", ret);
>> +		return ret;
>> +	}
> 
> Again, no need to pair phylink_connect_phy() close to phylink_start()
> if there's somewhere more convenient to place it. Operation with the
> PHY doesn't begin until phylink_start() is called.
> 
> My review comment last time was purely about where phylink_start()
> and phylink_stop() were being called. It's the placement of these
> two functions that are key.

Understood. Calling phylink_connect_phy() first in this function looks
simpler. I modified the code in the following way:

@@ -1385,13 +1390,20 @@ static int tn40_open(struct net_device *dev)
 	struct tn40_priv *priv = netdev_priv(dev);
 	int ret;
 
+	ret = phylink_connect_phy(priv->phylink, priv->phydev);
+	if (ret) {
+		netdev_err(dev, "failed to connect to phy %d\n", ret);
+		return ret;
+	}
 	tn40_sw_reset(priv);
 	ret = tn40_start(priv);
 	if (ret) {
+		phylink_disconnect_phy(priv->phylink);
 		netdev_err(dev, "failed to start %d\n", ret);
 		return ret;
 	}
 	napi_enable(&priv->napi);
+	phylink_start(priv->phylink);
 	netif_start_queue(priv->ndev);
 	return 0;
 }


Thanks a lot!