Re: [PATCH net-next v9 11/12] net: pcs: airoha: add PCS driver for Airoha AN7581 SoC

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] SPEED_10 configuration incorrectly sets MAC link mode to 100M instead of 10M.
- [High] AN_FORCE_CL37 auto-negotiation register is mistakenly written to the hsgmii_pcs regmap instead of hsgmii_an.
- [High] Generic PHY is acquired for USB port types but never initialized or powered on.
- [High] Stack variables are used uninitialized due to unchecked `regmap_read()` return values.
- [High] Initialization of a single port incorrectly resets and modifies shared hardware state (reset lines and PLLs) used by other active ports.
- [High] Zero-delay MMIO polling loop immediately after a hardware trigger causes unreliable signal detection.
- [Medium] Read-modify-write operation on W1C (write-1-to-clear) interrupt status registers accidentally clears unmasked interrupts.
--

--- Patch [11]: [PATCH net-next v9 11/12] net: pcs: airoha: add PCS driver for Airoha AN7581 SoC ---

commit 87ad2d5e05008754a1a8736668451b5aa64ba606
Author: Christian Marangi [off-list ref]
Date:   Fri Jul 17 06:54:11 2026 +0000

    net: pcs: airoha: add PCS driver for Airoha AN7581 SoC

This commit adds a new PCS driver for the Airoha AN7581 SoC, supporting
Ethernet, PON, PCIe, and USB SERDES. The driver provides support for various
operating modes including USXGMII, 10G-BASER, 2500BASE-X, 1000BASE-X, and
SGMII.

[ ... ]

+static int airoha_pcs_setup_scu(struct airoha_pcs_priv *priv,
+				int index, phy_interface_t interface)
+{
+	const struct airoha_pcs_match_data *data = priv->data;
+	int ret;
+
+	switch (data->port_type) {
+	case AIROHA_PCS_ETH:
+		airoha_pcs_setup_scu_eth(priv, interface);
+		break;
+	case AIROHA_PCS_PON:
+		airoha_pcs_setup_scu_pon(priv, interface);
+		break;
+	case AIROHA_PCS_PCIE:
+		airoha_pcs_setup_scu_pcie(priv, index, interface);
+		break;
+	case AIROHA_PCS_USB:
+		break;
+	}
+
+	/* TODO better handle reset from MAC */
+	ret = reset_control_bulk_assert(ARRAY_SIZE(priv->rsts),
+					priv->rsts);

[Severity: High]
Does this assert shared device-level reset lines that might affect other
active ports on the MAC? If so, this could interrupt connections on those
ports during initialization.

+	if (ret)
+		return ret;
+
+	ret = reset_control_bulk_deassert(ARRAY_SIZE(priv->rsts),
+					  priv->rsts);
+	if (ret)
+		return ret;
+
+	return 0;
+}

[ ... ]

+static void airoha_pcs_interrupt_init_usxgmii(struct airoha_pcs_priv *priv,
+					      int index)
+{
+	struct airoha_pcs_maps *maps = &priv->maps[index];
+
+	/* Disable every Interrupt */
+	regmap_clear_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_CTRL_0,
+			  AIROHA_PCS_USXGMII_T_TYPE_T_INT_EN |
+			  AIROHA_PCS_USXGMII_T_TYPE_D_INT_EN |
+			  AIROHA_PCS_USXGMII_T_TYPE_C_INT_EN |
+			  AIROHA_PCS_USXGMII_T_TYPE_S_INT_EN);
+
+	regmap_clear_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_CTRL_1,
+			  AIROHA_PCS_USXGMII_R_TYPE_C_INT_EN |
+			  AIROHA_PCS_USXGMII_R_TYPE_S_INT_EN |
+			  AIROHA_PCS_USXGMII_TXPCS_FSM_ENC_ERR_INT_EN |
+			  AIROHA_PCS_USXGMII_T_TYPE_E_INT_EN);
+
+	regmap_clear_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_CTRL_2,
+			  AIROHA_PCS_USXGMII_RPCS_FSM_DEC_ERR_INT_EN |
+			  AIROHA_PCS_USXGMII_R_TYPE_E_INT_EN |
+			  AIROHA_PCS_USXGMII_R_TYPE_T_INT_EN |
+			  AIROHA_PCS_USXGMII_R_TYPE_D_INT_EN);
+
+	regmap_clear_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_CTRL_3,
+			  AIROHA_PCS_USXGMII_FAIL_SYNC_XOR_ST_INT_EN |
+			  AIROHA_PCS_USXGMII_RX_BLOCK_LOCK_ST_INT_EN |
+			  AIROHA_PCS_USXGMII_LINK_UP_ST_INT_EN |
+			  AIROHA_PCS_USXGMII_HI_BER_ST_INT_EN);
+
+	regmap_clear_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_CTRL_4,
+			  AIROHA_PCS_USXGMII_LINK_DOWN_ST_INT_EN);
+
+	/* Clear any pending interrupt */
+	regmap_set_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_INT_STA_2,
+			AIROHA_PCS_USXGMII_RPCS_FSM_DEC_ERR_INT |
+			AIROHA_PCS_USXGMII_R_TYPE_E_INT |
+			AIROHA_PCS_USXGMII_R_TYPE_T_INT |
+			AIROHA_PCS_USXGMII_R_TYPE_D_INT);

[Severity: Medium]
Will using a read-modify-write operation like regmap_set_bits on a
write-1-to-clear interrupt status register inadvertently read all pending
interrupts as 1s and write them back, thereby clearing all pending
interrupts and not just the masked ones?

+
+	regmap_set_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_INT_STA_3,
+			AIROHA_PCS_USXGMII_FAIL_SYNC_XOR_ST_INT |
+			AIROHA_PCS_USXGMII_RX_BLOCK_LOCK_ST_INT |
+			AIROHA_PCS_USXGMII_LINK_UP_ST_INT |
+			AIROHA_PCS_USXGMII_HI_BER_ST_INT);

[ ... ]

+static void airoha_pcs_get_state_sgmii(struct airoha_pcs_priv *priv,
+				       unsigned int neg_mode, int index,
+				       struct phylink_link_state *state)
+{
+	struct airoha_pcs_maps *maps = &priv->maps[index];
+	u32 bmsr, lpa;
+
+	regmap_read(maps->hsgmii_an, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_1,
+		    &bmsr);
+	regmap_read(maps->hsgmii_an, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_5,
+		    &lpa);
+
+	bmsr = (AIROHA_PCS_HSGMII_AN_SGMII_AN_COMPLETE |

[Severity: High]
If regmap_read() fails here, wouldn't bmsr and lpa be left uninitialized?
Evaluating uninitialized stack variables might lead to random state machine
behavior. Can we add return value checks here?

+		AIROHA_PCS_HSGMII_AN_SGMII_REMOTE_FAULT |
+		AIROHA_PCS_HSGMII_AN_SGMII_AN_ABILITY |
+		AIROHA_PCS_HSGMII_AN_SGMII_LINK_STATUS) & bmsr;
+	lpa = AIROHA_PCS_HSGMII_AN_SGMII_PARTNER_ABILITY & lpa;
+
+	phylink_mii_c22_pcs_decode_state(state, neg_mode, bmsr, lpa);
+}

[ ... ]

+static int airoha_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
+			     phy_interface_t interface,
+			     const unsigned long *advertising,
+			     bool permit_pause_to_mac)
+{

[ ... ]

+	if (interface == PHY_INTERFACE_MODE_1000BASEX &&
+	    neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) {
+		regmap_set_bits(maps->hsgmii_pcs, AIROHA_PCS_HSGMII_PCS_CTROL_1,
+				AIROHA_PCS_SGMII_SEND_AN_ERR_EN);
+
+		regmap_set_bits(maps->hsgmii_pcs, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_FORCE_CL37,
+				AIROHA_PCS_HSGMII_AN_FORCE_AN_DONE);

[Severity: High]
Is AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_FORCE_CL37 being written to the wrong
regmap here? It appears to be an auto-negotiation register that should be
written to the hsgmii_an block, rather than hsgmii_pcs.

+	}
+
+	if (interface == PHY_INTERFACE_MODE_2500BASEX) {
+		regmap_set_bits(maps->hsgmii_an, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_0,
+				AIROHA_PCS_HSGMII_AN_SGMII_RESET_PHY);
+	}

[ ... ]

+static void airoha_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
+			       phy_interface_t interface, int speed, int duplex)
+{

[ ... ]

+		if (interface == PHY_INTERFACE_MODE_SGMII ||
+		    interface == PHY_INTERFACE_MODE_2500BASEX) {
+			u32 ck_gen_mode;
+			u32 speed_reg;
+			u32 if_mode;
+
+			switch (speed) {
+			case SPEED_2500:
+				speed_reg = AIROHA_PCS_LINK_MODE_P0_2_5G;
+				break;
+			case SPEED_1000:
+				speed_reg = AIROHA_PCS_LINK_MODE_P0_1G;
+				if_mode = AIROHA_PCS_HSGMII_AN_SPEED_FORCE_MODE_1000;
+				ck_gen_mode = AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE_1000;
+				break;
+			case SPEED_100:
+				speed_reg = AIROHA_PCS_LINK_MODE_P0_100M;
+				if_mode = AIROHA_PCS_HSGMII_AN_SPEED_FORCE_MODE_100;
+				ck_gen_mode = AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE_100;
+				break;
+			case SPEED_10:
+				speed_reg = AIROHA_PCS_LINK_MODE_P0_100M;
+				if_mode = AIROHA_PCS_HSGMII_AN_SPEED_FORCE_MODE_10;
+				ck_gen_mode = AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE_10;

[Severity: High]
Does SPEED_10 mistakenly assign the 100M constant to speed_reg instead of
AIROHA_PCS_LINK_MODE_P0_10M? This looks like it might incorrectly program the
hardware to 100Mbps.

+				break;
+			}
+
+			if (interface == PHY_INTERFACE_MODE_SGMII) {
+				regmap_update_bits(maps->hsgmii_an, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_13,
+						   AIROHA_PCS_HSGMII_AN_SPEED_FORCE_MODE,
+						   if_mode);

[ ... ]

+static int airoha_pcs_probe(struct platform_device *pdev)
+{
+	const struct airoha_pcs_match_data *data;
+	struct device *dev = &pdev->dev;
+	struct airoha_pcs_priv *priv;
+	int index, ret;
+
+	data = of_device_get_match_data(dev);
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->ports = devm_kcalloc(dev, data->num_port,
+				   sizeof(*priv->ports), GFP_KERNEL);
+	if (!priv->ports)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	priv->data = data;
+
+	if (data->port_type == AIROHA_PCS_USB) {
+		struct phy *phy;
+
+		phy = devm_phy_get(dev, NULL);
+		if (IS_ERR(phy))
+			return dev_err_probe(dev, PTR_ERR(phy), "failed to get phy\n");
+
+		priv->phy = phy;

[Severity: High]
The generic PHY is acquired for USB port types, but does it ever get
initialized or powered on anywhere else in the driver? It appears the hardware
might remain powered down without a call to phy_init() and phy_power_on().

+	}
+
+	switch (data->port_type) {

[ ... ]

[ ... ]

+static void an7581_pcs_jcpll_bringup(struct airoha_pcs_priv *priv,
+				     int index, phy_interface_t interface)
+{
+	struct regmap_field **pcs_ana_fields = priv->pcs_ana_fields[index];
+	struct regmap *pcs_pma;
+	u32 kband_vref;
+
+	switch (interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+	case PHY_INTERFACE_MODE_1000BASEX:
+	case PHY_INTERFACE_MODE_2500BASEX:
+		kband_vref = 0x10;
+		break;
+	case PHY_INTERFACE_MODE_USXGMII:
+	case PHY_INTERFACE_MODE_10GBASER:
+		kband_vref = 0xf;
+		break;
+	default:
+		return;
+	}
+
+	/* This comment only apply to Serdes PCIe that expose
+	 * 2 PCS.
+	 *
+	 * The Serdes PCIe expose 2 PCS but always require
+	 * the PMA for the first PCS to be configured
+	 * for correct functionality for JCPLL.
+	 */
+	pcs_pma = priv->pcs_pma[0];
+
+	/* Setup LDO */
+	usleep_range(200, 300);
+
+	regmap_field_set_bits(pcs_ana_fields[AN7581_PCS_JCPLL_SPARE_L],
+			      AIROHA_PCS_ANA_JCPLL_SPARE_L_LDO);
+
+	/* Setup RSTB */
+	regmap_field_write(pcs_ana_fields[AN7581_PCS_JCPLL_RST_DLY],
+			   AIROHA_PCS_ANA_JCPLL_RST_DLY_150_200);
+	regmap_field_write(pcs_ana_fields[AN7581_PCS_JCPLL_PLL_RSTB], 0x1);

[Severity: High]
Does this code unconditionally modify port 0's JCPLL state and reset it, even
when port 1 is being configured? If so, this could potentially destroy the
active hardware state of the other port.

+
+	/* Enable PLL force selection and Force Disable */
+	regmap_update_bits(pcs_pma, AIROHA_PCS_PMA_PXP_JCPLL_CKOUT_EN,
+			   AIROHA_PCS_PMA_FORCE_SEL_DA_JCPLL_EN |
+			   AIROHA_PCS_PMA_FORCE_DA_JCPLL_EN,
+			   AIROHA_PCS_PMA_FORCE_SEL_DA_JCPLL_EN);

[ ... ]

+static bool an7581_pcs_have_rx_signal(struct airoha_pcs_priv *priv, int index)
+{
+	struct regmap *pcs_pma = priv->pcs_pma[index];
+	unsigned int count = 0;
+	u32 val;
+	int i;
+
+	regmap_write(pcs_pma, AIROHA_PCS_PMA_DIG_RESERVE_0,
+		     AIROHA_PCS_TRIGGER_RX_SIDGET_SCAN);
+
+	/* Scan 6 times for RX sigdet module to detect RX signal */
+	for (i = 0; i < AIROHA_PCS_MAX_RX_SIGDET_TRY; i++) {
+		regmap_read(pcs_pma, AIROHA_PCS_PMA_DIG_RO_RESERVE_2,
+			    &val);

[Severity: High]
Are these back-to-back regmap reads executed immediately after the hardware
trigger without any delay? Since analog signal scans usually require some
settling time, could the loop finish before the hardware completes the scan,
resulting in unreliable signal detection?

+		if (val & AIROHA_PCS_RX_SIGDET)
+			count++;
+	}
+
+	/* Consider signal presence if we detect signal at least 4 times */
+	return count >= AIROHA_PCS_MAX_RX_SIGDET_PRESENCE_CNT;
+}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717065448.1498335-1-ansuelsmth@gmail.com?part=11