Re: [PATCH net v2 1/3] net: stmmac: propagate platform mac_finish errors

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Incomplete fix: the bug class the commit message claims to close
  ('Phylink then treats the interface as successfully configured and can…
- [Low] Continuation-line misalignment introduced by the patch in
  stmmac_mac_finish(): after adding 'return ', the open parenthesis of…

--

net: stmmac: propagate platform mac_finish errors

stmmac_mac_finish() previously dropped the return value of the platform
mac_finish callback and always told phylink the configuration succeeded.
The patch returns the callback result instead, keeping the zero return for
the case where no platform callback is installed.

No Link: tags present in the commit header.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1fb5f804ea23..1350e92943ac 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1048,8 +1048,8 @@ static int stmmac_mac_finish(struct phylink_config *config, unsigned int mode,
>  	struct stmmac_priv *priv = netdev_priv(ndev);
>  
>  	if (priv->plat->mac_finish)
> -		priv->plat->mac_finish(ndev, priv->plat->bsp_priv, mode,
> -				       interface);
> +		return priv->plat->mac_finish(ndev, priv->plat->bsp_priv, mode,
> +					     interface);

[Severity: High]
The commit message says that without this change "Phylink then treats the
interface as successfully configured and can report carrier even though the
SerDes is not ready".  Does that failure mode still remain reachable on the
Intel mGbE ports after this patch?

intel_mac_finish() in dwmac-intel.c is the other in-tree mac_finish
implementer (installed by ehl_sgmii_data(), ehl_pse0_sgmii1g_data(),
ehl_pse1_sgmii1g_data() and adln_sgmii_phy0_data()), and it ends with:

drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c:intel_mac_finish() {
	...
	priv->plat->phy_interface = interface;

	intel_serdes_powerdown(ndev, intel_priv);
	intel_serdes_powerup(ndev, intel_priv);

	return ret;
}

ret here is the value from intel_set_reg_access(), normally zero, so the
return value of intel_serdes_powerup() is dropped.  That function has three
timeout exits fed by serdes_status_poll():

	data = serdes_status_poll(priv, serdes_phy_addr,
				  SERDES_GSR0,
				  SERDES_PLL_CLK,
				  SERDES_PLL_CLK);

	if (data) {
		dev_err(priv->device, "Serdes PLL clk request timeout\n");
		return data;
	}

with the other two covering the lane reset reflection and the
SERDES_PWR_ST_P0 transition.  serdes_status_poll() returns -ETIMEDOUT after
10 retries.

Since intel_serdes_powerdown() has already dropped the lane to P3, a timeout
in intel_serdes_powerup() leaves the lane out of P0 or held in reset, yet
intel_mac_finish() returns 0.  phylink_major_config() only records a failure
for err < 0, so phylink still sees success and can raise carrier.

Would it make sense to add a companion change so that intel_mac_finish()
returns the intel_serdes_powerup() result, so that the propagation added
here has an effect on this platform too?

[Severity: Low]
This isn't a bug, but the continuation line no longer lines up with the open
parenthesis after "return " was inserted: the argument list now starts one
column further right, so "interface);" needs one more space to satisfy
checkpatch.pl --strict ("CHECK: Alignment should match open parenthesis").

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SJ2PR11MB8401F5C3A075DE2A0013E063E4872%40SJ2PR11MB8401.namprd11.prod.outlook.com