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

Message-ID: <20240822172401.43fe8dd2@kernel.org>
Date: Thu, 22 Aug 2024 17:24:01 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Jacky Chou <jacky_chou@...eedtech.com>
Cc: <davem@...emloft.net>, <edumazet@...gle.com>, <pabeni@...hat.com>,
 <u.kleine-koenig@...gutronix.de>, <netdev@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] net: ftgmac100: Get link speed and duplex for NC-SI

On Thu, 22 Aug 2024 11:19:45 +0800 Jacky Chou wrote:
> 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

> 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

>  #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

>  	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?
-- 
pw-bot: cr