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

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

[netdev-bot+sashiko]

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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1846 bytes --]

> From: Thomas Karpiniec <tom@wendy.sh>
> 
> stmmac_mac_finish() discards the platform callback's return value and
> always reports success to phylink. For example, the Qualcomm ETHQOS
> callback can return an error from phy_set_mode_ext() if SerDes
> configuration fails. Phylink then treats the interface as successfully
> configured and can report carrier even though the SerDes is not ready.
> 
> Return the platform callback's result so that phylink can report the
> failure and keep the link down until a subsequent configuration succeeds.
> Keep returning zero when no platform callback is installed.
> 
> Fixes: e654cfc718d4 ("net: stmmac: configure SerDes on mac_finish")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM sparse
> Signed-off-by: Thomas Karpiniec <tom@wendy.sh>
> Acked-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 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);
>  
>  	return 0;
>  }
> -- 
> 2.54.0 (Apple Git-157)
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Thomas Karpiniec]

From: Thomas Karpiniec <tom@wendy.sh>

stmmac_mac_finish() discards the platform callback's return value and
always reports success to phylink. For example, the Qualcomm ETHQOS
callback can return an error from phy_set_mode_ext() if SerDes
configuration fails. Phylink then treats the interface as successfully
configured and can report carrier even though the SerDes is not ready.

Return the platform callback's result so that phylink can report the
failure and keep the link down until a subsequent configuration succeeds.
Keep returning zero when no platform callback is installed.

Fixes: e654cfc718d4 ("net: stmmac: configure SerDes on mac_finish")
Cc: stable@vger.kernel.org
Assisted-by: LLM sparse
Signed-off-by: Thomas Karpiniec <tom@wendy.sh>
Acked-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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);
 
 	return 0;
 }
-- 
2.54.0 (Apple Git-157)