Re: [PATCH net-next v2 4/9] net: stmmac: qcom-ethqos: convert ethqos_rgmii_macro_init() to void

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] In `ethqos_rgmii_macro_init()`
  (drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c) the patch…

--

net: stmmac: qcom-ethqos: convert ethqos_rgmii_macro_init() to void

The patch changes ethqos_rgmii_macro_init() to return void because its only
caller ignores the return value, and it drops the speed validation from that
function on the grounds that the same check exists higher up the call stack.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index ac7d6d3e205a1..44270c25d874f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -371,9 +371,8 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
>  	return 0;
>  }
>  
> -static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
> +static void ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
>  {
> -	struct device *dev = &ethqos->pdev->dev;
>  	unsigned int prg_rclk_dly, loopback;
>  	unsigned int phase_shift;
>  
> @@ -384,11 +383,6 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
>  	/* Select RGMII, write 0 to interface select */
>  	rgmii_clrmask(ethqos, RGMII_CONFIG_INTF_SEL, RGMII_IO_MACRO_CONFIG);
>  
> -	if (speed != SPEED_1000 && speed != SPEED_100 && speed != SPEED_10) {
> -		dev_err(dev, "Invalid speed %d\n", speed);
> -		return -EINVAL;
> -	}
> -

[Severity: Medium]
The commit message says "the speed validation duplicates a check higher up
the call stack".  Which check is that referring to?

The only path here is:

  stmmac_mac_link_up()
    -> priv->plat->fix_mac_speed()
      -> ethqos_fix_mac_speed_rgmii()
        -> ethqos_rgmii_macro_init()

stmmac_mac_link_up() only bails out in the default arm of its speed switch:

	case SPEED_2500:
		if (interface == PHY_INTERFACE_MODE_USXGMII)
			ctrl |= priv->hw->link.xgmii.speed2500;
		else
			ctrl |= priv->hw->link.speed2500;
		break;
	...
	default:
		netdev_err(priv->dev,
			   "unsupported speed %s on %s, leaving the MAC disabled\n",
			   phy_speed_to_str(speed), phy_modes(interface));
		return;
	}

	if (priv->plat->fix_mac_speed)
		priv->plat->fix_mac_speed(priv->plat->bsp_priv, interface,
					  speed, mode);

so SPEED_2500 and everything above it are accepted and forwarded verbatim to
fix_mac_speed() regardless of the interface mode.

ethqos_fix_mac_speed_rgmii() does no RGMII-specific speed validation either;
it only distinguishes speed == SPEED_1000 from speed == SPEED_100/SPEED_10
while restoring POR values and bringing up the DLL, then passes speed
straight through.

So the removed test rejected speeds that are invalid for RGMII, while what
remains upstream only rejects speeds the core does not recognise at all.
Are these really the same check?

A speed above 1000 does appear reachable on an RGMII port through a fixed
link.  phylink_parse_fixedlink() keeps the requested speed even when it does
not match the validated supported set:

	if (c) {
		...
	} else {
		phylink_warn(pl, "fixed link %s duplex %dMbps not recognised\n",
			     pl->link_config.duplex == DUPLEX_FULL ? "full" : "half",
			     pl->link_config.speed);
	}

and phylink_get_fixed_state() then hands that speed to mac_link_up() via
*state = pl->link_config.  With phy-mode = "rgmii" plus
fixed-link { speed = <2500>; full-duplex; }, does 2500 now reach
ethqos_rgmii_macro_init() and silently take the non-1000 branches
(BYPASS_TX_ID_EN set, POS_NEG_DATA_SEL and PROG_SWAP cleared,
PRG_RCLK_DLY_CODE = 5, RX_PROG_SWAP cleared on pre-GE3 parts) while neither
MAX_SPD_PRG_2 nor MAX_SPD_PRG_9 is programmed, with the DLL already locked
by the caller?

The "Invalid speed %d" dev_err() was the only indication that the RGMII
configuration is unsupported.  Would it make sense to move the check into
ethqos_fix_mac_speed_rgmii(), before the POR restore and the DLL
reset/enable sequence, rather than dropping it?

For reference, no later patch in this series reinstates a speed check:
ethqos_fix_mac_speed_rgmii() and ethqos_rgmii_macro_init() still accept any
speed at the end of the series, and the newly added
ethqos_rgmii_id_macro_init() is equally unvalidated.

>  	rgmii_setmask(ethqos, RGMII_CONFIG_DDR_MODE, RGMII_IO_MACRO_CONFIG);
>  
>  	if (speed == SPEED_1000) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908-shikra_ethernet-v2-0-bbe3389d0652%40oss.qualcomm.com