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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Per-port `phylink_pcs_ops` configuration callbacks unconditionally reset global hardware lines and reconfigure shared PLLs, which will silently drop active links and corrupt state on other ports in a multi-port configuration.
- [High] `airoha_pcs_post_config` unconditionally overwrites flow control settings, ignoring pause negotiation.
- [High] Concurrent hardware state modification inside `pcs_get_state` races with configuration paths, leading to state corruption.
- [High] Tight read loop immediately after triggering a hardware scan will fail to detect the RX signal because it does not allow the hardware sufficient time to complete the operation.
- [Medium] Using `regmap_set_bits()` to clear pending interrupts performs a read-modify-write on a Write-1-to-Clear (W1C) interrupt status register, inadvertently clearing unrelated pending interrupts.
- [Medium] Multiple hardware configuration routines evaluate uninitialized stack variables when `regmap_read()` fails.
- [Medium] Switch statement fallthrough leaves `speed_reg` uninitialized for `PHY_INTERFACE_MODE_2500BASEX` if the requested speed differs from `SPEED_2500`.
- [Medium] The hardware reset assertion and deassertion occur back-to-back without any propagation delay, which may result in a glitch-like pulse that fails to properly reset the PHY/MAC.
--

--- Patch [10]: [PATCH net-next v10 10/11] net: pcs: airoha: add PCS driver for Airoha AN7581 SoC ---
commit 30773bae6473b72be8d07e5ac639902c307ba3ac
Author: Christian Marangi <ansuelsmth@gmail.com>

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

Add PCS driver for Airoha AN7581 SoC for Ethernet/PON/PCIe/USB SERDES
and permit usage of external PHY or connected SFP cage. Supported modes
are USXGMII, 10G-BASER, 2500BASE-X, 1000BASE-X and SGMII.

> diff --git a/drivers/net/pcs/airoha/pcs-airoha-common.c b/drivers/net/pcs/airoha/pcs-airoha-common.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-airoha-common.c
[ ... ]
> +static int airoha_pcs_setup_scu(struct airoha_pcs_priv *priv,
> +				int index, phy_interface_t interface)
> +{
[ ... ]
> +	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]
When configuring a multi-port device like the PCIe interface, does toggling
these global reset lines during port 1's bringup disrupt active traffic on
port 0?

[Severity: Medium]
Is there a risk of the reset pulse being too short? The deassert follows
immediately after assert without any hold-time delay, which might prevent the
hardware blocks from fully resetting.

[ ... ]
> +static void airoha_pcs_interrupt_init_usxgmii(struct airoha_pcs_priv *priv,
> +					      int index)
> +{
[ ... ]
> +	/* 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]
Could using regmap_set_bits() here unintentionally clear other pending
interrupts? Since these are Write-1-to-Clear (W1C) registers, a
read-modify-write might write back '1's for unrelated active interrupts that
happened to be pending during the read.

[ ... ]
> +static void airoha_pcs_get_state_usxgmii(struct airoha_pcs_priv *priv, int index,
> +					 struct phylink_link_state *state)
> +{
> +	const struct airoha_pcs_match_data *data = priv->data;
> +	struct airoha_pcs_maps *maps = &priv->maps[index];
> +	u32 an_done = 0, lpa = 0;
> +
> +	/* Trigger HW workaround if needed. If an error is reported,
> +	 * consider link down and test again later.
> +	 */
> +	if (data->rxlock_workaround && data->rxlock_workaround(priv, index)) {
> +		state->link = false;
> +		return;
> +	}
> +
> +	/* Toggle AN Status */
> +	regmap_set_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_AN_CONTROL_6,
> +			AIROHA_PCS_USXGMII_TOG_PCS_AUTONEG_STS);

[Severity: High]
Since phylink_pcs_ops->pcs_get_state can be called asynchronously by polling
workqueues, can these state-modifying hardware writes (and the multi-step
resets triggered in the workaround) collide with other configuration routines?

[ ... ]
> +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:
[ ... ]
> +			}
[ ... ]
> +			regmap_update_bits(maps->multi_sgmii, AIROHA_PCS_MULTI_SGMII_SGMII_STS_CTRL_0,
> +					   AIROHA_PCS_LINK_MODE_P0 |
> +					   AIROHA_PCS_FORCE_SPD_MODE_P0,
> +					   speed_reg |
> +					   AIROHA_PCS_FORCE_SPD_MODE_P0);

[Severity: Medium]
If the interface is PHY_INTERFACE_MODE_2500BASEX but the speed evaluates to
something other than SPEED_2500, could this write uninitialized memory from
speed_reg to the hardware?

[ ... ]
> +static int airoha_pcs_post_config(struct phylink_pcs *pcs,
> +				  phy_interface_t interface)
> +{
[ ... ]
> +	/* Enable TX/RX flow control */
> +	regmap_set_bits(maps->pcs_mac, AIROHA_PCS_XFI_MAC_XFI_GIB_CFG,
> +			AIROHA_PCS_XFI_TX_FC_EN);
> +	regmap_set_bits(maps->pcs_mac, AIROHA_PCS_XFI_MAC_XFI_GIB_CFG,
> +			AIROHA_PCS_XFI_RX_FC_EN);

[Severity: High]
Does this override the pause negotiation settings applied earlier in
pcs_config? Unconditionally writing these bits appears to ignore the
permit_pause_to_mac configuration entirely.

> diff --git a/drivers/net/pcs/airoha/pcs-an7581.c b/drivers/net/pcs/airoha/pcs-an7581.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-an7581.c
[ ... ]
> +static void an7581_pcs_jcpll_bringup(struct airoha_pcs_priv *priv,
> +				     int index, phy_interface_t interface)
> +{
[ ... ]
> +	/* 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]
Why is pcs_pma[0] hardcoded here (and similarly in an7581_pcs_txpll_bringup)
when setting up the shared PLLs? Similar to the global reset lines, wouldn't
reconfiguring the shared PLL during port 1's bringup disrupt an already
active link on port 0?

[ ... ]
> +static unsigned int an7581_pcs_apply_cdr_pr_idac(struct airoha_pcs_priv *priv,
> +						 int index, u32 cdr_pr_idac)
> +{
> +	struct regmap *pcs_pma = priv->pcs_pma[index];
> +	u32 val;
[ ... ]
> +	regmap_read(pcs_pma, AIROHA_PCS_PMA_RX_FREQDET, &val);
> +
> +	return FIELD_GET(AIROHA_PCS_PMA_FL_OUT, val);

[Severity: Medium]
If regmap_read() fails, wouldn't val remain uninitialized stack garbage?
Should the return code be checked before extracting fields from it?

[ ... ]
> +static int an7581_pcs_phya_bringup(struct airoha_pcs_priv *priv,
> +				   int index, phy_interface_t interface)
> +{
[ ... ]
> +	regmap_read(pcs_pma, AIROHA_PCS_PMA_RX_FREQDET, &val);
> +	if (!(val & AIROHA_PCS_PMA_FBCK_LOCK)) {

[Severity: Medium]
Is it safe to evaluate val without checking the return value of
regmap_read() first?

[ ... ]
> +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]
Will this tight loop execute too quickly for the physical hardware scan to
complete? Without a short delay between triggering the scan and polling the
results, it seems this might return false negatives.

[Severity: Medium]
Also, val is being evaluated here without checking if regmap_read() succeeded.

[ ... ]
> +int an7581_pcs_rxlock_workaround(struct airoha_pcs_priv *priv, int index)
> +{
> +	struct airoha_pcs_maps *maps = &priv->maps[index];
> +	u32 val;
> +
> +	/* Check if PCS is UP or Down */
> +	regmap_read(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_STUS_1, &val);
> +	if (val & AIROHA_PCS_USXGMII_PCS_RX_LINK_STATUS_UP)

[Severity: Medium]
Similar to the previous reads, val could be uninitialized if regmap_read()
fails.

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