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

[Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>]

On Fri, Sep 11, 2026 at 11:25:12AM +0000, netdev-bot+sashiko@kernel.org wrote:
> 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) {
> 
> [ ... ]
> 

In my opinion, there's something already very wrong if we have a
fixed-link setup with phy-mode as RGMII and speed anything greater
than 1000. So I am not sure this comment makes sense.

	Ayaan