[PATCH net-next v2 00/15] net: ftgmac100: Various probe cleanups

[PATCH net-next v2 00/15] net: ftgmac100: Various probe cleanups

[Jacky Chou]

The probe function of the ftgmac100 is rather complex, due to the way
it has evolved over time, dealing with poor DT descriptions, and new
variants of the MAC.

Make use of DT match data to identify the MAC variant, rather than
looking at the compatible string all the time.

Make use of devm_ calls to simplify cleanup. This indirectly fixes
inconsistent goto label names.

Always probe the MDIO bus, when it exists. This simplifies the logic a
bit.

Move code into helpers to simply probe.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
Changes in v2:
- Add net-next prefix.
- [08/15] Updated commit message.
- [04/15] Deleted {}.
- Link to v1: https://lore.kernel.org/r/20260105-ftgmac-cleanup-v1-0-b68e4a3d8fbe@aspeedtech.com

---
Andrew Lunn (15):
      net: ftgmac100: List all compatibles
      net: ftgmac100: Add match data containing MAC ID
      net: ftgmac100: Replace all of_device_is_compatible()
      net: ftgmac100: Use devm_alloc_etherdev()
      net: ftgmac100: Use devm_request_memory_region/devm_ioremap
      net: ftgmac100: Use devm_clk_get_enabled
      net: ftgmac100: Simplify error handling for ftgmac100_initial_mac
      net: ftgmac100: Move NCSI probe code into a helper
      net: ftgmac100: Always register the MDIO bus when it exists
      net: ftgmac100: Simplify legacy MDIO setup
      net: ftgmac100: Move DT probe into a helper
      net: ftgmac100: Remove redundant PHY_POLL
      net: ftgmac100: Simplify error handling for ftgmac100_setup_mdio
      net: ftgmac100: Simplify condition on HW arbitration
      net: ftgmac100: Fix wrong netif_napi_del in release

 drivers/net/ethernet/faraday/ftgmac100.c | 305 +++++++++++++++++--------------
 1 file changed, 170 insertions(+), 135 deletions(-)
---
base-commit: d4596891e72cbf155d61798a81ce9d36b69bfaf4
change-id: 20251208-ftgmac-cleanup-20b223bf4681

Best regards,
-- 
Jacky Chou <jacky_chou@aspeedtech.com>

[PATCH net-next v2 01/15] net: ftgmac100: List all compatibles

[Jacky Chou]

From: Andrew Lunn <andrew@lunn.ch>

As a step towards cleanup the probe function, list each compatible the
driver supports.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index a863f7841210..bd768a93b9e6 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -2091,6 +2091,9 @@ static void ftgmac100_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id ftgmac100_of_match[] = {
+	{ .compatible = "aspeed,ast2400-mac" },
+	{ .compatible = "aspeed,ast2500-mac" },
+	{ .compatible = "aspeed,ast2600-mac" },
 	{ .compatible = "faraday,ftgmac100" },
 	{ }
 };

-- 
2.34.1

[PATCH net-next v2 02/15] net: ftgmac100: Add match data containing MAC ID

[Jacky Chou]

From: Andrew Lunn <andrew@lunn.ch>

The driver supports 4 different versions of the FTGMAC core.  Extend
the compatible matching to include match data, which indicates the
version of the MAC. Default to the initial Faraday device if DT is not
being used. Lookup the match data early in probe to keep error handing
simple.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 55 +++++++++++++++++++++++++++++---
 1 file changed, 50 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index bd768a93b9e6..104eb7b1f5bb 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -33,6 +33,17 @@
 
 #define DRV_NAME	"ftgmac100"
 
+enum ftgmac100_mac_id {
+	FTGMAC100_FARADAY = 1,
+	FTGMAC100_AST2400,
+	FTGMAC100_AST2500,
+	FTGMAC100_AST2600
+};
+
+struct ftgmac100_match_data {
+	enum ftgmac100_mac_id mac_id;
+};
+
 /* Arbitrary values, I am not sure the HW has limits */
 #define MAX_RX_QUEUE_ENTRIES	1024
 #define MAX_TX_QUEUE_ENTRIES	1024
@@ -66,6 +77,8 @@ struct ftgmac100 {
 	struct resource *res;
 	void __iomem *base;
 
+	enum ftgmac100_mac_id mac_id;
+
 	/* Rx ring */
 	unsigned int rx_q_entries;
 	struct ftgmac100_rxdes *rxdes;
@@ -1835,6 +1848,8 @@ static bool ftgmac100_has_child_node(struct device_node *np, const char *name)
 
 static int ftgmac100_probe(struct platform_device *pdev)
 {
+	const struct ftgmac100_match_data *match_data;
+	enum ftgmac100_mac_id mac_id;
 	struct resource *res;
 	int irq;
 	struct net_device *netdev;
@@ -1843,6 +1858,16 @@ static int ftgmac100_probe(struct platform_device *pdev)
 	struct device_node *np;
 	int err = 0;
 
+	np = pdev->dev.of_node;
+	if (np) {
+		match_data = of_device_get_match_data(&pdev->dev);
+		if (!match_data)
+			return -EINVAL;
+		mac_id = match_data->mac_id;
+	} else {
+		mac_id = FTGMAC100_FARADAY;
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
 		return -ENXIO;
@@ -1870,6 +1895,7 @@ static int ftgmac100_probe(struct platform_device *pdev)
 	priv = netdev_priv(netdev);
 	priv->netdev = netdev;
 	priv->dev = &pdev->dev;
+	priv->mac_id = mac_id;
 	INIT_WORK(&priv->reset_task, ftgmac100_reset_task);
 
 	/* map io memory */
@@ -1900,7 +1926,6 @@ static int ftgmac100_probe(struct platform_device *pdev)
 	if (err)
 		goto err_phy_connect;
 
-	np = pdev->dev.of_node;
 	if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac") ||
 		   of_device_is_compatible(np, "aspeed,ast2500-mac") ||
 		   of_device_is_compatible(np, "aspeed,ast2600-mac"))) {
@@ -2090,11 +2115,31 @@ static void ftgmac100_remove(struct platform_device *pdev)
 	free_netdev(netdev);
 }
 
+static const struct ftgmac100_match_data ftgmac100_match_data_ast2400 = {
+	.mac_id = FTGMAC100_AST2400
+};
+
+static const struct ftgmac100_match_data ftgmac100_match_data_ast2500 = {
+	.mac_id = FTGMAC100_AST2500
+};
+
+static const struct ftgmac100_match_data ftgmac100_match_data_ast2600 = {
+	.mac_id = FTGMAC100_AST2600
+};
+
+static const struct ftgmac100_match_data ftgmac100_match_data_faraday = {
+	.mac_id = FTGMAC100_FARADAY
+};
+
 static const struct of_device_id ftgmac100_of_match[] = {
-	{ .compatible = "aspeed,ast2400-mac" },
-	{ .compatible = "aspeed,ast2500-mac" },
-	{ .compatible = "aspeed,ast2600-mac" },
-	{ .compatible = "faraday,ftgmac100" },
+	{ .compatible = "aspeed,ast2400-mac",
+	  .data = &ftgmac100_match_data_ast2400},
+	{ .compatible = "aspeed,ast2500-mac",
+	  .data = &ftgmac100_match_data_ast2500 },
+	{ .compatible = "aspeed,ast2600-mac",
+	  .data = &ftgmac100_match_data_ast2600 },
+	{ .compatible = "faraday,ftgmac100",
+	  .data = &ftgmac100_match_data_faraday },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ftgmac100_of_match);

-- 
2.34.1

[PATCH net-next v2 03/15] net: ftgmac100: Replace all of_device_is_compatible()

[Jacky Chou]

From: Andrew Lunn <andrew@lunn.ch>

Now that the priv structure includes the MAC ID, make use of it
instead of the more expensive of_device_is_compatible().

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 104eb7b1f5bb..f07167cabf39 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1720,8 +1720,8 @@ static int ftgmac100_setup_mdio(struct net_device *netdev)
 	if (!priv->mii_bus)
 		return -EIO;
 
-	if (of_device_is_compatible(np, "aspeed,ast2400-mac") ||
-	    of_device_is_compatible(np, "aspeed,ast2500-mac")) {
+	if (priv->mac_id == FTGMAC100_AST2400 ||
+	    priv->mac_id == FTGMAC100_AST2500) {
 		/* The AST2600 has a separate MDIO controller */
 
 		/* For the AST2400 and AST2500 this driver only supports the
@@ -1926,9 +1926,9 @@ static int ftgmac100_probe(struct platform_device *pdev)
 	if (err)
 		goto err_phy_connect;
 
-	if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac") ||
-		   of_device_is_compatible(np, "aspeed,ast2500-mac") ||
-		   of_device_is_compatible(np, "aspeed,ast2600-mac"))) {
+	if (priv->mac_id == FTGMAC100_AST2400 ||
+	    priv->mac_id == FTGMAC100_AST2500 ||
+	    priv->mac_id == FTGMAC100_AST2600) {
 		priv->rxdes0_edorr_mask = BIT(30);
 		priv->txdes0_edotr_mask = BIT(30);
 		priv->is_aspeed = true;
@@ -1973,8 +1973,8 @@ static int ftgmac100_probe(struct platform_device *pdev)
 		 * available PHYs and register them.
 		 */
 		if (of_get_property(np, "phy-handle", NULL) &&
-		    (of_device_is_compatible(np, "aspeed,ast2400-mac") ||
-		     of_device_is_compatible(np, "aspeed,ast2500-mac"))) {
+		    (priv->mac_id == FTGMAC100_AST2400 ||
+		     priv->mac_id == FTGMAC100_AST2500)) {
 			err = ftgmac100_setup_mdio(netdev);
 			if (err)
 				goto err_setup_mdio;
@@ -2026,7 +2026,7 @@ static int ftgmac100_probe(struct platform_device *pdev)
 			goto err_phy_connect;
 
 		/* Disable ast2600 problematic HW arbitration */
-		if (of_device_is_compatible(np, "aspeed,ast2600-mac"))
+		if (priv->mac_id == FTGMAC100_AST2600)
 			iowrite32(FTGMAC100_TM_DEFAULT,
 				  priv->base + FTGMAC100_OFFSET_TM);
 	}
@@ -2044,11 +2044,11 @@ static int ftgmac100_probe(struct platform_device *pdev)
 		netdev->hw_features |= NETIF_F_HW_VLAN_CTAG_FILTER;
 
 	/* AST2400  doesn't have working HW checksum generation */
-	if (np && (of_device_is_compatible(np, "aspeed,ast2400-mac")))
+	if (priv->mac_id == FTGMAC100_AST2400)
 		netdev->hw_features &= ~NETIF_F_HW_CSUM;
 
 	/* AST2600 tx checksum with NCSI is broken */
-	if (priv->use_ncsi && of_device_is_compatible(np, "aspeed,ast2600-mac"))
+	if (priv->use_ncsi && priv->mac_id == FTGMAC100_AST2600)
 		netdev->hw_features &= ~NETIF_F_HW_CSUM;
 
 	if (np && of_get_property(np, "no-hw-checksum", NULL))

-- 
2.34.1

[PATCH net-next v2 04/15] net: ftgmac100: Use devm_alloc_etherdev()

[Jacky Chou]

From: Andrew Lunn <andrew@lunn.ch>

Make use of devm_alloc_etherdev() to simplify cleanup.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index f07167cabf39..397ada43c851 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1877,11 +1877,9 @@ static int ftgmac100_probe(struct platform_device *pdev)
 		return irq;
 
 	/* setup net_device */
-	netdev = alloc_etherdev(sizeof(*priv));
-	if (!netdev) {
-		err = -ENOMEM;
-		goto err_alloc_etherdev;
-	}
+	netdev = devm_alloc_etherdev(&pdev->dev, sizeof(*priv));
+	if (!netdev)
+		return -ENOMEM;
 
 	SET_NETDEV_DEV(netdev, &pdev->dev);
 
@@ -2080,8 +2078,6 @@ static int ftgmac100_probe(struct platform_device *pdev)
 err_ioremap:
 	release_resource(priv->res);
 err_req_mem:
-	free_netdev(netdev);
-err_alloc_etherdev:
 	return err;
 }
 
@@ -2112,7 +2108,6 @@ static void ftgmac100_remove(struct platform_device *pdev)
 	release_resource(priv->res);
 
 	netif_napi_del(&priv->napi);
-	free_netdev(netdev);
 }
 
 static const struct ftgmac100_match_data ftgmac100_match_data_ast2400 = {

-- 
2.34.1

[PATCH net-next v2 05/15] net: ftgmac100: Use devm_request_memory_region/devm_ioremap

[Jacky Chou]

From: Andrew Lunn <andrew@lunn.ch>

Make use of devm_ methods to request and remap the device memory to
simplify cleanup.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 397ada43c851..ec2e7ec23ddf 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1897,19 +1897,18 @@ static int ftgmac100_probe(struct platform_device *pdev)
 	INIT_WORK(&priv->reset_task, ftgmac100_reset_task);
 
 	/* map io memory */
-	priv->res = request_mem_region(res->start, resource_size(res),
-				       dev_name(&pdev->dev));
+	priv->res = devm_request_mem_region(&pdev->dev,
+					    res->start, resource_size(res),
+					    dev_name(&pdev->dev));
 	if (!priv->res) {
 		dev_err(&pdev->dev, "Could not reserve memory region\n");
-		err = -ENOMEM;
-		goto err_req_mem;
+		return -ENOMEM;
 	}
 
-	priv->base = ioremap(res->start, resource_size(res));
+	priv->base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
 	if (!priv->base) {
 		dev_err(&pdev->dev, "Failed to ioremap ethernet registers\n");
-		err = -EIO;
-		goto err_ioremap;
+		return -EIO;
 	}
 
 	netdev->irq = irq;
@@ -2074,10 +2073,6 @@ static int ftgmac100_probe(struct platform_device *pdev)
 		ncsi_unregister_dev(priv->ndev);
 	ftgmac100_destroy_mdio(netdev);
 err_setup_mdio:
-	iounmap(priv->base);
-err_ioremap:
-	release_resource(priv->res);
-err_req_mem:
 	return err;
 }
 
@@ -2104,9 +2099,6 @@ static void ftgmac100_remove(struct platform_device *pdev)
 	ftgmac100_phy_disconnect(netdev);
 	ftgmac100_destroy_mdio(netdev);
 
-	iounmap(priv->base);
-	release_resource(priv->res);
-
 	netif_napi_del(&priv->napi);
 }
 

-- 
2.34.1

[PATCH net-next v2 06/15] net: ftgmac100: Use devm_clk_get_enabled

[Jacky Chou]

From: Andrew Lunn <andrew@lunn.ch>

Make use of devm_ methods to request and enable clocks to simplify
cleanup.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index ec2e7ec23ddf..ffd86655bcc8 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1801,13 +1801,10 @@ static int ftgmac100_setup_clk(struct ftgmac100 *priv)
 	struct clk *clk;
 	int rc;
 
-	clk = devm_clk_get(priv->dev, NULL /* MACCLK */);
+	clk = devm_clk_get_enabled(priv->dev, NULL /* MACCLK */);
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 	priv->clk = clk;
-	rc = clk_prepare_enable(priv->clk);
-	if (rc)
-		return rc;
 
 	/* Aspeed specifies a 100MHz clock is required for up to
 	 * 1000Mbit link speeds. As NCSI is limited to 100Mbit, 25MHz
@@ -1816,21 +1813,15 @@ static int ftgmac100_setup_clk(struct ftgmac100 *priv)
 	rc = clk_set_rate(priv->clk, priv->use_ncsi ? FTGMAC_25MHZ :
 			  FTGMAC_100MHZ);
 	if (rc)
-		goto cleanup_clk;
+		return rc;
 
 	/* RCLK is for RMII, typically used for NCSI. Optional because it's not
 	 * necessary if it's the AST2400 MAC, or the MAC is configured for
 	 * RGMII, or the controller is not an ASPEED-based controller.
 	 */
-	priv->rclk = devm_clk_get_optional(priv->dev, "RCLK");
-	rc = clk_prepare_enable(priv->rclk);
-	if (!rc)
-		return 0;
+	priv->rclk = devm_clk_get_optional_enabled(priv->dev, "RCLK");
 
-cleanup_clk:
-	clk_disable_unprepare(priv->clk);
-
-	return rc;
+	return 0;
 }
 
 static bool ftgmac100_has_child_node(struct device_node *np, const char *name)
@@ -2064,8 +2055,6 @@ static int ftgmac100_probe(struct platform_device *pdev)
 	return 0;
 
 err_register_netdev:
-	clk_disable_unprepare(priv->rclk);
-	clk_disable_unprepare(priv->clk);
 err_phy_connect:
 	ftgmac100_phy_disconnect(netdev);
 err_ncsi_dev:
@@ -2088,9 +2077,6 @@ static void ftgmac100_remove(struct platform_device *pdev)
 		ncsi_unregister_dev(priv->ndev);
 	unregister_netdev(netdev);
 
-	clk_disable_unprepare(priv->rclk);
-	clk_disable_unprepare(priv->clk);
-
 	/* There's a small chance the reset task will have been re-queued,
 	 * during stop, make sure it's gone before we free the structure.
 	 */

-- 
2.34.1

[PATCH net-next v2 07/15] net: ftgmac100: Simplify error handling for ftgmac100_initial_mac

[Jacky Chou]

From: Andrew Lunn <andrew@lunn.ch>

ftgmac100_initial_mac() does not allocate any resources. All resources
by the probe function up until this call point use devm_ methods. So
just return the error code rather than use a goto.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index ffd86655bcc8..3bccf34cc8a4 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1912,7 +1912,7 @@ static int ftgmac100_probe(struct platform_device *pdev)
 	/* MAC address from chip or random one */
 	err = ftgmac100_initial_mac(priv);
 	if (err)
-		goto err_phy_connect;
+		return err;
 
 	if (priv->mac_id == FTGMAC100_AST2400 ||
 	    priv->mac_id == FTGMAC100_AST2500 ||

-- 
2.34.1

[PATCH net-next v2 08/15] net: ftgmac100: Move NCSI probe code into a helper

[Jacky Chou]

From: Andrew Lunn <andrew@lunn.ch>

To help reduce the complexity of the probe function move the NCSI
probe code into a helper. No functional change intended.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 63 ++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 3bccf34cc8a4..f1cb5dc37919 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1837,6 +1837,39 @@ static bool ftgmac100_has_child_node(struct device_node *np, const char *name)
 	return ret;
 }
 
+static int ftgmac100_probe_ncsi(struct net_device *netdev,
+				struct ftgmac100 *priv,
+				struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct phy_device *phydev;
+	int err;
+
+	if (!IS_ENABLED(CONFIG_NET_NCSI)) {
+		dev_err(&pdev->dev, "NCSI stack not enabled\n");
+		return -EINVAL;
+	}
+
+	dev_info(&pdev->dev, "Using NCSI interface\n");
+	priv->use_ncsi = true;
+	priv->ndev = ncsi_register_dev(netdev, ftgmac100_ncsi_handler);
+	if (!priv->ndev)
+		return -EINVAL;
+
+	phydev = fixed_phy_register(&ncsi_phy_status, np);
+	if (IS_ERR(phydev)) {
+		dev_err(&pdev->dev, "failed to register fixed PHY device\n");
+		return PTR_ERR(phydev);
+	}
+	err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link,
+				 PHY_INTERFACE_MODE_RMII);
+	if (err) {
+		dev_err(&pdev->dev, "Connecting PHY failed\n");
+		fixed_phy_unregister(phydev);
+	}
+	return err;
+}
+
 static int ftgmac100_probe(struct platform_device *pdev)
 {
 	const struct ftgmac100_match_data *match_data;
@@ -1844,7 +1877,6 @@ static int ftgmac100_probe(struct platform_device *pdev)
 	struct resource *res;
 	int irq;
 	struct net_device *netdev;
-	struct phy_device *phydev;
 	struct ftgmac100 *priv;
 	struct device_node *np;
 	int err = 0;
@@ -1926,32 +1958,9 @@ static int ftgmac100_probe(struct platform_device *pdev)
 	}
 
 	if (np && of_get_property(np, "use-ncsi", NULL)) {
-		if (!IS_ENABLED(CONFIG_NET_NCSI)) {
-			dev_err(&pdev->dev, "NCSI stack not enabled\n");
-			err = -EINVAL;
-			goto err_phy_connect;
-		}
-
-		dev_info(&pdev->dev, "Using NCSI interface\n");
-		priv->use_ncsi = true;
-		priv->ndev = ncsi_register_dev(netdev, ftgmac100_ncsi_handler);
-		if (!priv->ndev) {
-			err = -EINVAL;
-			goto err_phy_connect;
-		}
-
-		phydev = fixed_phy_register(&ncsi_phy_status, np);
-		if (IS_ERR(phydev)) {
-			dev_err(&pdev->dev, "failed to register fixed PHY device\n");
-			err = PTR_ERR(phydev);
-			goto err_phy_connect;
-		}
-		err = phy_connect_direct(netdev, phydev, ftgmac100_adjust_link,
-					 PHY_INTERFACE_MODE_RMII);
-		if (err) {
-			dev_err(&pdev->dev, "Connecting PHY failed\n");
-			goto err_phy_connect;
-		}
+		err = ftgmac100_probe_ncsi(netdev, priv, pdev);
+		if (err)
+			goto err_setup_mdio;
 	} else if (np && (of_phy_is_fixed_link(np) ||
 			  of_get_property(np, "phy-handle", NULL))) {
 		struct phy_device *phy;

-- 
2.34.1

[PATCH net-next v2 09/15] net: ftgmac100: Always register the MDIO bus when it exists

[Jacky Chou]

From: Andrew Lunn <andrew@lunn.ch>

Both the Aspeed 2400 and 2500 and the original faraday version of the
MAC have MDIO bus controllers as part of the MAC. Since it exists,
always registering it makes the code simpler, and causes no harm. If
there is no mdio node in device tree, of_mdiobus_register() will fall
back to mdiobus_register(), making it safe.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index f1cb5dc37919..931fdf3d07d1 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1957,6 +1957,14 @@ static int ftgmac100_probe(struct platform_device *pdev)
 		priv->txdes0_edotr_mask = BIT(15);
 	}
 
+	if (priv->mac_id == FTGMAC100_FARADAY ||
+	    priv->mac_id == FTGMAC100_AST2400 ||
+	    priv->mac_id == FTGMAC100_AST2500) {
+		err = ftgmac100_setup_mdio(netdev);
+		if (err)
+			goto err_phy_connect;
+	}
+
 	if (np && of_get_property(np, "use-ncsi", NULL)) {
 		err = ftgmac100_probe_ncsi(netdev, priv, pdev);
 		if (err)
@@ -1965,18 +1973,6 @@ static int ftgmac100_probe(struct platform_device *pdev)
 			  of_get_property(np, "phy-handle", NULL))) {
 		struct phy_device *phy;
 
-		/* Support "mdio"/"phy" child nodes for ast2400/2500 with
-		 * an embedded MDIO controller. Automatically scan the DTS for
-		 * available PHYs and register them.
-		 */
-		if (of_get_property(np, "phy-handle", NULL) &&
-		    (priv->mac_id == FTGMAC100_AST2400 ||
-		     priv->mac_id == FTGMAC100_AST2500)) {
-			err = ftgmac100_setup_mdio(netdev);
-			if (err)
-				goto err_setup_mdio;
-		}
-
 		phy = of_phy_get_and_connect(priv->netdev, np,
 					     &ftgmac100_adjust_link);
 		if (!phy) {
@@ -1999,9 +1995,6 @@ static int ftgmac100_probe(struct platform_device *pdev)
 		 * PHYs.
 		 */
 		priv->use_ncsi = false;
-		err = ftgmac100_setup_mdio(netdev);
-		if (err)
-			goto err_setup_mdio;
 
 		err = ftgmac100_mii_probe(netdev);
 		if (err) {

-- 
2.34.1

[PATCH net-next v2 10/15] net: ftgmac100: Simplify legacy MDIO setup

[Jacky Chou]

From: Andrew Lunn <andrew@lunn.ch>

There are old device trees which place the PHY nodes directly in the
MAC nodes, rather than within an MDIO container node.

The probe logic indicates that the use of NCSI and the legacy
placement of PHYs is mutually exclusive. Hence priv->use_ncsi cannot
be true, so there is no reason to set it false.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 931fdf3d07d1..f5e69abb1fcf 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1994,8 +1994,6 @@ static int ftgmac100_probe(struct platform_device *pdev)
 		 * child node. Automatically scan the MDIO bus for available
 		 * PHYs.
 		 */
-		priv->use_ncsi = false;
-
 		err = ftgmac100_mii_probe(netdev);
 		if (err) {
 			dev_err(priv->dev, "MII probe failed!\n");

-- 
2.34.1

[PATCH net-next v2 11/15] net: ftgmac100: Move DT probe into a helper

[Jacky Chou]

From: Andrew Lunn <andrew@lunn.ch>

By moving all the DT probe code into a helper, the complex if else if
else structure can be simplified. No functional change intended.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 89 +++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index f5e69abb1fcf..71ea7062446e 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1870,6 +1870,58 @@ static int ftgmac100_probe_ncsi(struct net_device *netdev,
 	return err;
 }
 
+static int ftgmac100_probe_dt(struct net_device *netdev,
+			      struct platform_device *pdev,
+			      struct ftgmac100 *priv,
+			      struct device_node *np)
+{
+	struct phy_device *phy;
+	int err;
+
+	if (of_get_property(np, "use-ncsi", NULL))
+		return ftgmac100_probe_ncsi(netdev, priv, pdev);
+
+	if (of_phy_is_fixed_link(np) ||
+	    of_get_property(np, "phy-handle", NULL)) {
+		/* Support "mdio"/"phy" child nodes for ast2400/2500
+		 * with an embedded MDIO controller. Automatically
+		 * scan the DTS for available PHYs and register
+		 * them. 2600 has an independent MDIO controller, not
+		 * part of the MAC.
+		 */
+		phy = of_phy_get_and_connect(priv->netdev, np,
+					     &ftgmac100_adjust_link);
+		if (!phy) {
+			dev_err(&pdev->dev, "Failed to connect to phy\n");
+			return -EINVAL;
+		}
+
+		/* Indicate that we support PAUSE frames (see comment in
+		 * Documentation/networking/phy.rst)
+		 */
+		phy_support_asym_pause(phy);
+
+		/* Display what we found */
+		phy_attached_info(phy);
+		return 0;
+	}
+
+	if (!ftgmac100_has_child_node(np, "mdio")) {
+		/* Support legacy ASPEED devicetree descriptions that
+		 * decribe a MAC with an embedded MDIO controller but
+		 * have no "mdio" child node. Automatically scan the
+		 * MDIO bus for available PHYs.
+		 */
+		err = ftgmac100_mii_probe(netdev);
+		if (err) {
+			dev_err(priv->dev, "MII probe failed!\n");
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 static int ftgmac100_probe(struct platform_device *pdev)
 {
 	const struct ftgmac100_match_data *match_data;
@@ -1965,41 +2017,10 @@ static int ftgmac100_probe(struct platform_device *pdev)
 			goto err_phy_connect;
 	}
 
-	if (np && of_get_property(np, "use-ncsi", NULL)) {
-		err = ftgmac100_probe_ncsi(netdev, priv, pdev);
+	if (np) {
+		err = ftgmac100_probe_dt(netdev, pdev, priv, np);
 		if (err)
-			goto err_setup_mdio;
-	} else if (np && (of_phy_is_fixed_link(np) ||
-			  of_get_property(np, "phy-handle", NULL))) {
-		struct phy_device *phy;
-
-		phy = of_phy_get_and_connect(priv->netdev, np,
-					     &ftgmac100_adjust_link);
-		if (!phy) {
-			dev_err(&pdev->dev, "Failed to connect to phy\n");
-			err = -EINVAL;
 			goto err_phy_connect;
-		}
-
-		/* Indicate that we support PAUSE frames (see comment in
-		 * Documentation/networking/phy.rst)
-		 */
-		phy_support_asym_pause(phy);
-
-		/* Display what we found */
-		phy_attached_info(phy);
-	} else if (np && !ftgmac100_has_child_node(np, "mdio")) {
-		/* Support legacy ASPEED devicetree descriptions that decribe a
-		 * MAC with an embedded MDIO controller but have no "mdio"
-		 * child node. Automatically scan the MDIO bus for available
-		 * PHYs.
-		 */
-		err = ftgmac100_mii_probe(netdev);
-		if (err) {
-			dev_err(priv->dev, "MII probe failed!\n");
-			goto err_ncsi_dev;
-		}
-
 	}
 
 	priv->rst = devm_reset_control_get_optional_exclusive(priv->dev, NULL);
@@ -2057,11 +2078,9 @@ static int ftgmac100_probe(struct platform_device *pdev)
 err_register_netdev:
 err_phy_connect:
 	ftgmac100_phy_disconnect(netdev);
-err_ncsi_dev:
 	if (priv->ndev)
 		ncsi_unregister_dev(priv->ndev);
 	ftgmac100_destroy_mdio(netdev);
-err_setup_mdio:
 	return err;
 }
 

-- 
2.34.1

[PATCH net-next v2 12/15] net: ftgmac100: Remove redundant PHY_POLL

[Jacky Chou]

From: Andrew Lunn <andrew@lunn.ch>

When an MDIO bus is allocated, the irqs for each PHY are set to
polling. Remove the redundant code in the MAC driver which does the
same.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 71ea7062446e..1440a4b358e3 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1712,7 +1712,7 @@ static int ftgmac100_setup_mdio(struct net_device *netdev)
 	struct platform_device *pdev = to_platform_device(priv->dev);
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *mdio_np;
-	int i, err = 0;
+	int err = 0;
 	u32 reg;
 
 	/* initialize mdio bus */
@@ -1740,9 +1740,6 @@ static int ftgmac100_setup_mdio(struct net_device *netdev)
 	priv->mii_bus->read = ftgmac100_mdiobus_read;
 	priv->mii_bus->write = ftgmac100_mdiobus_write;
 
-	for (i = 0; i < PHY_MAX_ADDR; i++)
-		priv->mii_bus->irq[i] = PHY_POLL;
-
 	mdio_np = of_get_child_by_name(np, "mdio");
 
 	err = of_mdiobus_register(priv->mii_bus, mdio_np);

-- 
2.34.1

[PATCH net-next v2 13/15] net: ftgmac100: Simplify error handling for ftgmac100_setup_mdio

[Jacky Chou]

From: Andrew Lunn <andrew@lunn.ch>

ftgmac100_setup_mdio() cleans up any resources it gets on error. All
resources obtained by the probe function up until this call point use
devm_ methods. So just return the error code rather than use a goto.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 1440a4b358e3..93c1ef819abc 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -2011,7 +2011,7 @@ static int ftgmac100_probe(struct platform_device *pdev)
 	    priv->mac_id == FTGMAC100_AST2500) {
 		err = ftgmac100_setup_mdio(netdev);
 		if (err)
-			goto err_phy_connect;
+			return err;
 	}
 
 	if (np) {

-- 
2.34.1

[PATCH net-next v2 14/15] net: ftgmac100: Simplify condition on HW arbitration

[Jacky Chou]

From: Andrew Lunn <andrew@lunn.ch>

The MAC ID is sufficient to indicate this is a ast2600.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 93c1ef819abc..d2ebb7360f4a 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -2030,13 +2030,13 @@ static int ftgmac100_probe(struct platform_device *pdev)
 		err = ftgmac100_setup_clk(priv);
 		if (err)
 			goto err_phy_connect;
-
-		/* Disable ast2600 problematic HW arbitration */
-		if (priv->mac_id == FTGMAC100_AST2600)
-			iowrite32(FTGMAC100_TM_DEFAULT,
-				  priv->base + FTGMAC100_OFFSET_TM);
 	}
 
+	/* Disable ast2600 problematic HW arbitration */
+	if (priv->mac_id == FTGMAC100_AST2600)
+		iowrite32(FTGMAC100_TM_DEFAULT,
+			  priv->base + FTGMAC100_OFFSET_TM);
+
 	/* Default ring sizes */
 	priv->rx_q_entries = priv->new_rx_q_entries = DEF_RX_QUEUE_ENTRIES;
 	priv->tx_q_entries = priv->new_tx_q_entries = DEF_TX_QUEUE_ENTRIES;

-- 
2.34.1

[PATCH net-next v2 15/15] net: ftgmac100: Fix wrong netif_napi_del in release

[Jacky Chou]

From: Andrew Lunn <andrew@lunn.ch>

netif_napi_add() is called in open. There is a symmetric call to
netif_napi_del() in stop. Remove to wrong call to netif_napi_del() in
release.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index d2ebb7360f4a..fa33eeadb630 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -2100,8 +2100,6 @@ static void ftgmac100_remove(struct platform_device *pdev)
 
 	ftgmac100_phy_disconnect(netdev);
 	ftgmac100_destroy_mdio(netdev);
-
-	netif_napi_del(&priv->napi);
 }
 
 static const struct ftgmac100_match_data ftgmac100_match_data_ast2400 = {

-- 
2.34.1

Re: [net-next,v2,09/15] net: ftgmac100: Always register the MDIO bus when it exists

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/ai-review.html
---
net: ftgmac100: Always register the MDIO bus when it exists

This patch moves MDIO bus registration earlier in the probe function
for FARADAY, AST2400, and AST2500 MACs, simplifying the code by
registering it unconditionally when the hardware has an MDIO controller.

Note: These issues appear to be fixed later in the series by commits
8961daa5f2c0 ("Simplify error handling for ftgmac100_setup_mdio") and
046e51399f0f ("Move DT probe into a helper").

> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index f1cb5dc37919..931fdf3d07d1 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -1957,6 +1957,14 @@ static int ftgmac100_probe(struct platform_device *pdev)
>  		priv->txdes0_edotr_mask = BIT(15);
>  	}
>
> +	if (priv->mac_id == FTGMAC100_FARADAY ||
> +	    priv->mac_id == FTGMAC100_AST2400 ||
> +	    priv->mac_id == FTGMAC100_AST2500) {
> +		err = ftgmac100_setup_mdio(netdev);
> +		if (err)
> +			goto err_phy_connect;
> +	}

If ftgmac100_setup_mdio() fails after of_mdiobus_register() returns an
error, it calls mdiobus_free(priv->mii_bus) but does not set priv->mii_bus
to NULL. The goto err_phy_connect then leads to ftgmac100_destroy_mdio()
which checks if (!priv->mii_bus) but finds a dangling pointer, resulting
in mdiobus_unregister() on freed memory followed by a double free.

Could this cause a use-after-free and double-free when MDIO registration
fails? The error path in ftgmac100_setup_mdio() frees mii_bus without
clearing the pointer, and err_phy_connect eventually calls
ftgmac100_destroy_mdio() which operates on the stale pointer.

> +
> +	if (np && of_get_property(np, "use-ncsi", NULL)) {
> +		err = ftgmac100_probe_ncsi(netdev, priv, pdev);
> +		if (err)
> +			goto err_setup_mdio;

Since MDIO is now set up before the NCSI probe for FARADAY/AST2400/AST2500,
if ftgmac100_probe_ncsi() fails, the goto err_setup_mdio skips the MDIO
cleanup at ftgmac100_destroy_mdio(). Does this leak the mii_bus allocation
when NCSI probe fails on these MAC variants?

[ ... ]
-- 
pw-bot: changes-requested

This is an AI-generated review. Report issues at https://example.com

Re: [net-next,v2,06/15] net: ftgmac100: Use devm_clk_get_enabled

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: ftgmac100: Use devm_clk_get_enabled

This patch converts the ftgmac100 driver to use devm_clk_get_enabled()
and devm_clk_get_optional_enabled() to simplify clock management and
cleanup paths.

> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index ec2e7ec23ddf..ffd86655bcc8 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -1801,13 +1801,10 @@ static int ftgmac100_setup_clk(struct ftgmac100 *priv)
>  	struct clk *clk;
>  	int rc;
>
> -	clk = devm_clk_get(priv->dev, NULL /* MACCLK */);
> +	clk = devm_clk_get_enabled(priv->dev, NULL /* MACCLK */);
>  	if (IS_ERR(clk))
>  		return PTR_ERR(clk);
>  	priv->clk = clk;
> -	rc = clk_prepare_enable(priv->clk);
> -	if (rc)
> -		return rc;
>
>  	/* Aspeed specifies a 100MHz clock is required for up to
>  	 * 1000Mbit link speeds. As NCSI is limited to 100Mbit, 25MHz
> @@ -1816,21 +1813,15 @@ static int ftgmac100_setup_clk(struct ftgmac100 *priv)
>  	rc = clk_set_rate(priv->clk, priv->use_ncsi ? FTGMAC_25MHZ :
>  			  FTGMAC_100MHZ);
>  	if (rc)
> -		goto cleanup_clk;
> +		return rc;
>
>  	/* RCLK is for RMII, typically used for NCSI. Optional because it's not
>  	 * necessary if it's the AST2400 MAC, or the MAC is configured for
>  	 * RGMII, or the controller is not an ASPEED-based controller.
>  	 */
> -	priv->rclk = devm_clk_get_optional(priv->dev, "RCLK");
> -	rc = clk_prepare_enable(priv->rclk);
> -	if (!rc)
> -		return 0;
> +	priv->rclk = devm_clk_get_optional_enabled(priv->dev, "RCLK");
>
> -cleanup_clk:
> -	clk_disable_unprepare(priv->clk);
> -
> -	return rc;
> +	return 0;
>  }

Should the return value of devm_clk_get_optional_enabled() be checked
here? The old code would fail probe if RCLK existed but
clk_prepare_enable() failed. The new code unconditionally returns 0,
ignoring any potential error from devm_clk_get_optional_enabled().

Looking at other drivers using this API, most check for IS_ERR(). For
example, cctrng_probe() in drivers/char/hw_random/cctrng.c does:

    drvdata->clk = devm_clk_get_optional_enabled(dev, NULL);
    if (IS_ERR(drvdata->clk))
        return dev_err_probe(...);

While the clock is optional (may not exist), if it does exist but fails
to enable, this seems like a condition that should cause probe to fail
rather than succeed silently?

[ ... ]
-- 
pw-bot: changes-requested

Re: [net-next,v2,09/15] net: ftgmac100: Always register the MDIO bus when it exists

[Simon Horman]

On Tue, Jan 20, 2026 at 12:14:07PM +0000, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/ai-review.html
> ---
> net: ftgmac100: Always register the MDIO bus when it exists
> 
> This patch moves MDIO bus registration earlier in the probe function
> for FARADAY, AST2400, and AST2500 MACs, simplifying the code by
> registering it unconditionally when the hardware has an MDIO controller.
> 
> Note: These issues appear to be fixed later in the series by commits
> 8961daa5f2c0 ("Simplify error handling for ftgmac100_setup_mdio") and
> 046e51399f0f ("Move DT probe into a helper").
> 
> > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> > index f1cb5dc37919..931fdf3d07d1 100644
> > --- a/drivers/net/ethernet/faraday/ftgmac100.c
> > +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> > @@ -1957,6 +1957,14 @@ static int ftgmac100_probe(struct platform_device *pdev)
> >  		priv->txdes0_edotr_mask = BIT(15);
> >  	}
> >
> > +	if (priv->mac_id == FTGMAC100_FARADAY ||
> > +	    priv->mac_id == FTGMAC100_AST2400 ||
> > +	    priv->mac_id == FTGMAC100_AST2500) {
> > +		err = ftgmac100_setup_mdio(netdev);
> > +		if (err)
> > +			goto err_phy_connect;
> > +	}
> 
> If ftgmac100_setup_mdio() fails after of_mdiobus_register() returns an
> error, it calls mdiobus_free(priv->mii_bus) but does not set priv->mii_bus
> to NULL. The goto err_phy_connect then leads to ftgmac100_destroy_mdio()
> which checks if (!priv->mii_bus) but finds a dangling pointer, resulting
> in mdiobus_unregister() on freed memory followed by a double free.
> 
> Could this cause a use-after-free and double-free when MDIO registration
> fails? The error path in ftgmac100_setup_mdio() frees mii_bus without
> clearing the pointer, and err_phy_connect eventually calls
> ftgmac100_destroy_mdio() which operates on the stale pointer.
> 
> > +
> > +	if (np && of_get_property(np, "use-ncsi", NULL)) {
> > +		err = ftgmac100_probe_ncsi(netdev, priv, pdev);
> > +		if (err)
> > +			goto err_setup_mdio;
> 
> Since MDIO is now set up before the NCSI probe for FARADAY/AST2400/AST2500,
> if ftgmac100_probe_ncsi() fails, the goto err_setup_mdio skips the MDIO
> cleanup at ftgmac100_destroy_mdio(). Does this leak the mii_bus allocation
> when NCSI probe fails on these MAC variants?
> 
> [ ... ]
> -- 
> pw-bot: changes-requested
> 
> This is an AI-generated review. Report issues at https://example.com

Sorry for the bogus last line here. A local configuration error on my side.
But the rest of what is above is good, IMHO.

Re: [net-next,v2,09/15] net: ftgmac100: Always register the MDIO bus when it exists

[Andrew Lunn]

On Tue, Jan 20, 2026 at 12:14:07PM +0000, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/ai-review.html
> ---
> net: ftgmac100: Always register the MDIO bus when it exists
> 
> This patch moves MDIO bus registration earlier in the probe function
> for FARADAY, AST2400, and AST2500 MACs, simplifying the code by
> registering it unconditionally when the hardware has an MDIO controller.
> 
> Note: These issues appear to be fixed later in the series by commits
> 8961daa5f2c0 ("Simplify error handling for ftgmac100_setup_mdio") and
> 046e51399f0f ("Move DT probe into a helper").
> 
> > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> > index f1cb5dc37919..931fdf3d07d1 100644
> > --- a/drivers/net/ethernet/faraday/ftgmac100.c
> > +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> > @@ -1957,6 +1957,14 @@ static int ftgmac100_probe(struct platform_device *pdev)
> >  		priv->txdes0_edotr_mask = BIT(15);
> >  	}
> >
> > +	if (priv->mac_id == FTGMAC100_FARADAY ||
> > +	    priv->mac_id == FTGMAC100_AST2400 ||
> > +	    priv->mac_id == FTGMAC100_AST2500) {
> > +		err = ftgmac100_setup_mdio(netdev);
> > +		if (err)
> > +			goto err_phy_connect;
> > +	}
> 
> If ftgmac100_setup_mdio() fails after of_mdiobus_register() returns an
> error, it calls mdiobus_free(priv->mii_bus) but does not set priv->mii_bus
> to NULL. The goto err_phy_connect then leads to ftgmac100_destroy_mdio()
> which checks if (!priv->mii_bus) but finds a dangling pointer, resulting
> in mdiobus_unregister() on freed memory followed by a double free.

The call to register the MDIO bus has been moved earlier. That means
the call to ftgmac100_destroy_mdio() the bus also needs to move
earlier on the error cleanup path, before err_phy_connect:

	Andrew

[net-next,v2,06/15] net: ftgmac100: Use devm_clk_get_enabled

[Jacky Chou]

> 寄件者: Simon Horman <horms@kernel.org>
> 寄件日期: Tuesday, January 20, 2026 8:20 PM
> 收件者: Jacky Chou <jacky_chou@aspeedtech.com>
> 副本: Simon Horman <horms@kernel.org>; davem@davemloft.net;
> kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
> andrew+netdev@lunn.ch; netdev@vger.kernel.org
> 主旨: Re: [net-next,v2,06/15] net: ftgmac100: Use devm_clk_get_enabled
> 
> This is an AI-generated review of your patch. The human sending this email has
> considered the AI review valid, or at least plausible.
> 
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> net: ftgmac100: Use devm_clk_get_enabled
> 
> This patch converts the ftgmac100 driver to use devm_clk_get_enabled() and
> devm_clk_get_optional_enabled() to simplify clock management and cleanup
> paths.
> 
> > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
> > b/drivers/net/ethernet/faraday/ftgmac100.c
> > index ec2e7ec23ddf..ffd86655bcc8 100644
> > --- a/drivers/net/ethernet/faraday/ftgmac100.c
> > +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> > @@ -1801,13 +1801,10 @@ static int ftgmac100_setup_clk(struct
> ftgmac100 *priv)
> >  	struct clk *clk;
> >  	int rc;
> >
> > -	clk = devm_clk_get(priv->dev, NULL /* MACCLK */);
> > +	clk = devm_clk_get_enabled(priv->dev, NULL /* MACCLK */);
> >  	if (IS_ERR(clk))
> >  		return PTR_ERR(clk);
> >  	priv->clk = clk;
> > -	rc = clk_prepare_enable(priv->clk);
> > -	if (rc)
> > -		return rc;
> >
> >  	/* Aspeed specifies a 100MHz clock is required for up to
> >  	 * 1000Mbit link speeds. As NCSI is limited to 100Mbit, 25MHz @@
> > -1816,21 +1813,15 @@ static int ftgmac100_setup_clk(struct ftgmac100
> *priv)
> >  	rc = clk_set_rate(priv->clk, priv->use_ncsi ? FTGMAC_25MHZ :
> >  			  FTGMAC_100MHZ);
> >  	if (rc)
> > -		goto cleanup_clk;
> > +		return rc;
> >
> >  	/* RCLK is for RMII, typically used for NCSI. Optional because it's not
> >  	 * necessary if it's the AST2400 MAC, or the MAC is configured for
> >  	 * RGMII, or the controller is not an ASPEED-based controller.
> >  	 */
> > -	priv->rclk = devm_clk_get_optional(priv->dev, "RCLK");
> > -	rc = clk_prepare_enable(priv->rclk);
> > -	if (!rc)
> > -		return 0;
> > +	priv->rclk = devm_clk_get_optional_enabled(priv->dev, "RCLK");
> >
> > -cleanup_clk:
> > -	clk_disable_unprepare(priv->clk);
> > -
> > -	return rc;
> > +	return 0;
> >  }
> 
> Should the return value of devm_clk_get_optional_enabled() be checked here?
> The old code would fail probe if RCLK existed but
> clk_prepare_enable() failed. The new code unconditionally returns 0, ignoring
> any potential error from devm_clk_get_optional_enabled().
> 
> Looking at other drivers using this API, most check for IS_ERR(). For example,
> cctrng_probe() in drivers/char/hw_random/cctrng.c does:
> 
>     drvdata->clk = devm_clk_get_optional_enabled(dev, NULL);
>     if (IS_ERR(drvdata->clk))
>         return dev_err_probe(...);
> 
> While the clock is optional (may not exist), if it does exist but fails to enable,
> this seems like a condition that should cause probe to fail rather than succeed
> silently?

Agreed for AI review.
We missed checking the return value from devm_clk_get_optional_enabled().
I will fix this in the next version.

Thanks,
Jacky

[net-next,v2,09/15] net: ftgmac100: Always register the MDIO bus when it exists

[Jacky Chou]

Hi Andrew,


> On Tue, Jan 20, 2026 at 12:14:07PM +0000, Simon Horman wrote:
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > Full review at: https://netdev-ai.bots.linux.dev/ai-review.html
> > ---
> > net: ftgmac100: Always register the MDIO bus when it exists
> >
> > This patch moves MDIO bus registration earlier in the probe function
> > for FARADAY, AST2400, and AST2500 MACs, simplifying the code by
> > registering it unconditionally when the hardware has an MDIO controller.
> >
> > Note: These issues appear to be fixed later in the series by commits
> > 8961daa5f2c0 ("Simplify error handling for ftgmac100_setup_mdio") and
> > 046e51399f0f ("Move DT probe into a helper").
> >
> > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
> > > b/drivers/net/ethernet/faraday/ftgmac100.c
> > > index f1cb5dc37919..931fdf3d07d1 100644
> > > --- a/drivers/net/ethernet/faraday/ftgmac100.c
> > > +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> > > @@ -1957,6 +1957,14 @@ static int ftgmac100_probe(struct
> platform_device *pdev)
> > >  		priv->txdes0_edotr_mask = BIT(15);
> > >  	}
> > >
> > > +	if (priv->mac_id == FTGMAC100_FARADAY ||
> > > +	    priv->mac_id == FTGMAC100_AST2400 ||
> > > +	    priv->mac_id == FTGMAC100_AST2500) {
> > > +		err = ftgmac100_setup_mdio(netdev);
> > > +		if (err)
> > > +			goto err_phy_connect;
> > > +	}
> >
> > If ftgmac100_setup_mdio() fails after of_mdiobus_register() returns an
> > error, it calls mdiobus_free(priv->mii_bus) but does not set
> > priv->mii_bus to NULL. The goto err_phy_connect then leads to
> > ftgmac100_destroy_mdio() which checks if (!priv->mii_bus) but finds a
> > dangling pointer, resulting in mdiobus_unregister() on freed memory
> followed by a double free.
> 
> The call to register the MDIO bus has been moved earlier. That means the call
> to ftgmac100_destroy_mdio() the bus also needs to move earlier on the error
> cleanup path, before err_phy_connect:
> 

I've been thinking about this part. One possible approach is to switch to
devm_mdiobus_alloc() and devm_of_mdiobus_register() instead of
mdiobus_alloc() and of_mdiobus_register().

With devm-managed MDIO resources, the explicit cleanup via
ftgmac100_destroy_mdio() would no longer be necessary, and the error paths
would become simpler and safer.

This should also avoid ordering issues in the error cleanup paths when MDIO
registration is moved earlier in the probe sequence.

Thanks,
Jacky

Re: [net-next,v2,09/15] net: ftgmac100: Always register the MDIO bus when it exists

[Andrew Lunn]

> I've been thinking about this part. One possible approach is to switch to
> devm_mdiobus_alloc() and devm_of_mdiobus_register() instead of
> mdiobus_alloc() and of_mdiobus_register().

If i remember correctly, a later patch does that. Which is why this AI
bot points out the problem goes away later in the series.

I was trying to keep the patches simple, do one thing, be obviously
correct. Moving the code and changing to devm_* at the same time is
not "do one thing".

If it is possible to fix the issue by just moving the call to destroy
earlier in the error path, i would do that. That is the logical fix to
this patch. We then have the setup and the cleanup in symmetry.

   Andrew

Re: [net-next,v2,09/15] net: ftgmac100: Always register the MDIO bus when it exists

[Andrew Lunn]

> > > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c
> > > > b/drivers/net/ethernet/faraday/ftgmac100.c
> > > > index f1cb5dc37919..931fdf3d07d1 100644
> > > > --- a/drivers/net/ethernet/faraday/ftgmac100.c
> > > > +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> > > > @@ -1957,6 +1957,14 @@ static int ftgmac100_probe(struct
> > platform_device *pdev)
> > > >  		priv->txdes0_edotr_mask = BIT(15);
> > > >  	}
> > > >
> > > > +	if (priv->mac_id == FTGMAC100_FARADAY ||
> > > > +	    priv->mac_id == FTGMAC100_AST2400 ||
> > > > +	    priv->mac_id == FTGMAC100_AST2500) {
> > > > +		err = ftgmac100_setup_mdio(netdev);
> > > > +		if (err)
> > > > +			goto err_phy_connect;
> > > > +	}
> > >
> > > If ftgmac100_setup_mdio() fails after of_mdiobus_register() returns an
> > > error, it calls mdiobus_free(priv->mii_bus) but does not set
> > > priv->mii_bus to NULL. The goto err_phy_connect then leads to
> > > ftgmac100_destroy_mdio() which checks if (!priv->mii_bus) but finds a
> > > dangling pointer, resulting in mdiobus_unregister() on freed memory
> > followed by a double free.

I took another look at this. The goto err_phy_connect is wrong.  At
this point, if there is an error nothing else needs cleaning up, so
the goto should be replaced with a return err;

It is not however the full solution. If ftgmac100_probe_ncsi() fails,
it jumps to err_setup_mdio, which currently just does a return,
leaking the mdiobus. The ftgmac100_destroy_mdio() call needs moving
after the err_setup_mdio: label.

This is one of the problems with this driver. The labels are not
consistent. Some label names indicate what failed, others indicate
what needs cleaning up. The later patches solve this by using devm_ so
removing all these labels.

	 Andrew

[net-next,v2,09/15] net: ftgmac100: Always register the MDIO bus when it exists

[Jacky Chou]

> > > > > +	if (priv->mac_id == FTGMAC100_FARADAY ||
> > > > > +	    priv->mac_id == FTGMAC100_AST2400 ||
> > > > > +	    priv->mac_id == FTGMAC100_AST2500) {
> > > > > +		err = ftgmac100_setup_mdio(netdev);
> > > > > +		if (err)
> > > > > +			goto err_phy_connect;
> > > > > +	}
> > > >
> > > > If ftgmac100_setup_mdio() fails after of_mdiobus_register()
> > > > returns an error, it calls mdiobus_free(priv->mii_bus) but does
> > > > not set
> > > > priv->mii_bus to NULL. The goto err_phy_connect then leads to
> > > > ftgmac100_destroy_mdio() which checks if (!priv->mii_bus) but
> > > > finds a dangling pointer, resulting in mdiobus_unregister() on
> > > > freed memory
> > > followed by a double free.
> 
> I took another look at this. The goto err_phy_connect is wrong.  At this point,
> if there is an error nothing else needs cleaning up, so the goto should be
> replaced with a return err;
> 

Agreed.
When ftgmac100_setup_mdio() fails, the MDIO bus is not registered and the
probe function returns an error immediately.

> It is not however the full solution. If ftgmac100_probe_ncsi() fails, it jumps to
> err_setup_mdio, which currently just does a return, leaking the mdiobus. The
> ftgmac100_destroy_mdio() call needs moving after the err_setup_mdio: label.
> 

In NC-SI mode, MDIO is not required.
Therefore, always registering the MDIO bus is redundant when the interface
operates in NC-SI mode.

Perhaps the adjustments in this patch could be moved to follow
"net: ftgmac100: Move DT probe into a helper".
And add to this location in ftgmac100_probe_dt() like below.

static int ftgmac100_probe_dt(struct net_device *netdev,
			      struct platform_device *pdev,
			      struct ftgmac100 *priv,
			      struct device_node *np)
{
	struct phy_device *phy;
	int err;

	if (of_get_property(np, "use-ncsi", NULL))
		return ftgmac100_probe_ncsi(netdev, priv, pdev);


======== > Add here
    if (priv->mac_id == FTGMAC100_FARADAY ||
	    priv->mac_id == FTGMAC100_AST2400 ||
	    priv->mac_id == FTGMAC100_AST2500) {
		err = ftgmac100_setup_mdio(netdev);
		if (err)
			return err;
	}
========

	if (of_phy_is_fixed_link(np) ||
	    of_get_property(np, "phy-handle", NULL)) {
		/* Support "mdio"/"phy" child nodes for ast2400/2500
		 * with an embedded MDIO controller. Automatically
		 * scan the DTS for available PHYs and register
		 * them. 2600 has an independent MDIO controller, not
		 * part of the MAC.
		 */
		phy = of_phy_get_and_connect(priv->netdev, np,
					     &ftgmac100_adjust_link);
		if (!phy) {
			dev_err(&pdev->dev, "Failed to connect to phy\n");
			return -EINVAL;
		}

		/* Indicate that we support PAUSE frames (see comment in
		 * Documentation/networking/phy.rst)
		 */
		phy_support_asym_pause(phy);

		/* Display what we found */
		phy_attached_info(phy);
		return 0;
	}
....
}

Thanks,
Jacky

Re: [net-next,v2,09/15] net: ftgmac100: Always register the MDIO bus when it exists

[Andrew Lunn]

On Thu, Jan 29, 2026 at 06:07:20AM +0000, Jacky Chou wrote:
> > > > > > +	if (priv->mac_id == FTGMAC100_FARADAY ||
> > > > > > +	    priv->mac_id == FTGMAC100_AST2400 ||
> > > > > > +	    priv->mac_id == FTGMAC100_AST2500) {
> > > > > > +		err = ftgmac100_setup_mdio(netdev);
> > > > > > +		if (err)
> > > > > > +			goto err_phy_connect;
> > > > > > +	}
> > > > >
> > > > > If ftgmac100_setup_mdio() fails after of_mdiobus_register()
> > > > > returns an error, it calls mdiobus_free(priv->mii_bus) but does
> > > > > not set
> > > > > priv->mii_bus to NULL. The goto err_phy_connect then leads to
> > > > > ftgmac100_destroy_mdio() which checks if (!priv->mii_bus) but
> > > > > finds a dangling pointer, resulting in mdiobus_unregister() on
> > > > > freed memory
> > > > followed by a double free.
> > 
> > I took another look at this. The goto err_phy_connect is wrong.  At this point,
> > if there is an error nothing else needs cleaning up, so the goto should be
> > replaced with a return err;
> > 
> 
> Agreed.
> When ftgmac100_setup_mdio() fails, the MDIO bus is not registered and the
> probe function returns an error immediately.
> 
> > It is not however the full solution. If ftgmac100_probe_ncsi() fails, it jumps to
> > err_setup_mdio, which currently just does a return, leaking the mdiobus. The
> > ftgmac100_destroy_mdio() call needs moving after the err_setup_mdio: label.
> > 
> 
> In NC-SI mode, MDIO is not required.
> Therefore, always registering the MDIO bus is redundant when the interface
> operates in NC-SI mode.

But it physically exists, and in theory, you could make use of it,
hang an Ethernet switch off it, etc. Linux does not care what MDIO bus
a switch, or a PHY uses. It does not need to be the one directly
associated to the MAC. The 2700 has MDIO in its own address space, its
own driver, with its own life cycle, etc.

This series is all about making probe simpler, ready to make it more
complex again to sort out the problems with RGMII delays. Always
registering the bus just helps reduces the complexity.

	Andrew

[net-next,v2,09/15] net: ftgmac100: Always register the MDIO bus when it exists

[Jacky Chou]

> > > > > > > +	    priv->mac_id == FTGMAC100_AST2400 ||
> > > > > > > +	    priv->mac_id == FTGMAC100_AST2500) {
> > > > > > > +		err = ftgmac100_setup_mdio(netdev);
> > > > > > > +		if (err)
> > > > > > > +			goto err_phy_connect;
> > > > > > > +	}
> > > > > >
> > > > > > If ftgmac100_setup_mdio() fails after of_mdiobus_register()
> > > > > > returns an error, it calls mdiobus_free(priv->mii_bus) but
> > > > > > does not set
> > > > > > priv->mii_bus to NULL. The goto err_phy_connect then leads to
> > > > > > ftgmac100_destroy_mdio() which checks if (!priv->mii_bus) but
> > > > > > finds a dangling pointer, resulting in mdiobus_unregister() on
> > > > > > freed memory
> > > > > followed by a double free.
> > >
> > > I took another look at this. The goto err_phy_connect is wrong.  At
> > > this point, if there is an error nothing else needs cleaning up, so
> > > the goto should be replaced with a return err;
> > >
> >
> > Agreed.
> > When ftgmac100_setup_mdio() fails, the MDIO bus is not registered and
> > the probe function returns an error immediately.
> >
> > > It is not however the full solution. If ftgmac100_probe_ncsi()
> > > fails, it jumps to err_setup_mdio, which currently just does a
> > > return, leaking the mdiobus. The
> > > ftgmac100_destroy_mdio() call needs moving after the err_setup_mdio:
> label.
> > >
> >
> > In NC-SI mode, MDIO is not required.
> > Therefore, always registering the MDIO bus is redundant when the
> > interface operates in NC-SI mode.
> 
> But it physically exists, and in theory, you could make use of it, hang an
> Ethernet switch off it, etc. Linux does not care what MDIO bus a switch, or a
> PHY uses. It does not need to be the one directly associated to the MAC. The
> 2700 has MDIO in its own address space, its own driver, with its own life cycle,
> etc.
> 
> This series is all about making probe simpler, ready to make it more complex
> again to sort out the problems with RGMII delays. Always registering the bus
> just helps reduces the complexity.
> 

I agree. Even in the NCSI case, registering the MDIO bus can still be useful for other devices, such as hanging an external switch or PHY off it.
Thank you for the suggestion - always registering the bus does help reduce the overall probe complexity and makes future extensions easier.

Thanks,
Jacky