netdev - RE: [PATCH v2] net: ftgmac100: Get link speed and duplex for NC-SI

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] [day] [month] [year] [list]

Message-ID:
 <SEYPR06MB5134C3B5487B03BB25D807B89D882@SEYPR06MB5134.apcprd06.prod.outlook.com>
Date: Fri, 23 Aug 2024 03:46:41 +0000
From: Jacky Chou <jacky_chou@...eedtech.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: "davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"u.kleine-koenig@...gutronix.de" <u.kleine-koenig@...gutronix.de>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] net: ftgmac100: Get link speed and duplex for NC-SI

> > The ethtool of this driver uses the phy API of ethtool to get the link
> > information from PHY driver.
> > Because the NC-SI is forced on 100Mbps and full duplex, the driver
> > connects a fixed-link phy driver for NC-SI.
> 
> replace: the driver connects -> connect
I will refine the commit message.

> 
> > The ethtool will get the link information from the fixed-link phy
> > driver.
> 
> Hm. I defer to the PHY experts on the merits.
> 
> > Signed-off-by: Jacky Chou <jacky_chou@...eedtech.com>
> > ---
> > v2:
> >   - use static for struct fixed_phy_status ncsi_phy_status
> >   - Stop phy device at net_device stop when using NC-SI.
> >   - Start phy device at net_device start when using NC-SI.
> > ---
> >  drivers/net/ethernet/faraday/ftgmac100.c | 24
> > ++++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
> > b/drivers/net/ethernet/faraday/ftgmac100.c
> > index fddfd1dd5070..93862b027be0 100644
> > --- a/drivers/net/ethernet/faraday/ftgmac100.c
> > +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/of_net.h>
> >  #include <net/ip.h>
> >  #include <net/ncsi.h>
> > +#include <linux/phy_fixed.h>
> 
> Keep the headers sorted, put the new one after of_net.h
Agree. I will adjust the <linux/phy_fixed.h> header.

> 
> >  #include "ftgmac100.h"
> >
> 
> > @@ -1794,6 +1805,7 @@ static int ftgmac100_probe(struct platform_device
> *pdev)
> >  	struct net_device *netdev;
> >  	struct ftgmac100 *priv;
> >  	struct device_node *np;
> > +	struct phy_device *phydev;
> 
> keep the variable declarations sorted longest to shortest if possible
Agree. I will change the variable next version.

> 
> >  	int err = 0;
> >
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -1879,6
> > +1891,14 @@ static int ftgmac100_probe(struct platform_device *pdev)
> >  			err = -EINVAL;
> >  			goto err_phy_connect;
> >  		}
> > +
> > +		phydev = fixed_phy_register(PHY_POLL, &ncsi_phy_status, NULL);
> > +		err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link,
> > +					 PHY_INTERFACE_MODE_MII);
> > +		if (err) {
> > +			dev_err(&pdev->dev, "Connecting PHY failed\n");
> > +			goto err_phy_connect;
> > +		}
> 
> Very suspicious that you register it but you never unregister it.
> Are you sure the error path and .remove don't need to be changed?
Agree. It needs the unregister for the PHY device.
When using NC-SI to register fixed-link PHY device, I wiil add the 
unregister function in ftgmac100_phy_disconnect().
Thanks.