[PATCH net-next v3 0/5] net: lan743x: Add SFP support for PCI11x1x

[PATCH net-next v3 0/5] net: lan743x: Add SFP support for PCI11x1x

[Thangaraj Samynathan]

This series adds SFP module support for the Microchip PCI11x1x Ethernet
controller family by integrating with the kernel's phylink/SFP subsystem.

The PCI11x1x device contains an embedded PCIe switch with downstream
ports for the Ethernet controller and a multi-function peripheral endpoint
(GPIO, I2C, UART, SPI controllers). SFP support relies on this peripheral
endpoint: the GPIO controller provides the SFP control/status signals and
the I2C controller provides access to the SFP module EEPROM. SFP capability
is hardware-strapped and detected at probe time.

Patch 1 renames the misleading is_sgmii_en flag to is_pcs_en to better
reflect that it tracks PCS interface enablement rather than a specific
SGMII mode. No functional change.

Patch 2 reads the SFP enable straps (STRAP_SFP_USE_EN_ and STRAP_SFP_EN_)
from the PCI11x1x strap registers and stores the result in a new
is_sfp_support_en flag. A validation check ensures PCS is also enabled
whenever SFP support is requested, since SFP operation requires the PCS
interface.

Patch 3 registers software nodes to describe the SFP hardware topology to
the kernel: the GPIO device (for TX-fault, TX-disable, LOS, mod-def0, and
rate-select0 signals), the I2C adapter (for SFP EEPROM access), the SFP
cage node, and a phylink node configured for in-band-status mode. The driver
navigates the PCI topology at probe time to locate the paired peripheral
controller functions.

Patch 4 registers an SFP platform device backed by the SFP software node
when SFP support is enabled, and associates the I2C adapter's firmware node
so the SFP subsystem can manage module detection and EEPROM reads.

Patch 5 adds a dedicated PCS MDIO bus and an XPCS instance for SFP link
management. C45 read/write callbacks are wired to the existing internal
SGMII access functions. The mac_select_pcs phylink callback returns the XPCS
instance when present. SGMII and 2.5GBASE-X interface modes are enabled in
phylink to support the range of SFP modules. The phylink connect path is
also updated to skip the fallback PHY scan when SFP support is active.

Based on original work by Raju Lakkaraju:
https://lwn.net/ml/all/20240911161054.4494-1-Raju.Lakkaraju%40microchip.com/

Change Log:
===========
v2 -> v3:
  - Add patch 1/5 to rename is_sgmii_en -> is_pcs_en as a prerequisite
    cleanup before the SFP series [patch 1/5]
  - Update error message to reference strap names:
    "SFP_EN strap specified without SGMII_EN strap" [patch 2/5]
  - Use str_enable_disable() helper for debug logging instead of open-coded
    ternary [patch 2/5]
  - Add is_sfp_support_en initialisation to false in probe [patch 2/5]
  - Move <linux/i2c.h>, <linux/gpio/machine.h>, <linux/auxiliary_bus.h>
    includes, PCI1XXXX_*/PCI11X1X_* macros, NODE_PROP macro and helper
    structs (pci1xxxx_i2c, gp_aux_data_type, auxiliary_device_wrapper,
    aux_bus_device) from lan743x_main.c to lan743x_main.h [patch 3/5]
  - Add lan743x_swnodes_unregister() helper with kfree inside the if block
    [patch 3/5]
  - Move software node registration out of lan743x_hardware_init() into
    lan743x_phylink_create() with proper error unwind, so it is not called
    on PM resume path [patch 3/5]
  - Move software node unregistration from lan743x_full_cleanup() into
    lan743x_destroy_phylink() to be symmetric with registration and handle
    probe failure path correctly [patch 3/5]
  - Use kzalloc_obj() instead of kzalloc() for sw_nodes allocation [patch 3/5]
  - Fix typo: "SPIcontrollers" -> "SPI controllers" in comment [patch 3/5]
  - Add cleanup_sfp: label in probe error path; null sfp_dev and i2c_adap
    pointers after unregister in lan743x_full_cleanup() [patch 4/5]
  - Move <linux/platform_device.h> to lan743x_main.h where
    struct platform_device *sfp_dev is declared [patch 4/5]
  - Fix probe failure path: goto cleanup_hardware -> goto cleanup_phylink
    on lan743x_sfp_register() failure [patch 4/5]
  - Consolidate phylink PCS and 2500Base-X support into a single patch
    [patch 5/5]
  - Switch from struct dw_xpcs * to struct phylink_pcs * internally; use
    xpcs_create_pcs_mdiodev() instead of xpcs_create_mdiodev() +
    xpcs_to_phylink_pcs(), and xpcs_destroy_pcs() instead of
    xpcs_destroy() [patch 5/5]
  - Use devm_mdiobus_alloc() instead of mdiobus_alloc() for pcs_mdiobus
    [patch 5/5]
  - Move <linux/phylink.h> and <linux/pcs/pcs-xpcs.h> from
    lan743x_main.c to lan743x_main.h [patch 5/5]

v1 -> v2:
  - Split the patches to 'PHYLINK' and 'SFP' parts
  - Change variable name from 'chip_rev' to 'fpga_rev'
  - SFP GPIO definitions and other macros move from lan743x_main.c to
    lan743x_main.h file
  - Change from 'PCI11X1X_' to 'PCI11X1X_EVB_PCI11010_' strings for
    GPIO macros
  - Add platform_device_unregister() when sfp register fail
  - Add two new patches to this patch series

Thangaraj Samynathan (5):
  net: lan743x: rename is_sgmii_en to is_pcs_en
  net: lan743x: read SFP straps from PCI11x1x device
  net: lan743x: Add support to software-nodes for SFP
  net: lan743x: Register SFP platform device for PCI11x1x
  net: lan743x: Add PCS/XPCS support for SFP on PCI11x1x

 drivers/net/ethernet/microchip/Kconfig        |   3 +
 .../net/ethernet/microchip/lan743x_ethtool.c  |   4 +-
 drivers/net/ethernet/microchip/lan743x_main.c | 367 +++++++++++++++++-
 drivers/net/ethernet/microchip/lan743x_main.h |  91 ++++-
 4 files changed, 447 insertions(+), 18 deletions(-)

-- 
2.34.1

[PATCH net-next v3 1/5] net: lan743x: rename is_sgmii_en to is_pcs_en

[Thangaraj Samynathan]

The flag is_sgmii_en indicates whether the PCS interface is enabled.
However, the name implies that the interface is strictly SGMII, which is
misleading.

Rename the variable to is_pcs_en to better reflect that the flag
represents the PCS enable state rather than a specific interface mode.
Update all references and logs accordingly (but keep device registers
unchanged to match its documentation).

No functional change intended.

Signed-off-by: Thangaraj Samynathan <thangaraj.s@microchip.com>
---
 .../net/ethernet/microchip/lan743x_ethtool.c   |  4 ++--
 drivers/net/ethernet/microchip/lan743x_main.c  | 18 +++++++++---------
 drivers/net/ethernet/microchip/lan743x_main.h  |  2 +-
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_ethtool.c b/drivers/net/ethernet/microchip/lan743x_ethtool.c
index 9195419ecee0..743acfd56554 100644
--- a/drivers/net/ethernet/microchip/lan743x_ethtool.c
+++ b/drivers/net/ethernet/microchip/lan743x_ethtool.c
@@ -1312,7 +1312,7 @@ static int lan743x_get_regs_len(struct net_device *dev)
 	struct lan743x_adapter *adapter = netdev_priv(dev);
 	u32 num_regs = MAX_LAN743X_ETH_COMMON_REGS;
 
-	if (adapter->is_sgmii_en)
+	if (adapter->is_pcs_en)
 		num_regs += MAX_LAN743X_ETH_SGMII_REGS;
 
 	return num_regs * sizeof(u32);
@@ -1333,7 +1333,7 @@ static void lan743x_get_regs(struct net_device *dev,
 	lan743x_common_regs(dev, p);
 	p = (u32 *)p + MAX_LAN743X_ETH_COMMON_REGS;
 
-	if (adapter->is_sgmii_en) {
+	if (adapter->is_pcs_en) {
 		lan743x_sgmii_regs(dev, p);
 		p = (u32 *)p + MAX_LAN743X_ETH_SGMII_REGS;
 	}
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index f3332417162e..fad4a246e06e 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -59,22 +59,22 @@ static void pci11x1x_strap_get_status(struct lan743x_adapter *adapter)
 	      hw_cfg & HW_CFG_RST_PROTECT_)) ||
 	    (strap & STRAP_READ_USE_SGMII_EN_)) {
 		if (strap & STRAP_READ_SGMII_EN_)
-			adapter->is_sgmii_en = true;
+			adapter->is_pcs_en = true;
 		else
-			adapter->is_sgmii_en = false;
+			adapter->is_pcs_en = false;
 	} else {
 		fpga_rev = lan743x_csr_read(adapter, FPGA_REV);
 		if (fpga_rev) {
 			if (fpga_rev & FPGA_SGMII_OP)
-				adapter->is_sgmii_en = true;
+				adapter->is_pcs_en = true;
 			else
-				adapter->is_sgmii_en = false;
+				adapter->is_pcs_en = false;
 		} else {
-			adapter->is_sgmii_en = false;
+			adapter->is_pcs_en = false;
 		}
 	}
 	netif_dbg(adapter, drv, adapter->netdev,
-		  "SGMII I/F %sable\n", adapter->is_sgmii_en ? "En" : "Dis");
+		  "PCS I/F %s\n", str_enable_disable(adapter->is_pcs_en));
 }
 
 static bool is_pci11x1x_chip(struct lan743x_adapter *adapter)
@@ -1361,7 +1361,7 @@ static void lan743x_phy_interface_select(struct lan743x_adapter *adapter)
 	data = lan743x_csr_read(adapter, MAC_CR);
 	id_rev = adapter->csr.id_rev & ID_REV_ID_MASK_;
 
-	if (adapter->is_pci11x1x && adapter->is_sgmii_en)
+	if (adapter->is_pci11x1x && adapter->is_pcs_en)
 		adapter->phy_interface = PHY_INTERFACE_MODE_SGMII;
 	else if (id_rev == ID_REV_ID_LAN7430_)
 		adapter->phy_interface = PHY_INTERFACE_MODE_GMII;
@@ -3515,7 +3515,7 @@ static int lan743x_hardware_init(struct lan743x_adapter *adapter,
 		mutex_init(&adapter->sgmii_rw_lock);
 		pci11x1x_set_rfe_rd_fifo_threshold(adapter);
 		sgmii_ctl = lan743x_csr_read(adapter, SGMII_CTL);
-		if (adapter->is_sgmii_en) {
+		if (adapter->is_pcs_en) {
 			sgmii_ctl |= SGMII_CTL_SGMII_ENABLE_;
 			sgmii_ctl &= ~SGMII_CTL_SGMII_POWER_DN_;
 		} else {
@@ -3580,7 +3580,7 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
 
 	adapter->mdiobus->priv = (void *)adapter;
 	if (adapter->is_pci11x1x) {
-		if (adapter->is_sgmii_en) {
+		if (adapter->is_pcs_en) {
 			netif_dbg(adapter, drv, adapter->netdev,
 				  "SGMII operation\n");
 			adapter->mdiobus->read = lan743x_mdiobus_read_c22;
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 160d94a7cee6..f0fa0580b04e 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -1070,7 +1070,7 @@ struct lan743x_adapter {
 	struct lan743x_tx       tx[PCI11X1X_USED_TX_CHANNELS];
 	struct lan743x_rx       rx[LAN743X_USED_RX_CHANNELS];
 	bool			is_pci11x1x;
-	bool			is_sgmii_en;
+	bool			is_pcs_en;
 	/* protect ethernet syslock */
 	spinlock_t		eth_syslock_spinlock;
 	bool			eth_syslock_en;
-- 
2.34.1

[PATCH net-next v3 2/5] net: lan743x: read SFP straps from PCI11x1x device

[Thangaraj Samynathan]

Reads the SFP enable bits from the strap registers to determine
if the hardware is configured for SFP usage.

- Add STRAP_SFP_USE_EN_ and STRAP_SFP_EN_ definitions to read SFP
straps.
- Store SFP status in the adapter's is_sfp_support_en flag.
- Add a validation check to ensure PCS is enabled when SFP support
is requested, as SFP operation requires the PCS interface.
- Refactor debug logging to use the str_enable_disable() helper for
  consistency with modern kernel standards.

Signed-off-by: Thangaraj Samynathan <thangaraj.s@microchip.com>
---
 drivers/net/ethernet/microchip/lan743x_main.c | 16 ++++++++++++++++
 drivers/net/ethernet/microchip/lan743x_main.h |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index fad4a246e06e..867310dbe9ba 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -62,6 +62,12 @@ static void pci11x1x_strap_get_status(struct lan743x_adapter *adapter)
 			adapter->is_pcs_en = true;
 		else
 			adapter->is_pcs_en = false;
+
+		if ((strap & STRAP_SFP_USE_EN_) && (strap & STRAP_SFP_EN_))
+			adapter->is_sfp_support_en = true;
+		else
+			adapter->is_sfp_support_en = false;
+
 	} else {
 		fpga_rev = lan743x_csr_read(adapter, FPGA_REV);
 		if (fpga_rev) {
@@ -73,8 +79,17 @@ static void pci11x1x_strap_get_status(struct lan743x_adapter *adapter)
 			adapter->is_pcs_en = false;
 		}
 	}
+
+	if (adapter->is_pci11x1x && !adapter->is_pcs_en &&
+	    adapter->is_sfp_support_en) {
+		netif_err(adapter, drv, adapter->netdev,
+			  "Invalid EEPROM configuration: SFP_EN strap specified without SGMII_EN strap\n");
+	}
+
 	netif_dbg(adapter, drv, adapter->netdev,
 		  "PCS I/F %s\n", str_enable_disable(adapter->is_pcs_en));
+	netif_dbg(adapter, drv, adapter->netdev,
+		  "SFP support %s\n", str_enable_disable(adapter->is_sfp_support_en));
 }
 
 static bool is_pci11x1x_chip(struct lan743x_adapter *adapter)
@@ -3665,6 +3680,7 @@ static int lan743x_pcidev_probe(struct pci_dev *pdev,
 			      NETIF_MSG_LINK | NETIF_MSG_IFUP |
 			      NETIF_MSG_IFDOWN | NETIF_MSG_TX_QUEUED;
 	netdev->max_mtu = LAN743X_MAX_FRAME_SIZE;
+	adapter->is_sfp_support_en = false;
 
 	of_get_mac_address(pdev->dev.of_node, adapter->mac_address);
 
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index f0fa0580b04e..26c30dc2e55c 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -37,6 +37,8 @@
 
 #define STRAP_READ			(0x0C)
 #define STRAP_READ_USE_SGMII_EN_	BIT(22)
+#define STRAP_SFP_USE_EN_              BIT(31)
+#define STRAP_SFP_EN_                  BIT(15)
 #define STRAP_READ_SGMII_EN_		BIT(6)
 #define STRAP_READ_SGMII_REFCLK_	BIT(5)
 #define STRAP_READ_SGMII_2_5G_		BIT(4)
@@ -1081,6 +1083,7 @@ struct lan743x_adapter {
 	u8			max_tx_channels;
 	u8			used_tx_channels;
 	u8			max_vector_count;
+	bool                    is_sfp_support_en;
 
 #define LAN743X_ADAPTER_FLAG_OTP		BIT(0)
 	u32			flags;
-- 
2.34.1

[PATCH net-next v3 3/5] net: lan743x: Add support to software-nodes for SFP

[Thangaraj Samynathan]

Register software nodes and define the required device properties.
The software node hierarchy describes the hardware connections for
SFP support, including:
 - GPIO pins used for SFP control and status signals (TX fault,
   TX disable, LOS, mod-def0, rate-select0)
 - the I2C bus used for SFP module EEPROM access
 - the SFP cage signal connections
 - the phylink configuration in in-band status mode

Signed-off-by: Thangaraj Samynathan <thangaraj.s@microchip.com>
---
 drivers/net/ethernet/microchip/Kconfig        |   2 +
 drivers/net/ethernet/microchip/lan743x_main.c | 196 +++++++++++++++++-
 drivers/net/ethernet/microchip/lan743x_main.h |  81 ++++++++
 3 files changed, 277 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
index ee046468652c..a83db3c7404f 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -50,6 +50,8 @@ config LAN743X
 	select CRC16
 	select CRC32
 	select PHYLINK
+	select I2C_PCI1XXXX
+	select GP_PCI1XXXX
 	help
 	  Support for the Microchip LAN743x and PCI11x1x families of PCI
 	  Express Ethernet devices
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 867310dbe9ba..b90b35cef2e6 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -16,6 +16,7 @@
 #include <linux/iopoll.h>
 #include <linux/crc16.h>
 #include <linux/phylink.h>
+
 #include "lan743x_main.h"
 #include "lan743x_ethtool.h"
 
@@ -112,6 +113,91 @@ static void lan743x_pci_cleanup(struct lan743x_adapter *adapter)
 	pci_disable_device(adapter->pdev);
 }
 
+static void *pci1xxxx_perif_drvdata_get(struct lan743x_adapter *adapter,
+					u16 perif_id)
+{
+	struct pci_dev *pdev = adapter->pdev;
+	struct pci_bus *perif_bus;
+	struct pci_dev *perif_dev;
+	struct pci_dev *br_dev;
+	struct pci_bus *br_bus;
+	struct pci_dev *dev;
+
+	/* PCI11x1x devices' PCIe topology consists of a top level pcie
+	 * switch with up to four downstream ports, some of which have
+	 * integrated endpoints connected to them. One of the downstream ports
+	 * has an embedded single function pcie ethernet controller which is
+	 * handled by this driver. Another downstream port has an
+	 * embedded multifunction pcie endpoint, with four pcie functions
+	 * (the "peripheral controllers": I2C controller, GPIO controller,
+	 * UART controllers, SPI controllers)
+	 * The code below navigates the PCI11x1x topology
+	 * to find (by matching its PCI device ID) the peripheral controller
+	 * that should be paired to the embedded ethernet controller.
+	 */
+
+	br_dev = pci_upstream_bridge(pdev);
+	if (!br_dev) {
+		netif_err(adapter, drv, adapter->netdev,
+			  "upstream bridge not found\n");
+		return br_dev;
+	}
+
+	br_bus = br_dev->bus;
+	list_for_each_entry(dev, &br_bus->devices, bus_list) {
+		if (dev->vendor == PCI1XXXX_VENDOR_ID &&
+		    (dev->device & ~PCI1XXXX_DEV_MASK) == PCI1XXXX_BR_PERIF_ID) {
+			perif_bus = dev->subordinate;
+			list_for_each_entry(perif_dev, &perif_bus->devices,
+					    bus_list) {
+				if (perif_dev->vendor == PCI1XXXX_VENDOR_ID &&
+				    (perif_dev->device & ~PCI1XXXX_DEV_MASK) ==
+				     perif_id)
+					return pci_get_drvdata(perif_dev);
+			}
+		}
+	}
+
+	netif_err(adapter, drv, adapter->netdev,
+		  "pci1xxxx peripheral (0x%X) device not found\n", perif_id);
+
+	return NULL;
+}
+
+static int pci1xxxx_i2c_adapter_get(struct lan743x_adapter *adapter)
+{
+	struct pci1xxxx_i2c *i2c_drvdata;
+
+	i2c_drvdata = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_I2C_ID);
+	if (!i2c_drvdata)
+		return -EPROBE_DEFER;
+
+	adapter->i2c_adap = &i2c_drvdata->adap;
+	strscpy(adapter->nodes->i2c_name, adapter->i2c_adap->name,
+		sizeof(adapter->nodes->i2c_name));
+	netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
+		  adapter->i2c_adap->name);
+
+	return 0;
+}
+
+static int pci1xxxx_gpio_dev_get(struct lan743x_adapter *adapter)
+{
+	struct aux_bus_device *aux_bus;
+	struct device *gpio_dev;
+
+	aux_bus = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_GPIO_ID);
+	if (!aux_bus)
+		return -EPROBE_DEFER;
+
+	gpio_dev = &aux_bus->aux_device_wrapper[1]->aux_dev.dev;
+	strscpy(adapter->nodes->gpio_name, dev_name(gpio_dev),
+		sizeof(adapter->nodes->gpio_name));
+	netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
+		  adapter->nodes->gpio_name);
+	return 0;
+}
+
 static int lan743x_pci_init(struct lan743x_adapter *adapter,
 			    struct pci_dev *pdev)
 {
@@ -2890,6 +2976,96 @@ static int lan743x_rx_open(struct lan743x_rx *rx)
 	return ret;
 }
 
+static void lan743x_swnodes_unregister(struct lan743x_adapter *adapter)
+{
+	if (adapter->nodes) {
+		software_node_unregister_node_group(adapter->nodes->group);
+		kfree(adapter->nodes);
+		adapter->nodes = NULL;
+	}
+}
+
+static int lan743x_swnodes_register(struct lan743x_adapter *adapter)
+{
+	struct pci_dev *pdev = adapter->pdev;
+	struct lan743x_sw_nodes *nodes;
+	struct software_node *swnodes;
+	int ret;
+	u32 id;
+
+	nodes = kzalloc_obj(*nodes);
+	if (!nodes)
+		return -ENOMEM;
+
+	adapter->nodes = nodes;
+
+	ret = pci1xxxx_gpio_dev_get(adapter);
+	if (ret < 0)
+		return ret;
+
+	ret = pci1xxxx_i2c_adapter_get(adapter);
+	if (ret < 0)
+		return ret;
+
+	id = (pdev->bus->number << 8) | pdev->devfn;
+	snprintf(nodes->sfp_name, sizeof(nodes->sfp_name), "sfp-%d", id);
+	snprintf(nodes->phylink_name, sizeof(nodes->phylink_name),
+		 "mchp-pci1xxxx-phylink-%d", id);
+
+	swnodes = nodes->swnodes;
+
+	nodes->gpio_props[0] = PROPERTY_ENTRY_STRING("pinctrl-names", "default");
+	swnodes[SWNODE_GPIO] = NODE_PROP(nodes->gpio_name, nodes->gpio_props);
+
+	nodes->tx_fault_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO],
+							 PCI11X1X_TX_FAULT_GPIO,
+							 GPIO_ACTIVE_HIGH);
+	nodes->tx_disable_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO],
+							   PCI11X1X_TX_DIS_GPIO,
+							   GPIO_ACTIVE_HIGH);
+	nodes->mod_def0_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO],
+							 PCI11X1X_MOD_DEF0_GPIO,
+							 GPIO_ACTIVE_LOW);
+	nodes->los_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO],
+						    PCI11X1X_LOS_GPIO,
+						    GPIO_ACTIVE_HIGH);
+	nodes->rate_sel0_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO],
+							  PCI11X1X_RATE_SEL0_GPIO,
+							  GPIO_ACTIVE_HIGH);
+
+	nodes->i2c_props[0] = PROPERTY_ENTRY_STRING("pinctrl-names", "default");
+	swnodes[SWNODE_I2C] = NODE_PROP(nodes->i2c_name, nodes->i2c_props);
+	nodes->i2c_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_I2C]);
+
+	nodes->sfp_props[0] = PROPERTY_ENTRY_STRING("compatible", "sff,sfp");
+	nodes->sfp_props[1] = PROPERTY_ENTRY_REF_ARRAY("i2c-bus", nodes->i2c_ref);
+	nodes->sfp_props[2] = PROPERTY_ENTRY_REF_ARRAY("tx-fault-gpios",
+						       nodes->tx_fault_ref);
+	nodes->sfp_props[3] = PROPERTY_ENTRY_REF_ARRAY("tx-disable-gpios",
+						       nodes->tx_disable_ref);
+	nodes->sfp_props[4] = PROPERTY_ENTRY_REF_ARRAY("mod-def0-gpios",
+						       nodes->mod_def0_ref);
+	nodes->sfp_props[5] = PROPERTY_ENTRY_REF_ARRAY("los-gpios",
+						       nodes->los_ref);
+	nodes->sfp_props[6] = PROPERTY_ENTRY_REF_ARRAY("rate-select0-gpios",
+						       nodes->rate_sel0_ref);
+	swnodes[SWNODE_SFP] = NODE_PROP(nodes->sfp_name, nodes->sfp_props);
+	nodes->sfp_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_SFP]);
+	nodes->phylink_props[0] = PROPERTY_ENTRY_STRING("managed",
+							"in-band-status");
+	nodes->phylink_props[1] = PROPERTY_ENTRY_REF_ARRAY("sfp",
+							   nodes->sfp_ref);
+	swnodes[SWNODE_PHYLINK] = NODE_PROP(nodes->phylink_name,
+					    nodes->phylink_props);
+
+	nodes->group[SWNODE_GPIO] = &swnodes[SWNODE_GPIO];
+	nodes->group[SWNODE_I2C] = &swnodes[SWNODE_I2C];
+	nodes->group[SWNODE_SFP] = &swnodes[SWNODE_SFP];
+	nodes->group[SWNODE_PHYLINK] = &swnodes[SWNODE_PHYLINK];
+
+	return software_node_register_node_group(nodes->group);
+}
+
 static int lan743x_phylink_sgmii_config(struct lan743x_adapter *adapter)
 {
 	u32 sgmii_ctl;
@@ -3134,7 +3310,9 @@ static const struct phylink_mac_ops lan743x_phylink_mac_ops = {
 static int lan743x_phylink_create(struct lan743x_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
+	struct fwnode_handle *fwnode = NULL;
 	struct phylink *pl;
+	int ret;
 
 	adapter->phylink_config.dev = &netdev->dev;
 	adapter->phylink_config.type = PHYLINK_NETDEV;
@@ -3174,11 +3352,24 @@ static int lan743x_phylink_create(struct lan743x_adapter *adapter)
 	       adapter->phylink_config.supported_interfaces,
 	       sizeof(adapter->phylink_config.lpi_interfaces));
 
-	pl = phylink_create(&adapter->phylink_config, NULL,
-			    adapter->phy_interface, &lan743x_phylink_mac_ops);
+	if (adapter->is_sfp_support_en) {
+		ret = lan743x_swnodes_register(adapter);
+		if (ret) {
+			netdev_err(netdev, "failed to register software nodes\n");
+			return ret;
+		}
+		fwnode = software_node_fwnode(adapter->nodes->group[SWNODE_PHYLINK]);
+		if (!fwnode) {
+			lan743x_swnodes_unregister(adapter);
+			return -ENODEV;
+		}
+	}
 
+	pl = phylink_create(&adapter->phylink_config, fwnode,
+			    adapter->phy_interface, &lan743x_phylink_mac_ops);
 	if (IS_ERR(pl)) {
 		netdev_err(netdev, "Could not create phylink (%pe)\n", pl);
+		lan743x_swnodes_unregister(adapter);
 		return PTR_ERR(pl);
 	}
 
@@ -3485,6 +3676,7 @@ static void lan743x_destroy_phylink(struct lan743x_adapter *adapter)
 {
 	phylink_destroy(adapter->phylink);
 	adapter->phylink = NULL;
+	lan743x_swnodes_unregister(adapter);
 }
 
 static void lan743x_full_cleanup(struct lan743x_adapter *adapter)
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 26c30dc2e55c..48db45d41502 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -4,14 +4,59 @@
 #ifndef _LAN743X_H
 #define _LAN743X_H
 
+#include <linux/auxiliary_bus.h>
+#include <linux/gpio/machine.h>
+#include <linux/i2c.h>
 #include <linux/phy.h>
 #include <linux/phylink.h>
+#include <linux/property.h>
 #include "lan743x_ptp.h"
 
 #define DRIVER_AUTHOR   "Bryan Whitehead <Bryan.Whitehead@microchip.com>"
 #define DRIVER_DESC "LAN743x PCIe Gigabit Ethernet Driver"
 #define DRIVER_NAME "lan743x"
 
+#define PCI1XXXX_VENDOR_ID		0x1055
+#define PCI1XXXX_BR_PERIF_ID		0xA00C
+#define PCI1XXXX_PERIF_I2C_ID		0xA003
+#define PCI1XXXX_PERIF_GPIO_ID		0xA005
+#define PCI1XXXX_DEV_MASK		GENMASK(7, 4)
+#define PCI11X1X_TX_FAULT_GPIO		46
+#define PCI11X1X_TX_DIS_GPIO		47
+#define PCI11X1X_RATE_SEL0_GPIO		48
+#define PCI11X1X_LOS_GPIO		49
+#define PCI11X1X_MOD_DEF0_GPIO		51
+
+#define NODE_PROP(_NAME, _PROP)		\
+	((const struct software_node) {	\
+		.name = _NAME,		\
+		.properties = _PROP,	\
+	 })
+
+struct pci1xxxx_i2c {
+	struct completion i2c_xfer_done;
+	bool i2c_xfer_in_progress;
+	struct i2c_adapter adap;
+	void __iomem *i2c_base;
+	u32 freq;
+	u32 flags;
+};
+
+struct gp_aux_data_type {
+	int irq_num;
+	resource_size_t region_start;
+	resource_size_t region_length;
+};
+
+struct auxiliary_device_wrapper {
+	struct auxiliary_device aux_dev;
+	struct gp_aux_data_type gp_aux_data;
+};
+
+struct aux_bus_device {
+	struct auxiliary_device_wrapper *aux_device_wrapper[2];
+};
+
 /* Register Definitions */
 #define ID_REV				(0x00)
 #define ID_REV_ID_MASK_			(0xFFFF0000)
@@ -1049,6 +1094,40 @@ enum lan743x_sgmii_lsd {
 
 #define MAC_SUPPORTED_WAKES  (WAKE_BCAST | WAKE_UCAST | WAKE_MCAST | \
 			      WAKE_MAGIC | WAKE_ARP)
+
+enum lan743x_swnodes {
+	SWNODE_GPIO = 0,
+	SWNODE_I2C,
+	SWNODE_SFP,
+	SWNODE_PHYLINK,
+	SWNODE_MAX
+};
+
+#define I2C_DRV_NAME           48
+#define GPIO_DRV_NAME          32
+#define SFP_NODE_NAME          32
+#define PHYLINK_NODE_NAME      32
+
+struct lan743x_sw_nodes {
+	char gpio_name[GPIO_DRV_NAME];
+	char i2c_name[I2C_DRV_NAME];
+	char sfp_name[SFP_NODE_NAME];
+	char phylink_name[PHYLINK_NODE_NAME];
+	struct property_entry gpio_props[1];
+	struct property_entry i2c_props[1];
+	struct property_entry sfp_props[8];
+	struct property_entry phylink_props[2];
+	struct software_node_ref_args i2c_ref[1];
+	struct software_node_ref_args tx_fault_ref[1];
+	struct software_node_ref_args tx_disable_ref[1];
+	struct software_node_ref_args mod_def0_ref[1];
+	struct software_node_ref_args los_ref[1];
+	struct software_node_ref_args rate_sel0_ref[1];
+	struct software_node_ref_args sfp_ref[1];
+	struct software_node swnodes[SWNODE_MAX];
+	const struct software_node *group[SWNODE_MAX + 1];
+};
+
 struct lan743x_adapter {
 	struct net_device       *netdev;
 	struct mii_bus		*mdiobus;
@@ -1092,6 +1171,8 @@ struct lan743x_adapter {
 	struct phylink		*phylink;
 	struct phylink_config	phylink_config;
 	int			rx_tstamp_filter;
+	struct lan743x_sw_nodes *nodes;
+	struct i2c_adapter      *i2c_adap;
 };
 
 #define LAN743X_COMPONENT_FLAG_RX(channel)  BIT(20 + (channel))
-- 
2.34.1

[PATCH net-next v3 4/5] net: lan743x: Register SFP platform device for PCI11x1x

[Thangaraj Samynathan]

Register an SFP platform device when SFP support is enabled on PCI11x1x
platforms.

Use the software node describing the SFP cage as the firmware node
for the platform device. Associate the I2C adapter with its
corresponding software node for SFP module management over I2C.

Register the SFP platform device during probe and unregister it during
driver cleanup.

Signed-off-by: Thangaraj Samynathan <thangaraj.s@microchip.com>
---
 drivers/net/ethernet/microchip/Kconfig        |  1 +
 drivers/net/ethernet/microchip/lan743x_main.c | 51 ++++++++++++++++++-
 drivers/net/ethernet/microchip/lan743x_main.h |  2 +
 3 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
index a83db3c7404f..9857053dabf8 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -52,6 +52,7 @@ config LAN743X
 	select PHYLINK
 	select I2C_PCI1XXXX
 	select GP_PCI1XXXX
+	select SFP
 	help
 	  Support for the Microchip LAN743x and PCI11x1x families of PCI
 	  Express Ethernet devices
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index b90b35cef2e6..88a2d11552f8 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -3066,6 +3066,31 @@ static int lan743x_swnodes_register(struct lan743x_adapter *adapter)
 	return software_node_register_node_group(nodes->group);
 }
 
+static int lan743x_sfp_register(struct lan743x_adapter *adapter)
+{
+	struct pci_dev *pdev = adapter->pdev;
+	struct platform_device_info sfp_info;
+	struct platform_device *sfp_dev;
+
+	memset(&sfp_info, 0, sizeof(sfp_info));
+	sfp_info.parent = &adapter->pdev->dev;
+	sfp_info.fwnode = software_node_fwnode(adapter->nodes->group[SWNODE_SFP]);
+	sfp_info.name = "sfp";
+	sfp_info.id = (pdev->bus->number << 8) | pdev->devfn;
+	sfp_dev = platform_device_register_full(&sfp_info);
+	if (IS_ERR(sfp_dev)) {
+		netif_err(adapter, drv, adapter->netdev,
+			  "Failed to register SFP device\n");
+		return PTR_ERR(sfp_dev);
+	}
+
+	adapter->sfp_dev = sfp_dev;
+	netif_dbg(adapter, drv, adapter->netdev,
+		  "SFP platform device registered");
+
+	return 0;
+}
+
 static int lan743x_phylink_sgmii_config(struct lan743x_adapter *adapter)
 {
 	u32 sgmii_ctl;
@@ -3682,6 +3707,12 @@ static void lan743x_destroy_phylink(struct lan743x_adapter *adapter)
 static void lan743x_full_cleanup(struct lan743x_adapter *adapter)
 {
 	unregister_netdev(adapter->netdev);
+	if (adapter->sfp_dev) {
+		platform_device_unregister(adapter->sfp_dev);
+		adapter->sfp_dev = NULL;
+	}
+	if (adapter->i2c_adap)
+		adapter->i2c_adap = NULL;
 
 	lan743x_destroy_phylink(adapter);
 	lan743x_mdiobus_cleanup(adapter);
@@ -3909,11 +3940,29 @@ static int lan743x_pcidev_probe(struct pci_dev *pdev,
 		goto cleanup_mdiobus;
 	}
 
+	if (adapter->is_sfp_support_en) {
+		adapter->i2c_adap->dev.fwnode =
+			software_node_fwnode(adapter->nodes->group[SWNODE_I2C]);
+
+		ret = lan743x_sfp_register(adapter);
+		if (ret < 0) {
+			netif_err(adapter, probe, netdev,
+				  "failed to sfp register (%d)\n", ret);
+			goto cleanup_phylink;
+		}
+	}
+
 	ret = register_netdev(adapter->netdev);
 	if (ret < 0)
-		goto cleanup_phylink;
+		goto cleanup_sfp;
 	return 0;
 
+cleanup_sfp:
+	if (adapter->sfp_dev) {
+		platform_device_unregister(adapter->sfp_dev);
+		adapter->sfp_dev = NULL;
+	}
+
 cleanup_phylink:
 	lan743x_destroy_phylink(adapter);
 
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 48db45d41502..fd1c2842b4c8 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -9,6 +9,7 @@
 #include <linux/i2c.h>
 #include <linux/phy.h>
 #include <linux/phylink.h>
+#include <linux/platform_device.h>
 #include <linux/property.h>
 #include "lan743x_ptp.h"
 
@@ -1173,6 +1174,7 @@ struct lan743x_adapter {
 	int			rx_tstamp_filter;
 	struct lan743x_sw_nodes *nodes;
 	struct i2c_adapter      *i2c_adap;
+	struct platform_device  *sfp_dev;
 };
 
 #define LAN743X_COMPONENT_FLAG_RX(channel)  BIT(20 + (channel))
-- 
2.34.1

[PATCH net-next v3 5/5] net: lan743x: Add PCS/XPCS support for SFP on PCI11x1x

[Thangaraj Samynathan]

Add a PCS MII bus and XPCS instance to support SFP modules on PCI11x1x
platforms.

Register a dedicated mdiobus for PCS access when SFP support is enabled
and initialize it with C45 read/write callbacks. Implement generic PCS
read/write functions that use the internal SGMII access functions for
the PCS.

Integrate the XPCS instance with phylink by providing a mac_select_pcs
callback. Support SGMII and 2.5GBASE-X interfaces in phylink, allowing
proper link configuration for SFP modules.

Cleanup the PCS mdiobus and XPCS instance during driver removal. Update
adapter structure to hold PCS mdiobus and XPCS references.

Signed-off-by: Thangaraj Samynathan <thangaraj.s@microchip.com>
---
 drivers/net/ethernet/microchip/lan743x_main.c | 86 ++++++++++++++++++-
 drivers/net/ethernet/microchip/lan743x_main.h |  3 +
 2 files changed, 86 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 88a2d11552f8..485fbc678481 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -15,7 +15,6 @@
 #include <linux/rtnetlink.h>
 #include <linux/iopoll.h>
 #include <linux/crc16.h>
-#include <linux/phylink.h>
 
 #include "lan743x_main.h"
 #include "lan743x_ethtool.h"
@@ -1137,6 +1136,28 @@ static int lan743x_get_lsd(int speed, int duplex, u8 mss)
 	return lsd;
 }
 
+static int pci11x1x_pcs_read(struct mii_bus *bus, int addr, int devnum,
+			     int regnum)
+{
+	struct lan743x_adapter *adapter = bus->priv;
+
+	if (addr)
+		return -EOPNOTSUPP;
+
+	return lan743x_sgmii_read(adapter, devnum, regnum);
+}
+
+static int pci11x1x_pcs_write(struct mii_bus *bus, int addr, int devnum,
+			      int regnum, u16 val)
+{
+	struct lan743x_adapter *adapter = bus->priv;
+
+	if (addr)
+		return -EOPNOTSUPP;
+
+	return lan743x_sgmii_write(adapter, devnum, regnum, val);
+}
+
 static int lan743x_sgmii_mpll_set(struct lan743x_adapter *adapter,
 				  u16 baud)
 {
@@ -3199,6 +3220,18 @@ static void lan743x_mac_eee_enable(struct lan743x_adapter *adapter, bool enable)
 	lan743x_csr_write(adapter, MAC_CR, mac_cr);
 }
 
+static struct phylink_pcs *lan743x_phylink_mac_select(struct phylink_config *config,
+						      phy_interface_t interface)
+{
+	struct net_device *netdev = to_net_dev(config->dev);
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+
+	if (adapter->xpcs)
+		return adapter->xpcs;
+
+	return NULL;
+}
+
 static void lan743x_phylink_mac_config(struct phylink_config *config,
 				       unsigned int link_an_mode,
 				       const struct phylink_link_state *state)
@@ -3330,6 +3363,7 @@ static const struct phylink_mac_ops lan743x_phylink_mac_ops = {
 	.mac_link_up = lan743x_phylink_mac_link_up,
 	.mac_disable_tx_lpi = lan743x_mac_disable_tx_lpi,
 	.mac_enable_tx_lpi = lan743x_mac_enable_tx_lpi,
+	.mac_select_pcs = lan743x_phylink_mac_select,
 };
 
 static int lan743x_phylink_create(struct lan743x_adapter *adapter)
@@ -3353,6 +3387,7 @@ static int lan743x_phylink_create(struct lan743x_adapter *adapter)
 
 	switch (adapter->phy_interface) {
 	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_2500BASEX:
 		__set_bit(PHY_INTERFACE_MODE_SGMII,
 			  adapter->phylink_config.supported_interfaces);
 		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
@@ -3416,12 +3451,13 @@ static int lan743x_phylink_connect(struct lan743x_adapter *adapter)
 	struct device_node *dn = adapter->pdev->dev.of_node;
 	struct net_device *dev = adapter->netdev;
 	struct phy_device *phydev;
-	int ret;
+	int ret = 0;
 
 	if (dn)
 		ret = phylink_of_phy_connect(adapter->phylink, dn, 0);
 
-	if (!dn || (ret && !lan743x_phy_handle_exists(dn))) {
+	if (!adapter->is_sfp_support_en &&
+	    (!dn || (ret && !lan743x_phy_handle_exists(dn)))) {
 		phydev = phy_find_first(adapter->mdiobus);
 		if (phydev) {
 			/* attach the mac to the phy */
@@ -3694,6 +3730,9 @@ static void lan743x_hardware_cleanup(struct lan743x_adapter *adapter)
 
 static void lan743x_mdiobus_cleanup(struct lan743x_adapter *adapter)
 {
+	if (adapter->xpcs)
+		xpcs_destroy_pcs(adapter->xpcs);
+
 	mdiobus_unregister(adapter->mdiobus);
 }
 
@@ -3806,6 +3845,42 @@ static int lan743x_hardware_init(struct lan743x_adapter *adapter,
 	return 0;
 }
 
+static int lan743x_pcs_mdiobus_init(struct lan743x_adapter *adapter)
+{
+	struct phylink_pcs *pcs;
+	int ret;
+
+	adapter->pcs_mdiobus = devm_mdiobus_alloc(&adapter->pdev->dev);
+	if (!(adapter->pcs_mdiobus)) {
+		ret = -ENOMEM;
+		goto return_error;
+	}
+
+	adapter->pcs_mdiobus->priv = (void *)adapter;
+	adapter->pcs_mdiobus->read_c45 = pci11x1x_pcs_read;
+	adapter->pcs_mdiobus->write_c45 = pci11x1x_pcs_write;
+	adapter->pcs_mdiobus->name = "lan743x-pcs-mdiobus-c45";
+	netif_dbg(adapter, drv, adapter->netdev, "lan743x-pcs-mdiobus-c45\n");
+	snprintf(adapter->pcs_mdiobus->id, MII_BUS_ID_SIZE, "pci-pcs-%s", pci_name(adapter->pdev));
+
+	if (!adapter->phy_interface)
+		lan743x_phy_interface_select(adapter);
+
+	pcs = xpcs_create_pcs_mdiodev(adapter->pcs_mdiobus, 0);
+	if (IS_ERR(pcs)) {
+		netdev_err(adapter->netdev, "failed to create xpcs\n");
+		ret = PTR_ERR(pcs);
+		goto return_error;
+	}
+
+	adapter->xpcs = pcs;
+	return 0;
+
+return_error:
+	mdiobus_free(adapter->pcs_mdiobus);
+	return ret;
+}
+
 static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
 {
 	int ret;
@@ -3927,6 +4002,11 @@ static int lan743x_pcidev_probe(struct pci_dev *pdev,
 	if (ret)
 		goto cleanup_hardware;
 
+	if (adapter->is_sfp_support_en) {
+		ret = lan743x_pcs_mdiobus_init(adapter);
+		if (ret)
+			goto cleanup_hardware;
+	}
 	adapter->netdev->netdev_ops = &lan743x_netdev_ops;
 	adapter->netdev->ethtool_ops = &lan743x_ethtool_ops;
 	adapter->netdev->features = NETIF_F_SG | NETIF_F_TSO |
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index fd1c2842b4c8..9740c2628f94 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -8,6 +8,7 @@
 #include <linux/gpio/machine.h>
 #include <linux/i2c.h>
 #include <linux/phy.h>
+#include <linux/pcs/pcs-xpcs.h>
 #include <linux/phylink.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
@@ -1132,6 +1133,7 @@ struct lan743x_sw_nodes {
 struct lan743x_adapter {
 	struct net_device       *netdev;
 	struct mii_bus		*mdiobus;
+	struct mii_bus		*pcs_mdiobus;
 	int                     msg_enable;
 #ifdef CONFIG_PM
 	u32			wolopts;
@@ -1169,6 +1171,7 @@ struct lan743x_adapter {
 	u32			flags;
 	u32			hw_cfg;
 	phy_interface_t		phy_interface;
+	struct phylink_pcs      *xpcs;
 	struct phylink		*phylink;
 	struct phylink_config	phylink_config;
 	int			rx_tstamp_filter;
-- 
2.34.1

Re: [PATCH net-next v3 2/5] net: lan743x: read SFP straps from PCI11x1x device

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: lan743x: read SFP straps from PCI11x1x device

This commit reads SFP enable bits from the PCI11x1x strap registers and
stores the result in a new adapter->is_sfp_support_en flag, with a
validation message when SFP support appears to be requested without SGMII.

> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index fad4a246e06e..867310dbe9ba 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -62,6 +62,12 @@ static void pci11x1x_strap_get_status(struct lan743x_adapter *adapter)
>  			adapter->is_pcs_en = true;
>  		else
>  			adapter->is_pcs_en = false;
> +
> +		if ((strap & STRAP_SFP_USE_EN_) && (strap & STRAP_SFP_EN_))
> +			adapter->is_sfp_support_en = true;
> +		else
> +			adapter->is_sfp_support_en = false;
> +

[Low]
Is there a reason STRAP_SFP_USE_EN_ and STRAP_SFP_EN_ are only evaluated
inside the branch gated by (strap & STRAP_READ_USE_SGMII_EN_) (or the A0
workaround)?

If a board is strapped with STRAP_SFP_USE_EN_=1 but
STRAP_READ_USE_SGMII_EN_=0, the SFP straps are silently ignored,
is_sfp_support_en keeps its default of false, and no diagnostic is
emitted.

The two "use strap" gate bits look independent (one for SGMII, one for
SFP); nesting the SFP decode inside the SGMII decode appears to conflate
them.  The "SFP_EN without SGMII_EN" validation block below will also
never fire in this configuration because is_sfp_support_en is forced to
false.

>  	} else {
>  		fpga_rev = lan743x_csr_read(adapter, FPGA_REV);
>  		if (fpga_rev) {
> @@ -73,8 +79,17 @@ static void pci11x1x_strap_get_status(struct lan743x_adapter *adapter)
>  			adapter->is_pcs_en = false;
>  		}
>  	}

[Low]
In the else (FPGA/non-strap) branch, adapter->is_sfp_support_en is never
assigned, while adapter->is_pcs_en is written in every sub-branch.  The
code relies on the one-time initialization of is_sfp_support_en = false
in lan743x_pcidev_probe().

Today pci11x1x_strap_get_status() is only called once from
lan743x_hardware_init() during probe, so the current behavior is well
defined.  Would it be worth setting adapter->is_sfp_support_en = false in
this else branch as well to mirror is_pcs_en and keep the function
idempotent if it is ever called from a reset, resume, or error-recovery
path later?

> +
> +	if (adapter->is_pci11x1x && !adapter->is_pcs_en &&
> +	    adapter->is_sfp_support_en) {
> +		netif_err(adapter, drv, adapter->netdev,
> +			  "Invalid EEPROM configuration: SFP_EN strap specified without SGMII_EN strap\n");
> +	}

[Medium]
The commit message describes this as:

    Add a validation check to ensure PCS is enabled when SFP support
    is requested, as SFP operation requires the PCS interface.

Does this block actually enforce that invariant?  It logs the invalid
combination but leaves adapter->is_sfp_support_en set to true and does
not propagate an error out of pci11x1x_strap_get_status() or
lan743x_hardware_init().

Later patches in the same series (for example "net: lan743x: Add PCS/XPCS
support for SFP on PCI11x1x") gate lan743x_pcs_mdiobus_init() and the
swnodes/SFP platform-device registration on is_sfp_support_en alone,
without an is_pcs_en check.  With a misconfigured EEPROM the driver would
then attempt PCS/XPCS MDIO operations on hardware whose PCS is disabled.

Would it be preferable to either force adapter->is_sfp_support_en = false
when the invalid combination is detected, or return an error from
pci11x1x_strap_get_status() so lan743x_hardware_init() can fail probe?

> +
>  	netif_dbg(adapter, drv, adapter->netdev,
>  		  "PCS I/F %s\n", str_enable_disable(adapter->is_pcs_en));
> +	netif_dbg(adapter, drv, adapter->netdev,
> +		  "SFP support %s\n", str_enable_disable(adapter->is_sfp_support_en));
>  }

[Low]
The commit message says:

    Refactor debug logging to use the str_enable_disable() helper for
    consistency with modern kernel standards.

Is this description accurate?  The existing "PCS I/F %s" netif_dbg()
already uses str_enable_disable() prior to this patch (it was introduced
by the earlier "net: lan743x: rename is_sgmii_en to is_pcs_en" commit).

This patch only adds a new netif_dbg() for is_sfp_support_en using the
same helper; no existing debug line is refactored.  Could the bullet be
reworded to reflect that a new debug line was added rather than an
existing one refactored?

> diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
> index f0fa0580b04e..26c30dc2e55c 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.h
> +++ b/drivers/net/ethernet/microchip/lan743x_main.h
> @@ -37,6 +37,8 @@
>  
>  #define STRAP_READ			(0x0C)
>  #define STRAP_READ_USE_SGMII_EN_	BIT(22)
> +#define STRAP_SFP_USE_EN_              BIT(31)
> +#define STRAP_SFP_EN_                  BIT(15)
>  #define STRAP_READ_SGMII_EN_		BIT(6)

[Low]
The two new STRAP_SFP_*_ macros use spaces for alignment while every
surrounding STRAP_* definition uses tabs.  Was this intentional, or
should they be tab-aligned to match the rest of the file (and avoid a
checkpatch warning)?

>  #define STRAP_READ_SGMII_REFCLK_	BIT(5)
>  #define STRAP_READ_SGMII_2_5G_		BIT(4)
> @@ -1081,6 +1083,7 @@ struct lan743x_adapter {
>  	u8			max_tx_channels;
>  	u8			used_tx_channels;
>  	u8			max_vector_count;
> +	bool                    is_sfp_support_en;

[Low]
Same question here: the new is_sfp_support_en field uses spaces for
alignment while the surrounding struct members use tabs.  Should this be
tab-aligned for consistency with the rest of struct lan743x_adapter?

>  
>  #define LAN743X_ADAPTER_FLAG_OTP		BIT(0)
>  	u32			flags;
-- 
pw-bot: cr

Re: [PATCH net-next v3 3/5] net: lan743x: Add support to software-nodes for SFP

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: lan743x: Add support to software-nodes for SFP

This change registers a software-node hierarchy (GPIO/I2C/SFP/phylink)
for the LAN743x so that SFP cages on PCI11x1x devices can be described
to phylink/sfp. It wires the lan743x probe to the sibling pci1xxxx I2C
and GPIO peripheral drivers by walking the PCIe topology and dereferencing
their drvdata.

A few questions on the patch below.

> diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
> index ee046468652c..a83db3c7404f 100644
> --- a/drivers/net/ethernet/microchip/Kconfig
> +++ b/drivers/net/ethernet/microchip/Kconfig
> @@ -50,6 +50,8 @@ config LAN743X
>  	select CRC16
>  	select CRC32
>  	select PHYLINK
> +	select I2C_PCI1XXXX
> +	select GP_PCI1XXXX
>  	help
>  	  Support for the Microchip LAN743x and PCI11x1x families of PCI
>  	  Express Ethernet devices

[High]
Should LAN743X inherit the dependencies of the symbols it now selects?

GP_PCI1XXXX in drivers/misc/mchp_pci1xxxx/Kconfig carries:

    depends on PCI
    depends on GPIOLIB
    depends on NVMEM_SYSFS

and I2C_PCI1XXXX is sourced inside an "if I2C" block in drivers/i2c/Kconfig,
so it has an implicit depends on I2C. The LAN743X entry here does not
depend on I2C, GPIOLIB, or NVMEM_SYSFS. On configurations where any of
those are disabled (randconfig, allmodconfig variants), would this
produce unmet-direct-dependency warnings from sym_warn_unmet_dep() and
potentially fail to link because the selected drivers' helpers are not
built?

Would adding "depends on I2C && GPIOLIB && NVMEM_SYSFS" (or gating the
SFP glue behind a nested Kconfig symbol) be appropriate here?

> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index 867310dbe9ba..b90b35cef2e6 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -112,6 +113,91 @@ static void lan743x_pci_cleanup(struct lan743x_adapter *adapter)
> +static void *pci1xxxx_perif_drvdata_get(struct lan743x_adapter *adapter,
> +					u16 perif_id)
> +{
> +	struct pci_dev *pdev = adapter->pdev;
> +	struct pci_bus *perif_bus;
> +	struct pci_dev *perif_dev;
> +	struct pci_dev *br_dev;
> +	struct pci_bus *br_bus;
> +	struct pci_dev *dev;
[ ... ]
> +	br_bus = br_dev->bus;
> +	list_for_each_entry(dev, &br_bus->devices, bus_list) {
> +		if (dev->vendor == PCI1XXXX_VENDOR_ID &&
> +		    (dev->device & ~PCI1XXXX_DEV_MASK) == PCI1XXXX_BR_PERIF_ID) {
> +			perif_bus = dev->subordinate;
> +			list_for_each_entry(perif_dev, &perif_bus->devices,
> +					    bus_list) {
> +				if (perif_dev->vendor == PCI1XXXX_VENDOR_ID &&
> +				    (perif_dev->device & ~PCI1XXXX_DEV_MASK) ==
> +				     perif_id)
> +					return pci_get_drvdata(perif_dev);
> +			}
> +		}
> +	}

[High]
Is it safe to walk br_bus->devices and perif_bus->devices without
holding pci_bus_sem?

Both pci_device_add() and pci_destroy_dev() take down_write(&pci_bus_sem)
when mutating pci_bus->devices, and pci_walk_bus() / other in-tree
consumers either wrap the iteration in down_read(&pci_bus_sem) or assert
lockdep_assert_held(&pci_bus_sem). If a PCI hotplug or sibling
bind/unbind happens concurrently here, could the traversal corrupt, loop,
or step through a pci_dev that is being freed?

Relatedly, pci_get_drvdata(perif_dev) is returned directly with no
reference acquired on the peer device or its driver. Can the following
sequence occur?

    lan743x probe stores adapter->i2c_adap = &i2c_drvdata->adap
    user unbinds i2c-mchp-pci1xxxx via sysfs
    i2c_drvdata (and its embedded i2c_adapter) is freed
    SFP EEPROM access via the software-node fwnode still dereferences it

Would using pci_get_device() (or pci_dev_get() under pci_bus_sem) plus a
device_link_add() from the peer device to the lan743x consumer be a more
robust pairing, or alternatively routing this coupling through the
auxiliary bus?

> +static int pci1xxxx_i2c_adapter_get(struct lan743x_adapter *adapter)
> +{
> +	struct pci1xxxx_i2c *i2c_drvdata;
> +
> +	i2c_drvdata = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_I2C_ID);
> +	if (!i2c_drvdata)
> +		return -EPROBE_DEFER;
> +
> +	adapter->i2c_adap = &i2c_drvdata->adap;

[Medium]
The cast here relies on struct pci1xxxx_i2c being textually redeclared
in lan743x_main.h, identical to the authoritative definition in
drivers/i2c/busses/i2c-mchp-pci1xxxx.c. Nothing ties the two copies
together at build time (no shared header, no BUILD_BUG_ON on sizeof or
offsetof), and there is no runtime check that perif_dev->driver is the
expected pci_driver before the drvdata is cast.

If the i2c driver later reorders or resizes a field, would
&i2c_drvdata->adap silently read from a wrong offset? And if the peer
PCI function is bound to vfio-pci or any other driver,
pci_get_drvdata() returns an unrelated pointer which is then
dereferenced as struct pci1xxxx_i2c. Would it make sense to add a
narrow accessor exported by the peripheral driver, e.g. something like
pci1xxxx_i2c_get_adapter(pdev), so the layout is not duplicated across
subsystems?

The same question applies to the redeclarations of struct
auxiliary_device_wrapper, struct gp_aux_data_type and struct
aux_bus_device in lan743x_main.h.

> +static int pci1xxxx_gpio_dev_get(struct lan743x_adapter *adapter)
> +{
> +	struct aux_bus_device *aux_bus;
> +	struct device *gpio_dev;
> +
> +	aux_bus = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_GPIO_ID);
> +	if (!aux_bus)
> +		return -EPROBE_DEFER;
> +
> +	gpio_dev = &aux_bus->aux_device_wrapper[1]->aux_dev.dev;

[Medium]
Is the hardcoded index [1] here guaranteed by anything?

In drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c the wrapper array has
slot 0 = gp_otp_e2p and slot 1 = gp_gpio today, but that ordering is an
internal detail of the GP driver and is not enforced by a shared type,
a name check, or a documented contract. If the GP driver ever reorders
the registration, fails partway so slot 1 is unpopulated, or adds a
new slot, would this dereference pick up the wrong aux_dev (or a NULL)?

Would it be preferable to look up the GPIO aux device by name (e.g.
matching aux_dev.name == "gp_gpio") or expose a named accessor from
the GP driver?

> @@ -2890,6 +2976,96 @@ static int lan743x_rx_open(struct lan743x_rx *rx)
> +static int lan743x_swnodes_register(struct lan743x_adapter *adapter)
> +{
> +	struct pci_dev *pdev = adapter->pdev;
> +	struct lan743x_sw_nodes *nodes;
> +	struct software_node *swnodes;
> +	int ret;
> +	u32 id;
> +
> +	nodes = kzalloc_obj(*nodes);
> +	if (!nodes)
> +		return -ENOMEM;
> +
> +	adapter->nodes = nodes;
> +
> +	ret = pci1xxxx_gpio_dev_get(adapter);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = pci1xxxx_i2c_adapter_get(adapter);
> +	if (ret < 0)
> +		return ret;

[High]
Does this leak the lan743x_sw_nodes allocation on error?

adapter->nodes is set unconditionally before the two peripheral lookups,
both of which commonly return -EPROBE_DEFER while the sibling pci1xxxx
drivers are still probing. On that error, lan743x_swnodes_register()
returns without freeing nodes and without clearing adapter->nodes.

The caller in lan743x_phylink_create() handles the failure as:

    ret = lan743x_swnodes_register(adapter);
    if (ret) {
        netdev_err(netdev, "failed to register software nodes\n");
        return ret;
    }

without calling lan743x_swnodes_unregister(). The probe error path
(lan743x_pcidev_probe) then jumps to cleanup_mdiobus, which does not
reach lan743x_destroy_phylink() (the only caller of
lan743x_swnodes_unregister). Each -EPROBE_DEFER retry would allocate a
fresh struct and discard the previous one.

The same concern applies to the software_node_register_node_group()
return at the end of this function: its failure is returned to a caller
that does not call lan743x_swnodes_unregister().

Would cleaning up in lan743x_swnodes_register() itself on every error
path (kfree(nodes); adapter->nodes = NULL) be appropriate?

> +	nodes->gpio_props[0] = PROPERTY_ENTRY_STRING("pinctrl-names", "default");
> +	swnodes[SWNODE_GPIO] = NODE_PROP(nodes->gpio_name, nodes->gpio_props);
[ ... ]
> +	nodes->i2c_props[0] = PROPERTY_ENTRY_STRING("pinctrl-names", "default");
> +	swnodes[SWNODE_I2C] = NODE_PROP(nodes->i2c_name, nodes->i2c_props);
[ ... ]
> +	nodes->phylink_props[0] = PROPERTY_ENTRY_STRING("managed",
> +							"in-band-status");
> +	nodes->phylink_props[1] = PROPERTY_ENTRY_REF_ARRAY("sfp",
> +							   nodes->sfp_ref);
> +	swnodes[SWNODE_PHYLINK] = NODE_PROP(nodes->phylink_name,
> +					    nodes->phylink_props);

[High]
Do the property_entry arrays need a trailing NULL sentinel?

The struct definition in lan743x_main.h sizes them to the exact number
of used entries:

    struct property_entry gpio_props[1];
    struct property_entry i2c_props[1];
    struct property_entry sfp_props[8];    /* 7 used + terminator */
    struct property_entry phylink_props[2];

The swnode code in drivers/base/swnode.c iterates with

    for (; prop->name; prop++)

in property_entry_get() and property_entries_dup(), which relies on a
zeroed terminator. Only sfp_props[8] includes such a slot.

Because gpio_props, i2c_props, sfp_props and phylink_props are laid out
contiguously in struct lan743x_sw_nodes, when iteration walks past
gpio_props[0] or i2c_props[0] it would land on the next array's entries
(for example i2c_props[0].name = "pinctrl-names", then sfp_props entries
"compatible"/"sff,sfp", "i2c-bus", ...), only terminating at
sfp_props[7]. The GPIO and I2C software nodes could silently acquire SFP
properties.

For phylink_props[2], iteration would continue into i2c_ref[0], whose
first field swnode is a non-NULL pointer; would that pointer then be
interpreted as prop->name and passed to strcmp() during lookups for
names other than "managed"/"sfp", producing an out-of-bounds read of
kernel memory?

Would sizing each array with +1 for a zero terminator address this?

> @@ -3134,7 +3310,9 @@ static const struct phylink_mac_ops lan743x_phylink_mac_ops = {
>  static int lan743x_phylink_create(struct lan743x_adapter *adapter)
>  {
>  	struct net_device *netdev = adapter->netdev;
> +	struct fwnode_handle *fwnode = NULL;
>  	struct phylink *pl;
> +	int ret;
[ ... ]
> +	if (adapter->is_sfp_support_en) {
> +		ret = lan743x_swnodes_register(adapter);
> +		if (ret) {
> +			netdev_err(netdev, "failed to register software nodes\n");
> +			return ret;
> +		}
> +		fwnode = software_node_fwnode(adapter->nodes->group[SWNODE_PHYLINK]);
> +		if (!fwnode) {
> +			lan743x_swnodes_unregister(adapter);
> +			return -ENODEV;
> +		}
> +	}
>  
> +	pl = phylink_create(&adapter->phylink_config, fwnode,
> +			    adapter->phy_interface, &lan743x_phylink_mac_ops);
>  	if (IS_ERR(pl)) {
>  		netdev_err(netdev, "Could not create phylink (%pe)\n", pl);
> +		lan743x_swnodes_unregister(adapter);
>  		return PTR_ERR(pl);
>  	}

Thanks for reading.

Re: [PATCH net-next v3 4/5] net: lan743x: Register SFP platform device for PCI11x1x

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: lan743x: Register SFP platform device for PCI11x1x

This commit registers an SFP platform device for PCI11x1x when SFP support
is enabled, using the SFP software node as its firmware node and associating
the I2C adapter with its own software node for module management.

> diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
> index a83db3c7404f..9857053dabf8 100644
> --- a/drivers/net/ethernet/microchip/Kconfig
> +++ b/drivers/net/ethernet/microchip/Kconfig
> @@ -52,6 +52,7 @@ config LAN743X
>  	select PHYLINK
>  	select I2C_PCI1XXXX
>  	select GP_PCI1XXXX
> +	select SFP

[Medium]
Does this select need to inherit SFP's own dependencies?  In
drivers/net/pse-pd/Kconfig the SFP symbol is declared as:

    config SFP
        tristate "SFP cage support"
        depends on NETDEVICES && PHYLIB
        depends on HWMON || HWMON=n
        ...

With HWMON=m and LAN743X=y, select SFP forces SFP=y, which violates
the HWMON || HWMON=n constraint and produces an unmet direct
dependencies warning.

For comparison, drivers/net/ethernet/wangxun/Kconfig uses:

    select HWMON if TXGBE=y
    select SFP

Would adding either depends on HWMON || HWMON=n to LAN743X, or a
conditional HWMON select, be preferable here?

>  	help
>  	  Support for the Microchip LAN743x and PCI11x1x families of PCI
>  	  Express Ethernet devices
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index b90b35cef2e6..88a2d11552f8 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -3066,6 +3066,31 @@ static int lan743x_swnodes_register(struct lan743x_adapter *adapter)
>  	return software_node_register_node_group(nodes->group);
>  }
>  
> +static int lan743x_sfp_register(struct lan743x_adapter *adapter)
> +{
> +	struct pci_dev *pdev = adapter->pdev;
> +	struct platform_device_info sfp_info;
> +	struct platform_device *sfp_dev;
> +
> +	memset(&sfp_info, 0, sizeof(sfp_info));
> +	sfp_info.parent = &adapter->pdev->dev;
> +	sfp_info.fwnode = software_node_fwnode(adapter->nodes->group[SWNODE_SFP]);
> +	sfp_info.name = "sfp";
> +	sfp_info.id = (pdev->bus->number << 8) | pdev->devfn;

[Medium]
Should the platform device id incorporate pci_domain_nr(pdev->bus)?
PCI bus numbers are only unique within a PCI domain, so on a
multi-domain system two PCI11x1x devices in different domains that
happen to share the same bus/devfn would both try to register as
sfp.<same_id>.

The second platform_device_add() would then fail with -EEXIST from
device_add() and SFP would not come up on that device.  Something
like:

    sfp_info.id = (pci_domain_nr(pdev->bus) << 16) |
                  (pdev->bus->number << 8) | pdev->devfn;

would avoid that collision.

> +	sfp_dev = platform_device_register_full(&sfp_info);
> +	if (IS_ERR(sfp_dev)) {
> +		netif_err(adapter, drv, adapter->netdev,
> +			  "Failed to register SFP device\n");
> +		return PTR_ERR(sfp_dev);
> +	}
> +
> +	adapter->sfp_dev = sfp_dev;
> +	netif_dbg(adapter, drv, adapter->netdev,
> +		  "SFP platform device registered");
> +
> +	return 0;
> +}
> +

[ ... ]

> @@ -3682,6 +3707,12 @@ static void lan743x_destroy_phylink(struct lan743x_adapter *adapter)
>  static void lan743x_full_cleanup(struct lan743x_adapter *adapter)
>  {
>  	unregister_netdev(adapter->netdev);
> +	if (adapter->sfp_dev) {
> +		platform_device_unregister(adapter->sfp_dev);
> +		adapter->sfp_dev = NULL;
> +	}
> +	if (adapter->i2c_adap)
> +		adapter->i2c_adap = NULL;

[High]
Can this leave a dangling fwnode on the i2c_adap device?  The
i2c_adap is fetched from a different PCI function (the pci1xxxx_i2c
driver via pci1xxxx_i2c_adapter_get()) and outlives the lan743x
instance.

In lan743x_pcidev_probe() the code does:

    adapter->i2c_adap->dev.fwnode =
        software_node_fwnode(adapter->nodes->group[SWNODE_I2C]);

But on teardown only the local pointer is cleared:

    if (adapter->i2c_adap)
        adapter->i2c_adap = NULL;

Meanwhile lan743x_destroy_phylink() calls
lan743x_swnodes_unregister() which invokes
software_node_unregister_node_group() and frees adapter->nodes.
After that, i2c_adap->dev.fwnode still references the released
swnode-backed fwnode.

Any subsequent access to i2c_adap->dev.fwnode (driver unload/rebind
of pci1xxxx_i2c, sysfs reads, fwnode iteration, or fwnode ops
dispatch) would then dereference freed memory.

Should the teardown also restore i2c_adap->dev.fwnode to what it was
before the probe-side assignment, symmetric to the mutation done in
lan743x_pcidev_probe()?

>  
>  	lan743x_destroy_phylink(adapter);
>  	lan743x_mdiobus_cleanup(adapter);
> @@ -3909,11 +3940,29 @@ static int lan743x_pcidev_probe(struct pci_dev *pdev,
>  		goto cleanup_mdiobus;
>  	}
>  
> +	if (adapter->is_sfp_support_en) {
> +		adapter->i2c_adap->dev.fwnode =
> +			software_node_fwnode(adapter->nodes->group[SWNODE_I2C]);
> +
> +		ret = lan743x_sfp_register(adapter);
> +		if (ret < 0) {
> +			netif_err(adapter, probe, netdev,
> +				  "failed to sfp register (%d)\n", ret);
> +			goto cleanup_phylink;
> +		}
> +	}
> +
>  	ret = register_netdev(adapter->netdev);
>  	if (ret < 0)
> -		goto cleanup_phylink;
> +		goto cleanup_sfp;
>  	return 0;
>  
> +cleanup_sfp:
> +	if (adapter->sfp_dev) {
> +		platform_device_unregister(adapter->sfp_dev);
> +		adapter->sfp_dev = NULL;
> +	}
> +
>  cleanup_phylink:
>  	lan743x_destroy_phylink(adapter);
>

Re: [PATCH net-next v3 5/5] net: lan743x: Add PCS/XPCS support for SFP on PCI11x1x

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: lan743x: Add PCS/XPCS support for SFP on PCI11x1x

Adds a dedicated PCS mdiobus and an XPCS instance driven through the
existing SGMII register access helpers, integrates the xpcs with phylink
via mac_select_pcs, and extends phylink support to cover SGMII and
2500BASE-X for SFP modules on PCI11x1x.

> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index 88a2d11552f8..485fbc678481 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c

[ ... ]

> @@ -3806,6 +3845,42 @@ static int lan743x_hardware_init(struct lan743x_adapter *adapter,
>  	return 0;
>  }
>  
> +static int lan743x_pcs_mdiobus_init(struct lan743x_adapter *adapter)
> +{
> +	struct phylink_pcs *pcs;
> +	int ret;
> +
> +	adapter->pcs_mdiobus = devm_mdiobus_alloc(&adapter->pdev->dev);
> +	if (!(adapter->pcs_mdiobus)) {
> +		ret = -ENOMEM;
> +		goto return_error;
> +	}
> +
> +	adapter->pcs_mdiobus->priv = (void *)adapter;
> +	adapter->pcs_mdiobus->read_c45 = pci11x1x_pcs_read;
> +	adapter->pcs_mdiobus->write_c45 = pci11x1x_pcs_write;
> +	adapter->pcs_mdiobus->name = "lan743x-pcs-mdiobus-c45";
> +	netif_dbg(adapter, drv, adapter->netdev, "lan743x-pcs-mdiobus-c45\n");
> +	sn printf(adapter->pcs_mdiobus->id, MII_BUS_ID_SIZE, "pci-pcs-%s", pci_name(adapter->pdev));
> +
> +	if (!adapter->phy_interface)
> +		lan743x_phy_interface_select(adapter);
> +
> +	pcs = xpcs_create_pcs_mdiodev(adapter->pcs_mdiobus, 0);

[Critical]
Is this pcs_mdiobus ever registered?  The bus is obtained from
devm_mdiobus_alloc() and then handed straight to xpcs_create_pcs_mdiodev()
without a mdiobus_register()/devm_mdiobus_register() call in between, so
it stays in MDIOBUS_ALLOCATED state.

bus->mdio_lock is only initialized by __mdiobus_register():

    drivers/net/phy/mdio_bus_provider.c:__mdiobus_register() {
        ...
        mutex_init(&bus->mdio_lock);
        ...
    }

mdiobus_alloc_size() does not initialize it. xpcs_create_pcs_mdiodev()
flows into xpcs_create() -> xpcs_init_id() -> xpcs_identify() and each
C45 read grabs bus->mdio_lock via mutex_lock().

Can this end up locking an uninitialized mutex during probe on
CONFIG_DEBUG_MUTEXES or lockdep kernels, and silently corrupting state
otherwise?

Similarly, mdio_device_create() sets mdiodev->dev.parent = &bus->dev on
a struct device that has never been through device_register(), which
other xpcs consumers (txgbe_mdio_pcs_init, ksz9477_pcs_create,
sja1105_mdiobus_pcs_register) avoid by registering the bus first.

> +	if (IS_ERR(pcs)) {
> +		netdev_err(adapter->netdev, "failed to create xpcs\n");
> +		ret = PTR_ERR(pcs);
> +		goto return_error;
> +	}
> +
> +	adapter->xpcs = pcs;
> +	return 0;
> +
> +return_error:
> +	mdiobus_free(adapter->pcs_mdiobus);
> +	return ret;
> +}

[Critical, Critical]
Can this error path dereference a NULL pointer and also double-free the
bus?

When devm_mdiobus_alloc() returns NULL, adapter->pcs_mdiobus is NULL and
the code still jumps to return_error, which calls mdiobus_free():

    drivers/net/phy/mdio_bus_provider.c:mdiobus_free() {
        if (bus->state == MDIOBUS_ALLOCATED) {
            kfree(bus);
            return;
        }
        ...
    }

There is no NULL guard, so the OOM path dereferences NULL.

On the xpcs-create failure path, the bus was allocated by
devm_mdiobus_alloc(), which registers devm_mdiobus_free() as a devres
destructor that calls mdiobus_free() at device teardown
(drivers/net/phy/mdio_devres.c). Calling mdiobus_free() here manually
frees the bus (state == MDIOBUS_ALLOCATED takes the kfree(bus) path),
and then devres runs again on the same pointer.

Would it be simpler to drop the return_error cleanup entirely and just
return the error, letting devres free the bus?

> +
>  static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
>  {
>  	int ret;
> @@ -3927,6 +4002,11 @@ static int lan743x_pcidev_probe(struct pci_dev *pdev,
>  	if (ret)
>  		goto cleanup_hardware;
>  
> +	if (adapter->is_sfp_support_en) {
> +		ret = lan743x_pcs_mdiobus_init(adapter);
> +		if (ret)
> +			goto cleanup_hardware;
> +	}

[High]
Is cleanup_hardware the right target here?

lan743x_mdiobus_init() was just called above and has already invoked
mdiobus_register(adapter->mdiobus), so adapter->mdiobus is in state
MDIOBUS_REGISTERED. Jumping to cleanup_hardware skips cleanup_mdiobus
-> lan743x_mdiobus_cleanup() -> mdiobus_unregister(adapter->mdiobus).

The bus is then torn down only by the devm_mdiobus_free devres
destructor, which reaches mdiobus_free() on a still-registered bus and
trips:

    drivers/net/phy/mdio_bus_provider.c:mdiobus_free() {
        ...
        WARN(bus->state != MDIOBUS_UNREGISTERED,
             "%s: not in UNREGISTERED state\n", bus->id);
        ...
    }

device_del() on the bus device is never called. Should this instead
goto cleanup_mdiobus?

[Medium]
On PCI11x1x, pci11x1x_strap_get_status() can leave is_sfp_support_en
true while is_pcs_en is false (SFP strap set, SGMII strap cleared) with
only a netif_err().  In that case lan743x_hardware_init() takes the
SGMII-disabled path and writes SGMII_CTL with SGMII_CTL_SGMII_POWER_DN_
set and SGMII_CTL_SGMII_ENABLE_ cleared.

Then lan743x_pcidev_probe() still enters this branch because
is_sfp_support_en is true, invoking lan743x_pcs_mdiobus_init() which
drives xpcs_create_pcs_mdiodev() -> xpcs_identify() -> multiple C45
reads through pci11x1x_pcs_read() -> lan743x_sgmii_read():

    drivers/net/ethernet/microchip/lan743x_main.c:lan743x_sgmii_read() {
        ...
        lan743x_csr_write(adapter, SGMII_ACC, mmd_access);
        ret = lan743x_sgmii_wait_till_not_busy(adapter);
        ...
    }

With the SGMII block powered down, can the busy bit ever clear, or will
every read hit the 1-second readx_poll_timeout, producing an extremely
slow probe and a bogus xpcs identification?

>  	adapter->netdev->netdev_ops = &lan743x_netdev_ops;
>  	adapter->netdev->ethtool_ops = &lan743x_ethtool_ops;
>  	adapter->netdev->features = NETIF_F_SG | NETIF_F_TSO |