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

[PATCH net v2 0/3] stmmac: fix ETHQOS SerDes interface handling

[Thomas Karpiniec]

From: Thomas Karpiniec <tom@wendy.sh>

With a QCA8081 PHY and phy-mode = "2500base-x", stmmac reports only
2500BASE-X in phylink's supported interface bitmap. Phylink consequently
removes the PHY's lower-speed copper modes, preventing a link with a
gigabit switch.

The QCA8081 uses SGMII for lower copper speeds and 2500BASE-X for 2.5Gbps.
ETHQOS already switches the SerDes accordingly. Add the integrated PCS
interface bitmap to phylink's supported interfaces and mark the Qualcomm
SerDes as supporting 2500BASE-X. Also propagate platform mac_finish()
errors instead of reporting a successfully configured link.

The existing Qualcomm PCS ANE handling is unchanged. Physical cable swaps
produced transient PCS events during the unplug interval, but carrier and
PCS counters remained unchanged for 60 seconds after each final link.

This exact series was tested on an IQ-8275 EVK with cold boots and
physical cable swaps at 1Gbps and 2.5Gbps. Two 100-packet tests at 1Gbps
had no loss. Injecting -ETIMEDOUT from mac_finish() held carrier down at
both speeds, and both recovered after disabling injection and restarting
the interface. The test did not cover a speed transition with the cable
remaining connected or traffic at 2.5Gbps.

The tests used regulator_ignore_unused: without it, the upstream board
tree loses PCS link because it lacks the vendor REFGEN always-on
workaround. No other kernel or device-tree patches were applied.

Changes in v2:
- Replace the qcom-ethqos get_interfaces() callback with integrated PCS
  interface propagation and the 2500BASE-X capability flag.

Thomas Karpiniec (3):
  net: stmmac: propagate platform mac_finish errors
  net: stmmac: advertise integrated PCS interfaces to phylink
  net: stmmac: qcom-ethqos: enable 2500BASE-X in integrated PCS

 .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    |  1 +
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c      | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)


base-commit: 46bc52d13594848023e681860df8700c8db14354
-- 
2.54.0 (Apple Git-157)

[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)

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 --]