[PATCH net 0/4] net: axienet: Fix deferred probe loop

[PATCH net 0/4] net: axienet: Fix deferred probe loop

[Sean Anderson]

Upon further investigation, the EPROBE_DEFER loop outlined in [1] can
occur even without the PCS subsystem, as described in patch 4/4. The
second patch is a general fix, and could be applied even without the
auxdev conversion.

[1] https://lore.kernel.org/all/20250610183459.3395328-1-sean.anderson@linux.dev/


Sean Anderson (4):
  auxiliary: Allow empty id
  net: axienet: Fix resource release ordering
  net: axienet: Rearrange lifetime functions
  net: axienet: Split into MAC and MDIO drivers

 drivers/base/auxiliary.c                      |   6 +-
 drivers/net/ethernet/xilinx/Kconfig           |   1 +
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 391 +++++++++---------
 .../net/ethernet/xilinx/xilinx_axienet_mdio.c |  32 +-
 include/linux/auxiliary_bus.h                 |   4 +-
 5 files changed, 234 insertions(+), 200 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty

[PATCH net 1/4] auxiliary: Allow empty id

[Sean Anderson]

Support creating auxiliary devices with the id included as part of the
name. This allows for non-decimal ids, which may be more appropriate for
auxiliary devices created as children of memory-mapped devices. For
example, a name like "xilinx_emac.mac.802c0000" could be achieved by
setting .name to "mac.802c0000" and .id to AUXILIARY_DEVID_NONE.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/base/auxiliary.c      | 6 +++++-
 include/linux/auxiliary_bus.h | 4 +++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
index dba7c8e13a53..64a0d5e2eb83 100644
--- a/drivers/base/auxiliary.c
+++ b/drivers/base/auxiliary.c
@@ -331,7 +331,11 @@ int __auxiliary_device_add(struct auxiliary_device *auxdev, const char *modname)
 		return -EINVAL;
 	}
 
-	ret = dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id);
+	if (auxdev->id == AUXILIARY_DEVID_NONE)
+		ret = dev_set_name(dev, "%s.%s", modname, auxdev->name);
+	else
+		ret = dev_set_name(dev, "%s.%s.%d", modname, auxdev->name,
+				   auxdev->id);
 	if (ret) {
 		dev_err(dev, "auxiliary device dev_set_name failed: %d\n", ret);
 		return ret;
diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
index 4086afd0cc6b..76904cf2c3dd 100644
--- a/include/linux/auxiliary_bus.h
+++ b/include/linux/auxiliary_bus.h
@@ -51,6 +51,8 @@
  * unregisters the auxiliary device.
  */
 
+#define AUXILIARY_DEVID_NONE	(-1)
+
 /**
  * struct auxiliary_device - auxiliary device object.
  * @dev: Device,
@@ -269,7 +271,7 @@ struct auxiliary_device *__devm_auxiliary_device_create(struct device *dev,
 
 #define devm_auxiliary_device_create(dev, devname, platform_data)     \
 	__devm_auxiliary_device_create(dev, KBUILD_MODNAME, devname,  \
-				       platform_data, 0)
+				       platform_data, AUXILIARY_DEVID_NONE)
 
 /**
  * module_auxiliary_driver() - Helper macro for registering an auxiliary driver
-- 
2.35.1.1320.gc452695387.dirty

[PATCH net 2/4] net: axienet: Fix resource release ordering

[Sean Anderson]

Device-managed resources are released after manually-managed resources.
Therefore, once any manually-managed resource is acquired, all further
resources must be manually-managed too.

Convert all resources before the MDIO bus is created into device-managed
resources. In all cases but one there are already devm variants available.

Fixes: 46aa27df8853 ("net: axienet: Use devm_* calls")
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 .../net/ethernet/xilinx/xilinx_axienet_main.c | 89 ++++++++-----------
 1 file changed, 37 insertions(+), 52 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 6011d7eae0c7..1f277e5e4a62 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -2744,6 +2744,11 @@ static void axienet_dma_err_handler(struct work_struct *work)
 	axienet_setoptions(ndev, lp->options);
 }
 
+static void axienet_disable_misc(void *clocks)
+{
+	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, clocks);
+}
+
 /**
  * axienet_probe - Axi Ethernet probe function.
  * @pdev:	Pointer to platform device structure.
@@ -2767,7 +2772,7 @@ static int axienet_probe(struct platform_device *pdev)
 	int addr_width = 32;
 	u32 value;
 
-	ndev = alloc_etherdev(sizeof(*lp));
+	ndev = devm_alloc_etherdev(&pdev->dev, sizeof(*lp));
 	if (!ndev)
 		return -ENOMEM;
 
@@ -2795,22 +2800,17 @@ static int axienet_probe(struct platform_device *pdev)
 	seqcount_mutex_init(&lp->hw_stats_seqcount, &lp->stats_lock);
 	INIT_DEFERRABLE_WORK(&lp->stats_work, axienet_refresh_stats);
 
-	lp->axi_clk = devm_clk_get_optional(&pdev->dev, "s_axi_lite_clk");
+	lp->axi_clk = devm_clk_get_optional_enabled(&pdev->dev,
+						    "s_axi_lite_clk");
 	if (!lp->axi_clk) {
 		/* For backward compatibility, if named AXI clock is not present,
 		 * treat the first clock specified as the AXI clock.
 		 */
-		lp->axi_clk = devm_clk_get_optional(&pdev->dev, NULL);
-	}
-	if (IS_ERR(lp->axi_clk)) {
-		ret = PTR_ERR(lp->axi_clk);
-		goto free_netdev;
-	}
-	ret = clk_prepare_enable(lp->axi_clk);
-	if (ret) {
-		dev_err(&pdev->dev, "Unable to enable AXI clock: %d\n", ret);
-		goto free_netdev;
+		lp->axi_clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
 	}
+	if (IS_ERR(lp->axi_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(lp->axi_clk),
+				     "could not get AXI clock\n");
 
 	lp->misc_clks[0].id = "axis_clk";
 	lp->misc_clks[1].id = "ref_clk";
@@ -2818,18 +2818,23 @@ static int axienet_probe(struct platform_device *pdev)
 
 	ret = devm_clk_bulk_get_optional(&pdev->dev, XAE_NUM_MISC_CLOCKS, lp->misc_clks);
 	if (ret)
-		goto cleanup_clk;
+		return dev_err_probe(&pdev->dev, ret,
+				     "could not get misc. clocks\n");
 
 	ret = clk_bulk_prepare_enable(XAE_NUM_MISC_CLOCKS, lp->misc_clks);
 	if (ret)
-		goto cleanup_clk;
+		return dev_err_probe(&pdev->dev, ret,
+				     "could not enable misc. clocks\n");
+
+	ret = devm_add_action_or_reset(&pdev->dev, axienet_disable_misc,
+				       lp->misc_clks);
+	if (ret)
+		return ret;
 
 	/* Map device registers */
 	lp->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &ethres);
-	if (IS_ERR(lp->regs)) {
-		ret = PTR_ERR(lp->regs);
-		goto cleanup_clk;
-	}
+	if (IS_ERR(lp->regs))
+		return PTR_ERR(lp->regs);
 	lp->regs_start = ethres->start;
 
 	/* Setup checksum offload, but default to off if not specified */
@@ -2898,19 +2903,17 @@ static int axienet_probe(struct platform_device *pdev)
 			lp->phy_mode = PHY_INTERFACE_MODE_1000BASEX;
 			break;
 		default:
-			ret = -EINVAL;
-			goto cleanup_clk;
+			return -EINVAL;
 		}
 	} else {
 		ret = of_get_phy_mode(pdev->dev.of_node, &lp->phy_mode);
 		if (ret)
-			goto cleanup_clk;
+			return ret;
 	}
 	if (lp->switch_x_sgmii && lp->phy_mode != PHY_INTERFACE_MODE_SGMII &&
 	    lp->phy_mode != PHY_INTERFACE_MODE_1000BASEX) {
 		dev_err(&pdev->dev, "xlnx,switch-x-sgmii only supported with SGMII or 1000BaseX\n");
-		ret = -EINVAL;
-		goto cleanup_clk;
+		return -EINVAL;
 	}
 
 	if (!of_property_present(pdev->dev.of_node, "dmas")) {
@@ -2925,7 +2928,7 @@ static int axienet_probe(struct platform_device *pdev)
 				dev_err(&pdev->dev,
 					"unable to get DMA resource\n");
 				of_node_put(np);
-				goto cleanup_clk;
+				return ret;
 			}
 			lp->dma_regs = devm_ioremap_resource(&pdev->dev,
 							     &dmares);
@@ -2942,19 +2945,17 @@ static int axienet_probe(struct platform_device *pdev)
 		}
 		if (IS_ERR(lp->dma_regs)) {
 			dev_err(&pdev->dev, "could not map DMA regs\n");
-			ret = PTR_ERR(lp->dma_regs);
-			goto cleanup_clk;
+			return PTR_ERR(lp->dma_regs);
 		}
 		if (lp->rx_irq <= 0 || lp->tx_irq <= 0) {
 			dev_err(&pdev->dev, "could not determine irqs\n");
-			ret = -ENOMEM;
-			goto cleanup_clk;
+			return -ENOMEM;
 		}
 
 		/* Reset core now that clocks are enabled, prior to accessing MDIO */
 		ret = __axienet_device_reset(lp);
 		if (ret)
-			goto cleanup_clk;
+			return ret;
 
 		/* Autodetect the need for 64-bit DMA pointers.
 		 * When the IP is configured for a bus width bigger than 32 bits,
@@ -2981,14 +2982,13 @@ static int axienet_probe(struct platform_device *pdev)
 		}
 		if (!IS_ENABLED(CONFIG_64BIT) && lp->features & XAE_FEATURE_DMA_64BIT) {
 			dev_err(&pdev->dev, "64-bit addressable DMA is not compatible with 32-bit architecture\n");
-			ret = -EINVAL;
-			goto cleanup_clk;
+			return -EINVAL;
 		}
 
 		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(addr_width));
 		if (ret) {
 			dev_err(&pdev->dev, "No suitable DMA available\n");
-			goto cleanup_clk;
+			return ret;
 		}
 		netif_napi_add(ndev, &lp->napi_rx, axienet_rx_poll);
 		netif_napi_add(ndev, &lp->napi_tx, axienet_tx_poll);
@@ -2998,15 +2998,12 @@ static int axienet_probe(struct platform_device *pdev)
 
 		lp->eth_irq = platform_get_irq_optional(pdev, 0);
 		if (lp->eth_irq < 0 && lp->eth_irq != -ENXIO) {
-			ret = lp->eth_irq;
-			goto cleanup_clk;
+			return lp->eth_irq;
 		}
 		tx_chan = dma_request_chan(lp->dev, "tx_chan0");
-		if (IS_ERR(tx_chan)) {
-			ret = PTR_ERR(tx_chan);
-			dev_err_probe(lp->dev, ret, "No Ethernet DMA (TX) channel found\n");
-			goto cleanup_clk;
-		}
+		if (IS_ERR(tx_chan))
+			return dev_err_probe(lp->dev, PTR_ERR(tx_chan),
+					     "No Ethernet DMA (TX) channel found\n");
 
 		cfg.reset = 1;
 		/* As name says VDMA but it has support for DMA channel reset */
@@ -3014,7 +3011,7 @@ static int axienet_probe(struct platform_device *pdev)
 		if (ret < 0) {
 			dev_err(&pdev->dev, "Reset channel failed\n");
 			dma_release_channel(tx_chan);
-			goto cleanup_clk;
+			return ret;
 		}
 
 		dma_release_channel(tx_chan);
@@ -3119,13 +3116,6 @@ static int axienet_probe(struct platform_device *pdev)
 		put_device(&lp->pcs_phy->dev);
 	if (lp->mii_bus)
 		axienet_mdio_teardown(lp);
-cleanup_clk:
-	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, lp->misc_clks);
-	clk_disable_unprepare(lp->axi_clk);
-
-free_netdev:
-	free_netdev(ndev);
-
 	return ret;
 }
 
@@ -3143,11 +3133,6 @@ static void axienet_remove(struct platform_device *pdev)
 		put_device(&lp->pcs_phy->dev);
 
 	axienet_mdio_teardown(lp);
-
-	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, lp->misc_clks);
-	clk_disable_unprepare(lp->axi_clk);
-
-	free_netdev(ndev);
 }
 
 static void axienet_shutdown(struct platform_device *pdev)
-- 
2.35.1.1320.gc452695387.dirty

[PATCH net 3/4] net: axienet: Rearrange lifetime functions

[Sean Anderson]

Rearrange the lifetime functions (probe, remove, etc.) in preparation
for the next commit. No functional change intended.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 .../net/ethernet/xilinx/xilinx_axienet_main.c | 252 +++++++++---------
 1 file changed, 133 insertions(+), 119 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 1f277e5e4a62..c2512c04a88f 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -2749,6 +2749,134 @@ static void axienet_disable_misc(void *clocks)
 	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, clocks);
 }
 
+static int axienet_mac_probe(struct axienet_local *lp)
+{
+	struct net_device *ndev = lp->ndev;
+	struct device_node *np;
+	int ret;
+
+	SET_NETDEV_DEV(ndev, lp->dev);
+	if (lp->phy_mode == PHY_INTERFACE_MODE_SGMII ||
+	    lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX) {
+		np = of_parse_phandle(lp->dev->of_node, "pcs-handle", 0);
+		if (!np) {
+			/* Deprecated: Always use "pcs-handle" for pcs_phy.
+			 * Falling back to "phy-handle" here is only for
+			 * backward compatibility with old device trees.
+			 */
+			np = of_parse_phandle(lp->dev->of_node, "phy-handle", 0);
+		}
+		if (!np) {
+			dev_err(lp->dev,
+				"pcs-handle (preferred) or phy-handle required for 1000BaseX/SGMII\n");
+			return -EINVAL;
+		}
+		lp->pcs_phy = of_mdio_find_device(np);
+		of_node_put(np);
+		if (!lp->pcs_phy)
+			return -EPROBE_DEFER;
+		lp->pcs.ops = &axienet_pcs_ops;
+		lp->pcs.poll = true;
+	}
+
+	lp->phylink_config.dev = &ndev->dev;
+	lp->phylink_config.type = PHYLINK_NETDEV;
+	lp->phylink_config.mac_managed_pm = true;
+	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
+		MAC_10FD | MAC_100FD | MAC_1000FD;
+
+	__set_bit(lp->phy_mode, lp->phylink_config.supported_interfaces);
+	if (lp->switch_x_sgmii) {
+		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
+			  lp->phylink_config.supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_SGMII,
+			  lp->phylink_config.supported_interfaces);
+	}
+
+	lp->phylink = phylink_create(&lp->phylink_config, lp->dev->fwnode,
+				     lp->phy_mode,
+				     &axienet_phylink_ops);
+	if (IS_ERR(lp->phylink)) {
+		ret = PTR_ERR(lp->phylink);
+		dev_err(lp->dev, "phylink_create error (%i)\n", ret);
+		goto cleanup_pcs;
+	}
+
+	ret = register_netdev(ndev);
+	if (ret) {
+		dev_err(lp->dev, "register_netdev() error (%i)\n", ret);
+		goto cleanup_phylink;
+	}
+
+	return 0;
+
+cleanup_phylink:
+	phylink_destroy(lp->phylink);
+cleanup_pcs:
+	if (lp->pcs_phy)
+		put_device(&lp->pcs_phy->dev);
+	return ret;
+}
+
+static void axienet_mac_remove(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct axienet_local *lp = netdev_priv(ndev);
+
+	unregister_netdev(ndev);
+	phylink_destroy(lp->phylink);
+	if (lp->pcs_phy)
+		put_device(&lp->pcs_phy->dev);
+}
+
+static void axienet_mac_shutdown(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+
+	rtnl_lock();
+	netif_device_detach(ndev);
+
+	if (netif_running(ndev))
+		dev_close(ndev);
+
+	rtnl_unlock();
+}
+
+static int axienet_suspend(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
+
+	if (!netif_running(ndev))
+		return 0;
+
+	netif_device_detach(ndev);
+
+	rtnl_lock();
+	axienet_stop(ndev);
+	rtnl_unlock();
+
+	return 0;
+}
+
+static int axienet_resume(struct device *dev)
+{
+	struct net_device *ndev = dev_get_drvdata(dev);
+
+	if (!netif_running(ndev))
+		return 0;
+
+	rtnl_lock();
+	axienet_open(ndev);
+	rtnl_unlock();
+
+	netif_device_attach(ndev);
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(axienet_pm_ops,
+				axienet_suspend, axienet_resume);
+
 /**
  * axienet_probe - Axi Ethernet probe function.
  * @pdev:	Pointer to platform device structure.
@@ -3051,69 +3179,10 @@ static int axienet_probe(struct platform_device *pdev)
 		dev_warn(&pdev->dev,
 			 "error registering MDIO bus: %d\n", ret);
 
-	if (lp->phy_mode == PHY_INTERFACE_MODE_SGMII ||
-	    lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX) {
-		np = of_parse_phandle(pdev->dev.of_node, "pcs-handle", 0);
-		if (!np) {
-			/* Deprecated: Always use "pcs-handle" for pcs_phy.
-			 * Falling back to "phy-handle" here is only for
-			 * backward compatibility with old device trees.
-			 */
-			np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
-		}
-		if (!np) {
-			dev_err(&pdev->dev, "pcs-handle (preferred) or phy-handle required for 1000BaseX/SGMII\n");
-			ret = -EINVAL;
-			goto cleanup_mdio;
-		}
-		lp->pcs_phy = of_mdio_find_device(np);
-		if (!lp->pcs_phy) {
-			ret = -EPROBE_DEFER;
-			of_node_put(np);
-			goto cleanup_mdio;
-		}
-		of_node_put(np);
-		lp->pcs.ops = &axienet_pcs_ops;
-		lp->pcs.poll = true;
-	}
+	ret = axienet_mac_probe(lp);
+	if (!ret)
+		return 0;
 
-	lp->phylink_config.dev = &ndev->dev;
-	lp->phylink_config.type = PHYLINK_NETDEV;
-	lp->phylink_config.mac_managed_pm = true;
-	lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
-		MAC_10FD | MAC_100FD | MAC_1000FD;
-
-	__set_bit(lp->phy_mode, lp->phylink_config.supported_interfaces);
-	if (lp->switch_x_sgmii) {
-		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
-			  lp->phylink_config.supported_interfaces);
-		__set_bit(PHY_INTERFACE_MODE_SGMII,
-			  lp->phylink_config.supported_interfaces);
-	}
-
-	lp->phylink = phylink_create(&lp->phylink_config, pdev->dev.fwnode,
-				     lp->phy_mode,
-				     &axienet_phylink_ops);
-	if (IS_ERR(lp->phylink)) {
-		ret = PTR_ERR(lp->phylink);
-		dev_err(&pdev->dev, "phylink_create error (%i)\n", ret);
-		goto cleanup_mdio;
-	}
-
-	ret = register_netdev(lp->ndev);
-	if (ret) {
-		dev_err(lp->dev, "register_netdev() error (%i)\n", ret);
-		goto cleanup_phylink;
-	}
-
-	return 0;
-
-cleanup_phylink:
-	phylink_destroy(lp->phylink);
-
-cleanup_mdio:
-	if (lp->pcs_phy)
-		put_device(&lp->pcs_phy->dev);
 	if (lp->mii_bus)
 		axienet_mdio_teardown(lp);
 	return ret;
@@ -3124,69 +3193,14 @@ static void axienet_remove(struct platform_device *pdev)
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct axienet_local *lp = netdev_priv(ndev);
 
-	unregister_netdev(ndev);
-
-	if (lp->phylink)
-		phylink_destroy(lp->phylink);
-
-	if (lp->pcs_phy)
-		put_device(&lp->pcs_phy->dev);
-
+	axienet_mac_remove(pdev);
 	axienet_mdio_teardown(lp);
 }
 
-static void axienet_shutdown(struct platform_device *pdev)
-{
-	struct net_device *ndev = platform_get_drvdata(pdev);
-
-	rtnl_lock();
-	netif_device_detach(ndev);
-
-	if (netif_running(ndev))
-		dev_close(ndev);
-
-	rtnl_unlock();
-}
-
-static int axienet_suspend(struct device *dev)
-{
-	struct net_device *ndev = dev_get_drvdata(dev);
-
-	if (!netif_running(ndev))
-		return 0;
-
-	netif_device_detach(ndev);
-
-	rtnl_lock();
-	axienet_stop(ndev);
-	rtnl_unlock();
-
-	return 0;
-}
-
-static int axienet_resume(struct device *dev)
-{
-	struct net_device *ndev = dev_get_drvdata(dev);
-
-	if (!netif_running(ndev))
-		return 0;
-
-	rtnl_lock();
-	axienet_open(ndev);
-	rtnl_unlock();
-
-	netif_device_attach(ndev);
-
-	return 0;
-}
-
-static DEFINE_SIMPLE_DEV_PM_OPS(axienet_pm_ops,
-				axienet_suspend, axienet_resume);
-
 static struct platform_driver axienet_driver = {
 	.probe = axienet_probe,
 	.remove = axienet_remove,
-	.shutdown = axienet_shutdown,
+	.shutdown = axienet_mac_shutdown,
 	.driver = {
 		 .name = "xilinx_axienet",
 		 .pm = &axienet_pm_ops,
-- 
2.35.1.1320.gc452695387.dirty

[PATCH net 4/4] net: axienet: Split into MAC and MDIO drivers

[Sean Anderson]

Returning EPROBE_DEFER after probing a bus may result in an infinite
probe loop if the EPROBE_DEFER error is never resolved. For example, if
the PCS is located on another MDIO bus and that MDIO bus is missing its
driver then we will always return EPROBE_DEFER. But if there are any
devices on our own MDIO bus (such as PHYs), those devices will be
successfully bound before we fail our own probe. This will cause the
deferred probing infrastructure to continuously try to probe our device.

To prevent this, split the MAC and MDIO functionality into separate
auxiliary devices. These can then be re-probed independently.

Fixes: 1a02556086fc ("net: axienet: Properly handle PCS/PMA PHY for 1000BaseX mode")
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/net/ethernet/xilinx/Kconfig           |  1 +
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 76 +++++++++++--------
 .../net/ethernet/xilinx/xilinx_axienet_mdio.c | 32 +++++---
 3 files changed, 69 insertions(+), 40 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/Kconfig b/drivers/net/ethernet/xilinx/Kconfig
index 7502214cc7d5..3b940d2d3115 100644
--- a/drivers/net/ethernet/xilinx/Kconfig
+++ b/drivers/net/ethernet/xilinx/Kconfig
@@ -27,6 +27,7 @@ config XILINX_AXI_EMAC
 	tristate "Xilinx 10/100/1000 AXI Ethernet support"
 	depends on HAS_IOMEM
 	depends on XILINX_DMA
+	select AUXILIARY_BUS
 	select PHYLINK
 	select DIMLIB
 	help
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index c2512c04a88f..2f26474b16f6 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -22,6 +22,7 @@
  *  - Add support for extended VLAN support.
  */
 
+#include <linux/auxiliary_bus.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/etherdevice.h>
@@ -2749,13 +2750,16 @@ static void axienet_disable_misc(void *clocks)
 	clk_bulk_disable_unprepare(XAE_NUM_MISC_CLOCKS, clocks);
 }
 
-static int axienet_mac_probe(struct axienet_local *lp)
+static int axienet_mac_probe(struct auxiliary_device *auxdev,
+			     const struct auxiliary_device_id *id)
 {
+	struct axienet_local *lp = auxdev->dev.platform_data;
 	struct net_device *ndev = lp->ndev;
 	struct device_node *np;
 	int ret;
 
-	SET_NETDEV_DEV(ndev, lp->dev);
+	auxiliary_set_drvdata(auxdev, ndev);
+	SET_NETDEV_DEV(ndev, &auxdev->dev);
 	if (lp->phy_mode == PHY_INTERFACE_MODE_SGMII ||
 	    lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX) {
 		np = of_parse_phandle(lp->dev->of_node, "pcs-handle", 0);
@@ -2818,9 +2822,9 @@ static int axienet_mac_probe(struct axienet_local *lp)
 	return ret;
 }
 
-static void axienet_mac_remove(struct platform_device *pdev)
+static void axienet_mac_remove(struct auxiliary_device *auxdev)
 {
-	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct net_device *ndev = auxiliary_get_drvdata(auxdev);
 	struct axienet_local *lp = netdev_priv(ndev);
 
 	unregister_netdev(ndev);
@@ -2829,9 +2833,9 @@ static void axienet_mac_remove(struct platform_device *pdev)
 		put_device(&lp->pcs_phy->dev);
 }
 
-static void axienet_mac_shutdown(struct platform_device *pdev)
+static void axienet_mac_shutdown(struct auxiliary_device *auxdev)
 {
-	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct net_device *ndev = auxiliary_get_drvdata(auxdev);
 
 	rtnl_lock();
 	netif_device_detach(ndev);
@@ -2877,6 +2881,24 @@ static int axienet_resume(struct device *dev)
 static DEFINE_SIMPLE_DEV_PM_OPS(axienet_pm_ops,
 				axienet_suspend, axienet_resume);
 
+static const struct auxiliary_device_id xilinx_axienet_mac_id_table[] = {
+	{ .name = KBUILD_MODNAME ".mac", },
+	{ },
+};
+MODULE_DEVICE_TABLE(auxiliary, xilinx_axienet_mac_id_table);
+
+static struct auxiliary_driver xilinx_axienet_mac = {
+	.name = "mac",
+	.id_table = xilinx_axienet_mac_id_table,
+	.probe = axienet_mac_probe,
+	.remove = axienet_mac_remove,
+	.shutdown = axienet_mac_shutdown,
+	.driver = {
+		 .pm = &axienet_pm_ops,
+	},
+};
+module_auxiliary_driver(xilinx_axienet_mac)
+
 /**
  * axienet_probe - Axi Ethernet probe function.
  * @pdev:	Pointer to platform device structure.
@@ -2892,12 +2914,14 @@ static DEFINE_SIMPLE_DEV_PM_OPS(axienet_pm_ops,
 static int axienet_probe(struct platform_device *pdev)
 {
 	int ret;
+	struct auxiliary_device *auxdev;
 	struct device_node *np;
 	struct axienet_local *lp;
 	struct net_device *ndev;
 	struct resource *ethres;
 	u8 mac_addr[ETH_ALEN];
 	int addr_width = 32;
+	char name[20];
 	u32 value;
 
 	ndev = devm_alloc_etherdev(&pdev->dev, sizeof(*lp));
@@ -2906,7 +2930,6 @@ static int axienet_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, ndev);
 
-	SET_NETDEV_DEV(ndev, &pdev->dev);
 	ndev->features = NETIF_F_SG;
 	ndev->ethtool_ops = &axienet_ethtool_ops;
 
@@ -3174,36 +3197,27 @@ static int axienet_probe(struct platform_device *pdev)
 	lp->tx_dma_cr = axienet_calc_cr(lp, XAXIDMA_DFT_TX_THRESHOLD,
 					XAXIDMA_DFT_TX_USEC);
 
-	ret = axienet_mdio_setup(lp);
-	if (ret)
-		dev_warn(&pdev->dev,
-			 "error registering MDIO bus: %d\n", ret);
-
-	ret = axienet_mac_probe(lp);
-	if (!ret)
-		return 0;
-
-	if (lp->mii_bus)
-		axienet_mdio_teardown(lp);
-	return ret;
-}
-
-static void axienet_remove(struct platform_device *pdev)
-{
-	struct net_device *ndev = platform_get_drvdata(pdev);
-	struct axienet_local *lp = netdev_priv(ndev);
-
-	axienet_mac_remove(pdev);
-	axienet_mdio_teardown(lp);
+	snprintf(name, sizeof(name), "mdio.%llx",
+		 (unsigned long long)lp->regs_start);
+	auxdev = devm_auxiliary_device_create(&pdev->dev, name, lp);
+	if (IS_ERR(auxdev))
+		return dev_err_probe(&pdev->dev, PTR_ERR(auxdev),
+				     "could not create MDIO bus\n");
+
+	snprintf(name, sizeof(name), "mac.%llx",
+		 (unsigned long long)lp->regs_start);
+	auxdev = devm_auxiliary_device_create(&pdev->dev, name, lp);
+	if (IS_ERR(auxdev))
+		return dev_err_probe(&pdev->dev, PTR_ERR(auxdev),
+				     "could not create MAC\n");
+
+	return 0;
 }
 
 static struct platform_driver axienet_driver = {
 	.probe = axienet_probe,
-	.remove = axienet_remove,
-	.shutdown = axienet_mac_shutdown,
 	.driver = {
 		 .name = "xilinx_axienet",
-		 .pm = &axienet_pm_ops,
 		 .of_match_table = axienet_of_match,
 	},
 };
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c b/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c
index 9ca2643c921e..8874525ec3f3 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c
@@ -9,6 +9,7 @@
  * Copyright (c) 2010 - 2012 Xilinx, Inc. All rights reserved.
  */
 
+#include <linux/auxiliary_bus.h>
 #include <linux/clk.h>
 #include <linux/of_address.h>
 #include <linux/of_mdio.h>
@@ -277,12 +278,15 @@ static int axienet_mdio_enable(struct axienet_local *lp, struct device_node *np)
  * Sets up the MDIO interface by initializing the MDIO clock.
  * Register the MDIO interface.
  **/
-int axienet_mdio_setup(struct axienet_local *lp)
+static int axienet_mdio_probe(struct auxiliary_device *auxdev,
+			      const struct auxiliary_device_id *id)
 {
+	struct axienet_local *lp = auxdev->dev.platform_data;
 	struct device_node *mdio_node;
 	struct mii_bus *bus;
 	int ret;
 
+	auxiliary_set_drvdata(auxdev, lp);
 	bus = mdiobus_alloc();
 	if (!bus)
 		return -ENOMEM;
@@ -294,7 +298,7 @@ int axienet_mdio_setup(struct axienet_local *lp)
 	bus->name = "Xilinx Axi Ethernet MDIO";
 	bus->read = axienet_mdio_read;
 	bus->write = axienet_mdio_write;
-	bus->parent = lp->dev;
+	bus->parent = &auxdev->dev;
 	lp->mii_bus = bus;
 
 	mdio_node = of_get_child_by_name(lp->dev->of_node, "mdio");
@@ -317,15 +321,25 @@ int axienet_mdio_setup(struct axienet_local *lp)
 	return ret;
 }
 
-/**
- * axienet_mdio_teardown - MDIO remove function
- * @lp:		Pointer to axienet local data structure.
- *
- * Unregisters the MDIO and frees any associate memory for mii bus.
- */
-void axienet_mdio_teardown(struct axienet_local *lp)
+static void axienet_mdio_remove(struct auxiliary_device *auxdev)
 {
+	struct axienet_local *lp = auxiliary_get_drvdata(auxdev);
+
 	mdiobus_unregister(lp->mii_bus);
 	mdiobus_free(lp->mii_bus);
 	lp->mii_bus = NULL;
 }
+
+static const struct auxiliary_device_id xilinx_axienet_mdio_id_table[] = {
+	{ .name = KBUILD_MODNAME ".mdio", },
+	{ },
+};
+MODULE_DEVICE_TABLE(auxiliary, xilinx_axienet_mdio_id_table);
+
+static struct auxiliary_driver xilinx_axienet_mdio = {
+	.name = "mdio",
+	.id_table = xilinx_axienet_mdio_id_table,
+	.probe = axienet_mdio_probe,
+	.remove = axienet_mdio_remove,
+};
+module_auxiliary_driver(xilinx_axienet_mdio)
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH net 4/4] net: axienet: Split into MAC and MDIO drivers

[Jakub Kicinski]

On Thu, 19 Jun 2025 16:05:37 -0400 Sean Anderson wrote:
> Returning EPROBE_DEFER after probing a bus may result in an infinite
> probe loop if the EPROBE_DEFER error is never resolved. For example, if
> the PCS is located on another MDIO bus and that MDIO bus is missing its
> driver then we will always return EPROBE_DEFER. But if there are any
> devices on our own MDIO bus (such as PHYs), those devices will be
> successfully bound before we fail our own probe. This will cause the
> deferred probing infrastructure to continuously try to probe our device.
> 
> To prevent this, split the MAC and MDIO functionality into separate
> auxiliary devices. These can then be re-probed independently.

There's a, pardon the expression, C++-like build failure here
culminating in:

drivers/net/ethernet/xilinx/xilinx_axienet_main.c:3225:1: error: redefinition of '__exittest'
drivers/net/ethernet/xilinx/xilinx_axienet_main.c:3225:1: error: redefinition of '__inittest'
drivers/net/ethernet/xilinx/xilinx_axienet_main.c:3225:1: error: redefinition of 'init_module'
drivers/net/ethernet/xilinx/xilinx_axienet_main.c:3225:1: error: redefinition of 'cleanup_module'

I'm guessing the existing module_platform_driver() and the new
module_auxiliary_driver() don't want to be friends when this 
code is built as a module?
-- 
pw-bot: cr

Re: [PATCH net 4/4] net: axienet: Split into MAC and MDIO drivers

[Sean Anderson]

On 6/19/25 19:10, Jakub Kicinski wrote:
> On Thu, 19 Jun 2025 16:05:37 -0400 Sean Anderson wrote:
>> Returning EPROBE_DEFER after probing a bus may result in an infinite
>> probe loop if the EPROBE_DEFER error is never resolved. For example, if
>> the PCS is located on another MDIO bus and that MDIO bus is missing its
>> driver then we will always return EPROBE_DEFER. But if there are any
>> devices on our own MDIO bus (such as PHYs), those devices will be
>> successfully bound before we fail our own probe. This will cause the
>> deferred probing infrastructure to continuously try to probe our device.
>> 
>> To prevent this, split the MAC and MDIO functionality into separate
>> auxiliary devices. These can then be re-probed independently.
> 
> There's a, pardon the expression, C++-like build failure here
> culminating in:
> 
> drivers/net/ethernet/xilinx/xilinx_axienet_main.c:3225:1: error: redefinition of '__exittest'
> drivers/net/ethernet/xilinx/xilinx_axienet_main.c:3225:1: error: redefinition of '__inittest'
> drivers/net/ethernet/xilinx/xilinx_axienet_main.c:3225:1: error: redefinition of 'init_module'
> drivers/net/ethernet/xilinx/xilinx_axienet_main.c:3225:1: error: redefinition of 'cleanup_module'
> 
> I'm guessing the existing module_platform_driver() and the new
> module_auxiliary_driver() don't want to be friends when this 
> code is built as a module?

Hm, I thought I had built this as a module. I guess not. Will fix.

--Sean

Re: [PATCH net 0/4] net: axienet: Fix deferred probe loop

[Greg Kroah-Hartman]

On Thu, Jun 19, 2025 at 04:05:33PM -0400, Sean Anderson wrote:
> Upon further investigation, the EPROBE_DEFER loop outlined in [1] can
> occur even without the PCS subsystem, as described in patch 4/4. The
> second patch is a general fix, and could be applied even without the
> auxdev conversion.
> 
> [1] https://lore.kernel.org/all/20250610183459.3395328-1-sean.anderson@linux.dev/

I have no idea what this summary means at all, which isn't a good start
to a patch series :(

What problem are you trying to solve?  What overall solution did you
come up with?  Who is supposed to be reviewing any of this?

totally confused,

greg k-h

Re: [PATCH net 1/4] auxiliary: Allow empty id

[Greg Kroah-Hartman]

On Thu, Jun 19, 2025 at 04:05:34PM -0400, Sean Anderson wrote:
> Support creating auxiliary devices with the id included as part of the
> name. This allows for non-decimal ids, which may be more appropriate for
> auxiliary devices created as children of memory-mapped devices. For
> example, a name like "xilinx_emac.mac.802c0000" could be achieved by
> setting .name to "mac.802c0000" and .id to AUXILIARY_DEVID_NONE.

I don't see the justification for this, sorry.  An id is just an id, it
doesn't matter what is is and nothing should be relying on it to be the
same across reboots or anywhere else.  The only requirement is that it
be unique at this point in time in the system.

We're having this same discussion on a different thread for a different
bus as well.  This isn't something new, it's been hashed out and
resolved 20+ years ago...

So no, this change isn't ok to make at this point in time, sorry.

greg k-h

Re: [PATCH net 4/4] net: axienet: Split into MAC and MDIO drivers

[kernel test robot]

Hi Sean,

kernel test robot noticed the following build errors:

[auto build test ERROR on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Anderson/auxiliary-Allow-empty-id/20250620-040839
base:   net/main
patch link:    https://lore.kernel.org/r/20250619200537.260017-5-sean.anderson%40linux.dev
patch subject: [PATCH net 4/4] net: axienet: Split into MAC and MDIO drivers
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20250620/202506202126.EmqQsj0w-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250620/202506202126.EmqQsj0w-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506202126.EmqQsj0w-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/net/ethernet/xilinx/xilinx_axienet_main.c:3225:1: error: redefinition of '__inittest'
    3225 | module_platform_driver(axienet_driver);
         | ^
   include/linux/platform_device.h:292:2: note: expanded from macro 'module_platform_driver'
     292 |         module_driver(__platform_driver, platform_driver_register, \
         |         ^
   include/linux/device/driver.h:261:3: note: expanded from macro 'module_driver'
     261 | } \
         |   ^
   include/linux/module.h:131:42: note: expanded from macro '\
   module_init'
     131 |         static inline initcall_t __maybe_unused __inittest(void)                \
         |                                                 ^
   drivers/net/ethernet/xilinx/xilinx_axienet_main.c:2900:1: note: previous definition is here
    2900 | module_auxiliary_driver(xilinx_axienet_mac)
         | ^
   include/linux/auxiliary_bus.h:289:2: note: expanded from macro 'module_auxiliary_driver'
     289 |         module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
         |         ^
   include/linux/device/driver.h:261:3: note: expanded from macro 'module_driver'
     261 | } \
         |   ^
   include/linux/module.h:131:42: note: expanded from macro '\
   module_init'
     131 |         static inline initcall_t __maybe_unused __inittest(void)                \
         |                                                 ^
>> drivers/net/ethernet/xilinx/xilinx_axienet_main.c:3225:1: error: redefinition of 'init_module'
    3225 | module_platform_driver(axienet_driver);
         | ^
   include/linux/platform_device.h:292:2: note: expanded from macro 'module_platform_driver'
     292 |         module_driver(__platform_driver, platform_driver_register, \
         |         ^
   include/linux/device/driver.h:261:3: note: expanded from macro 'module_driver'
     261 | } \
         |   ^
   include/linux/module.h:133:6: note: expanded from macro '\
   module_init'
     133 |         int init_module(void) __copy(initfn)                    \
         |             ^
   drivers/net/ethernet/xilinx/xilinx_axienet_main.c:2900:1: note: previous definition is here
    2900 | module_auxiliary_driver(xilinx_axienet_mac)
         | ^
   include/linux/auxiliary_bus.h:289:2: note: expanded from macro 'module_auxiliary_driver'
     289 |         module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
         |         ^
   include/linux/device/driver.h:261:3: note: expanded from macro 'module_driver'
     261 | } \
         |   ^
   include/linux/module.h:133:6: note: expanded from macro '\
   module_init'
     133 |         int init_module(void) __copy(initfn)                    \
         |             ^
>> drivers/net/ethernet/xilinx/xilinx_axienet_main.c:3225:1: error: redefinition of '__exittest'
    3225 | module_platform_driver(axienet_driver);
         | ^
   include/linux/platform_device.h:292:2: note: expanded from macro 'module_platform_driver'
     292 |         module_driver(__platform_driver, platform_driver_register, \
         |         ^
   include/linux/device/driver.h:266:3: note: expanded from macro 'module_driver'
     266 | } \
         |   ^
   include/linux/module.h:139:42: note: expanded from macro '\
   module_exit'
     139 |         static inline exitcall_t __maybe_unused __exittest(void)                \
         |                                                 ^
   drivers/net/ethernet/xilinx/xilinx_axienet_main.c:2900:1: note: previous definition is here
    2900 | module_auxiliary_driver(xilinx_axienet_mac)
         | ^
   include/linux/auxiliary_bus.h:289:2: note: expanded from macro 'module_auxiliary_driver'
     289 |         module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
         |         ^
   include/linux/device/driver.h:266:3: note: expanded from macro 'module_driver'
     266 | } \
         |   ^
   include/linux/module.h:139:42: note: expanded from macro '\
   module_exit'
     139 |         static inline exitcall_t __maybe_unused __exittest(void)                \
         |                                                 ^
>> drivers/net/ethernet/xilinx/xilinx_axienet_main.c:3225:1: error: redefinition of 'cleanup_module'
    3225 | module_platform_driver(axienet_driver);
         | ^
   include/linux/platform_device.h:292:2: note: expanded from macro 'module_platform_driver'
     292 |         module_driver(__platform_driver, platform_driver_register, \
         |         ^
   include/linux/device/driver.h:266:3: note: expanded from macro 'module_driver'
     266 | } \
         |   ^
   include/linux/module.h:141:7: note: expanded from macro '\
   module_exit'
     141 |         void cleanup_module(void) __copy(exitfn)                \
         |              ^
   drivers/net/ethernet/xilinx/xilinx_axienet_main.c:2900:1: note: previous definition is here
    2900 | module_auxiliary_driver(xilinx_axienet_mac)
         | ^
   include/linux/auxiliary_bus.h:289:2: note: expanded from macro 'module_auxiliary_driver'
     289 |         module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
         |         ^
   include/linux/device/driver.h:266:3: note: expanded from macro 'module_driver'
     266 | } \
         |   ^
   include/linux/module.h:141:7: note: expanded from macro '\
   module_exit'
     141 |         void cleanup_module(void) __copy(exitfn)                \
         |              ^
   4 errors generated.


vim +/__inittest +3225 drivers/net/ethernet/xilinx/xilinx_axienet_main.c

8a3b7a252dca9f Daniel Borkmann  2012-01-19  3224  
2be586205ca2b8 Srikanth Thokala 2015-05-05 @3225  module_platform_driver(axienet_driver);
8a3b7a252dca9f Daniel Borkmann  2012-01-19  3226  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net 1/4] auxiliary: Allow empty id

[Sean Anderson]

On 6/20/25 01:13, Greg Kroah-Hartman wrote:
> On Thu, Jun 19, 2025 at 04:05:34PM -0400, Sean Anderson wrote:
>> Support creating auxiliary devices with the id included as part of the
>> name. This allows for non-decimal ids, which may be more appropriate for
>> auxiliary devices created as children of memory-mapped devices. For
>> example, a name like "xilinx_emac.mac.802c0000" could be achieved by
>> setting .name to "mac.802c0000" and .id to AUXILIARY_DEVID_NONE.
> 
> I don't see the justification for this, sorry.  An id is just an id, it
> doesn't matter what is is and nothing should be relying on it to be the
> same across reboots or anywhere else.  The only requirement is that it
> be unique at this point in time in the system.

It identifies the device in log messages. Without this you have to read
sysfs to determine what device is (for example) producing an error. This
may be inconvenient to do if the error prevents the system from booting.
This series converts a platform device with a legible ID like
"802c0000.ethernet" to an auxiliary device, and I believe descriptive
device names produce a better developer experience.

This is also shorter and simpler than auto-generated IDs.

--Sean

Re: [PATCH net 0/4] net: axienet: Fix deferred probe loop

[Sean Anderson]

On 6/20/25 01:10, Greg Kroah-Hartman wrote:
> On Thu, Jun 19, 2025 at 04:05:33PM -0400, Sean Anderson wrote:
>> Upon further investigation, the EPROBE_DEFER loop outlined in [1] can
>> occur even without the PCS subsystem, as described in patch 4/4. The
>> second patch is a general fix, and could be applied even without the
>> auxdev conversion.
>> 
>> [1] https://lore.kernel.org/all/20250610183459.3395328-1-sean.anderson@linux.dev/
> 
> I have no idea what this summary means at all, which isn't a good start
> to a patch series :(
> 
> What problem are you trying to solve?

See patch 4/4.

> What overall solution did you come up with?

See patch 4/4.

> Who is supposed to be reviewing any of this?

Netdev. Hence "PATCH net".

And see [1] above for background. I will quote it more-extensively next time.

--Sean

Re: [PATCH net 0/4] net: axienet: Fix deferred probe loop

[Greg Kroah-Hartman]

On Fri, Jun 20, 2025 at 11:41:52AM -0400, Sean Anderson wrote:
> On 6/20/25 01:10, Greg Kroah-Hartman wrote:
> > On Thu, Jun 19, 2025 at 04:05:33PM -0400, Sean Anderson wrote:
> >> Upon further investigation, the EPROBE_DEFER loop outlined in [1] can
> >> occur even without the PCS subsystem, as described in patch 4/4. The
> >> second patch is a general fix, and could be applied even without the
> >> auxdev conversion.
> >> 
> >> [1] https://lore.kernel.org/all/20250610183459.3395328-1-sean.anderson@linux.dev/
> > 
> > I have no idea what this summary means at all, which isn't a good start
> > to a patch series :(
> > 
> > What problem are you trying to solve?
> 
> See patch 4/4.

That's not what should be in patch 0/4 then, right?

> > What overall solution did you come up with?
> 
> See patch 4/4.

Again, why write a 0/4 summary at all then?

> > Who is supposed to be reviewing any of this?
> 
> Netdev. Hence "PATCH net".
> 
> And see [1] above for background. I will quote it more-extensively next time.

Referring to random links doesn't always work as we deal with thousands
of patches daily, and sometimes don't even have internet access (like
when reviewing patches on long flights/train rides...)

Make things self-contained please.

thanks,

greg k-h

Re: [PATCH net 1/4] auxiliary: Allow empty id

[Greg Kroah-Hartman]

On Fri, Jun 20, 2025 at 11:37:40AM -0400, Sean Anderson wrote:
> On 6/20/25 01:13, Greg Kroah-Hartman wrote:
> > On Thu, Jun 19, 2025 at 04:05:34PM -0400, Sean Anderson wrote:
> >> Support creating auxiliary devices with the id included as part of the
> >> name. This allows for non-decimal ids, which may be more appropriate for
> >> auxiliary devices created as children of memory-mapped devices. For
> >> example, a name like "xilinx_emac.mac.802c0000" could be achieved by
> >> setting .name to "mac.802c0000" and .id to AUXILIARY_DEVID_NONE.
> > 
> > I don't see the justification for this, sorry.  An id is just an id, it
> > doesn't matter what is is and nothing should be relying on it to be the
> > same across reboots or anywhere else.  The only requirement is that it
> > be unique at this point in time in the system.
> 
> It identifies the device in log messages. Without this you have to read
> sysfs to determine what device is (for example) producing an error.

That's fine, read sysfs :)

> This
> may be inconvenient to do if the error prevents the system from booting.
> This series converts a platform device with a legible ID like
> "802c0000.ethernet" to an auxiliary device, and I believe descriptive
> device names produce a better developer experience.

You can still have 802c0000.ethernet be the prefix of the name, that's
fine.

> This is also shorter and simpler than auto-generated IDs.

Please stick with auto-generated ids, they will work properly here.

thanks,

greg k-h

Re: [PATCH net 1/4] auxiliary: Allow empty id

[Sean Anderson]

On 6/20/25 12:02, Greg Kroah-Hartman wrote:
> On Fri, Jun 20, 2025 at 11:37:40AM -0400, Sean Anderson wrote:
>> On 6/20/25 01:13, Greg Kroah-Hartman wrote:
>> > On Thu, Jun 19, 2025 at 04:05:34PM -0400, Sean Anderson wrote:
>> >> Support creating auxiliary devices with the id included as part of the
>> >> name. This allows for non-decimal ids, which may be more appropriate for
>> >> auxiliary devices created as children of memory-mapped devices. For
>> >> example, a name like "xilinx_emac.mac.802c0000" could be achieved by
>> >> setting .name to "mac.802c0000" and .id to AUXILIARY_DEVID_NONE.
>> > 
>> > I don't see the justification for this, sorry.  An id is just an id, it
>> > doesn't matter what is is and nothing should be relying on it to be the
>> > same across reboots or anywhere else.  The only requirement is that it
>> > be unique at this point in time in the system.
>> 
>> It identifies the device in log messages. Without this you have to read
>> sysfs to determine what device is (for example) producing an error.
> 
> That's fine, read sysfs :)

I should not have to read sysfs to decode boot output. If there is an
error during boot I should be able to determine the offending device.
This very important when the boot process fails before init is started,
and very convenient otherwise. 

>> This
>> may be inconvenient to do if the error prevents the system from booting.
>> This series converts a platform device with a legible ID like
>> "802c0000.ethernet" to an auxiliary device, and I believe descriptive
>> device names produce a better developer experience.
> 
> You can still have 802c0000.ethernet be the prefix of the name, that's
> fine.

This is not possible due to how the auxiliary bus works. If device's
name is in the form "foo.id", then the driver must have an
auxiliary_device_id in its id_table with .name = "foo". So the address
*must* come after the last period in the name.

--Sean

>> This is also shorter and simpler than auto-generated IDs.
> 
> Please stick with auto-generated ids, they will work properly here.
> 
> thanks,
> 
> greg k-h

Re: [PATCH net 1/4] auxiliary: Allow empty id

[Greg Kroah-Hartman]

On Fri, Jun 20, 2025 at 12:09:29PM -0400, Sean Anderson wrote:
> On 6/20/25 12:02, Greg Kroah-Hartman wrote:
> > On Fri, Jun 20, 2025 at 11:37:40AM -0400, Sean Anderson wrote:
> >> On 6/20/25 01:13, Greg Kroah-Hartman wrote:
> >> > On Thu, Jun 19, 2025 at 04:05:34PM -0400, Sean Anderson wrote:
> >> >> Support creating auxiliary devices with the id included as part of the
> >> >> name. This allows for non-decimal ids, which may be more appropriate for
> >> >> auxiliary devices created as children of memory-mapped devices. For
> >> >> example, a name like "xilinx_emac.mac.802c0000" could be achieved by
> >> >> setting .name to "mac.802c0000" and .id to AUXILIARY_DEVID_NONE.
> >> > 
> >> > I don't see the justification for this, sorry.  An id is just an id, it
> >> > doesn't matter what is is and nothing should be relying on it to be the
> >> > same across reboots or anywhere else.  The only requirement is that it
> >> > be unique at this point in time in the system.
> >> 
> >> It identifies the device in log messages. Without this you have to read
> >> sysfs to determine what device is (for example) producing an error.
> > 
> > That's fine, read sysfs :)
> 
> I should not have to read sysfs to decode boot output. If there is an
> error during boot I should be able to determine the offending device.
> This very important when the boot process fails before init is started,
> and very convenient otherwise. 

The boot log will show you the name of the device that is having a
problem.  And you get to pick a portion of that name to make it make
some kind of sense to users if you want.

> >> This
> >> may be inconvenient to do if the error prevents the system from booting.
> >> This series converts a platform device with a legible ID like
> >> "802c0000.ethernet" to an auxiliary device, and I believe descriptive
> >> device names produce a better developer experience.
> > 
> > You can still have 802c0000.ethernet be the prefix of the name, that's
> > fine.
> 
> This is not possible due to how the auxiliary bus works. If device's
> name is in the form "foo.id", then the driver must have an
> auxiliary_device_id in its id_table with .name = "foo". So the address
> *must* come after the last period in the name.

So what is the new name without this aux patch that looks so wrong?
What is the current log line before and after the change you made?

thanks,

greg k-h

Re: [PATCH net 1/4] auxiliary: Allow empty id

[Sean Anderson]

On 6/20/25 12:15, Greg Kroah-Hartman wrote:
> On Fri, Jun 20, 2025 at 12:09:29PM -0400, Sean Anderson wrote:
>> On 6/20/25 12:02, Greg Kroah-Hartman wrote:
>> > On Fri, Jun 20, 2025 at 11:37:40AM -0400, Sean Anderson wrote:
>> >> On 6/20/25 01:13, Greg Kroah-Hartman wrote:
>> >> > On Thu, Jun 19, 2025 at 04:05:34PM -0400, Sean Anderson wrote:
>> >> >> Support creating auxiliary devices with the id included as part of the
>> >> >> name. This allows for non-decimal ids, which may be more appropriate for
>> >> >> auxiliary devices created as children of memory-mapped devices. For
>> >> >> example, a name like "xilinx_emac.mac.802c0000" could be achieved by
>> >> >> setting .name to "mac.802c0000" and .id to AUXILIARY_DEVID_NONE.
>> >> > 
>> >> > I don't see the justification for this, sorry.  An id is just an id, it
>> >> > doesn't matter what is is and nothing should be relying on it to be the
>> >> > same across reboots or anywhere else.  The only requirement is that it
>> >> > be unique at this point in time in the system.
>> >> 
>> >> It identifies the device in log messages. Without this you have to read
>> >> sysfs to determine what device is (for example) producing an error.
>> > 
>> > That's fine, read sysfs :)
>> 
>> I should not have to read sysfs to decode boot output. If there is an
>> error during boot I should be able to determine the offending device.
>> This very important when the boot process fails before init is started,
>> and very convenient otherwise. 
> 
> The boot log will show you the name of the device that is having a
> problem.  And you get to pick a portion of that name to make it make
> some kind of sense to users if you want.

As noted below, I can't! The name has to be in a very particular format
which does not allow for any differentiation *except* in the "id" portion.
Really the only thing I want to do is print the id in hexadecimal rather
than decimal.

>> >> This
>> >> may be inconvenient to do if the error prevents the system from booting.
>> >> This series converts a platform device with a legible ID like
>> >> "802c0000.ethernet" to an auxiliary device, and I believe descriptive
>> >> device names produce a better developer experience.
>> > 
>> > You can still have 802c0000.ethernet be the prefix of the name, that's
>> > fine.
>> 
>> This is not possible due to how the auxiliary bus works. If device's
>> name is in the form "foo.id", then the driver must have an
>> auxiliary_device_id in its id_table with .name = "foo". So the address
>> *must* come after the last period in the name.
> 
> So what is the new name without this aux patch that looks so wrong?
> What is the current log line before and after the change you made?

Well, without this patch if you have multiple devices the subsequent
ones can't be created because they all have id 0 and this conflicts in sysfs.

With this patch it looks something like

[    4.781268] xilinx_axienet 80200000.ethernet: autodetected 64-bit DMA range
[   21.889563] xilinx_emac.mac xilinx_emac.mac.80200000 net4: renamed from eth0
[   32.296965] xilinx_emac.mac xilinx_emac.mac.80200000 net4: PHY [axienet-80200000:05] driver [RTL8211F Gigabit Ethernet] (irq=70)
[   32.313456] xilinx_emac.mac xilinx_emac.mac.80200000 net4: configuring for inband/sgmii link mode
[   65.095419] xilinx_emac.mac xilinx_emac.mac.80200000 net4: Link is Up - 1Gbps/Full - flow control rx/tx

I also prototyped a version of PLATFORM_DEVID_AUTO which looks roughly
like:

[    5.424220] xilinx_axienet 80240000.ethernet: autodetected 64-bit DMA range
[  178.249494] xilinx_emac.mac xilinx_emac.mac.1-auto net4: renamed from eth0
[  178.714048] xilinx_emac.mac xilinx_emac.mac.1-auto net4: PHY [axienet-80200000:05] driver [RTL8211F Gigabit Ethernet] (irq=70)
[  178.731272] xilinx_emac.mac xilinx_emac.mac.1-auto net4: configuring for inband/sgmii link mode
[  182.818831] xilinx_emac.mac xilinx_emac.mac.1-auto net4: Link is Up - 1Gbps/Full - flow control rx/tx

The only identifying part of the name is the "net4" part of the netdev.
However, if there's a failure before creating the netdev then userspace
will never have a chance to rename it. For example, you might get

[    4.947215] xilinx_emac.mac xilinx_emac.mac.1-auto (unnamed net_device) (uninitialized): incorrect link mode  for in-band status

which leaves you with no clue as to what device went wrong.

--Sean

Re: [PATCH net 0/4] net: axienet: Fix deferred probe loop

[Sean Anderson]

On 6/20/25 12:01, Greg Kroah-Hartman wrote:
> On Fri, Jun 20, 2025 at 11:41:52AM -0400, Sean Anderson wrote:
>> On 6/20/25 01:10, Greg Kroah-Hartman wrote:
>> > On Thu, Jun 19, 2025 at 04:05:33PM -0400, Sean Anderson wrote:
>> >> Upon further investigation, the EPROBE_DEFER loop outlined in [1] can
>> >> occur even without the PCS subsystem, as described in patch 4/4. The
>> >> second patch is a general fix, and could be applied even without the
>> >> auxdev conversion.
>> >> 
>> >> [1] https://lore.kernel.org/all/20250610183459.3395328-1-sean.anderson@linux.dev/
>> > 
>> > I have no idea what this summary means at all, which isn't a good start
>> > to a patch series :(
>> > 
>> > What problem are you trying to solve?
>> 
>> See patch 4/4.
> 
> That's not what should be in patch 0/4 then, right?
> 
>> > What overall solution did you come up with?
>> 
>> See patch 4/4.
> 
> Again, why write a 0/4 summary at all then?

So if I decide in v2 that some patch other than "auxiliary: Allow empty
id" has to come first then the series still has the same subject. This
makes it easier for maintainers to figure out which v1 the v2 is for.

>> > Who is supposed to be reviewing any of this?
>> 
>> Netdev. Hence "PATCH net".
>> 
>> And see [1] above for background. I will quote it more-extensively next time.
> 
> Referring to random links doesn't always work as we deal with thousands
> of patches daily, and sometimes don't even have internet access (like
> when reviewing patches on long flights/train rides...)

Well, the link contains the message-id, so you are more than welcome to
look it up in your email client. But to spare you the trouble I will
quote from it next time in addition to linking.

--Sean

Re: [PATCH net 4/4] net: axienet: Split into MAC and MDIO drivers

[Andrew Lunn]

On Thu, Jun 19, 2025 at 04:05:37PM -0400, Sean Anderson wrote:
> Returning EPROBE_DEFER after probing a bus may result in an infinite
> probe loop if the EPROBE_DEFER error is never resolved.

That sounds like a core problem. I also thought there was a time
limit, how long the system will repeat probes for drivers which defer.

This seems like the wrong fix to me.

	Andrew

Re: [PATCH net 4/4] net: axienet: Split into MAC and MDIO drivers

[Sean Anderson]

On 6/21/25 03:33, Andrew Lunn wrote:
> On Thu, Jun 19, 2025 at 04:05:37PM -0400, Sean Anderson wrote:
>> Returning EPROBE_DEFER after probing a bus may result in an infinite
>> probe loop if the EPROBE_DEFER error is never resolved.
> 
> That sounds like a core problem. I also thought there was a time
> limit, how long the system will repeat probes for drivers which defer.
> 
> This seems like the wrong fix to me.

I agree. My first attempt to fix this did so by ignoring deferred probes
from child devices, which would prevent "recursive" loops like this one
[1]. But I was informed that failing with EPROBE_DEFER after creating a
bus was not allowed at all, hence this patch.

Limiting the number of deferred probe attempts (at least before
continuing to boot) is a good idea in theory, but would not address the
root of the issue. Setting a good threshold is not obvious, since there
are almost certainly systems out there that are missing some device
links and have a lot of deferred probes. 

--Sean

[1] https://lore.kernel.org/all/CAGETcx-koKBvSXTHChYYF-qSU-r1cBUbLghJZcqtJOGQZjn3BA@mail.gmail.com/

Re: [PATCH net 4/4] net: axienet: Split into MAC and MDIO drivers

[Andrew Lunn]

On Mon, Jun 23, 2025 at 11:16:08AM -0400, Sean Anderson wrote:
> On 6/21/25 03:33, Andrew Lunn wrote:
> > On Thu, Jun 19, 2025 at 04:05:37PM -0400, Sean Anderson wrote:
> >> Returning EPROBE_DEFER after probing a bus may result in an infinite
> >> probe loop if the EPROBE_DEFER error is never resolved.
> > 
> > That sounds like a core problem. I also thought there was a time
> > limit, how long the system will repeat probes for drivers which defer.
> > 
> > This seems like the wrong fix to me.
> 
> I agree. My first attempt to fix this did so by ignoring deferred probes
> from child devices, which would prevent "recursive" loops like this one
> [1]. But I was informed that failing with EPROBE_DEFER after creating a
> bus was not allowed at all, hence this patch.

O.K. So why not change the order so that you know you have all the
needed dependencies before registering the MDIO bus?

Quoting your previous email:

> Returning EPROBE_DEFER after probing a bus may result in an infinite
> probe loop if the EPROBE_DEFER error is never resolved. For example,
> if the PCS is located on another MDIO bus and that MDIO bus is
> missing its driver then we will always return EPROBE_DEFER.

Why not get a reference on the PCS device before registering the MDIO
bus?

	Andrew

Re: [PATCH net 4/4] net: axienet: Split into MAC and MDIO drivers

[Sean Anderson]

On 6/23/25 14:27, Andrew Lunn wrote:
> On Mon, Jun 23, 2025 at 11:16:08AM -0400, Sean Anderson wrote:
>> On 6/21/25 03:33, Andrew Lunn wrote:
>> > On Thu, Jun 19, 2025 at 04:05:37PM -0400, Sean Anderson wrote:
>> >> Returning EPROBE_DEFER after probing a bus may result in an infinite
>> >> probe loop if the EPROBE_DEFER error is never resolved.
>> > 
>> > That sounds like a core problem. I also thought there was a time
>> > limit, how long the system will repeat probes for drivers which defer.
>> > 
>> > This seems like the wrong fix to me.
>> 
>> I agree. My first attempt to fix this did so by ignoring deferred probes
>> from child devices, which would prevent "recursive" loops like this one
>> [1]. But I was informed that failing with EPROBE_DEFER after creating a
>> bus was not allowed at all, hence this patch.
> 
> O.K. So why not change the order so that you know you have all the
> needed dependencies before registering the MDIO bus?
> 
> Quoting your previous email:
> 
>> Returning EPROBE_DEFER after probing a bus may result in an infinite
>> probe loop if the EPROBE_DEFER error is never resolved. For example,
>> if the PCS is located on another MDIO bus and that MDIO bus is
>> missing its driver then we will always return EPROBE_DEFER.
> 
> Why not get a reference on the PCS device before registering the MDIO
> bus?

Because the PCS may be on the MDIO bus. This is probably the most-common
case.

--Sean

Re: [PATCH net 4/4] net: axienet: Split into MAC and MDIO drivers

[Andrew Lunn]

On Mon, Jun 23, 2025 at 02:48:53PM -0400, Sean Anderson wrote:
> On 6/23/25 14:27, Andrew Lunn wrote:
> > On Mon, Jun 23, 2025 at 11:16:08AM -0400, Sean Anderson wrote:
> >> On 6/21/25 03:33, Andrew Lunn wrote:
> >> > On Thu, Jun 19, 2025 at 04:05:37PM -0400, Sean Anderson wrote:
> >> >> Returning EPROBE_DEFER after probing a bus may result in an infinite
> >> >> probe loop if the EPROBE_DEFER error is never resolved.
> >> > 
> >> > That sounds like a core problem. I also thought there was a time
> >> > limit, how long the system will repeat probes for drivers which defer.
> >> > 
> >> > This seems like the wrong fix to me.
> >> 
> >> I agree. My first attempt to fix this did so by ignoring deferred probes
> >> from child devices, which would prevent "recursive" loops like this one
> >> [1]. But I was informed that failing with EPROBE_DEFER after creating a
> >> bus was not allowed at all, hence this patch.
> > 
> > O.K. So why not change the order so that you know you have all the
> > needed dependencies before registering the MDIO bus?
> > 
> > Quoting your previous email:
> > 
> >> Returning EPROBE_DEFER after probing a bus may result in an infinite
> >> probe loop if the EPROBE_DEFER error is never resolved. For example,
> >> if the PCS is located on another MDIO bus and that MDIO bus is
> >> missing its driver then we will always return EPROBE_DEFER.
> > 
> > Why not get a reference on the PCS device before registering the MDIO
> > bus?
> 
> Because the PCS may be on the MDIO bus. This is probably the most-common
> case.

So you are saying the PCS is physically there, but the driver is
missing because of configuration errors? Then it sounds like a kconfig
issue?

Or are you saying the driver has been built but then removed from
/lib/modules/

	Andrew

Re: [PATCH net 4/4] net: axienet: Split into MAC and MDIO drivers

[Sean Anderson]

On 6/23/25 18:45, Andrew Lunn wrote:
> On Mon, Jun 23, 2025 at 02:48:53PM -0400, Sean Anderson wrote:
>> On 6/23/25 14:27, Andrew Lunn wrote:
>> > On Mon, Jun 23, 2025 at 11:16:08AM -0400, Sean Anderson wrote:
>> >> On 6/21/25 03:33, Andrew Lunn wrote:
>> >> > On Thu, Jun 19, 2025 at 04:05:37PM -0400, Sean Anderson wrote:
>> >> >> Returning EPROBE_DEFER after probing a bus may result in an infinite
>> >> >> probe loop if the EPROBE_DEFER error is never resolved.
>> >> > 
>> >> > That sounds like a core problem. I also thought there was a time
>> >> > limit, how long the system will repeat probes for drivers which defer.
>> >> > 
>> >> > This seems like the wrong fix to me.
>> >> 
>> >> I agree. My first attempt to fix this did so by ignoring deferred probes
>> >> from child devices, which would prevent "recursive" loops like this one
>> >> [1]. But I was informed that failing with EPROBE_DEFER after creating a
>> >> bus was not allowed at all, hence this patch.
>> > 
>> > O.K. So why not change the order so that you know you have all the
>> > needed dependencies before registering the MDIO bus?
>> > 
>> > Quoting your previous email:
>> > 
>> >> Returning EPROBE_DEFER after probing a bus may result in an infinite
>> >> probe loop if the EPROBE_DEFER error is never resolved. For example,
>> >> if the PCS is located on another MDIO bus and that MDIO bus is
>> >> missing its driver then we will always return EPROBE_DEFER.
>> > 
>> > Why not get a reference on the PCS device before registering the MDIO
>> > bus?
>> 
>> Because the PCS may be on the MDIO bus. This is probably the most-common
>> case.
> 
> So you are saying the PCS is physically there, but the driver is
> missing because of configuration errors? Then it sounds like a kconfig
> issue?
> 
> Or are you saying the driver has been built but then removed from
> /lib/modules/

The latter. Or maybe someone just forgot to install it (or include it
with their initramfs). Or maybe there was some error with the MDIO bus.

There are two mutually-exclusive scenarios (that can both occur in the
same system). First, the PCS can be attached to our own MDIO bus:

MAC
 |
 +->MDIO
     |
     +->PCS
     +->PHY (etc)

In this scenario, we have to probe the MDIO bus before we can look up
the PCS, since otherwise the PCS will always be missing when we look for
it. But if we do things in the right order then we can't get
EPROBE_DEFER, and so there's no risk of a probe loop.

Second, the PCS can be attached to some other MDIO bus:

MAC              MDIO
 |                 |
 +->MDIO           +->PCS
      |
      +->PHY (etc)

In this scenario, the MDIO bus might not be present for whatever reason
and we have the possibility of an EPROBE_DEFER error. If that happens,
we will end up in a probe loop because the PHY on the MDIO bus
incremented deferred_trigger_count when it probed successfully:

deferred_probe_work_func()
  driver_probe_device(MAC)
    axienet_probe(MAC)
      mdiobus_register(MDIO)
        device_add(PHY)
          (probe successful)
          driver_bound(PHY)
            driver_deferred_probe_trigger()
      return -EPROBE_DEFER
    driver_deferred_probe_add(MAC)
    // deferred_trigger_count changed, so...
    driver_deferred_probe_trigger()

--Sean 

Re: [PATCH net 4/4] net: axienet: Split into MAC and MDIO drivers

[Sean Anderson]

Hi Andrew,

On 6/23/25 19:16, Sean Anderson wrote:
> On 6/23/25 18:45, Andrew Lunn wrote:
>> On Mon, Jun 23, 2025 at 02:48:53PM -0400, Sean Anderson wrote:
>>> On 6/23/25 14:27, Andrew Lunn wrote:
>>> > On Mon, Jun 23, 2025 at 11:16:08AM -0400, Sean Anderson wrote:
>>> >> On 6/21/25 03:33, Andrew Lunn wrote:
>>> >> > On Thu, Jun 19, 2025 at 04:05:37PM -0400, Sean Anderson wrote:
>>> >> >> Returning EPROBE_DEFER after probing a bus may result in an infinite
>>> >> >> probe loop if the EPROBE_DEFER error is never resolved.
>>> >> > 
>>> >> > That sounds like a core problem. I also thought there was a time
>>> >> > limit, how long the system will repeat probes for drivers which defer.
>>> >> > 
>>> >> > This seems like the wrong fix to me.
>>> >> 
>>> >> I agree. My first attempt to fix this did so by ignoring deferred probes
>>> >> from child devices, which would prevent "recursive" loops like this one
>>> >> [1]. But I was informed that failing with EPROBE_DEFER after creating a
>>> >> bus was not allowed at all, hence this patch.
>>> > 
>>> > O.K. So why not change the order so that you know you have all the
>>> > needed dependencies before registering the MDIO bus?
>>> > 
>>> > Quoting your previous email:
>>> > 
>>> >> Returning EPROBE_DEFER after probing a bus may result in an infinite
>>> >> probe loop if the EPROBE_DEFER error is never resolved. For example,
>>> >> if the PCS is located on another MDIO bus and that MDIO bus is
>>> >> missing its driver then we will always return EPROBE_DEFER.
>>> > 
>>> > Why not get a reference on the PCS device before registering the MDIO
>>> > bus?
>>> 
>>> Because the PCS may be on the MDIO bus. This is probably the most-common
>>> case.
>> 
>> So you are saying the PCS is physically there, but the driver is
>> missing because of configuration errors? Then it sounds like a kconfig
>> issue?
>> 
>> Or are you saying the driver has been built but then removed from
>> /lib/modules/
> 
> The latter. Or maybe someone just forgot to install it (or include it
> with their initramfs). Or maybe there was some error with the MDIO bus.
> 
> There are two mutually-exclusive scenarios (that can both occur in the
> same system). First, the PCS can be attached to our own MDIO bus:
> 
> MAC
>  |
>  +->MDIO
>      |
>      +->PCS
>      +->PHY (etc)
> 
> In this scenario, we have to probe the MDIO bus before we can look up
> the PCS, since otherwise the PCS will always be missing when we look for
> it. But if we do things in the right order then we can't get
> EPROBE_DEFER, and so there's no risk of a probe loop.
> 
> Second, the PCS can be attached to some other MDIO bus:
> 
> MAC              MDIO
>  |                 |
>  +->MDIO           +->PCS
>       |
>       +->PHY (etc)
> 
> In this scenario, the MDIO bus might not be present for whatever reason
> and we have the possibility of an EPROBE_DEFER error. If that happens,
> we will end up in a probe loop because the PHY on the MDIO bus
> incremented deferred_trigger_count when it probed successfully:
> 
> deferred_probe_work_func()
>   driver_probe_device(MAC)
>     axienet_probe(MAC)
>       mdiobus_register(MDIO)
>         device_add(PHY)
>           (probe successful)
>           driver_bound(PHY)
>             driver_deferred_probe_trigger()
>       return -EPROBE_DEFER
>     driver_deferred_probe_add(MAC)
>     // deferred_trigger_count changed, so...
>     driver_deferred_probe_trigger()

Does the above scenario make sense? As I see it, the only approaches are

- Modify the driver core to detect and mitigate this sort of scenario
  (NACKed by Greg).
- Split the driver into MAC and MDIO parts (this patch).
- Modify phylink to allow connecting a PCS after phylink_create but
  before phylink_start. This is tricky because the PCS can affect the
  supported phy interfaces, and phy interfaces are validated in
  phylink_create.
- Defer phylink_create to ndo_open. This means that all the
  netdev/ethtool ops that use phylink now need to check ip the netdev is
  open and fall back to some other implementation. I don't think we can
  just return -EINVAL or whatever because using ethtool on a down device
  has historically worked. I am wary of breaking userspace because some
  tool assumes it can get_ksettings while the netdev is down.

Do you see any other options? IMO, aside from the first option, the
second one has the best UX. With the latter two, you could have a netdev
that never comes up and the user may not have very good insight as to
why. E.g. it may not be obvious that the user should try to bring the
netdev up again after the PCS is probed. By waiting to create the netdev
until after we successfully probe the PCS we show up in
devices_deferred and the netdev can be brought up as usual.

--Sean