Re: [PATCH net-next v16 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 7 potential issue(s) to consider:
- [High] pcs: device-wide reset affects active sibling ports
- [High] pcs: missing phy_init and phy_power_on for generic PHY
- [High] pcs: missing locking and state tracking for shared PLLs
- [High] pcs: incorrect read-modify-write on W1C interrupt registers
- [Medium] pcs: missing cleanup for forced 1000BASEX AN flags
- [Medium] pcs: immediate reset deassertion lacks required delay
- [Medium] pcs: insufficient delay in RX signal hardware polling loop
--

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

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

This commit adds a new PCS driver for the Airoha AN7581 SoC. It supports
Ethernet, PON, PCIe, and USB SERDES blocks in various modes including USXGMII,
10G-BASER, 2500BASE-X, and SGMII.

> 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
> @@ -133,6 +133,6 @@ static int airoha_pcs_setup_scu(struct airoha_pcs_priv *priv,
>  
>  	ret = reset_control_bulk_assert(ARRAY_SIZE(priv->rsts),
>  					priv->rsts);
>  	if (ret)
>  		return ret;
>  
>  	ret = reset_control_bulk_deassert(ARRAY_SIZE(priv->rsts),
>  					  priv->rsts);

[Severity: High]
This performs a device-wide macro reset every time the function is called for a
single port. For multi-port devices like AIROHA_PCS_PCIE, wouldn't a
configuration change (like link up or down) on one port reset the entire
MAC/PHY block, disrupting active sibling ports?

[Severity: Medium]
Also, asserting and immediately deasserting the reset without a delay (such as
usleep_range()) generates a near-instantaneous reset pulse. Is this sufficient
for the hardware IP to properly clear its internal state machines?

[ ... ]

> @@ -274,18 +274,18 @@ static void airoha_pcs_interrupt_init_usxgmii(struct airoha_pcs_priv *priv,
>  
>  	/* 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);
>  
>  	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);
>  
>  	regmap_set_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_INT_STA_4,
>  			AIROHA_PCS_USXGMII_LINK_DOWN_ST_INT);

[Severity: High]
Does using regmap_set_bits() (which performs a read-modify-write) on
Write-1-to-Clear (W1C) interrupt status registers unintentionally clear
unrelated pending interrupts? Since it reads the currently active interrupts
and writes them all back as 1s, it could drop all interrupts pending at the
time of the read. Should regmap_write() be used instead?

[ ... ]

> @@ -585,8 +585,8 @@ static int airoha_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
>  
>  	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]
These bits are set for out-of-band configurations, but there appears to be no
cleanup path (e.g., an else block) to clear them when inband is enabled. If
phylink later reconfigures the PCS with inband negotiation enabled, does the
PCS remain stuck with forced AN completion and broken inband autonegotiation?

[ ... ]

> @@ -776,3 +776,3 @@ static void airoha_pcs_pre_config(struct phylink_pcs *pcs,
>  
>  	/* Select HSGMII or USXGMII in SCU regs */
>  	airoha_pcs_setup_scu(priv, port->index, interface);

[Severity: High]
The per-port pcs_pre_config callback triggers the global reset function
(airoha_pcs_setup_scu). Could this cause active sibling ports to be abruptly
disconnected when a configuration change occurs on one port?

[ ... ]

> @@ -1111,5 +1111,5 @@ static int airoha_pcs_probe(struct platform_device *pdev)
>  	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]
This retrieves the uninitialized generic PHY. Is it possible that the USB PCS
port remains non-functional because its PHY is left in a suspended or
uninitialized state without calls to phy_init() and phy_power_on()?

> 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
> @@ -1058,3 +1058,3 @@ static void an7581_pcs_jcpll_bringup(struct airoha_pcs_priv *priv,
>  	/* 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];

[Severity: High]
This explicitly accesses a shared register map (pcs_pma[0]) from a per-port
callback. Since there is no mutex or initialization state tracking, wouldn't
concurrent or asynchronous bringups of the second port re-initialize shared PLLs
and disrupt the already active first port?

[ ... ]

> @@ -1205,5 +1205,5 @@ static void an7581_pcs_txpll_bringup(struct airoha_pcs_priv *priv,
>  	/* 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 TXPLL.
>  	 */
>  	pcs_pma = priv->pcs_pma[0];
>  
>  	/* Setup VCO LDO Output */
>  	regmap_field_write(pcs_ana_fields[AN7581_PCS_TXPLL_LDO_VCO_OUT], 0x1);
>  	regmap_field_write(pcs_ana_fields[AN7581_PCS_TXPLL_LDO_OUT], 0x1);
>  
>  	/* Setup RSTB */
>  	regmap_field_write(pcs_ana_fields[AN7581_PCS_TXPLL_PLL_RSTB], 0x1);

[Severity: High]
Similarly, this modifies the shared pcs_pma[0] and resets the PLL from a
per-port context, impacting both ports without locking.

[ ... ]

> @@ -1891,4 +1891,4 @@ int an7581_pcs_usb_bringup(struct airoha_pcs_priv *priv,
>  			   int index, phy_interface_t interface)
>  {
>  	int ret;
>  
>  	ret = phy_set_mode_ext(priv->phy, PHY_MODE_ETHERNET, interface);
>  	if (ret)
>  		return ret;

[Severity: High]
This configures the generic PHY mode, but initialization and power-on
(phy_init() and phy_power_on()) are completely omitted. Does this prevent the
USB PCS port from functioning correctly?

[ ... ]

> @@ -2073,6 +2073,6 @@ static bool an7581_pcs_have_rx_signal(struct airoha_pcs_priv *priv, int index)
>  
>  	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);
>  		if (val & AIROHA_PCS_RX_SIGDET)
>  			count++;
>  	}

[Severity: Medium]
This tight polling loop fails to give the hardware sufficient time to complete
the scan or debounce the signal since the reads execute in a fraction of a
microsecond. Could this cause the RX signal detection to read the same
instantaneous hardware state and become unreliable without a delay
(e.g., usleep_range()) between iterations?

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