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

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

[Sean Anderson]

Please refer to patch 4/4 for an extended look at the problem this
series is attempting to solve.

Changes in v2:
- Add example log output to commit message
- Fix building as a module
- Expand commit message with much more info on the problem and possible
  solutions

Sean Anderson (4):
  auxiliary: Support hexadecimal ids
  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 | 422 ++++++++++--------
 .../net/ethernet/xilinx/xilinx_axienet_mdio.c |  31 +-
 include/linux/auxiliary_bus.h                 |   4 +-
 5 files changed, 263 insertions(+), 201 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty

[PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Sean Anderson]

Support creating auxiliary devices with the id included as part of the
name. This allows for hexadecimal ids, which may be more appropriate for
auxiliary devices created as children of memory-mapped devices. If an
auxiliary device's id is set to AUXILIARY_DEVID_NONE, the name must
be of the form "name.id".

With this patch, dmesg logs from an auxiliary device might look 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

this is especially useful when compared to what might happen if there is
an error before userspace has the chance to assign a name to the netdev:

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

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

Changes in v2:
- Add example log output to commit message

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

(no changes since v1)

 .../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 v2 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>
---

(no changes since v1)

 .../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 v2 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. 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
(module not loaded, error in probe, etc.) 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 our own 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()

As I see it, this problem could be solved in the following four ways:

- Modify the driver core to detect and mitigate this sort of scenario
  (NACKed by Greg).
- Split the driver into MAC and MDIO parts (this commit).
- 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.

Aside from the first option, the second one (this commit) 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. For example, 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.

Per the second bullet point above, split the MAC and MDIO functionality
into separate auxiliary devices. If the MAC fails with EPROBE_DEFER,
then the MDIO bus will remain bound, preventing a probe loop.

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

Changes in v2:
- Fix building as a module
- Expand commit message with much more info on the problem and possible
  solutions

 drivers/net/ethernet/xilinx/Kconfig           |   1 +
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 107 ++++++++++++------
 .../net/ethernet/xilinx/xilinx_axienet_mdio.c |  31 +++--
 3 files changed, 98 insertions(+), 41 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..0638c4cafd4f 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,23 @@ 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,
+	},
+};
+
 /**
  * axienet_probe - Axi Ethernet probe function.
  * @pdev:	Pointer to platform device structure.
@@ -2892,12 +2913,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 +2929,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,41 +3196,62 @@ 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,
 	},
 };
 
-module_platform_driver(axienet_driver);
+extern struct auxiliary_driver xilinx_axienet_mdio;
+
+static int __init axienet_init(void)
+{
+	int ret;
+
+	ret = auxiliary_driver_register(&xilinx_axienet_mdio);
+	if (ret)
+		return ret;
+
+	ret = auxiliary_driver_register(&xilinx_axienet_mac);
+	if (ret)
+		goto unregister_mdio;
+
+	ret = platform_driver_register(&axienet_driver);
+	if (ret) {
+		auxiliary_driver_unregister(&xilinx_axienet_mac);
+unregister_mdio:
+		auxiliary_driver_unregister(&xilinx_axienet_mdio);
+	}
+	return ret;
+}
+module_init(axienet_init);
+
+static void __exit axienet_exit(void)
+{
+	platform_driver_register(&axienet_driver);
+	auxiliary_driver_unregister(&xilinx_axienet_mac);
+	auxiliary_driver_unregister(&xilinx_axienet_mdio);
+}
+module_exit(axienet_exit);
 
 MODULE_DESCRIPTION("Xilinx Axi Ethernet driver");
 MODULE_AUTHOR("Xilinx");
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c b/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c
index 9ca2643c921e..85792da17d0c 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,24 @@ 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);
+
+struct auxiliary_driver xilinx_axienet_mdio = {
+	.name = "mdio",
+	.id_table = xilinx_axienet_mdio_id_table,
+	.probe = axienet_mdio_probe,
+	.remove = axienet_mdio_remove,
+};
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Greg Kroah-Hartman]

On Tue, Jul 15, 2025 at 08:01:07PM -0400, Sean Anderson wrote:
> Support creating auxiliary devices with the id included as part of the
> name. This allows for hexadecimal ids, which may be more appropriate for
> auxiliary devices created as children of memory-mapped devices. If an
> auxiliary device's id is set to AUXILIARY_DEVID_NONE, the name must
> be of the form "name.id".
> 
> With this patch, dmesg logs from an auxiliary device might look 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
> 
> this is especially useful when compared to what might happen if there is
> an error before userspace has the chance to assign a name to the netdev:
> 
> [    4.947215] xilinx_emac.mac xilinx_emac.mac.1 (unnamed net_device) (uninitialized): incorrect link mode  for in-band status
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
> Changes in v2:
> - Add example log output to commit message

I rejected v1, why is this being sent again?

confused,

greg k-h

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Sean Anderson]

On 7/16/25 01:09, Greg Kroah-Hartman wrote:
> On Tue, Jul 15, 2025 at 08:01:07PM -0400, Sean Anderson wrote:
>> Support creating auxiliary devices with the id included as part of the
>> name. This allows for hexadecimal ids, which may be more appropriate for
>> auxiliary devices created as children of memory-mapped devices. If an
>> auxiliary device's id is set to AUXILIARY_DEVID_NONE, the name must
>> be of the form "name.id".
>> 
>> With this patch, dmesg logs from an auxiliary device might look 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
>> 
>> this is especially useful when compared to what might happen if there is
>> an error before userspace has the chance to assign a name to the netdev:
>> 
>> [    4.947215] xilinx_emac.mac xilinx_emac.mac.1 (unnamed net_device) (uninitialized): incorrect link mode  for in-band status
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>> 
>> Changes in v2:
>> - Add example log output to commit message
> 
> I rejected v1, why is this being sent again?

You asked for explanation, I provided it. I specifically pointed out why
I wanted to do things this way. But I got no response. So here in v2.

--Sean

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Greg Kroah-Hartman]

On Thu, Jul 17, 2025 at 11:49:37AM -0400, Sean Anderson wrote:
> On 7/16/25 01:09, Greg Kroah-Hartman wrote:
> > On Tue, Jul 15, 2025 at 08:01:07PM -0400, Sean Anderson wrote:
> >> Support creating auxiliary devices with the id included as part of the
> >> name. This allows for hexadecimal ids, which may be more appropriate for
> >> auxiliary devices created as children of memory-mapped devices. If an
> >> auxiliary device's id is set to AUXILIARY_DEVID_NONE, the name must
> >> be of the form "name.id".
> >> 
> >> With this patch, dmesg logs from an auxiliary device might look 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
> >> 
> >> this is especially useful when compared to what might happen if there is
> >> an error before userspace has the chance to assign a name to the netdev:
> >> 
> >> [    4.947215] xilinx_emac.mac xilinx_emac.mac.1 (unnamed net_device) (uninitialized): incorrect link mode  for in-band status
> >> 
> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> >> ---
> >> 
> >> Changes in v2:
> >> - Add example log output to commit message
> > 
> > I rejected v1, why is this being sent again?
> 
> You asked for explanation, I provided it. I specifically pointed out why
> I wanted to do things this way. But I got no response. So here in v2.

Again, I said, "do not do that, this is not how ids work in the driver
model", and you tried to show lots of reasons why you wanted to do it
this way despite me saying so.

So again, no, sorry, this isn't ok.  Don't attempt to encode information
in a device id like you are trying to do here, that's not what a device
id is for at all.  I need to go dig up my old patch that made all device
ids random numbers just to see what foolish assumptions busses and
userspace tools are making....

thanks,

greg k-h

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Sean Anderson]

On 7/17/25 11:59, Greg Kroah-Hartman wrote:
> On Thu, Jul 17, 2025 at 11:49:37AM -0400, Sean Anderson wrote:
>> On 7/16/25 01:09, Greg Kroah-Hartman wrote:
>> > On Tue, Jul 15, 2025 at 08:01:07PM -0400, Sean Anderson wrote:
>> >> Support creating auxiliary devices with the id included as part of the
>> >> name. This allows for hexadecimal ids, which may be more appropriate for
>> >> auxiliary devices created as children of memory-mapped devices. If an
>> >> auxiliary device's id is set to AUXILIARY_DEVID_NONE, the name must
>> >> be of the form "name.id".
>> >> 
>> >> With this patch, dmesg logs from an auxiliary device might look 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
>> >> 
>> >> this is especially useful when compared to what might happen if there is
>> >> an error before userspace has the chance to assign a name to the netdev:
>> >> 
>> >> [    4.947215] xilinx_emac.mac xilinx_emac.mac.1 (unnamed net_device) (uninitialized): incorrect link mode  for in-band status
>> >> 
>> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> >> ---
>> >> 
>> >> Changes in v2:
>> >> - Add example log output to commit message
>> > 
>> > I rejected v1, why is this being sent again?
>> 
>> You asked for explanation, I provided it. I specifically pointed out why
>> I wanted to do things this way. But I got no response. So here in v2.
> 
> Again, I said, "do not do that, this is not how ids work in the driver
> model", and you tried to show lots of reasons why you wanted to do it
> this way despite me saying so.
> 
> So again, no, sorry, this isn't ok.  Don't attempt to encode information
> in a device id like you are trying to do here, that's not what a device
> id is for at all.  I need to go dig up my old patch that made all device
> ids random numbers just to see what foolish assumptions busses and
> userspace tools are making....

But it *is* how ids work in platform devices. And because my auxiliary
devices are created by a platform device, it is guaranteed that the
platform device id is unique and that it will also be unique for
auxiliary devices. So there is no assumption here about the uniqueness
of any given id.

--Sean

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Greg Kroah-Hartman]

On Thu, Jul 17, 2025 at 12:04:15PM -0400, Sean Anderson wrote:
> On 7/17/25 11:59, Greg Kroah-Hartman wrote:
> > On Thu, Jul 17, 2025 at 11:49:37AM -0400, Sean Anderson wrote:
> >> On 7/16/25 01:09, Greg Kroah-Hartman wrote:
> >> > On Tue, Jul 15, 2025 at 08:01:07PM -0400, Sean Anderson wrote:
> >> >> Support creating auxiliary devices with the id included as part of the
> >> >> name. This allows for hexadecimal ids, which may be more appropriate for
> >> >> auxiliary devices created as children of memory-mapped devices. If an
> >> >> auxiliary device's id is set to AUXILIARY_DEVID_NONE, the name must
> >> >> be of the form "name.id".
> >> >> 
> >> >> With this patch, dmesg logs from an auxiliary device might look 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
> >> >> 
> >> >> this is especially useful when compared to what might happen if there is
> >> >> an error before userspace has the chance to assign a name to the netdev:
> >> >> 
> >> >> [    4.947215] xilinx_emac.mac xilinx_emac.mac.1 (unnamed net_device) (uninitialized): incorrect link mode  for in-band status
> >> >> 
> >> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> >> >> ---
> >> >> 
> >> >> Changes in v2:
> >> >> - Add example log output to commit message
> >> > 
> >> > I rejected v1, why is this being sent again?
> >> 
> >> You asked for explanation, I provided it. I specifically pointed out why
> >> I wanted to do things this way. But I got no response. So here in v2.
> > 
> > Again, I said, "do not do that, this is not how ids work in the driver
> > model", and you tried to show lots of reasons why you wanted to do it
> > this way despite me saying so.
> > 
> > So again, no, sorry, this isn't ok.  Don't attempt to encode information
> > in a device id like you are trying to do here, that's not what a device
> > id is for at all.  I need to go dig up my old patch that made all device
> > ids random numbers just to see what foolish assumptions busses and
> > userspace tools are making....
> 
> But it *is* how ids work in platform devices.

No one should ever use platform devices/bus as an excuse to do anything,
it's "wrong" in so many ways, but needs to be because of special
reasons.  No other bus should work like that, sorry.

> And because my auxiliary
> devices are created by a platform device, it is guaranteed that the
> platform device id is unique and that it will also be unique for
> auxiliary devices. So there is no assumption here about the uniqueness
> of any given id.

Then perhaps use the faux device api instead?

thanks,

greg k-h

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Sean Anderson]

On 7/17/25 12:21, Greg Kroah-Hartman wrote:
> On Thu, Jul 17, 2025 at 12:04:15PM -0400, Sean Anderson wrote:
>> On 7/17/25 11:59, Greg Kroah-Hartman wrote:
>> > On Thu, Jul 17, 2025 at 11:49:37AM -0400, Sean Anderson wrote:
>> >> On 7/16/25 01:09, Greg Kroah-Hartman wrote:
>> >> > On Tue, Jul 15, 2025 at 08:01:07PM -0400, Sean Anderson wrote:
>> >> >> Support creating auxiliary devices with the id included as part of the
>> >> >> name. This allows for hexadecimal ids, which may be more appropriate for
>> >> >> auxiliary devices created as children of memory-mapped devices. If an
>> >> >> auxiliary device's id is set to AUXILIARY_DEVID_NONE, the name must
>> >> >> be of the form "name.id".
>> >> >> 
>> >> >> With this patch, dmesg logs from an auxiliary device might look 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
>> >> >> 
>> >> >> this is especially useful when compared to what might happen if there is
>> >> >> an error before userspace has the chance to assign a name to the netdev:
>> >> >> 
>> >> >> [    4.947215] xilinx_emac.mac xilinx_emac.mac.1 (unnamed net_device) (uninitialized): incorrect link mode  for in-band status
>> >> >> 
>> >> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> >> >> ---
>> >> >> 
>> >> >> Changes in v2:
>> >> >> - Add example log output to commit message
>> >> > 
>> >> > I rejected v1, why is this being sent again?
>> >> 
>> >> You asked for explanation, I provided it. I specifically pointed out why
>> >> I wanted to do things this way. But I got no response. So here in v2.
>> > 
>> > Again, I said, "do not do that, this is not how ids work in the driver
>> > model", and you tried to show lots of reasons why you wanted to do it
>> > this way despite me saying so.
>> > 
>> > So again, no, sorry, this isn't ok.  Don't attempt to encode information
>> > in a device id like you are trying to do here, that's not what a device
>> > id is for at all.  I need to go dig up my old patch that made all device
>> > ids random numbers just to see what foolish assumptions busses and
>> > userspace tools are making....
>> 
>> But it *is* how ids work in platform devices.
> 
> No one should ever use platform devices/bus as an excuse to do anything,
> it's "wrong" in so many ways, but needs to be because of special
> reasons.  No other bus should work like that, sorry.
> 
>> And because my auxiliary
>> devices are created by a platform device, it is guaranteed that the
>> platform device id is unique and that it will also be unique for
>> auxiliary devices. So there is no assumption here about the uniqueness
>> of any given id.
> 
> Then perhaps use the faux device api instead?

There's *another* pseudo bus? OK the reason why is that faux was added
four months ago and there is nothing under Documentation for it. So I
had no idea it existed. I will have a look, but perhaps you should write
up some documentation about why someone might want to use a "faux" bus
over the auxiliary bus or MFD.

--Sean

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Greg Kroah-Hartman]

On Thu, Jul 17, 2025 at 12:27:44PM -0400, Sean Anderson wrote:
> On 7/17/25 12:21, Greg Kroah-Hartman wrote:
> > On Thu, Jul 17, 2025 at 12:04:15PM -0400, Sean Anderson wrote:
> >> On 7/17/25 11:59, Greg Kroah-Hartman wrote:
> >> > On Thu, Jul 17, 2025 at 11:49:37AM -0400, Sean Anderson wrote:
> >> >> On 7/16/25 01:09, Greg Kroah-Hartman wrote:
> >> >> > On Tue, Jul 15, 2025 at 08:01:07PM -0400, Sean Anderson wrote:
> >> >> >> Support creating auxiliary devices with the id included as part of the
> >> >> >> name. This allows for hexadecimal ids, which may be more appropriate for
> >> >> >> auxiliary devices created as children of memory-mapped devices. If an
> >> >> >> auxiliary device's id is set to AUXILIARY_DEVID_NONE, the name must
> >> >> >> be of the form "name.id".
> >> >> >> 
> >> >> >> With this patch, dmesg logs from an auxiliary device might look 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
> >> >> >> 
> >> >> >> this is especially useful when compared to what might happen if there is
> >> >> >> an error before userspace has the chance to assign a name to the netdev:
> >> >> >> 
> >> >> >> [    4.947215] xilinx_emac.mac xilinx_emac.mac.1 (unnamed net_device) (uninitialized): incorrect link mode  for in-band status
> >> >> >> 
> >> >> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> >> >> >> ---
> >> >> >> 
> >> >> >> Changes in v2:
> >> >> >> - Add example log output to commit message
> >> >> > 
> >> >> > I rejected v1, why is this being sent again?
> >> >> 
> >> >> You asked for explanation, I provided it. I specifically pointed out why
> >> >> I wanted to do things this way. But I got no response. So here in v2.
> >> > 
> >> > Again, I said, "do not do that, this is not how ids work in the driver
> >> > model", and you tried to show lots of reasons why you wanted to do it
> >> > this way despite me saying so.
> >> > 
> >> > So again, no, sorry, this isn't ok.  Don't attempt to encode information
> >> > in a device id like you are trying to do here, that's not what a device
> >> > id is for at all.  I need to go dig up my old patch that made all device
> >> > ids random numbers just to see what foolish assumptions busses and
> >> > userspace tools are making....
> >> 
> >> But it *is* how ids work in platform devices.
> > 
> > No one should ever use platform devices/bus as an excuse to do anything,
> > it's "wrong" in so many ways, but needs to be because of special
> > reasons.  No other bus should work like that, sorry.
> > 
> >> And because my auxiliary
> >> devices are created by a platform device, it is guaranteed that the
> >> platform device id is unique and that it will also be unique for
> >> auxiliary devices. So there is no assumption here about the uniqueness
> >> of any given id.
> > 
> > Then perhaps use the faux device api instead?
> 
> There's *another* pseudo bus? OK the reason why is that faux was added
> four months ago and there is nothing under Documentation for it. So I
> had no idea it existed. I will have a look, but perhaps you should write
> up some documentation about why someone might want to use a "faux" bus
> over the auxiliary bus or MFD.

"faux" is for when platform devices were being abused because someone
just wanted a device in the device tree, and did not use any of the
platform device resources.

Yes, more documentation is always a good idea, someday...

thanks,

greg k-h

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Sean Anderson]

On 7/17/25 12:33, Greg Kroah-Hartman wrote:
> On Thu, Jul 17, 2025 at 12:27:44PM -0400, Sean Anderson wrote:
>> On 7/17/25 12:21, Greg Kroah-Hartman wrote:
>> > On Thu, Jul 17, 2025 at 12:04:15PM -0400, Sean Anderson wrote:
>> >> On 7/17/25 11:59, Greg Kroah-Hartman wrote:
>> >> > On Thu, Jul 17, 2025 at 11:49:37AM -0400, Sean Anderson wrote:
>> >> >> On 7/16/25 01:09, Greg Kroah-Hartman wrote:
>> >> >> > On Tue, Jul 15, 2025 at 08:01:07PM -0400, Sean Anderson wrote:
>> >> >> >> Support creating auxiliary devices with the id included as part of the
>> >> >> >> name. This allows for hexadecimal ids, which may be more appropriate for
>> >> >> >> auxiliary devices created as children of memory-mapped devices. If an
>> >> >> >> auxiliary device's id is set to AUXILIARY_DEVID_NONE, the name must
>> >> >> >> be of the form "name.id".
>> >> >> >> 
>> >> >> >> With this patch, dmesg logs from an auxiliary device might look 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
>> >> >> >> 
>> >> >> >> this is especially useful when compared to what might happen if there is
>> >> >> >> an error before userspace has the chance to assign a name to the netdev:
>> >> >> >> 
>> >> >> >> [    4.947215] xilinx_emac.mac xilinx_emac.mac.1 (unnamed net_device) (uninitialized): incorrect link mode  for in-band status
>> >> >> >> 
>> >> >> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> >> >> >> ---
>> >> >> >> 
>> >> >> >> Changes in v2:
>> >> >> >> - Add example log output to commit message
>> >> >> > 
>> >> >> > I rejected v1, why is this being sent again?
>> >> >> 
>> >> >> You asked for explanation, I provided it. I specifically pointed out why
>> >> >> I wanted to do things this way. But I got no response. So here in v2.
>> >> > 
>> >> > Again, I said, "do not do that, this is not how ids work in the driver
>> >> > model", and you tried to show lots of reasons why you wanted to do it
>> >> > this way despite me saying so.
>> >> > 
>> >> > So again, no, sorry, this isn't ok.  Don't attempt to encode information
>> >> > in a device id like you are trying to do here, that's not what a device
>> >> > id is for at all.  I need to go dig up my old patch that made all device
>> >> > ids random numbers just to see what foolish assumptions busses and
>> >> > userspace tools are making....
>> >> 
>> >> But it *is* how ids work in platform devices.
>> > 
>> > No one should ever use platform devices/bus as an excuse to do anything,
>> > it's "wrong" in so many ways, but needs to be because of special
>> > reasons.  No other bus should work like that, sorry.
>> > 
>> >> And because my auxiliary
>> >> devices are created by a platform device, it is guaranteed that the
>> >> platform device id is unique and that it will also be unique for
>> >> auxiliary devices. So there is no assumption here about the uniqueness
>> >> of any given id.
>> > 
>> > Then perhaps use the faux device api instead?
>> 
>> There's *another* pseudo bus? OK the reason why is that faux was added
>> four months ago and there is nothing under Documentation for it. So I
>> had no idea it existed. I will have a look, but perhaps you should write
>> up some documentation about why someone might want to use a "faux" bus
>> over the auxiliary bus or MFD.
> 
> "faux" is for when platform devices were being abused because someone
> just wanted a device in the device tree, and did not use any of the
> platform device resources.

OK, well that's not this. These are real devices and there may be more
than one per system. Actually, that's the primary problem I wanted to
address with this patch: you can't create more than one device with
devm_auxiliary_device_create because they will have the same id (0).

Anyway, if you really think ids should be random or whatever, why not
just ida_alloc one in axiliary_device_init and ignore whatever's
provided? I'd say around half the auxiliary drivers just use 0 (or some
other constant), which is just as deterministic as using the device
address. Another third use ida_alloc (or xa_alloc) so all that could be
removed. And the remainder just use an address of some kind (which ends
up be formatted as decimal).

--Sean

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Leon Romanovsky]

On Thu, Jul 17, 2025 at 01:12:08PM -0400, Sean Anderson wrote:
> On 7/17/25 12:33, Greg Kroah-Hartman wrote:

<...>

> Anyway, if you really think ids should be random or whatever, why not
> just ida_alloc one in axiliary_device_init and ignore whatever's
> provided? I'd say around half the auxiliary drivers just use 0 (or some
> other constant), which is just as deterministic as using the device
> address.

I would say that auxiliary bus is not right fit for such devices. This
bus was introduced for more complex devices, like the one who has their
own ida_alloc logic.


> Another third use ida_alloc (or xa_alloc) so all that could be
> removed.

These ID numbers need to be per-device.

Thanks

> 
> --Sean

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Sean Anderson]

On 7/20/25 04:17, Leon Romanovsky wrote:
> On Thu, Jul 17, 2025 at 01:12:08PM -0400, Sean Anderson wrote:
>> On 7/17/25 12:33, Greg Kroah-Hartman wrote:
> 
> <...>
> 
>> Anyway, if you really think ids should be random or whatever, why not
>> just ida_alloc one in axiliary_device_init and ignore whatever's
>> provided? I'd say around half the auxiliary drivers just use 0 (or some
>> other constant), which is just as deterministic as using the device
>> address.
> 
> I would say that auxiliary bus is not right fit for such devices. This
> bus was introduced for more complex devices, like the one who has their
> own ida_alloc logic.

I'd say that around 2/3 of the auxiliary drivers that have non-constant
ids use ida_alloc solely for the auxiliary bus and for no other purpose.
I don't think that's the kind of complexity you're referring to.

>> Another third use ida_alloc (or xa_alloc) so all that could be
>> removed.
> 
> These ID numbers need to be per-device.

Why? They are arbitrary with no semantic meaning, right?

--Sean

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Leon Romanovsky]

On Mon, Jul 21, 2025 at 10:29:32AM -0400, Sean Anderson wrote:
> On 7/20/25 04:17, Leon Romanovsky wrote:
> > On Thu, Jul 17, 2025 at 01:12:08PM -0400, Sean Anderson wrote:
> >> On 7/17/25 12:33, Greg Kroah-Hartman wrote:
> > 
> > <...>
> > 
> >> Anyway, if you really think ids should be random or whatever, why not
> >> just ida_alloc one in axiliary_device_init and ignore whatever's
> >> provided? I'd say around half the auxiliary drivers just use 0 (or some
> >> other constant), which is just as deterministic as using the device
> >> address.
> > 
> > I would say that auxiliary bus is not right fit for such devices. This
> > bus was introduced for more complex devices, like the one who has their
> > own ida_alloc logic.
> 
> I'd say that around 2/3 of the auxiliary drivers that have non-constant
> ids use ida_alloc solely for the auxiliary bus and for no other purpose.
> I don't think that's the kind of complexity you're referring to.
> 
> >> Another third use ida_alloc (or xa_alloc) so all that could be
> >> removed.
> > 
> > These ID numbers need to be per-device.
> 
> Why? They are arbitrary with no semantic meaning, right?

Yes, officially there is no meaning, and this is how we would like to
keep it.

Right now, they are very correlated with with their respective PCI function number.
Is it important? No, however it doesn't mean that we should proactively harm user
experience just because we can do it.

[leonro@c ~]$ l /sys/bus/auxiliary/devices/
,,,
rwxrwxrwx 1 root root 0 Jul 21 15:25 mlx5_core.rdma.0 -> ../../../devices/pci0000:00/0000:00:02.7/0000:0
8:00.0/mlx5_core.rdma.0
lrwxrwxrwx 1 root root 0 Jul 21 15:25 mlx5_core.rdma.1 -> ../../../devices/pci0000:00/0000:00:02.7/0000:0
8:00.1/mlx5_core.rdma
...

Thanks

> 
> --Sean
> 

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Sean Anderson]

On 7/23/25 04:13, Leon Romanovsky wrote:
> On Mon, Jul 21, 2025 at 10:29:32AM -0400, Sean Anderson wrote:
>> On 7/20/25 04:17, Leon Romanovsky wrote:
>> > On Thu, Jul 17, 2025 at 01:12:08PM -0400, Sean Anderson wrote:
>> >> On 7/17/25 12:33, Greg Kroah-Hartman wrote:
>> > 
>> > <...>
>> > 
>> >> Anyway, if you really think ids should be random or whatever, why not
>> >> just ida_alloc one in axiliary_device_init and ignore whatever's
>> >> provided? I'd say around half the auxiliary drivers just use 0 (or some
>> >> other constant), which is just as deterministic as using the device
>> >> address.
>> > 
>> > I would say that auxiliary bus is not right fit for such devices. This
>> > bus was introduced for more complex devices, like the one who has their
>> > own ida_alloc logic.
>> 
>> I'd say that around 2/3 of the auxiliary drivers that have non-constant
>> ids use ida_alloc solely for the auxiliary bus and for no other purpose.
>> I don't think that's the kind of complexity you're referring to.
>> 
>> >> Another third use ida_alloc (or xa_alloc) so all that could be
>> >> removed.
>> > 
>> > These ID numbers need to be per-device.
>> 
>> Why? They are arbitrary with no semantic meaning, right?
> 
> Yes, officially there is no meaning, and this is how we would like to
> keep it.
> 
> Right now, they are very correlated with with their respective PCI function number.
> Is it important? No, however it doesn't mean that we should proactively harm user
> experience just because we can do it.
> 
> [leonro@c ~]$ l /sys/bus/auxiliary/devices/
> ,,,
> rwxrwxrwx 1 root root 0 Jul 21 15:25 mlx5_core.rdma.0 -> ../../../devices/pci0000:00/0000:00:02.7/0000:0
> 8:00.0/mlx5_core.rdma.0
> lrwxrwxrwx 1 root root 0 Jul 21 15:25 mlx5_core.rdma.1 -> ../../../devices/pci0000:00/0000:00:02.7/0000:0
> 8:00.1/mlx5_core.rdma

Well, I would certainly like to have semantic meaning for ids. But apparently
that is only allowed if you can sneak it past the review process.

--Sean

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Greg Kroah-Hartman]

On Thu, Jul 24, 2025 at 09:55:59AM -0400, Sean Anderson wrote:
> On 7/23/25 04:13, Leon Romanovsky wrote:
> > On Mon, Jul 21, 2025 at 10:29:32AM -0400, Sean Anderson wrote:
> >> On 7/20/25 04:17, Leon Romanovsky wrote:
> >> > On Thu, Jul 17, 2025 at 01:12:08PM -0400, Sean Anderson wrote:
> >> >> On 7/17/25 12:33, Greg Kroah-Hartman wrote:
> >> > 
> >> > <...>
> >> > 
> >> >> Anyway, if you really think ids should be random or whatever, why not
> >> >> just ida_alloc one in axiliary_device_init and ignore whatever's
> >> >> provided? I'd say around half the auxiliary drivers just use 0 (or some
> >> >> other constant), which is just as deterministic as using the device
> >> >> address.
> >> > 
> >> > I would say that auxiliary bus is not right fit for such devices. This
> >> > bus was introduced for more complex devices, like the one who has their
> >> > own ida_alloc logic.
> >> 
> >> I'd say that around 2/3 of the auxiliary drivers that have non-constant
> >> ids use ida_alloc solely for the auxiliary bus and for no other purpose.
> >> I don't think that's the kind of complexity you're referring to.
> >> 
> >> >> Another third use ida_alloc (or xa_alloc) so all that could be
> >> >> removed.
> >> > 
> >> > These ID numbers need to be per-device.
> >> 
> >> Why? They are arbitrary with no semantic meaning, right?
> > 
> > Yes, officially there is no meaning, and this is how we would like to
> > keep it.
> > 
> > Right now, they are very correlated with with their respective PCI function number.
> > Is it important? No, however it doesn't mean that we should proactively harm user
> > experience just because we can do it.
> > 
> > [leonro@c ~]$ l /sys/bus/auxiliary/devices/
> > ,,,
> > rwxrwxrwx 1 root root 0 Jul 21 15:25 mlx5_core.rdma.0 -> ../../../devices/pci0000:00/0000:00:02.7/0000:0
> > 8:00.0/mlx5_core.rdma.0
> > lrwxrwxrwx 1 root root 0 Jul 21 15:25 mlx5_core.rdma.1 -> ../../../devices/pci0000:00/0000:00:02.7/0000:0
> > 8:00.1/mlx5_core.rdma
> 
> Well, I would certainly like to have semantic meaning for ids. But apparently
> that is only allowed if you can sneak it past the review process.

Do I need to dust off my "make all ids random" patch again and actually
merge it just to prevent this from happening?

greg k-h

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Leon Romanovsky]

On Fri, Jul 25, 2025 at 07:02:24AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 24, 2025 at 09:55:59AM -0400, Sean Anderson wrote:
> > On 7/23/25 04:13, Leon Romanovsky wrote:
> > > On Mon, Jul 21, 2025 at 10:29:32AM -0400, Sean Anderson wrote:
> > >> On 7/20/25 04:17, Leon Romanovsky wrote:
> > >> > On Thu, Jul 17, 2025 at 01:12:08PM -0400, Sean Anderson wrote:
> > >> >> On 7/17/25 12:33, Greg Kroah-Hartman wrote:
> > >> > 
> > >> > <...>
> > >> > 
> > >> >> Anyway, if you really think ids should be random or whatever, why not
> > >> >> just ida_alloc one in axiliary_device_init and ignore whatever's
> > >> >> provided? I'd say around half the auxiliary drivers just use 0 (or some
> > >> >> other constant), which is just as deterministic as using the device
> > >> >> address.
> > >> > 
> > >> > I would say that auxiliary bus is not right fit for such devices. This
> > >> > bus was introduced for more complex devices, like the one who has their
> > >> > own ida_alloc logic.
> > >> 
> > >> I'd say that around 2/3 of the auxiliary drivers that have non-constant
> > >> ids use ida_alloc solely for the auxiliary bus and for no other purpose.
> > >> I don't think that's the kind of complexity you're referring to.
> > >> 
> > >> >> Another third use ida_alloc (or xa_alloc) so all that could be
> > >> >> removed.
> > >> > 
> > >> > These ID numbers need to be per-device.
> > >> 
> > >> Why? They are arbitrary with no semantic meaning, right?
> > > 
> > > Yes, officially there is no meaning, and this is how we would like to
> > > keep it.
> > > 
> > > Right now, they are very correlated with with their respective PCI function number.
> > > Is it important? No, however it doesn't mean that we should proactively harm user
> > > experience just because we can do it.
> > > 
> > > [leonro@c ~]$ l /sys/bus/auxiliary/devices/
> > > ,,,
> > > rwxrwxrwx 1 root root 0 Jul 21 15:25 mlx5_core.rdma.0 -> ../../../devices/pci0000:00/0000:00:02.7/0000:0
> > > 8:00.0/mlx5_core.rdma.0
> > > lrwxrwxrwx 1 root root 0 Jul 21 15:25 mlx5_core.rdma.1 -> ../../../devices/pci0000:00/0000:00:02.7/0000:0
> > > 8:00.1/mlx5_core.rdma
> > 
> > Well, I would certainly like to have semantic meaning for ids. But apparently
> > that is only allowed if you can sneak it past the review process.
> 
> Do I need to dust off my "make all ids random" patch again and actually
> merge it just to prevent this from happening?

After weekend thoughts on it. IDs need to be removed from the driver
access. Let's make them global at least.

Thanks

> 
> greg k-h
> 

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Greg Kroah-Hartman]

On Sun, Jul 27, 2025 at 11:07:05AM +0300, Leon Romanovsky wrote:
> On Fri, Jul 25, 2025 at 07:02:24AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jul 24, 2025 at 09:55:59AM -0400, Sean Anderson wrote:
> > > On 7/23/25 04:13, Leon Romanovsky wrote:
> > > > On Mon, Jul 21, 2025 at 10:29:32AM -0400, Sean Anderson wrote:
> > > >> On 7/20/25 04:17, Leon Romanovsky wrote:
> > > >> > On Thu, Jul 17, 2025 at 01:12:08PM -0400, Sean Anderson wrote:
> > > >> >> On 7/17/25 12:33, Greg Kroah-Hartman wrote:
> > > >> > 
> > > >> > <...>
> > > >> > 
> > > >> >> Anyway, if you really think ids should be random or whatever, why not
> > > >> >> just ida_alloc one in axiliary_device_init and ignore whatever's
> > > >> >> provided? I'd say around half the auxiliary drivers just use 0 (or some
> > > >> >> other constant), which is just as deterministic as using the device
> > > >> >> address.
> > > >> > 
> > > >> > I would say that auxiliary bus is not right fit for such devices. This
> > > >> > bus was introduced for more complex devices, like the one who has their
> > > >> > own ida_alloc logic.
> > > >> 
> > > >> I'd say that around 2/3 of the auxiliary drivers that have non-constant
> > > >> ids use ida_alloc solely for the auxiliary bus and for no other purpose.
> > > >> I don't think that's the kind of complexity you're referring to.
> > > >> 
> > > >> >> Another third use ida_alloc (or xa_alloc) so all that could be
> > > >> >> removed.
> > > >> > 
> > > >> > These ID numbers need to be per-device.
> > > >> 
> > > >> Why? They are arbitrary with no semantic meaning, right?
> > > > 
> > > > Yes, officially there is no meaning, and this is how we would like to
> > > > keep it.
> > > > 
> > > > Right now, they are very correlated with with their respective PCI function number.
> > > > Is it important? No, however it doesn't mean that we should proactively harm user
> > > > experience just because we can do it.
> > > > 
> > > > [leonro@c ~]$ l /sys/bus/auxiliary/devices/
> > > > ,,,
> > > > rwxrwxrwx 1 root root 0 Jul 21 15:25 mlx5_core.rdma.0 -> ../../../devices/pci0000:00/0000:00:02.7/0000:0
> > > > 8:00.0/mlx5_core.rdma.0
> > > > lrwxrwxrwx 1 root root 0 Jul 21 15:25 mlx5_core.rdma.1 -> ../../../devices/pci0000:00/0000:00:02.7/0000:0
> > > > 8:00.1/mlx5_core.rdma
> > > 
> > > Well, I would certainly like to have semantic meaning for ids. But apparently
> > > that is only allowed if you can sneak it past the review process.
> > 
> > Do I need to dust off my "make all ids random" patch again and actually
> > merge it just to prevent this from happening?
> 
> After weekend thoughts on it. IDs need to be removed from the driver
> access. Let's make them global at least.

Great, no objection from me, want to send a patch we can queue up for
6.18-rc1?

Re: [PATCH net v2 1/4] auxiliary: Support hexadecimal ids

[Leon Romanovsky]

On Sun, Jul 27, 2025 at 10:57:09AM +0200, Greg Kroah-Hartman wrote:
> On Sun, Jul 27, 2025 at 11:07:05AM +0300, Leon Romanovsky wrote:
> > On Fri, Jul 25, 2025 at 07:02:24AM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jul 24, 2025 at 09:55:59AM -0400, Sean Anderson wrote:
> > > > On 7/23/25 04:13, Leon Romanovsky wrote:
> > > > > On Mon, Jul 21, 2025 at 10:29:32AM -0400, Sean Anderson wrote:
> > > > >> On 7/20/25 04:17, Leon Romanovsky wrote:
> > > > >> > On Thu, Jul 17, 2025 at 01:12:08PM -0400, Sean Anderson wrote:
> > > > >> >> On 7/17/25 12:33, Greg Kroah-Hartman wrote:
> > > > >> > 
> > > > >> > <...>
> > > > >> > 
> > > > >> >> Anyway, if you really think ids should be random or whatever, why not
> > > > >> >> just ida_alloc one in axiliary_device_init and ignore whatever's
> > > > >> >> provided? I'd say around half the auxiliary drivers just use 0 (or some
> > > > >> >> other constant), which is just as deterministic as using the device
> > > > >> >> address.
> > > > >> > 
> > > > >> > I would say that auxiliary bus is not right fit for such devices. This
> > > > >> > bus was introduced for more complex devices, like the one who has their
> > > > >> > own ida_alloc logic.
> > > > >> 
> > > > >> I'd say that around 2/3 of the auxiliary drivers that have non-constant
> > > > >> ids use ida_alloc solely for the auxiliary bus and for no other purpose.
> > > > >> I don't think that's the kind of complexity you're referring to.
> > > > >> 
> > > > >> >> Another third use ida_alloc (or xa_alloc) so all that could be
> > > > >> >> removed.
> > > > >> > 
> > > > >> > These ID numbers need to be per-device.
> > > > >> 
> > > > >> Why? They are arbitrary with no semantic meaning, right?
> > > > > 
> > > > > Yes, officially there is no meaning, and this is how we would like to
> > > > > keep it.
> > > > > 
> > > > > Right now, they are very correlated with with their respective PCI function number.
> > > > > Is it important? No, however it doesn't mean that we should proactively harm user
> > > > > experience just because we can do it.
> > > > > 
> > > > > [leonro@c ~]$ l /sys/bus/auxiliary/devices/
> > > > > ,,,
> > > > > rwxrwxrwx 1 root root 0 Jul 21 15:25 mlx5_core.rdma.0 -> ../../../devices/pci0000:00/0000:00:02.7/0000:0
> > > > > 8:00.0/mlx5_core.rdma.0
> > > > > lrwxrwxrwx 1 root root 0 Jul 21 15:25 mlx5_core.rdma.1 -> ../../../devices/pci0000:00/0000:00:02.7/0000:0
> > > > > 8:00.1/mlx5_core.rdma
> > > > 
> > > > Well, I would certainly like to have semantic meaning for ids. But apparently
> > > > that is only allowed if you can sneak it past the review process.
> > > 
> > > Do I need to dust off my "make all ids random" patch again and actually
> > > merge it just to prevent this from happening?
> > 
> > After weekend thoughts on it. IDs need to be removed from the driver
> > access. Let's make them global at least.
> 
> Great, no objection from me, want to send a patch we can queue up for
> 6.18-rc1?

Sean proposed the initial idea to move IDs to aux core. Let's wait for him
to see if he wants to do such patch. If not, I'll send.

Thanks