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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: pcs: airoha: shared PCIe PLLs and resets modified concurrently without locking/refcounting
- [Medium] net: pcs: airoha: unhandled error return from airoha_pcs_setup_scu
- [Medium] net: pcs: airoha: sticky AN bits not cleared on neg_mode change
--

commit 5c878701dd1e9963dbed8ef216ebecbde4bf9140
Author: Christian Marangi <ansuelsmth@gmail.com>

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

This patch adds a PCS driver for the Airoha AN7581 SoC for
Ethernet/PON/PCIe/USB SERDES. It supports usage of external PHYs or
connected SFP cages across USXGMII, 10G-BASER, 2500BASE-X, 1000BASE-X,
and SGMII modes.

diff --git a/drivers/net/pcs/airoha/pcs-airoha-common.c b/drivers/net/pcs/airoha/pcs-airoha-common.c
--- a/drivers/net/pcs/airoha/pcs-airoha-common.c
+++ b/drivers/net/pcs/airoha/pcs-airoha-common.c
@@ -131,6 +131,8 @@ static int airoha_pcs_setup_scu(struct airoha_pcs_priv *priv,
 		break;
 	}
 
> 	ret = reset_control_bulk_assert(ARRAY_SIZE(priv->rsts),
> 					priv->rsts);

[Severity: High]
Could this unconditionally assert the global MAC and PHY resets during
individual port setup?

The driver supports dual-port PCIe setups (num_port = 2) which expose
two separate PCS ports to phylink. Since phylink manages each port
independently, if the first port is already active and passing traffic,
bringing up the second port will trigger airoha_pcs_pre_config(), which
calls airoha_pcs_setup_scu() and physically asserts these shared device-wide
resets. Would this instantly drop the active link on the first port?

[ ... ]

@@ -632,6 +632,9 @@ static int airoha_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
 			AIROHA_PCS_HSGMII_PCS_TX_ENABLE |
 			AIROHA_PCS_HSGMII_PCS_MODE2_EN);
 	}
 
> 	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_an, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_FORCE_CL37,
> 				AIROHA_PCS_HSGMII_AN_FORCE_AN_DONE);
> 	}

[Severity: Medium]
Do these bits remain sticky if the neg_mode dynamically switches back
to inband enabled?

When PHY_INTERFACE_MODE_1000BASEX is active with out-of-band negotiation,
the driver sets AIROHA_PCS_SGMII_SEND_AN_ERR_EN and
AIROHA_PCS_HSGMII_AN_FORCE_AN_DONE. If the neg_mode later changes to
PHYLINK_PCS_NEG_INBAND_ENABLED (e.g., swapping an SFP module that doesn't
support AN with one that does, or changing ethtool settings) while the
interface remains 1000BASE-X, phylink invokes pcs_config directly.

Because there is no else branch to explicitly clear these bits, it seems
they might permanently bypass auto-negotiation and break link establishment
for the newly attached inband module.

[ ... ]

@@ -744,6 +744,8 @@ static void airoha_pcs_pre_config(struct phylink_pcs *pcs,
 	maps = &priv->maps[port->index];
 
> 	/* Select HSGMII or USXGMII in SCU regs */
> 	airoha_pcs_setup_scu(priv, port->index, interface);

[Severity: Medium]
Is it intentional to ignore the return value from airoha_pcs_setup_scu() here?

The helper airoha_pcs_setup_scu() successfully identifies and returns
an error code if reset_control_bulk_assert() or deassert() fails. If
the deassert fails, the hardware remains stuck in reset.

By discarding this return value, the initialization continues, which might
lead to bus hangs during subsequent register accesses or silent link failures.
Could we at least log the failure via dev_err() to aid debugging, even
though this callback returns void?

[ ... ]

diff --git a/drivers/net/pcs/airoha/pcs-an7581.c b/drivers/net/pcs/airoha/pcs-an7581.c
--- a/drivers/net/pcs/airoha/pcs-an7581.c
+++ b/drivers/net/pcs/airoha/pcs-an7581.c
@@ -348,6 +348,15 @@ static void an7581_pcs_jcpll_bringup(struct airoha_pcs_priv *priv,
 	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);
> 
> 	/* 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);

[Severity: High]
Could hardcoding pcs_pma[0] here disrupt the active link on port 0 when
bringing up port 1?

When the second port is dynamically linked up, an7581_pcs_jcpll_bringup()
and an7581_pcs_txpll_bringup() unconditionally disable and reconfigure the
shared PLLs via this hardcoded priv->pcs_pma[0] regmap. Concurrent
configuration lacks cross-port serialization or refcounting.

If port 0 is already active and passing traffic, would this force-disable
the shared PLLs and disrupt the data path on the first port?

[ ... ]

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