Re: [PATCH net 1/2] 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] The commit's stated goal — stop phylink from treating an
  interface as successfully configured when the SerDes is not ready — is…
- [Low] The continuation line of the newly returned call is misaligned by
  one column.

--

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 interface was configured
successfully.  The patch forwards the callback's result instead, keeping the
zero return when no callback is installed.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1fb5f804ea234..1350e92943ac1 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 changelog says phylink "can report carrier even though the SerDes is
not ready", and that after this change phylink can "keep the link down
until a subsequent configuration succeeds".  Does that hold for the other
in-tree user of plat->mac_finish?

intel_mac_finish() in dwmac-intel.c ends with:

	ret = intel_set_reg_access(regs, max_regs);
	if (ret < 0)
		return ret;

	priv->plat->phy_interface = interface;

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

	return ret;

ret is the zero left by intel_set_reg_access(), and the result of
intel_serdes_powerup() is never assigned, so the callback returns 0 even
when the SerDes bring-up failed.

intel_serdes_powerup() returns the -ETIMEDOUT from serdes_status_poll()
(ten polls of SERDES_GSR0) at three points:

	if (data) {
		dev_err(priv->device, "Serdes PLL clk request timeout\n");
		return data;
	}
	...
	if (data) {
		dev_err(priv->device, "Serdes assert lane reset timeout\n");
		return data;
	}
	...
	if (data) {
		dev_err(priv->device, "Serdes power state P0 timeout.\n");
		return data;
	}

In each of those cases the preceding intel_serdes_powerdown() has already
put the lane in SERDES_PWR_ST_P3 (and gated SERDES_PHY_RX_CLK on PSE
parts), and plat->phy_interface has already been overwritten with no
rollback.

Since the Intel callback discards the status itself, can forwarding its
return value in stmmac_mac_finish() surface anything for the EHL/ADL-N
SGMII configurations that install intel_mac_finish (ehl_sgmii_data,
ehl_pse0/1_sgmii1g_data, adln_sgmii_phy0_data)?  The mac_finish contract
in include/linux/phylink.h states "Returns zero on success, or negative
errno on failure which will be reported to the kernel log", so should
intel_mac_finish() also return the intel_serdes_powerup() result for the
described behaviour to materialise on that platform?

[Severity: Low]
This isn't a bug, but the continuation line looks off by one column now.
The statement is two tabs deep, plus "return " and
"priv->plat->mac_finish(", which puts ndev at column 46, while the
continuation uses five tabs plus five spaces, i.e. column 45.  checkpatch
--strict should emit "CHECK: Alignment should match open parenthesis" for
this.

>  
>  	return 0;
>  }

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