Re: [RFC PATCH net-next v8 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 8 potential issue(s) to consider:
- [High] Bringing up a second port on multi-port devices (e.g., PCIe) inadvertently resets the shared global reset lines and analog PLLs, breaking the active link on the first port.
- [High] Sticky AN configuration bits break subsequent inband auto-negotiation configurations.
- [High] The driver acquires a generic PHY for USB ports but never initializes or powers it on.
- [High] Multiple regmap_read() calls fail to check for error codes, leading to uninitialized stack memory being consumed.
- [Medium] The RX signal detection loop polls the hardware status register 6 times back-to-back without any delay.
- [Medium] The return value of `airoha_pcs_setup_scu()` is ignored, which can lead to writing to hardware registers while the device is still held in reset.
- [Medium] Missing delay between reset assertion and deassertion.
- [Medium] Using read-modify-write on Write-1-to-Clear (W1C) interrupt status registers will unintentionally clear unhandled interrupts.
--

commit 1a7ffd4436659e2a919b7ea23223e2dc899f6f4c
Author: Christian Marangi <ansuelsmth@gmail.com>
Subject: net: pcs: airoha: add PCS driver for Airoha AN7581 SoC

This commit adds a new PCS driver for the Airoha AN7581 SoC, supporting
Ethernet, PON, PCIe, and USB SERDES. It registers as a PCS provider for
fwnode usage and supports various modes including USXGMII, 10G-BASER, and
SGMII.

> diff --git a/drivers/net/pcs/airoha/pcs-airoha-common.c b/drivers/net/pcs/airoha/pcs-airoha-common.c
> --- /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)
> +{
[ ... ]
> +	/* TODO better handle reset from MAC */
> +	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);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Will the hardware reliably detect this reset?  The reset is asserted and
immediately deasserted without any delay (such as usleep_range) between them,
which might result in a pulse too short for the hardware to sample correctly.

[Severity: High]
Does this unconditionally reset the global device-wide mac and phy reset lines
on every port's pre_config?  In multi-port scenarios like PCIe, bringing up a
second port seems like it would inadvertently reset these shared lines and
destroy the active configuration of the already-running first port.

> diff --git a/drivers/net/pcs/airoha/pcs-an7581.c b/drivers/net/pcs/airoha/pcs-an7581.c
> --- /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]
Could this hardcoded access to port 0's PMA registers break an active link?
By toggling the shared analog PLLs and resets via priv->pcs_pma[0] when
configuring port 1, it appears this might drop the link of port 0.

> @@ ... @@
> +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);
> +		if (val & AIROHA_PCS_RX_SIGDET)
> +			count++;
> +	}

[Severity: Medium]
Can the analog circuitry complete the scan in time?  The loop polls the status
register 6 times back-to-back without any temporal spacing, meaning the CPU
might instantaneously sample the exact same state in a few nanoseconds before
the hardware has time to vary its output.

> diff --git a/drivers/net/pcs/airoha/pcs-airoha-common.c b/drivers/net/pcs/airoha/pcs-airoha-common.c
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-airoha-common.c
> @@ ... @@
> +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)
> +{
[ ... ]
> +	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_pcs, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_FORCE_CL37,
> +				AIROHA_PCS_HSGMII_AN_FORCE_AN_DONE);
> +	}

[Severity: High]
Are these sticky AN configuration bits ever cleared if the port is later
reconfigured for a mode that requires inband auto-negotiation?  Since the
USB port does not issue a global reset during bringup, these force AN done
bits might permanently leak and break inband AN until a hard reboot.

> @@ ... @@
> +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;
> +	}

> diff --git a/drivers/net/pcs/airoha/pcs-an7581.c b/drivers/net/pcs/airoha/pcs-an7581.c
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-an7581.c
> @@ ... @@
> +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]
Does this code ever initialize or power on the generic PHY?  The PHY is
acquired during probe and its mode is set during bringup, but there are no
calls to phy_init() or phy_power_on().  Failing to do so leaves the PHY
unclocked or powered down, rendering the USB PCS non-functional.

> diff --git a/drivers/net/pcs/airoha/pcs-airoha-common.c b/drivers/net/pcs/airoha/pcs-airoha-common.c
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-airoha-common.c
> @@ ... @@
> +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);
> +
> +	/* MPI MBI disable */
> +	regmap_set_bits(maps->pcs_mac, AIROHA_PCS_XFI_MAC_XFI_GIB_CFG,

[Severity: Medium]
Is it safe to ignore the return value of airoha_pcs_setup_scu() here?
If the reset toggling fails, the hardware might remain held in reset,
which could cause subsequent MMIO writes to stall the bus or fail silently.

> @@ ... @@
> +static void airoha_pcs_get_state_sgmii(struct airoha_pcs_priv *priv,
> +				       unsigned int neg_mode, int index,
> +				       struct phylink_link_state *state)
> +{
> +	struct airoha_pcs_maps *maps = &priv->maps[index];
> +	u32 bmsr, lpa;
> +
> +	regmap_read(maps->hsgmii_an, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_1,
> +		    &bmsr);
> +	regmap_read(maps->hsgmii_an, AIROHA_PCS_HSGMII_AN_SGMII_REG_AN_5,
> +		    &lpa);

[Severity: High]
Does this code risk using uninitialized stack variables?  The return values of
regmap_read() are not checked. If the MMIO read fails due to power management
or hardware faults, random stack data in bmsr and lpa will be used to set
network state parameters, leaking garbage into the networking stack.

> @@ ... @@
> +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]
Will this unintentionally clear unhandled interrupts?  Using regmap_set_bits()
performs a read-modify-write operation.  If these status registers are
Write-1-to-Clear (W1C), it will read all active interrupts and write them back
as 1, silently clearing all pending interrupts instead of just the ones
specified in the mask.

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