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: <Zl2LSfGqvPUvUoRT@shell.armlinux.org.uk>
Date: Mon, 3 Jun 2024 10:22:17 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: 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, 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.

> +
>  	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.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!