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

Re: [PATCH net 1/2] 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] 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

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

[Thomas Karpiniec]

On 16/09/2026 10:03 pm, Mohd Ayaan Anwar wrote:
> On Wed, Sep 16, 2026 at 08:39:49PM +1000, Thomas Karpiniec wrote:
>> 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>
>> ---
>>   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;
>>   }
> 
> Just curious, are you trying to resolve the issue where an interface
> gets created depite:
>    qcom-dwmac-sgmii-phy 8909000.phy: QSERDES_COM_C_READY_STATUS timed-out
>    qcom-ethqos 23040000.ethernet eth0: __stmmac_open: Serdes powerup failed
> in dmesg?
No, I haven't seen that error or any specific SerDes faults. The error 
message you mention appears to be a failure in the serdes_powerup 
callback whereas this is propagating errors from mac_finish.

I just happened to notice the discarded return value while working in 
the area so I don't think it's related.

Tom

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

[Maxime Chevallier]

Hi,

On 9/16/26 12:39, Thomas Karpiniec wrote:
> 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>

Let's see if this uncovers other dirt under the stmmac phylink carpet :)

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

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

[Mohd Ayaan Anwar]

On Wed, Sep 16, 2026 at 08:39:49PM +1000, Thomas Karpiniec wrote:
> 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>
> ---
>  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;
>  }

Just curious, are you trying to resolve the issue where an interface
gets created depite:
  qcom-dwmac-sgmii-phy 8909000.phy: QSERDES_COM_C_READY_STATUS timed-out
  qcom-ethqos 23040000.ethernet eth0: __stmmac_open: Serdes powerup failed
in dmesg?

Anyways, the change looks reasonable to me:

Acked-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

	Ayaan

[PATCH net 1/2] 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>
---
 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.50.1 (Apple Git-155)