Re: [PATCH net-next v15 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 9 potential issue(s) to consider:
- [High] Incorrect read-modify-write pattern on Write-1-to-Clear (W1C) interrupt status registers causes silent drop of pending interrupts.
- [High] Missing `phy_init()` and `phy_power_on()` prevents the generic USB PHY from functioning.
- [High] Concurrent or sequential bringup of multi-port PCS configurations destructively resets shared hardware blocks.
- [High] Zero-nanosecond hardware reset pulse due to missing delay between assert and deassert.
- [High] Missing delay in RX signal detection polling loop outruns hardware analog circuitry.
- [Medium] Missing Kconfig dependency on CONFIG_PHYLINK leads to build failures.
- [Medium] Unconditional dereference of `of_device_get_match_data()` leads to a kernel panic.
- [Medium] Uninitialized stack variable `speed_reg` is written to hardware registers.
- [Medium] Ignored error return from hardware reset operation causes MMIO operations on un-reset or powered-down blocks.
--

commit 59d4d3d112fe3684eed0c56c85a2887fbba5ef98
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.

> diff --git a/drivers/net/pcs/airoha/Kconfig b/drivers/net/pcs/airoha/Kconfig
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/Kconfig
[ ... ]
> +config PCS_AIROHA_AN7581
> +	tristate "Airoha AN7581 PCS driver"
> +	depends on ARCH_AIROHA || COMPILE_TEST
> +	select PCS_AIROHA

[Severity: Medium]
Will this lead to build failures if CONFIG_PHYLINK is not selected? It
appears this driver directly calls phylink APIs like
phylink_mii_c22_pcs_decode_state() but doesn't depend on or select PHYLINK.

> +	help
> +	  This module provides helper to phylink for managing the Airoha
> +	  AN7581 PCS for SoC Ethernet and PON SERDES.
> 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)
> +{
> +	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;
> +	}
> +
> +	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]
Does this execute a zero-nanosecond hardware reset pulse? Calling deassert
immediately after assert without a small delay (such as usleep_range) might
violate the hardware's minimum reset assertion time, making the reset
unreliable.

[Severity: High]
Could unconditionally asserting the global mac and phy reset lines here disrupt
other active ports? Since the PCIE configuration supports multiple ports,
bringing up a new port appears to destructively reset the entire shared
MAC/PHY block.

> +	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];
[ ... ]
> +	/* 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: High]
Can this read-modify-write pattern on write-1-to-clear (W1C) interrupt status
registers silently drop pending interrupts?

By using regmap_set_bits(), the current register state is read (with any active
interrupts set to 1) and written back along with the mask. For W1C registers,
writing back those 1s clears all currently pending interrupts globally, instead
of just the targeted bits. It might be safer to use regmap_write() directly.

> +
> +	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);
[ ... ]
> +static void airoha_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
> +			       phy_interface_t interface, int speed, int duplex)
> +{
> +	struct airoha_pcs_port *port = to_airoha_pcs_port(pcs);
> +	struct airoha_pcs_priv *priv = port->priv;
> +	const struct airoha_pcs_match_data *data;
> +	struct airoha_pcs_maps *maps;
> +
> +	maps = &priv->maps[port->index];
> +	data = priv->data;
[ ... ]
> +		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_10M;
> +				if_mode = AIROHA_PCS_HSGMII_AN_SPEED_FORCE_MODE_10;
> +				ck_gen_mode = AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE_10;
> +				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);
> +
> +				regmap_update_bits(maps->hsgmii_pcs, AIROHA_PCS_HSGMII_PCS_AN_SGMII_MODE_FORCE,
> +						   AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE |
> +						   AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE_SEL,
> +						   ck_gen_mode |
> +						   AIROHA_PCS_HSGMII_PCS_FORCE_CUR_SGMII_MODE_SEL);
> +			}
> +
> +			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]
Could this result in uninitialized stack variables being written to the hardware
registers?

If phylink passes SPEED_UNKNOWN due to link loss or an unhandled forced mode,
the switch block is bypassed. This leaves speed_reg uninitialized before it
gets bitwise OR'd and passed into regmap_update_bits().

> +		}
> +	}
[ ... ]
> +static void airoha_pcs_pre_config(struct phylink_pcs *pcs,
> +				  phy_interface_t interface)
> +{
> +	struct airoha_pcs_port *port = to_airoha_pcs_port(pcs);
> +	struct airoha_pcs_priv *priv = port->priv;
> +	struct airoha_pcs_maps *maps;
> +
> +	maps = &priv->maps[port->index];
> +
> +	/* Select HSGMII or USXGMII in SCU regs */
> +	airoha_pcs_setup_scu(priv, port->index, interface);

[Severity: Medium]
Should the error code from airoha_pcs_setup_scu() be checked and handled here?

By ignoring the return value, the driver blindly proceeds to execute MMIO
operations using regmap_set_bits() against the pcs_mac registers. If the
reset operation fails, writing to an un-reset or powered-down block may
cause system bus hangs.

> +
> +	/* MPI MBI disable */
> +	regmap_set_bits(maps->pcs_mac, AIROHA_PCS_XFI_MAC_XFI_GIB_CFG,
> +			AIROHA_PCS_XFI_RXMPI_STOP |
> +			AIROHA_PCS_XFI_RXMBI_STOP |
> +			AIROHA_PCS_XFI_TXMPI_STOP |
> +			AIROHA_PCS_XFI_TXMBI_STOP);
[ ... ]
> +static int airoha_pcs_probe(struct platform_device *pdev)
> +{
> +	const struct airoha_pcs_match_data *data;
> +	struct fwnode_pcs_provider *pcs_provider;
> +	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,

[Severity: Medium]
Can this cause a NULL pointer dereference panic if the driver is bound
via sysfs to a platform device lacking an Open Firmware node?

of_device_get_match_data() returns NULL in that scenario, leading to an
unconditional dereference when reading 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]
Does this generic PHY require phy_init() and phy_power_on() to function?

While the driver acquires the PHY reference and later configures its mode in
an7581_pcs_usb_bringup() using phy_set_mode_ext(), it never initializes or
powers the PHY on. This might leave the PHY in a powered-down state, preventing
the link from coming up.

> +	}
[ ... ]
> 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)
> +{
> +	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];

[Severity: High]
Is there a risk that unconditionally writing to the shared PLL registers on
port 0's PMA could disrupt an already active port?

Since the driver lacks synchronization and reference counting, bringing up a
subsequent port may destructively reconfigure the shared PLL while the first
port is in use.

> +
> +	/* Setup LDO */
> +	usleep_range(200, 300);
[ ... ]
> +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]
Should this sequence include phy_init() and phy_power_on() for the generic
PHY? Without explicitly initializing and powering on the PHY, it might remain
in reset, preventing the network link from working.

> +
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX) {
[ ... ]
> +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 = 0;
> +	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);
> +		if (val & AIROHA_PCS_RX_SIGDET)
> +			count++;
> +	}

[Severity: High]
Does this tight polling loop outrun the hardware analog circuitry?

Without a delay (like usleep_range or cpu_relax) inside the loop, the CPU may
finish all 6 reads before the hardware has time to complete its scan and assert
the signal detection bit. This could lead to a false negative, causing the
driver to erroneously conclude the link is down.

> +
> +	/* 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/20260901083001.8044-1-ansuelsmth@gmail.com?part=11