Re: [PATCH net-next v7 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 11 potential issue(s) to consider:
- [High] Off-by-one error and lack of signedness check in `airoha_pcs_get` allows out-of-bounds array access.
- [High] Unconditional dereference of `match_data` during probe can cause a kernel panic if the driver is bound without a device tree node.
- [High] Switch statements for link speed leave critical register configuration variables completely uninitialized for unexpected speeds.
- [High] The acquired generic PHY for USB is never initialized or powered on.
- [High] Per-port interface mode is stored in the shared `priv->interface` global state, causing race conditions in multi-port devices.
- [High] Incorrect TX constant used for RX mode configuration, which breaks SGMII and 1000BASE-X on PCIe.
- [High] Per-port PCS operations modify shared device-level hardware state without synchronization, disrupting active ports.
- [Medium] Multiple state-reading functions fail to check the return value of `regmap_read()`, resulting in the use of uninitialized stack variables.
- [Low] The signal detection loop executes one more iteration than intended, potentially causing false positives.
- [Low] Assignments are separated by commas instead of semicolons, creating an unintended massive expression statement.
- [Low] The result of `airoha_pcs_setup_scu`, which performs critical reset assertions and deassertions, is silently ignored by `airoha_pcs_pre_config`.
--

commit 7fccf4b912ca13e1f4f141393b6432c311a781e1
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
> index 0000000000000..5d3b136ed2b59
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-airoha-common.c
> @@ -0,0 +1,1318 @@

[ ... ]

> +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 the global bulk reset lines for the entire block regardless
of which port is being configured?

Since the hardware supports up to two ports for PCIe, bringing up the second
port will assert the shared MAC and PHY block reset lines, which could disrupt
the already active port.

> +	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_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, lpa;
> +
> +	/* 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);
> +	regmap_clear_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_AN_CONTROL_6,
> +			  AIROHA_PCS_USXGMII_TOG_PCS_AUTONEG_STS);
> +
> +	regmap_read(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_AN_STATS_0, &lpa);
> +	regmap_read(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_AN_STATS_2, &an_done);
> +
> +	state->link = !!(lpa & MDIO_USXGMII_LINK);

[Severity: Medium]
Can regmap_read() fail and leave lpa and an_done completely uninitialized?

The return values of the read operations are not checked. If a read fails, the
uninitialized stack memory in these variables is then used in bitwise
operations to report the link state, leading to unpredictable results.

> +	state->an_complete = !!(an_done & AIROHA_PCS_USXGMII_PCS_AN_COMPLETE);
> +
> +	phylink_decode_usxgmii_word(state, 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)
> +{
> +	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;
> +	int index = port->index;
> +	u32 rate_adapt;
> +	int ret;
> +
> +	maps = &priv->maps[port->index];
> +	priv->interface = interface;

[Severity: High]
Does storing the interface mode in the shared priv structure cause race
conditions or state corruption for multi-port devices?

Since PCIe configuration uses two ports, configuring the second port will
overwrite the mode for the first port. When airoha_pcs_an_restart() is later
called, it will read the wrong interface mode. Should this state be stored
per-port in the airoha_pcs_port structure instead?

> +	data = priv->data;

[ ... ]

> +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 (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {

[ ... ]

> +	} else {
> +		if (interface == PHY_INTERFACE_MODE_USXGMII ||
> +		    interface == PHY_INTERFACE_MODE_10GBASER) {
> +			u32 mode;
> +			u32 rate_adapt;
> +
> +			switch (speed) {
> +			case SPEED_10000:
> +				rate_adapt = AIROHA_PCS_HSGMII_RATE_ADPT_FORCE_RATE_ADAPT_MODE_10000;
> +				mode = AIROHA_PCS_USXGMII_MODE_10000;
> +				break;
> +			case SPEED_5000:
> +				rate_adapt = AIROHA_PCS_HSGMII_RATE_ADPT_FORCE_RATE_ADAPT_MODE_5000;
> +				mode = AIROHA_PCS_USXGMII_MODE_5000;
> +				break;
> +			case SPEED_2500:
> +				rate_adapt = AIROHA_PCS_HSGMII_RATE_ADPT_FORCE_RATE_ADAPT_MODE_2500;
> +				mode = AIROHA_PCS_USXGMII_MODE_2500;
> +				break;
> +			case SPEED_1000:
> +				rate_adapt = AIROHA_PCS_HSGMII_RATE_ADPT_FORCE_RATE_ADAPT_MODE_1000;
> +				mode = AIROHA_PCS_USXGMII_MODE_1000;
> +				break;
> +			case SPEED_100:
> +				rate_adapt = AIROHA_PCS_HSGMII_RATE_ADPT_FORCE_RATE_ADAPT_MODE_100;
> +				mode = AIROHA_PCS_USXGMII_MODE_100;
> +				break;
> +			}
> +
> +			/* Force USXGMII to selected speed */
> +			regmap_update_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_AN_CONTROL_7,
> +					   AIROHA_PCS_USXGMII_MODE, mode);

[Severity: High]
Can an unhandled speed leave these local variables completely uninitialized?

The switch statements for speed lack a default case. For example, if SPEED_10
is passed in USXGMII mode, variables like mode and rate_adapt are never set.
The code then proceeds to write these uninitialized stack variables into
hardware control registers via regmap_update_bits().

> +
> +			if (interface == PHY_INTERFACE_MODE_10GBASER)
> +				regmap_update_bits(maps->hsgmii_rate_adp, AIROHA_PCS_HSGMII_RATE_ADAPT_CTRL_11,

[ ... ]

> +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: Low]
Is it safe to ignore the return value of airoha_pcs_setup_scu() here?

That function performs important reset assertions and deassertions. If
reset_control_bulk_deassert() fails, the hardware stays in reset, but this
function will silently proceed to perform register writes against a reset
block.

> +
> +	/* MPI MBI disable */
> +	regmap_set_bits(maps->pcs_mac, AIROHA_PCS_XFI_MAC_XFI_GIB_CFG,

[ ... ]

> +static int airoha_pcs_init_named_regmap(struct platform_device *pdev,
> +					const char *name, struct regmap **regmap)
> +{
> +	struct regmap_config regmap_config = { };
> +	void *base;
> +
> +	base = devm_platform_ioremap_resource_byname(pdev, name);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	regmap_config.name = name;
> +	regmap_config.reg_bits = 32,
> +	regmap_config.val_bits = 32,
> +	regmap_config.reg_stride = 4,

[Severity: Low]
Are these comma operators intentional?

The regmap_config fields are being initialized using commas instead of
semicolons. This creates a single expression rather than distinct statements,
which might confuse human readers and static analysis tools.

> +
> +	*regmap = devm_regmap_init_mmio(&pdev->dev, base, &regmap_config);
> +
> +	return PTR_ERR_OR_ZERO(*regmap);
> +}

[ ... ]

> +static struct phylink_pcs *airoha_pcs_get(struct fwnode_reference_args *pcsspec,
> +					  void *data)
> +{
> +	struct airoha_pcs_priv *priv = data;
> +	struct device *dev = priv->dev;
> +	int index = 0;
> +
> +	switch (priv->data->port_type) {
> +	case AIROHA_PCS_ETH:
> +	case AIROHA_PCS_PON:
> +	case AIROHA_PCS_USB:
> +		if (pcsspec->nargs) {
> +			dev_err(dev, "invalid number of cells in 'pcs-handle' property\n");
> +			return ERR_PTR(-EINVAL);
> +		}
> +
> +		break;
> +	case AIROHA_PCS_PCIE:
> +		if (pcsspec->nargs != 1) {
> +			dev_err(dev, "invalid number of cells in 'pcs-handle' property\n");
> +			return ERR_PTR(-EINVAL);
> +		}
> +
> +		break;
> +	}
> +
> +	if (pcsspec->nargs)
> +		index = pcsspec->args[0];
> +
> +	if (index > priv->data->num_port) {
> +		dev_err(dev, "invalid index cell in 'pcs-handle' property\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return &priv->ports[index].pcs;

[Severity: High]
Does this check allow an out-of-bounds array access?

The priv->ports array is sized by data->num_port. The check evaluates to false
when index equals data->num_port, allowing access one element past the end of
the array.

Also, since index is a signed integer initialized from a u64, could a very
large unsigned value truncate to a negative number and bypass this check
entirely?

> +}
> +
> +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,

[Severity: High]
Can of_device_get_match_data() return NULL here?

If the driver is bound manually via sysfs instead of through a device tree
node, this will return NULL. The code then unconditionally dereferences
data->num_port, which would cause a kernel panic. Should this include a NULL
check?

> +				   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 code acquire the PHY without ever initializing or powering it on?

The driver calls devm_phy_get() but there are no corresponding calls to
phy_init() or phy_power_on(). Without these, the PHY hardware might remain
unclocked or powered down, which could cause bus hangs when phy_set_mode_ext()
is called later in an7581_pcs_usb_bringup().

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

[ ... ]

> diff --git a/drivers/net/pcs/airoha/pcs-an7581.c b/drivers/net/pcs/airoha/pcs-an7581.c
> new file mode 100644
> index 0000000000000..638e20cac97ea
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-an7581.c
> @@ -0,0 +1,2093 @@

[ ... ]

> +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 it intentional to hardcode priv->pcs_pma[0] here even when configuring
port 1?

This re-initializes the shared PLL whenever any port is brought up. For a
dual-port PCIe configuration, initializing the second port will disrupt the
shared PLL and cause clock loss for the first port.

> +
> +	/* 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);

[ ... ]

> +static void an7581_pcs_rx_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 = priv->pcs_pma[index];
> +	u32 phyck_div, phyck_sel;
> +	u32 pr_cdr_beta_dac;
> +	u32 cdr_pr_buf_in_sr;
> +	bool cdr_pr_cap_en;
> +	u32 sigdet_vth_sel;
> +	u32 rx_rate_ctrl;
> +	u32 xfi_rx_mode;
> +	u32 osr;
> +
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +		osr = BIT(1) | BIT(0); /* 1.25G */
> +		pr_cdr_beta_dac = BIT(3);
> +		rx_rate_ctrl = 0;
> +		cdr_pr_cap_en = false;
> +		cdr_pr_buf_in_sr = BIT(2) | BIT(1) | BIT(0);
> +		sigdet_vth_sel = BIT(2) | BIT(1);
> +		phyck_div = BIT(5) | BIT(3) | BIT(0);
> +		phyck_sel = BIT(0);
> +		xfi_rx_mode = AIROHA_PCS_PMA_XFI_TX_MODE_1G25;

[Severity: High]
Is this using a TX mode constant for an RX configuration?

AIROHA_PCS_PMA_XFI_TX_MODE_1G25 has a different bit layout (shifted to bit 3)
compared to the RX mode mask which operates on bits 11 to 9.

When this is applied, the TX bits fall outside the RX mask and get filtered
out. This will write 0 to the RX mode field, improperly configuring it to 10G3
instead of 1G25.

> +		break;
> +	case 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;
> +	int i;
> +
> +	regmap_write(pcs_pma, AIROHA_PCS_PMA_DIG_RESERVE_0,
> +		     AIROHA_PCS_TRIGGER_RX_SIDGET_SCAN);
> +
> +	/* Scan 5 times for RX sigdet module to detect RX signal */
> +	for (i = 0; i <= 5; i++) {

[Severity: Low]
Does this loop execute six times instead of the five times mentioned in the
comment?

The loop uses i <= 5, which iterates for 0 through 5. Since the logic looks
for count >= 4 to determine a valid signal, checking six times lowers the
threshold percentage and might return true on marginal or false signals.

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

[ ... ]

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