diff mbox series

[net-next,v13,06/10] net: stmmac: qcom-ethqos: update phy_mode to the resolved interface in mac_finish()

Message ID 20260713-qcom-sa8255p-emac-v13-6-119f8699ef8e@oss.qualcomm.com (mailing list archive)
State New
Headers
Series net: stmmac: qcom-ethqos: add support for SCMI power domains |

Commit Message

Bartosz Golaszewski July 13, 2026, 1:20 p.m. UTC 
ethqos->phy_mode is read by serdes_powerup() and
qcom_ethqos_set_sgmii_loopback() to configure the SerDes and its
wrapper for the correct interface. It is set once at probe from the DT
phy-mode property and never updated.

phylink resolves the actual runtime interface and passes it to the
mac_finish() callback. On platforms that only ever operate at one speed
these two values are always the same, but when the interface changes
at runtime - for example switching between SGMII and 2500BASE-X — the
stored value becomes stale. The next serdes_powerup() call, on
a subsequent link cycle, then reinitialises the SerDes to the wrong mode.

Update ethqos->phy_mode from the interface argument in
ethqos_mac_finish_serdes() so that it always reflects the last
resolved interface.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

sashiko-bot@kernel.org July 14, 2026, 3:42 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Updating `ethqos->phy_mode` before clearing the SGMII loopback causes the loopback to remain erroneously enabled when switching away from 2500BASE-X.
--

--- Patch [6]: [PATCH net-next v13 06/10] net: stmmac: qcom-ethqos: update phy_mode to the resolved interface in mac_finish() ---
commit 3dfd6663f759c10c24659b5275b88ff5f976b420
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

net: stmmac: qcom-ethqos: update phy_mode to the resolved interface in mac_finish()

This commit updates the driver to overwrite ethqos->phy_mode with the
resolved phylink interface during the mac_finish() callback. This ensures
the stored mode matches runtime changes instead of being permanently stuck
at the device tree default.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 47b70b5e706f2..9d19203ae4bd7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -632,6 +632,12 @@ static int ethqos_mac_finish_serdes(struct net_device *ndev, void *priv,
>  	struct qcom_ethqos *ethqos = priv;
>  	int ret = 0;
>  
> +	/* The interface mode is only known once phylink has resolved it and
> +	 * may change at runtime, so keep it in sync for later
> +	 * serdes_powerup()/serdes_powerdown() calls.
> +	 */
> +	ethqos->phy_mode = interface;
> +
>  	qcom_ethqos_set_sgmii_loopback(ethqos, false);

[Severity: High]
Does updating ethqos->phy_mode here prevent the loopback from being disabled?

If the link negotiates from 2500BASE-X down to SGMII, this update changes
ethqos->phy_mode to SGMII before the loopback is disabled.

Looking at qcom_ethqos_set_sgmii_loopback(), it checks the current mode to
determine if it needs to act:

qcom_ethqos_set_sgmii_loopback() {
    if (!ethqos->needs_sgmii_loopback ||
        ethqos->phy_mode != PHY_INTERFACE_MODE_2500BASEX)
            return;
    ...
}

Will this cause the function to return early without clearing the loopback
when switching away from 2500BASE-X, leaving outgoing packets reflected
internally instead of transmitting to the wire?
Jakub Kicinski July 24, 2026, 10:45 p.m. UTC | #2 
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: stmmac: qcom-ethqos: update phy_mode to the resolved interface in mac_finish()

This patch updates ethqos->phy_mode from the interface argument in
ethqos_mac_finish_serdes() so that it always reflects the last resolved
interface, avoiding a stale value being observed by later
serdes_powerup()/serdes_powerdown() calls when the runtime interface
differs from the DT phy-mode.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index 47b70b5e706f..9d19203ae4bd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -632,6 +632,12 @@ static int ethqos_mac_finish_serdes(struct net_device *ndev, void *priv,
>  	struct qcom_ethqos *ethqos = priv;
>  	int ret = 0;
>  
> +	/* The interface mode is only known once phylink has resolved it and
> +	 * may change at runtime, so keep it in sync for later
> +	 * serdes_powerup()/serdes_powerdown() calls.
> +	 */
> +	ethqos->phy_mode = interface;
> +
>  	qcom_ethqos_set_sgmii_loopback(ethqos, false);

[High]
Is the ordering here safe with respect to qcom_ethqos_set_sgmii_loopback()?

That helper early-returns unless ethqos->phy_mode equals
PHY_INTERFACE_MODE_2500BASEX:

    static void
    qcom_ethqos_set_sgmii_loopback(struct qcom_ethqos *ethqos, bool enable)
    {
            if (!ethqos->needs_sgmii_loopback ||
                ethqos->phy_mode != PHY_INTERFACE_MODE_2500BASEX)
                    return;
            ...
    }

Because ethqos->phy_mode is now overwritten with the freshly resolved
interface before the qcom_ethqos_set_sgmii_loopback(ethqos, false) call,
does that clear-side path still fire when the interface transitions away
from 2500BASE-X?

Consider a needs_sgmii_loopback platform (for example sa8775p) with a DT
phy-mode of 2500BASE-X. At probe time ethqos_clks_config(true) asserts
SGMII_PHY_CNTRL1_SGMII_TX_TO_RX_LOOPBACK_EN in the wrapper while
ethqos->phy_mode is still the DT value 2500BASE-X. If phylink later
resolves the interface to SGMII and calls ethqos_mac_finish_serdes():

    ethqos->phy_mode = interface;                   /* now SGMII */
    qcom_ethqos_set_sgmii_loopback(ethqos, false);  /* early-return */

Does the port then enter SGMII operation with the wrapper TX-to-RX
loopback bit still asserted, which is the exact 2500BASE-X to SGMII
runtime-transition scenario the commit message says it wants to make
work?

Would calling qcom_ethqos_set_sgmii_loopback(ethqos, false) first,
against the old phy_mode, and only then assigning
ethqos->phy_mode = interface avoid this?

The commit message describes the change only in terms of
serdes_powerup()/serdes_powerdown():

    ethqos->phy_mode is read by serdes_powerup() and
    qcom_ethqos_set_sgmii_loopback() to configure the SerDes and its
    wrapper for the correct interface.
    ...
    Update ethqos->phy_mode from the interface argument in
    ethqos_mac_finish_serdes() so that it always reflects the last
    resolved interface.

Should it also mention that the assignment changes what the immediately
following qcom_ethqos_set_sgmii_loopback(ethqos, false) observes?

For reference, this appears to be addressed later in the same series by
commit 8d1efdf990fc ("net: stmmac: qcom-ethqos: fix SGMII loopback not
set on resume after speed change"), which extends the guard in
qcom_ethqos_set_sgmii_loopback() to also match PHY_INTERFACE_MODE_SGMII.
That follow-up commit message frames the fix as a set-side DMA-reset
timeout on resume, but does the same guard change also happen to unbreak
the clear-side path introduced here? If so, would it be worth either
reordering the two statements in this patch or folding the guard update
in ahead of this one to avoid a bisect window where the SGMII wrapper
loopback stays asserted after a 2500BASE-X to SGMII transition?

>  
>  	if (interface == PHY_INTERFACE_MODE_SGMII ||

[ ... ]
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index 47b70b5e706f221c01f1c0ae3b1acafae6641165..9d19203ae4bd7dc17aca068f5eb88ff830e4e906 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -632,6 +632,12 @@  static int ethqos_mac_finish_serdes(struct net_device *ndev, void *priv,
 	struct qcom_ethqos *ethqos = priv;
 	int ret = 0;
 
+	/* The interface mode is only known once phylink has resolved it and
+	 * may change at runtime, so keep it in sync for later
+	 * serdes_powerup()/serdes_powerdown() calls.
+	 */
+	ethqos->phy_mode = interface;
+
 	qcom_ethqos_set_sgmii_loopback(ethqos, false);
 
 	if (interface == PHY_INTERFACE_MODE_SGMII ||