[PATCH net-next v3 0/3] Add DWMAC glue driver for Motorcomm YT6801

[PATCH net-next v3 0/3] Add DWMAC glue driver for Motorcomm YT6801

[Yao Zi]

This series adds glue driver for Motorcomm YT6801 PCIe ethernet
controller, which is considered mostly compatible with DWMAC-4 IP by
inspecting the register layout[1]. It integrates a Motorcomm YT8531S PHY
(confirmed by reading PHY ID) and GMII is used to connect the PHY to
MAC[2].

The initialization logic of the MAC is mostly based on previous upstream
effort for the controller[3] and the Deepin-maintained downstream Linux
driver[4] licensed under GPL-2.0 according to its SPDX headers. However,
this series is a completely re-write of the previous patch series,
utilizing the existing DWMAC4 driver and introducing a glue driver only.

This series only aims to add basic networking functions for the
controller, features like WoL, RSS and LED control are omitted for now.
Testing is done on Loongson 3A5000 machine. Through a local GbE switch,
it reaches 869Mbps (TX)/943Mbps (RX) on average,

## YT6801 TX

Connecting to host 172.16.70.12, port 5201
[  5] local 172.16.70.230 port 43802 connected to 172.16.70.12 port 5201
[ ID] Interval           Transfer     Bitrate         Retr  Cwnd
[  5]   0.00-1.00   sec   102 MBytes   858 Mbits/sec    0    335 KBytes
[  5]   1.00-2.00   sec   103 MBytes   867 Mbits/sec    0    441 KBytes
[  5]   2.00-3.00   sec   103 MBytes   863 Mbits/sec    0    441 KBytes
[  5]   3.00-4.00   sec   104 MBytes   870 Mbits/sec    0    441 KBytes
[  5]   4.00-5.00   sec   104 MBytes   869 Mbits/sec    0    441 KBytes
[  5]   5.00-6.00   sec   104 MBytes   869 Mbits/sec    0    591 KBytes
[  5]   6.00-7.00   sec   104 MBytes   876 Mbits/sec    0    629 KBytes
[  5]   7.00-8.00   sec   105 MBytes   878 Mbits/sec    0    629 KBytes
[  5]   8.00-9.00   sec   104 MBytes   872 Mbits/sec    0    629 KBytes
[  5]   9.00-10.00  sec   104 MBytes   871 Mbits/sec    0    629 KBytes

## YT6801 RX

Connecting to host 172.16.70.230, port 5201
[  5] local 172.16.70.12 port 60866 connected to 172.16.70.230 port 5201
[ ID] Interval           Transfer     Bitrate         Retr  Cwnd
[  5]   0.00-1.00   sec   113 MBytes   949 Mbits/sec    0    352 KBytes
[  5]   1.00-2.00   sec   113 MBytes   945 Mbits/sec    0    352 KBytes
[  5]   2.00-3.00   sec   112 MBytes   938 Mbits/sec    0    352 KBytes
[  5]   3.00-4.00   sec   113 MBytes   945 Mbits/sec    0    352 KBytes
[  5]   4.00-5.00   sec   112 MBytes   938 Mbits/sec    0    407 KBytes
[  5]   5.00-6.00   sec   112 MBytes   940 Mbits/sec    0    407 KBytes
[  5]   6.00-7.00   sec   113 MBytes   945 Mbits/sec    0    436 KBytes
[  5]   7.00-8.00   sec   112 MBytes   941 Mbits/sec    0    436 KBytes
[  5]   8.00-9.00   sec   113 MBytes   947 Mbits/sec    0    645 KBytes
[  5]   9.00-10.00  sec   112 MBytes   939 Mbits/sec    0    645 KBytes

This series depends on v5 of series "Unify platform suspend/resume
routines for PCI DWMAC glue"[5] for a clean apply. It has been some time
since I sent v1 of the series, I'm sorry for the delay. Many thanks for
your time and review.

[1]: https://lore.kernel.org/all/Z_T6vv013jraCzSD@shell.armlinux.org.uk/
[2]: https://lore.kernel.org/all/a48d76ac-db08-46d5-9528-f046a7b541dc@motor-comm.com/
[3]: https://lore.kernel.org/all/a48d76ac-db08-46d5-9528-f046a7b541dc@motor-comm.com/
[4]: https://github.com/deepin-community/kernel/tree/dc61248a0e21/drivers/net/ethernet/motorcomm/yt6801
[5]: https://lore.kernel.org/netdev/20251124160417.51514-1-ziyao@disroot.org/

Changed from v2
- Rebase on top of next-20251124
- Switch to stmmac_plat_dat_alloc() then drop now redundant parameters
  from motorcomm_default_plat_data()
- Set STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP
- Add a comment indicating the possible source of CSR clock
- Link to v2: https://lore.kernel.org/netdev/20251111105252.53487-1-ziyao@disroot.org/

Changed from v1
- Drop (original) PATCH 1, add no vendor ID entry to linux/pci_ids.h
- Use PHY_INTERFACE_MODE_GMII instead of PHY_INTERFACE_MODE_INTERNAL
- Drop extra register read in motorcomm_efuse_read_byte()
- Rename EPHY_RESET to EPHY_MDIO_PHY_RESET, add a comment to reflect its
  function better
- Use the newly-introduced generic PCI suspend/resume routines
- Generate a random MAC address instead of failing to probe when no MAC
  address is programmed in eFuse (seen on some OEM EVBs).
- Collect Tested-by tags
- Link to v1: https://lore.kernel.org/netdev/20251014164746.50696-2-ziyao@disroot.org/

Yao Zi (3):
  net: phy: motorcomm: Support YT8531S PHY in YT6801 Ethernet controller
  net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller
  MAINTAINERS: Assign myself as maintainer of Motorcomm DWMAC glue
    driver

 MAINTAINERS                                   |   6 +
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |   9 +
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
 .../ethernet/stmicro/stmmac/dwmac-motorcomm.c | 378 ++++++++++++++++++
 drivers/net/phy/motorcomm.c                   |   4 +
 5 files changed, 398 insertions(+)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-motorcomm.c

-- 
2.51.2

[PATCH net-next v3 1/3] net: phy: motorcomm: Support YT8531S PHY in YT6801 Ethernet controller

[Yao Zi]

YT6801's internal PHY is confirmed as a GMII-capable variant of YT8531S
by a previous series[1] and reading PHY ID. Add support for
PHY_INTERFACE_MODE_GMII for YT8531S to allow the Ethernet driver to
reuse the PHY code for its internal PHY.

Link: https://lore.kernel.org/all/a48d76ac-db08-46d5-9528-f046a7b541dc@motor-comm.com/ # [1]
Co-developed-by: Frank Sae <Frank.Sae@motor-comm.com>
Signed-off-by: Frank Sae <Frank.Sae@motor-comm.com>
Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 drivers/net/phy/motorcomm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
index 89b5b19a9bd2..b751fbc6711a 100644
--- a/drivers/net/phy/motorcomm.c
+++ b/drivers/net/phy/motorcomm.c
@@ -910,6 +910,10 @@ static int ytphy_rgmii_clk_delay_config(struct phy_device *phydev)
 		val |= FIELD_PREP(YT8521_RC1R_RX_DELAY_MASK, rx_reg) |
 		       FIELD_PREP(YT8521_RC1R_GE_TX_DELAY_MASK, tx_reg);
 		break;
+	case PHY_INTERFACE_MODE_GMII:
+		if (phydev->drv->phy_id != PHY_ID_YT8531S)
+			return -EOPNOTSUPP;
+		break;
 	default: /* do not support other modes */
 		return -EOPNOTSUPP;
 	}
-- 
2.51.2

[PATCH net-next v3 2/3] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

[Yao Zi]

Motorcomm YT6801 is a PCIe ethernet controller based on DWMAC4 IP. It
integrates an GbE phy, supporting WOL, VLAN tagging and various types
of offloading. It ships an on-chip eFuse for storing various vendor
configuration, including MAC address.

This patch adds basic glue code for the controller, allowing it to be
set up and transmit data at a reasonable speed. Features like WOL could
be implemented in the future.

Signed-off-by: Yao Zi <ziyao@disroot.org>
Tested-by: Mingcong Bai <jeffbai@aosc.io>
Tested-by: Runhua He <hua@aosc.io>
Tested-by: Xi Ruoyao <xry111@xry111.site>
---
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |   9 +
 drivers/net/ethernet/stmicro/stmmac/Makefile  |   1 +
 .../ethernet/stmicro/stmmac/dwmac-motorcomm.c | 378 ++++++++++++++++++
 3 files changed, 388 insertions(+)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-motorcomm.c

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 907fe2e927f0..07088d03dbab 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -374,6 +374,15 @@ config DWMAC_LOONGSON
 	  This selects the LOONGSON PCI bus support for the stmmac driver,
 	  Support for ethernet controller on Loongson-2K1000 SoC and LS7A1000 bridge.
 
+config DWMAC_MOTORCOMM
+	tristate "Motorcomm PCI DWMAC support"
+	depends on PCI
+	select MOTORCOMM_PHY
+	select STMMAC_LIBPCI
+	help
+	  This enables glue driver for Motorcomm DWMAC-based PCI Ethernet
+	  controllers. Currently only YT6801 is supported.
+
 config STMMAC_PCI
 	tristate "STMMAC PCI bus support"
 	depends on PCI
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index 7bf528731034..c9263987ef8d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -48,4 +48,5 @@ obj-$(CONFIG_STMMAC_LIBPCI)	+= stmmac_libpci.o
 obj-$(CONFIG_STMMAC_PCI)	+= stmmac-pci.o
 obj-$(CONFIG_DWMAC_INTEL)	+= dwmac-intel.o
 obj-$(CONFIG_DWMAC_LOONGSON)	+= dwmac-loongson.o
+obj-$(CONFIG_DWMAC_MOTORCOMM)	+= dwmac-motorcomm.o
 stmmac-pci-objs:= stmmac_pci.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-motorcomm.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-motorcomm.c
new file mode 100644
index 000000000000..fc87192af446
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-motorcomm.c
@@ -0,0 +1,378 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * DWMAC glue driver for Motorcomm PCI Ethernet controllers
+ *
+ * Copyright (c) 2025 Yao Zi <ziyao@disroot.org>
+ */
+
+#include <linux/bits.h>
+#include <linux/dev_printk.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+#include <linux/stmmac.h>
+
+#include "dwmac4.h"
+#include "stmmac.h"
+#include "stmmac_libpci.h"
+
+#define DRIVER_NAME "dwmac-motorcomm"
+
+#define PCI_VENDOR_ID_MOTORCOMM			0x1f0a
+
+/* Register definition */
+#define EPHY_CTRL				0x1004
+/* Clearing this bit asserts resets for internal MDIO bus and PHY */
+#define  EPHY_MDIO_PHY_RESET			BIT(0)
+#define OOB_WOL_CTRL				0x1010
+#define  OOB_WOL_CTRL_DIS			BIT(0)
+#define MGMT_INT_CTRL0				0x1100
+#define MGMT_INT_CTRL0_MASK			GENMASK(31, 16)
+#define  MGMT_INT_CTRL0_MASK_RXCH		GENMASK(3, 0)
+#define  MGMT_INT_CTRL0_MASK_TXCH		BIT(4)
+#define  MGMT_INT_CTRL0_MASK_MISC		BIT(5)
+#define INT_MODERATION				0x1108
+#define INT_MODERATION_RX			GENMASK(11, 0)
+#define INT_MODERATION_TX			GENMASK(27, 16)
+#define EFUSE_OP_CTRL_0				0x1500
+#define  EFUSE_OP_MODE				GENMASK(1, 0)
+#define   EFUSE_OP_ROW_READ			0x1
+#define  EFUSE_OP_START				BIT(2)
+#define  EFUSE_OP_ADDR				GENMASK(15, 8)
+#define EFUSE_OP_CTRL_1				0x1504
+#define  EFUSE_OP_DONE				BIT(1)
+#define  EFUSE_OP_RD_DATA			GENMASK(31, 24)
+#define SYS_RESET				0x152c
+#define  SYS_RESET_RESET			BIT(31)
+#define GMAC_OFFSET				0x2000
+
+/* Constants */
+#define EFUSE_READ_TIMEOUT_US			20000
+#define EFUSE_PATCH_REGION_OFFSET		18
+#define EFUSE_PATCH_MAX_NUM			39
+#define EFUSE_ADDR_MACA0LR			0x1520
+#define EFUSE_ADDR_MACA0HR			0x1524
+
+struct motorcomm_efuse_patch {
+	__le16 addr;
+	__le32 data;
+} __packed;
+
+struct dwmac_motorcomm_priv {
+	void __iomem *base;
+	struct device *dev;
+};
+
+static int motorcomm_efuse_read_byte(struct dwmac_motorcomm_priv *priv,
+				     u8 offset, u8 *byte)
+{
+	u32 reg;
+	int ret;
+
+	writel(FIELD_PREP(EFUSE_OP_MODE, EFUSE_OP_ROW_READ)	|
+	       FIELD_PREP(EFUSE_OP_ADDR, offset)		|
+	       EFUSE_OP_START, priv->base + EFUSE_OP_CTRL_0);
+
+	ret = readl_poll_timeout(priv->base + EFUSE_OP_CTRL_1,
+				 reg, reg & EFUSE_OP_DONE, 2000,
+				 EFUSE_READ_TIMEOUT_US);
+
+	*byte = FIELD_GET(EFUSE_OP_RD_DATA, reg);
+
+	return ret;
+}
+
+static int motorcomm_efuse_read_patch(struct dwmac_motorcomm_priv *priv,
+				      u8 index,
+				      struct motorcomm_efuse_patch *patch)
+{
+	u8 buf[sizeof(*patch)], offset;
+	int i, ret;
+
+	for (i = 0; i < sizeof(*patch); i++) {
+		offset = EFUSE_PATCH_REGION_OFFSET + sizeof(*patch) * index + i;
+
+		ret = motorcomm_efuse_read_byte(priv, offset, &buf[i]);
+		if (ret)
+			return ret;
+	}
+
+	memcpy(patch, buf, sizeof(*patch));
+
+	return 0;
+}
+
+static int motorcomm_efuse_get_patch_value(struct dwmac_motorcomm_priv *priv,
+					   u16 addr, u32 *value)
+{
+	struct motorcomm_efuse_patch patch;
+	int i, ret;
+
+	for (i = 0; i < EFUSE_PATCH_MAX_NUM; i++) {
+		ret = motorcomm_efuse_read_patch(priv, i, &patch);
+		if (ret)
+			return ret;
+
+		if (patch.addr == 0) {
+			return -ENOENT;
+		} else if (le16_to_cpu(patch.addr) == addr) {
+			*value = le32_to_cpu(patch.data);
+			return 0;
+		}
+	}
+
+	return -ENOENT;
+}
+
+static int motorcomm_efuse_read_mac(struct dwmac_motorcomm_priv *priv, u8 *mac)
+{
+	u32 maca0lr, maca0hr;
+	int ret;
+
+	ret = motorcomm_efuse_get_patch_value(priv, EFUSE_ADDR_MACA0LR,
+					      &maca0lr);
+	if (ret)
+		return dev_err_probe(priv->dev, ret,
+				     "failed to read maca0lr from eFuse\n");
+
+	ret = motorcomm_efuse_get_patch_value(priv, EFUSE_ADDR_MACA0HR,
+					      &maca0hr);
+	if (ret)
+		return dev_err_probe(priv->dev, ret,
+				     "failed to read maca0hr from eFuse\n");
+
+	mac[0] = FIELD_GET(GENMASK(15, 8), maca0hr);
+	mac[1] = FIELD_GET(GENMASK(7, 0), maca0hr);
+	mac[2] = FIELD_GET(GENMASK(31, 24), maca0lr);
+	mac[3] = FIELD_GET(GENMASK(23, 16), maca0lr);
+	mac[4] = FIELD_GET(GENMASK(15, 8), maca0lr);
+	mac[5] = FIELD_GET(GENMASK(7, 0), maca0lr);
+
+	return 0;
+}
+
+static void motorcomm_deassert_mdio_phy_reset(struct dwmac_motorcomm_priv *priv)
+{
+	u32 reg = readl(priv->base + EPHY_CTRL);
+
+	reg |= EPHY_MDIO_PHY_RESET;
+
+	writel(reg, priv->base + EPHY_CTRL);
+}
+
+static void motorcomm_reset(struct dwmac_motorcomm_priv *priv)
+{
+	u32 reg = readl(priv->base + SYS_RESET);
+
+	reg &= ~SYS_RESET_RESET;
+	writel(reg, priv->base + SYS_RESET);
+
+	reg |= SYS_RESET_RESET;
+	writel(reg, priv->base + SYS_RESET);
+
+	motorcomm_deassert_mdio_phy_reset(priv);
+}
+
+static void motorcomm_init(struct dwmac_motorcomm_priv *priv)
+{
+	writel(0x0, priv->base + MGMT_INT_CTRL0);
+
+	writel(FIELD_PREP(INT_MODERATION_RX, 200) |
+	       FIELD_PREP(INT_MODERATION_TX, 200),
+	       priv->base + INT_MODERATION);
+
+	/*
+	 * OOB WOL must be disabled during normal operation, or DMA interrupts
+	 * cannot be delivered to the host.
+	 */
+	writel(OOB_WOL_CTRL_DIS, priv->base + OOB_WOL_CTRL);
+}
+
+static int motorcomm_resume(struct device *dev, void *bsp_priv)
+{
+	struct dwmac_motorcomm_priv *priv = bsp_priv;
+	int ret;
+
+	ret = stmmac_pci_plat_resume(dev, bsp_priv);
+	if (ret)
+		return ret;
+
+	/*
+	 * When recovering from D3hot, EPHY_MDIO_PHY_RESET is automatically
+	 * asserted, and must be deasserted for normal operation.
+	 */
+	motorcomm_deassert_mdio_phy_reset(priv);
+	motorcomm_init(priv);
+
+	return 0;
+}
+
+static struct plat_stmmacenet_data *
+motorcomm_default_plat_data(struct pci_dev *pdev)
+{
+	struct plat_stmmacenet_data *plat;
+	struct device *dev = &pdev->dev;
+
+	plat = stmmac_plat_dat_alloc(dev);
+	if (!plat)
+		return NULL;
+
+	plat->mdio_bus_data = devm_kzalloc(dev, sizeof(*plat->mdio_bus_data),
+					   GFP_KERNEL);
+	if (!plat->mdio_bus_data)
+		return NULL;
+
+	plat->dma_cfg = devm_kzalloc(dev, sizeof(*plat->dma_cfg), GFP_KERNEL);
+	if (!plat->dma_cfg)
+		return NULL;
+
+	plat->axi = devm_kzalloc(dev, sizeof(*plat->axi), GFP_KERNEL);
+	if (!plat->axi)
+		return NULL;
+
+	plat->dma_cfg->pbl		= DEFAULT_DMA_PBL;
+	plat->dma_cfg->pblx8		= true;
+	plat->dma_cfg->txpbl		= 32;
+	plat->dma_cfg->rxpbl		= 32;
+	plat->dma_cfg->eame		= true;
+	plat->dma_cfg->mixed_burst	= true;
+
+	plat->axi->axi_wr_osr_lmt	= 1;
+	plat->axi->axi_rd_osr_lmt	= 1;
+	plat->axi->axi_mb		= true;
+	plat->axi->axi_blen_regval	= DMA_AXI_BLEN4 | DMA_AXI_BLEN8 |
+					  DMA_AXI_BLEN16 | DMA_AXI_BLEN32;
+
+	plat->bus_id		= pci_dev_id(pdev);
+	plat->phy_interface	= PHY_INTERFACE_MODE_GMII;
+	/*
+	 * YT6801 requires an 25MHz clock input/oscillator to function, which
+	 * is likely the source of CSR clock.
+	 */
+	plat->clk_csr		= STMMAC_CSR_20_35M;
+	plat->tx_coe		= 1;
+	plat->rx_coe		= 1;
+	plat->clk_ref_rate	= 125000000;
+	plat->core_type		= DWMAC_CORE_GMAC4;
+	plat->suspend		= stmmac_pci_plat_suspend;
+	plat->resume		= motorcomm_resume;
+	plat->flags		= STMMAC_FLAG_TSO_EN |
+				  STMMAC_FLAG_EN_TX_LPI_CLK_PHY_CAP;
+
+	return plat;
+}
+
+static int motorcomm_setup_irq(struct pci_dev *pdev,
+			       struct stmmac_resources *res,
+			       struct plat_stmmacenet_data *plat)
+{
+	int ret;
+
+	ret = pci_alloc_irq_vectors(pdev, 6, 6, PCI_IRQ_MSIX);
+	if (ret > 0) {
+		res->rx_irq[0]	= pci_irq_vector(pdev, 0);
+		res->tx_irq[0]	= pci_irq_vector(pdev, 4);
+		res->irq	= pci_irq_vector(pdev, 5);
+
+		plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
+
+		return 0;
+	}
+
+	dev_info(&pdev->dev, "failed to allocate MSI-X vector: %d\n", ret);
+	dev_info(&pdev->dev, "try MSI instead\n");
+
+	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to allocate MSI\n");
+
+	res->irq = pci_irq_vector(pdev, 0);
+
+	return 0;
+}
+
+static int motorcomm_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct plat_stmmacenet_data *plat;
+	struct dwmac_motorcomm_priv *priv;
+	struct stmmac_resources res = {};
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = &pdev->dev;
+
+	plat = motorcomm_default_plat_data(pdev);
+	if (!plat)
+		return -ENOMEM;
+
+	plat->bsp_priv = priv;
+
+	ret = pcim_enable_device(pdev);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to enable device\n");
+
+	priv->base = pcim_iomap_region(pdev, 0, DRIVER_NAME);
+	if (IS_ERR(priv->base))
+		return dev_err_probe(&pdev->dev, PTR_ERR(priv->base),
+				     "failed to map IO region\n");
+
+	pci_set_master(pdev);
+
+	motorcomm_reset(priv);
+
+	ret = motorcomm_efuse_read_mac(priv, res.mac);
+	if (ret == -ENOENT) {
+		dev_warn(&pdev->dev, "eFuse contains no valid MAC address\n");
+		dev_warn(&pdev->dev, "fallback to random MAC address\n");
+
+		memset(res.mac, 0, sizeof(res.mac));
+	} else if (ret) {
+		return dev_err_probe(&pdev->dev, ret,
+				     "failed to read MAC address from eFuse\n");
+	}
+
+	ret = motorcomm_setup_irq(pdev, &res, plat);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to setup IRQ\n");
+
+	motorcomm_init(priv);
+
+	res.addr = priv->base + GMAC_OFFSET;
+
+	return stmmac_dvr_probe(&pdev->dev, plat, &res);
+}
+
+static void motorcomm_remove(struct pci_dev *pdev)
+{
+	stmmac_dvr_remove(&pdev->dev);
+	pci_free_irq_vectors(pdev);
+}
+
+static const struct pci_device_id dwmac_motorcomm_pci_id_table[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_MOTORCOMM, 0x6801) },
+	{ },
+};
+MODULE_DEVICE_TABLE(pci, dwmac_motorcomm_pci_id_table);
+
+static struct pci_driver dwmac_motorcomm_pci_driver = {
+	.name = DRIVER_NAME,
+	.id_table = dwmac_motorcomm_pci_id_table,
+	.probe = motorcomm_probe,
+	.remove = motorcomm_remove,
+	.driver = {
+		.pm = &stmmac_simple_pm_ops,
+	},
+};
+
+module_pci_driver(dwmac_motorcomm_pci_driver);
+
+MODULE_DESCRIPTION("DWMAC glue driver for Motorcomm PCI Ethernet controllers");
+MODULE_AUTHOR("Yao Zi <ziyao@disroot.org>");
+MODULE_LICENSE("GPL");
-- 
2.51.2

Re: [PATCH net-next v3 2/3] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

[Russell King (Oracle)]

On Mon, Nov 24, 2025 at 04:32:10PM +0000, Yao Zi wrote:
> +static int motorcomm_setup_irq(struct pci_dev *pdev,
> +			       struct stmmac_resources *res,
> +			       struct plat_stmmacenet_data *plat)
> +{
> +	int ret;
> +
> +	ret = pci_alloc_irq_vectors(pdev, 6, 6, PCI_IRQ_MSIX);
> +	if (ret > 0) {
> +		res->rx_irq[0]	= pci_irq_vector(pdev, 0);
> +		res->tx_irq[0]	= pci_irq_vector(pdev, 4);
> +		res->irq	= pci_irq_vector(pdev, 5);
> +
> +		plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
> +
> +		return 0;
> +	}
> +
> +	dev_info(&pdev->dev, "failed to allocate MSI-X vector: %d\n", ret);
> +	dev_info(&pdev->dev, "try MSI instead\n");
> +
> +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to allocate MSI\n");
> +
> +	res->irq = pci_irq_vector(pdev, 0);
> +
> +	return 0;
> +}
> +
> +static int motorcomm_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
...
> +	ret = motorcomm_setup_irq(pdev, &res, plat);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to setup IRQ\n");
> +
> +	motorcomm_init(priv);
> +
> +	res.addr = priv->base + GMAC_OFFSET;
> +
> +	return stmmac_dvr_probe(&pdev->dev, plat, &res);

If stmmac_dvr_probe() fails, then it will return an error code. This
leaves the PCI MSI interrupt allocated...

> +}
> +
> +static void motorcomm_remove(struct pci_dev *pdev)
> +{
> +	stmmac_dvr_remove(&pdev->dev);
> +	pci_free_irq_vectors(pdev);

... which stood out because of the presence of this function doing
stuff after the call to stmmac_dvr_remove().

So... reviewing the other stmmac PCI drivers:

- dwmac-intel calls pci_alloc_irq_vectors() but does not call
  pci_free_irq_vectors(). This looks like a bug.
- dwmac-intel calls pcim_enable_device() in its probe function, and
  also its intel_eth_pci_resume() - pcim_enable_device() is the devres
  managed function, so we end up adding more and more devres entries
  each time intel_eth_pci_resume() is resumed. Note that
  intel_eth_pci_suspend() doesn't disable the device. So, this should
  probably be the non-devres version.
- dwmac-loongson looks sane, but the checks for ld->multichan before
  calling loongson_dwmac_msi_clear() look unnecessary, as
  pci_free_irq_vectors() can be safely called even if MSI/MSI-X have
  not been (successfully) allocated.

So, I wonder whether there is scope to have a common way to clean up
PCI drivers. Could you look into this please?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v3 2/3] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

[Yao Zi]

Hi Russell,

Sorry for the late reply,

On Mon, Nov 24, 2025 at 07:06:12PM +0000, Russell King (Oracle) wrote:
> On Mon, Nov 24, 2025 at 04:32:10PM +0000, Yao Zi wrote:
> > +static int motorcomm_setup_irq(struct pci_dev *pdev,
> > +			       struct stmmac_resources *res,
> > +			       struct plat_stmmacenet_data *plat)
> > +{
> > +	int ret;
> > +
> > +	ret = pci_alloc_irq_vectors(pdev, 6, 6, PCI_IRQ_MSIX);
> > +	if (ret > 0) {
> > +		res->rx_irq[0]	= pci_irq_vector(pdev, 0);
> > +		res->tx_irq[0]	= pci_irq_vector(pdev, 4);
> > +		res->irq	= pci_irq_vector(pdev, 5);
> > +
> > +		plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
> > +
> > +		return 0;
> > +	}
> > +
> > +	dev_info(&pdev->dev, "failed to allocate MSI-X vector: %d\n", ret);
> > +	dev_info(&pdev->dev, "try MSI instead\n");
> > +
> > +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> > +	if (ret < 0)
> > +		return dev_err_probe(&pdev->dev, ret,
> > +				     "failed to allocate MSI\n");
> > +
> > +	res->irq = pci_irq_vector(pdev, 0);
> > +
> > +	return 0;
> > +}
> > +
> > +static int motorcomm_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > +{
> ...
> > +	ret = motorcomm_setup_irq(pdev, &res, plat);
> > +	if (ret)
> > +		return dev_err_probe(&pdev->dev, ret, "failed to setup IRQ\n");
> > +
> > +	motorcomm_init(priv);
> > +
> > +	res.addr = priv->base + GMAC_OFFSET;
> > +
> > +	return stmmac_dvr_probe(&pdev->dev, plat, &res);
> 
> If stmmac_dvr_probe() fails, then it will return an error code. This
> leaves the PCI MSI interrupt allocated...

This isn't true. MSI API is a little magical: when the device is enabled
through pcim_enable_device(), the device becomes devres-managed, and
a plain call to pci_alloc_irq_vectors() becomes managed, too, even if
its name doesn't indicate it's a devres-managed API.

pci_free_irq_vectors() will be automatically called on driver deattach.
See pcim_setup_msi_release() in drivers/pci/msi/msi.c, which is invoked
by pci_alloc_irq_vectors() internally.

> > +}
> > +
> > +static void motorcomm_remove(struct pci_dev *pdev)
> > +{
> > +	stmmac_dvr_remove(&pdev->dev);
> > +	pci_free_irq_vectors(pdev);
> 
> ... which stood out because of the presence of this function doing
> stuff after the call to stmmac_dvr_remove().

But yes, this call to pci_free_irq_vectors() is redundant, since
pci_free_irq_vectors() will be automatically invoked. I'll remove it in
the next version.

> So... reviewing the other stmmac PCI drivers:
> 
> - dwmac-intel calls pci_alloc_irq_vectors() but does not call
>   pci_free_irq_vectors(). This looks like a bug.

The driver does call pcim_enable_device() thus enables devres for the
PCI device, in which case manually calling pci_free_irq_vectors() is
unnecessary. I don't think there's a bug.

> - dwmac-intel calls pcim_enable_device() in its probe function, and
>   also its intel_eth_pci_resume() - pcim_enable_device() is the devres
>   managed function, so we end up adding more and more devres entries
>   each time intel_eth_pci_resume() is resumed. Note that
>   intel_eth_pci_suspend() doesn't disable the device. So, this should
>   probably be the non-devres version.

Agree.

> - dwmac-loongson looks sane, but the checks for ld->multichan before
>   calling loongson_dwmac_msi_clear() look unnecessary, as
>   pci_free_irq_vectors() can be safely called even if MSI/MSI-X have
>   not been (successfully) allocated.

loongson-dwmac doesn't enable devres for the PCI device (it calls
pci_enable_device() instead), so manually freeing the interrupts is
indeed necessary. However, I'd suggest moving to the devres variant and
simplify the error handling (and clean up) path.

> So, I wonder whether there is scope to have a common way to clean up
> PCI drivers. Could you look into this please?

In short, enabling the device with pcim_enable_device() to make it
devres-managed should be the best solution, in which case
pci_alloc_irq_vectors() is devres-managed, too, and at least we don't
need to worry about interrupts anymore on error handling/removal path.

Regards,
Yao Zi

> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v3 2/3] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

[Russell King (Oracle)]

On Fri, Dec 05, 2025 at 05:31:34AM +0000, Yao Zi wrote:
> Hi Russell,
> 
> Sorry for the late reply,
> 
> On Mon, Nov 24, 2025 at 07:06:12PM +0000, Russell King (Oracle) wrote:
> > On Mon, Nov 24, 2025 at 04:32:10PM +0000, Yao Zi wrote:
> > > +static int motorcomm_setup_irq(struct pci_dev *pdev,
> > > +			       struct stmmac_resources *res,
> > > +			       struct plat_stmmacenet_data *plat)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = pci_alloc_irq_vectors(pdev, 6, 6, PCI_IRQ_MSIX);
> > > +	if (ret > 0) {
> > > +		res->rx_irq[0]	= pci_irq_vector(pdev, 0);
> > > +		res->tx_irq[0]	= pci_irq_vector(pdev, 4);
> > > +		res->irq	= pci_irq_vector(pdev, 5);
> > > +
> > > +		plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
> > > +
> > > +		return 0;
> > > +	}
> > > +
> > > +	dev_info(&pdev->dev, "failed to allocate MSI-X vector: %d\n", ret);
> > > +	dev_info(&pdev->dev, "try MSI instead\n");
> > > +
> > > +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> > > +	if (ret < 0)
> > > +		return dev_err_probe(&pdev->dev, ret,
> > > +				     "failed to allocate MSI\n");
> > > +
> > > +	res->irq = pci_irq_vector(pdev, 0);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int motorcomm_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > +{
> > ...
> > > +	ret = motorcomm_setup_irq(pdev, &res, plat);
> > > +	if (ret)
> > > +		return dev_err_probe(&pdev->dev, ret, "failed to setup IRQ\n");
> > > +
> > > +	motorcomm_init(priv);
> > > +
> > > +	res.addr = priv->base + GMAC_OFFSET;
> > > +
> > > +	return stmmac_dvr_probe(&pdev->dev, plat, &res);
> > 
> > If stmmac_dvr_probe() fails, then it will return an error code. This
> > leaves the PCI MSI interrupt allocated...
> 
> This isn't true. MSI API is a little magical: when the device is enabled
> through pcim_enable_device(), the device becomes devres-managed, and
> a plain call to pci_alloc_irq_vectors() becomes managed, too, even if
> its name doesn't indicate it's a devres-managed API.
> 
> pci_free_irq_vectors() will be automatically called on driver deattach.
> See pcim_setup_msi_release() in drivers/pci/msi/msi.c, which is invoked
> by pci_alloc_irq_vectors() internally.

This looks very non-intuitive, and the documentation for
pci_alloc_irq_vectors() doesn't help:

 * Upon a successful allocation, the caller should use pci_irq_vector()
 * to get the Linux IRQ number to be passed to request_threaded_irq().
 * The driver must call pci_free_irq_vectors() on cleanup.
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

because if what you say is correct (and it looks like it is) then this
line is blatently incorrect.

Bjorn?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v3 2/3] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

[Bjorn Helgaas]

[+to Philipp, Thomas for MSI devres question]

On Fri, Dec 05, 2025 at 09:34:54AM +0000, Russell King (Oracle) wrote:
> On Fri, Dec 05, 2025 at 05:31:34AM +0000, Yao Zi wrote:
> > On Mon, Nov 24, 2025 at 07:06:12PM +0000, Russell King (Oracle) wrote:
> > > On Mon, Nov 24, 2025 at 04:32:10PM +0000, Yao Zi wrote:
> > > > +static int motorcomm_setup_irq(struct pci_dev *pdev,
> > > > +			       struct stmmac_resources *res,
> > > > +			       struct plat_stmmacenet_data *plat)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = pci_alloc_irq_vectors(pdev, 6, 6, PCI_IRQ_MSIX);
> > > > +	if (ret > 0) {
> > > > +		res->rx_irq[0]	= pci_irq_vector(pdev, 0);
> > > > +		res->tx_irq[0]	= pci_irq_vector(pdev, 4);
> > > > +		res->irq	= pci_irq_vector(pdev, 5);
> > > > +
> > > > +		plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
> > > > +
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	dev_info(&pdev->dev, "failed to allocate MSI-X vector: %d\n", ret);
> > > > +	dev_info(&pdev->dev, "try MSI instead\n");
> > > > +
> > > > +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> > > > +	if (ret < 0)
> > > > +		return dev_err_probe(&pdev->dev, ret,
> > > > +				     "failed to allocate MSI\n");
> > > > +
> > > > +	res->irq = pci_irq_vector(pdev, 0);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int motorcomm_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > > +{
> > > ...
> > > > +	ret = motorcomm_setup_irq(pdev, &res, plat);
> > > > +	if (ret)
> > > > +		return dev_err_probe(&pdev->dev, ret, "failed to setup IRQ\n");
> > > > +
> > > > +	motorcomm_init(priv);
> > > > +
> > > > +	res.addr = priv->base + GMAC_OFFSET;
> > > > +
> > > > +	return stmmac_dvr_probe(&pdev->dev, plat, &res);
> > > 
> > > If stmmac_dvr_probe() fails, then it will return an error code. This
> > > leaves the PCI MSI interrupt allocated...
> > 
> > This isn't true. MSI API is a little magical: when the device is enabled
> > through pcim_enable_device(), the device becomes devres-managed, and
> > a plain call to pci_alloc_irq_vectors() becomes managed, too, even if
> > its name doesn't indicate it's a devres-managed API.
> > 
> > pci_free_irq_vectors() will be automatically called on driver deattach.
> > See pcim_setup_msi_release() in drivers/pci/msi/msi.c, which is invoked
> > by pci_alloc_irq_vectors() internally.
> 
> This looks very non-intuitive, and the documentation for
> pci_alloc_irq_vectors() doesn't help:
> 
>  * Upon a successful allocation, the caller should use pci_irq_vector()
>  * to get the Linux IRQ number to be passed to request_threaded_irq().
>  * The driver must call pci_free_irq_vectors() on cleanup.
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> because if what you say is correct (and it looks like it is) then this
> line is blatently incorrect.
> 
> Bjorn?

Re: [PATCH net-next v3 2/3] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

[Philipp Stanner]

On Fri, 2025-12-05 at 16:16 -0600, Bjorn Helgaas wrote:
> [+to Philipp, Thomas for MSI devres question]
> 
> On Fri, Dec 05, 2025 at 09:34:54AM +0000, Russell King (Oracle) wrote:
> > On Fri, Dec 05, 2025 at 05:31:34AM +0000, Yao Zi wrote:
> > > On Mon, Nov 24, 2025 at 07:06:12PM +0000, Russell King (Oracle) wrote:
> > > > On Mon, Nov 24, 2025 at 04:32:10PM +0000, Yao Zi wrote:
> > > > > +static int motorcomm_setup_irq(struct pci_dev *pdev,
> > > > > +			       struct stmmac_resources *res,
> > > > > +			       struct plat_stmmacenet_data *plat)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = pci_alloc_irq_vectors(pdev, 6, 6, PCI_IRQ_MSIX);
> > > > > +	if (ret > 0) {
> > > > > +		res->rx_irq[0]	= pci_irq_vector(pdev, 0);
> > > > > +		res->tx_irq[0]	= pci_irq_vector(pdev, 4);
> > > > > +		res->irq	= pci_irq_vector(pdev, 5);
> > > > > +
> > > > > +		plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
> > > > > +
> > > > > +		return 0;
> > > > > +	}
> > > > > +
> > > > > +	dev_info(&pdev->dev, "failed to allocate MSI-X vector: %d\n", ret);
> > > > > +	dev_info(&pdev->dev, "try MSI instead\n");
> > > > > +
> > > > > +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> > > > > +	if (ret < 0)
> > > > > +		return dev_err_probe(&pdev->dev, ret,
> > > > > +				     "failed to allocate MSI\n");
> > > > > +
> > > > > +	res->irq = pci_irq_vector(pdev, 0);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int motorcomm_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > > > +{
> > > > ...
> > > > > +	ret = motorcomm_setup_irq(pdev, &res, plat);
> > > > > +	if (ret)
> > > > > +		return dev_err_probe(&pdev->dev, ret, "failed to setup IRQ\n");
> > > > > +
> > > > > +	motorcomm_init(priv);
> > > > > +
> > > > > +	res.addr = priv->base + GMAC_OFFSET;
> > > > > +
> > > > > +	return stmmac_dvr_probe(&pdev->dev, plat, &res);
> > > > 
> > > > If stmmac_dvr_probe() fails, then it will return an error code. This
> > > > leaves the PCI MSI interrupt allocated...
> > > 
> > > This isn't true. MSI API is a little magical: when the device is enabled

s/magical/confusing and explosive

;)

> > > through pcim_enable_device(), the device becomes devres-managed, and
> > > a plain call to pci_alloc_irq_vectors() becomes managed, too, even if
> > > its name doesn't indicate it's a devres-managed API.

Just to be clear: Callers of pci_setup_msi_context() are the last users
in PCI which express this strange behavior. All the other APIs behave
as one could expect from their name (pcim_ vs pci_).

> > > 
> > > pci_free_irq_vectors() will be automatically called on driver deattach.
> > > See pcim_setup_msi_release() in drivers/pci/msi/msi.c, which is invoked
> > > by pci_alloc_irq_vectors() internally.

Yes, this is correct.

> > 
> > This looks very non-intuitive, and the documentation for
> > pci_alloc_irq_vectors() doesn't help:
> > 
> >  * Upon a successful allocation, the caller should use pci_irq_vector()
> >  * to get the Linux IRQ number to be passed to request_threaded_irq().
> >  * The driver must call pci_free_irq_vectors() on cleanup.
> >    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > because if what you say is correct (and it looks like it is) then this
> > line is blatently incorrect.

True, this line is false. It should probably state "If you didn't
enable your PCI device with pcim_enable_device(), you must call
pci_free_irq_vectors() on cleanup."

I don't know whether calling pci_free_irq_vectors() is a bug (once
manually, once hidden by devres), though.

If it's not a bug, one could keep the docu that way or at least phrase
it in a way so that no additional users start relying on that hybrid
mechanism.

There is actually no reason anymore to call pcim_enable_device() at
all, other than that you get automatic device disablement.

> > 
> > Bjorn?

BTW, if PCI has a TODO list somewhere, removing that hybrid devres
feature for MSI should be on it, for obvious reasons.

The good news is that it's the last remainder of PCI hybrid devres and
getting rid of it would allow for removal of some additional code, too
(e.g., is_enabled bit and pcim_pin_device()).

The bad news is that it's not super trivial to remove. I looked into it
about two times and decided I can't invest that time currently. You
need to go over all drivers again to see who uses pcim_enable_device(),
then add free_irq_vecs() for them all and so on…

If you give me a pointer I can provide a TODO entry. In any case, feel
free to set me as a reviewer!

Regards
Philipp

Re: [PATCH net-next v3 2/3] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

[Russell King (Oracle)]

On Mon, Dec 08, 2025 at 10:54:36AM +0100, Philipp Stanner wrote:
> The bad news is that it's not super trivial to remove. I looked into it
> about two times and decided I can't invest that time currently. You
> need to go over all drivers again to see who uses pcim_enable_device(),
> then add free_irq_vecs() for them all and so on…

So that I can confirm, you're saying that all drivers that call
pci_alloc_irq_vectors() should call pci_free_irq_vectors() in their
->remove() method and not rely on the devres behaviour that
pcim_enable_device() will permit.

In terms of whether it's safe to call this twice, pci_free_irq_vectors()
calls pci_disable_msix() and pci_disable_msi().

pci_disable_msix() checks:

        if (!pci_msi_enabled() || !dev || !dev->msix_enabled)
                return;

which will set dev->msix_enabled to 0 via pci_msix_shutdown().

pci_disable_msi() does a similar check:

        if (!pci_msi_enabled() || !dev || !dev->msi_enabled)
                return;

and similarly pci_msi_shutdown() sets dev->msi_enabled to 0.

So my conclusion is it's safe to call pci_free_irq_vectors() twice for
the same device.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v3 2/3] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

[Philipp Stanner]

On Mon, 2025-12-08 at 10:15 +0000, Russell King (Oracle) wrote:
> On Mon, Dec 08, 2025 at 10:54:36AM +0100, Philipp Stanner wrote:
> > The bad news is that it's not super trivial to remove. I looked into it
> > about two times and decided I can't invest that time currently. You
> > need to go over all drivers again to see who uses pcim_enable_device(),
> > then add free_irq_vecs() for them all and so on…
> 
> So that I can confirm, you're saying that all drivers that call
> pci_alloc_irq_vectors() should call pci_free_irq_vectors() in their
> ->remove() method and not rely on the devres behaviour that
> pcim_enable_device() will permit.

"permit" is kind of a generous word. This behavior is dangerous and
there were bugs because of that in the past, because it confused
programmers. See:

f00059b4c1b0 drm/vboxvideo: fix mapping leaks


pcim_enable_device() used to switch all sorts of functions into managed
mode. As far as I could figure out through git, back in 2009 it was
intended that ALL pci functions are switched into managed mode that
way. That's also how it was documented.

The ecosystem then fractured, however. Some functions were always
managed (pcim_), some never, and some sometimes.

I removed all "sometimes managed" functions since 2024. The last
remainder is MSI.

If we want to remove that, we need to:
   1. Find all drivers that rely on pci_free_irq_vectors() being run
      automatically. IOW those that use pcim_enable_device() + wrappers
      around pci_setup_msi_context().
   2. Port those drivers to do the free_irq_vecs manually, if it's not
      a problem if it's called twice. If that were a problem, those
      drivers would also need to replace pcim_enable_device() with
      pci_enable_device().
   3. Once all drivers are ported, remove the devres code from msi.c
   4. Do associated cleanup work in PCI.

> 
> In terms of whether it's safe to call this twice, pci_free_irq_vectors()
> calls pci_disable_msix() and pci_disable_msi().
> 
> pci_disable_msix() checks:
> 
>         if (!pci_msi_enabled() || !dev || !dev->msix_enabled)
>                 return;
> 
> which will set dev->msix_enabled to 0 via pci_msix_shutdown().
> 
> pci_disable_msi() does a similar check:
> 
>         if (!pci_msi_enabled() || !dev || !dev->msi_enabled)
>                 return;
> 
> and similarly pci_msi_shutdown() sets dev->msi_enabled to 0.
> 
> So my conclusion is it's safe to call pci_free_irq_vectors() twice for
> the same device.
> 

Hm. Looks good.


P.

Re: [PATCH net-next v3 2/3] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

[Russell King (Oracle)]

On Mon, Dec 08, 2025 at 11:47:23AM +0100, Philipp Stanner wrote:
> On Mon, 2025-12-08 at 10:15 +0000, Russell King (Oracle) wrote:
> > On Mon, Dec 08, 2025 at 10:54:36AM +0100, Philipp Stanner wrote:
> > > The bad news is that it's not super trivial to remove. I looked into it
> > > about two times and decided I can't invest that time currently. You
> > > need to go over all drivers again to see who uses pcim_enable_device(),
> > > then add free_irq_vecs() for them all and so on…
> > 
> > So that I can confirm, you're saying that all drivers that call
> > pci_alloc_irq_vectors() should call pci_free_irq_vectors() in their
> > ->remove() method and not rely on the devres behaviour that
> > pcim_enable_device() will permit.
> 
> "permit" is kind of a generous word. This behavior is dangerous and
> there were bugs because of that in the past, because it confused
> programmers. See:
> 
> f00059b4c1b0 drm/vboxvideo: fix mapping leaks
> 
> 
> pcim_enable_device() used to switch all sorts of functions into managed
> mode. As far as I could figure out through git, back in 2009 it was
> intended that ALL pci functions are switched into managed mode that
> way. That's also how it was documented.
> 
> The ecosystem then fractured, however. Some functions were always
> managed (pcim_), some never, and some sometimes.
> 
> I removed all "sometimes managed" functions since 2024. The last
> remainder is MSI.
> 
> If we want to remove that, we need to:
>    1. Find all drivers that rely on pci_free_irq_vectors() being run
>       automatically. IOW those that use pcim_enable_device() + wrappers
>       around pci_setup_msi_context().
>    2. Port those drivers to do the free_irq_vecs manually, if it's not
>       a problem if it's called twice. If that were a problem, those
>       drivers would also need to replace pcim_enable_device() with
>       pci_enable_device().
>    3. Once all drivers are ported, remove the devres code from msi.c
>    4. Do associated cleanup work in PCI.
> 
> > 
> > In terms of whether it's safe to call this twice, pci_free_irq_vectors()
> > calls pci_disable_msix() and pci_disable_msi().
> > 
> > pci_disable_msix() checks:
> > 
> >         if (!pci_msi_enabled() || !dev || !dev->msix_enabled)
> >                 return;
> > 
> > which will set dev->msix_enabled to 0 via pci_msix_shutdown().
> > 
> > pci_disable_msi() does a similar check:
> > 
> >         if (!pci_msi_enabled() || !dev || !dev->msi_enabled)
> >                 return;
> > 
> > and similarly pci_msi_shutdown() sets dev->msi_enabled to 0.
> > 
> > So my conclusion is it's safe to call pci_free_irq_vectors() twice for
> > the same device.
> > 
> 
> Hm. Looks good.

So, what do you want to see for new drivers such as the one at the top
of this thread? Should they explicitly call pci_free_irq_vectors() even
though they call pcim_enable_device() ?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v3 2/3] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

[Philipp Stanner]

On Mon, 2025-12-08 at 10:53 +0000, Russell King (Oracle) wrote:
> On Mon, Dec 08, 2025 at 11:47:23AM +0100, Philipp Stanner wrote:
> > On Mon, 2025-12-08 at 10:15 +0000, Russell King (Oracle) wrote:
> > > On Mon, Dec 08, 2025 at 10:54:36AM +0100, Philipp Stanner wrote:
> > > > The bad news is that it's not super trivial to remove. I looked into it
> > > > about two times and decided I can't invest that time currently. You
> > > > need to go over all drivers again to see who uses pcim_enable_device(),
> > > > then add free_irq_vecs() for them all and so on…
> > > 
> > > So that I can confirm, you're saying that all drivers that call
> > > pci_alloc_irq_vectors() should call pci_free_irq_vectors() in their
> > > ->remove() method and not rely on the devres behaviour that
> > > pcim_enable_device() will permit.
> > 
> > "permit" is kind of a generous word. This behavior is dangerous and
> > there were bugs because of that in the past, because it confused
> > programmers. See:
> > 
> > f00059b4c1b0 drm/vboxvideo: fix mapping leaks
> > 
> > 
> > pcim_enable_device() used to switch all sorts of functions into managed
> > mode. As far as I could figure out through git, back in 2009 it was
> > intended that ALL pci functions are switched into managed mode that
> > way. That's also how it was documented.
> > 
> > The ecosystem then fractured, however. Some functions were always
> > managed (pcim_), some never, and some sometimes.
> > 
> > I removed all "sometimes managed" functions since 2024. The last
> > remainder is MSI.
> > 
> > If we want to remove that, we need to:
> >    1. Find all drivers that rely on pci_free_irq_vectors() being run
> >       automatically. IOW those that use pcim_enable_device() + wrappers
> >       around pci_setup_msi_context().
> >    2. Port those drivers to do the free_irq_vecs manually, if it's not
> >       a problem if it's called twice. If that were a problem, those
> >       drivers would also need to replace pcim_enable_device() with
> >       pci_enable_device().
> >    3. Once all drivers are ported, remove the devres code from msi.c
> >    4. Do associated cleanup work in PCI.
> > 
> > > 
> > > In terms of whether it's safe to call this twice, pci_free_irq_vectors()
> > > calls pci_disable_msix() and pci_disable_msi().
> > > 
> > > pci_disable_msix() checks:
> > > 
> > >         if (!pci_msi_enabled() || !dev || !dev->msix_enabled)
> > >                 return;
> > > 
> > > which will set dev->msix_enabled to 0 via pci_msix_shutdown().
> > > 
> > > pci_disable_msi() does a similar check:
> > > 
> > >         if (!pci_msi_enabled() || !dev || !dev->msi_enabled)
> > >                 return;
> > > 
> > > and similarly pci_msi_shutdown() sets dev->msi_enabled to 0.
> > > 
> > > So my conclusion is it's safe to call pci_free_irq_vectors() twice for
> > > the same device.
> > > 
> > 
> > Hm. Looks good.
> 
> So, what do you want to see for new drivers such as the one at the top
> of this thread? Should they explicitly call pci_free_irq_vectors() even
> though they call pcim_enable_device() ?


Yes, I think that's the right thing to do. It makes removing that
feature from MSI easier, since there will not be even more drivers to
port.

P.

Re: [PATCH net-next v3 2/3] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

[Yao Zi]

On Mon, Dec 08, 2025 at 10:54:36AM +0100, Philipp Stanner wrote:
> On Fri, 2025-12-05 at 16:16 -0600, Bjorn Helgaas wrote:
> > [+to Philipp, Thomas for MSI devres question]
> > 
> > On Fri, Dec 05, 2025 at 09:34:54AM +0000, Russell King (Oracle) wrote:
> > > On Fri, Dec 05, 2025 at 05:31:34AM +0000, Yao Zi wrote:
> > > > On Mon, Nov 24, 2025 at 07:06:12PM +0000, Russell King (Oracle) wrote:
> > > > > On Mon, Nov 24, 2025 at 04:32:10PM +0000, Yao Zi wrote:

...

> > > This looks very non-intuitive, and the documentation for
> > > pci_alloc_irq_vectors() doesn't help:
> > > 
> > >  * Upon a successful allocation, the caller should use pci_irq_vector()
> > >  * to get the Linux IRQ number to be passed to request_threaded_irq().
> > >  * The driver must call pci_free_irq_vectors() on cleanup.
> > >    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > 
> > > because if what you say is correct (and it looks like it is) then this
> > > line is blatently incorrect.
> 
> True, this line is false. It should probably state "If you didn't
> enable your PCI device with pcim_enable_device(), you must call
> pci_free_irq_vectors() on cleanup."
> 
> If it's not a bug, one could keep the docu that way or at least phrase
> it in a way so that no additional users start relying on that hybrid
> mechanism.

Thanks for the clarification, would you mind me sending a patch to fix
the description, and also mention the automatic clean-up behavior
shouldn't be relied anymore in new code?

...

> The good news is that it's the last remainder of PCI hybrid devres and
> getting rid of it would allow for removal of some additional code, too
> (e.g., is_enabled bit and pcim_pin_device()).
> 
> The bad news is that it's not super trivial to remove. I looked into it
> about two times and decided I can't invest that time currently. You
> need to go over all drivers again to see who uses pcim_enable_device(),
> then add free_irq_vecs() for them all and so on…

Do you think adding an implementation of pcim_alloc_irq_vectors(), that
always call pci_free_irq_vectors() regardless whether the PCI device is
managed, will help the conversion?

This will make it more trival to rewrite drivers depending on the
automatic clean-up behavior: since calling pci_free_irq_vectors()
several times is okay, we could simply change pci_alloc_irq_vectors() to
pcim_alloc_irq_vectors(), without considering where to call
pci_free_irq_vectors().

Introducing pcim_alloc_irq_vectors() will also help newly-introduced
drivers to reduce duplicated code to handle resource clean-up.

> If you give me a pointer I can provide a TODO entry. In any case, feel
> free to set me as a reviewer!

> Regards
> Philipp

Regards,
Yao Zi

Re: [PATCH net-next v3 2/3] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

[Philipp Stanner]

On Thu, 2025-12-11 at 14:23 +0000, Yao Zi wrote:
> On Mon, Dec 08, 2025 at 10:54:36AM +0100, Philipp Stanner wrote:
> > On Fri, 2025-12-05 at 16:16 -0600, Bjorn Helgaas wrote:
> > > [+to Philipp, Thomas for MSI devres question]
> > > 
> > > On Fri, Dec 05, 2025 at 09:34:54AM +0000, Russell King (Oracle) wrote:
> > > > On Fri, Dec 05, 2025 at 05:31:34AM +0000, Yao Zi wrote:
> > > > > On Mon, Nov 24, 2025 at 07:06:12PM +0000, Russell King (Oracle) wrote:
> > > > > > On Mon, Nov 24, 2025 at 04:32:10PM +0000, Yao Zi wrote:
> 
> ...
> 
> > > > This looks very non-intuitive, and the documentation for
> > > > pci_alloc_irq_vectors() doesn't help:
> > > > 
> > > >  * Upon a successful allocation, the caller should use pci_irq_vector()
> > > >  * to get the Linux IRQ number to be passed to request_threaded_irq().
> > > >  * The driver must call pci_free_irq_vectors() on cleanup.
> > > >    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > 
> > > > because if what you say is correct (and it looks like it is) then this
> > > > line is blatently incorrect.
> > 
> > True, this line is false. It should probably state "If you didn't
> > enable your PCI device with pcim_enable_device(), you must call
> > pci_free_irq_vectors() on cleanup."
> > 
> > If it's not a bug, one could keep the docu that way or at least phrase
> > it in a way so that no additional users start relying on that hybrid
> > mechanism.
> 
> Thanks for the clarification, would you mind me sending a patch to fix
> the description, and also mention the automatic clean-up behavior
> shouldn't be relied anymore in new code?

If I would mind if *you* send such a patch? I for sure wouldn't mind,
that's the entire idea of open source development ^^

I can of course review it if you +Cc me.

> 
> ...
> 
> > The good news is that it's the last remainder of PCI hybrid devres and
> > getting rid of it would allow for removal of some additional code, too
> > (e.g., is_enabled bit and pcim_pin_device()).
> > 
> > The bad news is that it's not super trivial to remove. I looked into it
> > about two times and decided I can't invest that time currently. You
> > need to go over all drivers again to see who uses pcim_enable_device(),
> > then add free_irq_vecs() for them all and so on…
> 
> Do you think adding an implementation of pcim_alloc_irq_vectors(), that
> always call pci_free_irq_vectors() regardless whether the PCI device is
> managed, will help the conversion?
> 
> This will make it more trival to rewrite drivers depending on the
> automatic clean-up behavior: since calling pci_free_irq_vectors()
> several times is okay, we could simply change pci_alloc_irq_vectors() to
> pcim_alloc_irq_vectors(), without considering where to call
> pci_free_irq_vectors().
> 
> Introducing pcim_alloc_irq_vectors() will also help newly-introduced
> drivers to reduce duplicated code to handle resource clean-up.

That's in fact how I have cleaned up the hybrid nature that was present
until this year in pci_request_region() et al.:

https://lore.kernel.org/linux-pci/20250519112959.25487-2-phasta@kernel.org/

It's one way to do it. First port everyone who relies on managed
behavior to a pcim_ function, and once they're all ported, remove the
hybrid nature from the pci_ function.

So that works, yes. The real question is just whether one wants a pcim_
function for the irq vectors. My personal impression is that this looks
like a useful feature; but my expertise with MSI is a bit limited.
There's also this strange kernel-wide msi_enabled global bool.. 

I guess the best way to find out is to try implementing it.

In case of doubt, the boring unmanaged pci_ version is the safe choice.
One contributor around here has once called the managed versions "the
crazy devres voodoo" :p

(BTW, just to be sure, pcim_ functions must not be interconnected with
pcim_enable() in the future anymore, nor shall they use global state.
All PCI devres functionality should purely work on the device file
through pure devres. The pcim_enable() interconnection cam only from
the hybrid feature.)


P.

> 
> > If you give me a pointer I can provide a TODO entry. In any case, feel
> > free to set me as a reviewer!
> 
> > Regards
> > Philipp
> 
> Regards,
> Yao Zi

Re: [PATCH net-next v3 2/3] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

[Russell King (Oracle)]

On Fri, Dec 05, 2025 at 05:31:34AM +0000, Yao Zi wrote:
> Hi Russell,
> 
> Sorry for the late reply,
> 
> On Mon, Nov 24, 2025 at 07:06:12PM +0000, Russell King (Oracle) wrote:
> > On Mon, Nov 24, 2025 at 04:32:10PM +0000, Yao Zi wrote:
> > > +static int motorcomm_setup_irq(struct pci_dev *pdev,
> > > +			       struct stmmac_resources *res,
> > > +			       struct plat_stmmacenet_data *plat)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = pci_alloc_irq_vectors(pdev, 6, 6, PCI_IRQ_MSIX);
> > > +	if (ret > 0) {
> > > +		res->rx_irq[0]	= pci_irq_vector(pdev, 0);
> > > +		res->tx_irq[0]	= pci_irq_vector(pdev, 4);
> > > +		res->irq	= pci_irq_vector(pdev, 5);
> > > +
> > > +		plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
> > > +
> > > +		return 0;
> > > +	}
> > > +
> > > +	dev_info(&pdev->dev, "failed to allocate MSI-X vector: %d\n", ret);
> > > +	dev_info(&pdev->dev, "try MSI instead\n");
> > > +
> > > +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> > > +	if (ret < 0)
> > > +		return dev_err_probe(&pdev->dev, ret,
> > > +				     "failed to allocate MSI\n");
> > > +
> > > +	res->irq = pci_irq_vector(pdev, 0);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int motorcomm_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > +{
> > ...
> > > +	ret = motorcomm_setup_irq(pdev, &res, plat);
> > > +	if (ret)
> > > +		return dev_err_probe(&pdev->dev, ret, "failed to setup IRQ\n");
> > > +
> > > +	motorcomm_init(priv);
> > > +
> > > +	res.addr = priv->base + GMAC_OFFSET;
> > > +
> > > +	return stmmac_dvr_probe(&pdev->dev, plat, &res);
> > 
> > If stmmac_dvr_probe() fails, then it will return an error code. This
> > leaves the PCI MSI interrupt allocated...
> 
> This isn't true. MSI API is a little magical: when the device is enabled
> through pcim_enable_device(), the device becomes devres-managed, and
> a plain call to pci_alloc_irq_vectors() becomes managed, too, even if
> its name doesn't indicate it's a devres-managed API.
> 
> pci_free_irq_vectors() will be automatically called on driver deattach.
> See pcim_setup_msi_release() in drivers/pci/msi/msi.c, which is invoked
> by pci_alloc_irq_vectors() internally.

As discussed in the sub-thread with Philipp Stanner, please explicitly
call pci_alloc_irq_vectors() in the cleanup path to avoid adding to
the burden of drivers that need to be fixed allow this "magical"
behaviour to be removed in the future.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next v3 3/3] MAINTAINERS: Assign myself as maintainer of Motorcomm DWMAC glue driver

[Yao Zi]

I volunteer to maintain the DWMAC glue driver for Motorcomm ethernet
controllers.

Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index bc0343b10489..b9bbf39e61ff 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17621,6 +17621,12 @@ F:	drivers/most/
 F:	drivers/staging/most/
 F:	include/linux/most.h
 
+MOTORCOMM DWMAC GLUE DRIVER
+M:	Yao Zi <ziyao@disroot.org>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	drivers/net/ethernet/stmicro/stmmac/dwmac-motorcomm.c
+
 MOTORCOMM PHY DRIVER
 M:	Frank <Frank.Sae@motor-comm.com>
 L:	netdev@vger.kernel.org
-- 
2.51.2