Re: [PATCH v2 phy-next 8/8] phy: lynx-10g: use RCW override procedure for dynamic protocol change

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Read-modify-write in `fsl_guts_rmw()` drops dynamic overrides of other lanes sharing the same register.
- [High] If `fsl_guts_init()` fails during `soc_device_register()`, the newly added call to `fsl_guts_lane_set_mode()` will trigger an uninitialized spinlock BUG.
--

commit b8f6b1d355d36a951d62ed32d06cbd69afef40c2
Author: Vladimir Oltean <vladimir.oltean@nxp.com>

phy: lynx-10g: use RCW override procedure for dynamic protocol change

This patch enables dynamic protocol changes beyond 1000Base-X/SGMII and
2500Base-X. It removes the previous limitations by utilizing the RCW
override procedure provided by the guts driver.

diff --git a/drivers/phy/freescale/phy-fsl-lynx-10g.c b/drivers/phy/freescale/phy-fsl-lynx-10g.c
index 38def160ef1aa..5ece7889aed7b 100644
--- a/drivers/phy/freescale/phy-fsl-lynx-10g.c
+++ b/drivers/phy/freescale/phy-fsl-lynx-10g.c

[ ... ]

@@ -1225,6 +1223,13 @@ static int lynx_10g_set_mode(struct phy *phy, enum phy_mode mode, int submode)
 	if (powered_up)
 		lynx_10g_lane_halt(phy);
 
+	if (lynx_10g_lane_mode_needs_rcw_override(lane, lane_mode)) {
+		err = fsl_guts_lane_set_mode(priv->info->index, lane->id,
+					     lane_mode);
                 ^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but does calling
fsl_guts_lane_set_mode() here expose a bug in fsl_guts_rmw() where dynamic
overrides on other lanes are dropped?

When fsl_guts_lane_set_mode() calls fsl_guts_rmw() to update the hardware mux,
it incorrectly reads from the boot-time status register (RCWSR) instead of the
control register (RCWCR):

drivers/soc/fsl/guts.c:fsl_guts_rmw() {
    ...
    u32 tmp = fsl_guts_read(&soc.dcfg_ccsr->rcwsr[offset]);

    tmp &= ~mask;
    tmp |= val;
    fsl_guts_write(&soc.dcfg_dcsr->rcwcr[offset], tmp);
    ...
}

Because RCWSR is read-only and reflects the boot-time configuration, and
multiple lanes share the same register, won't dynamically changing the
protocol of one lane revert any previous dynamic changes applied to the other
lanes?

[Severity: High]
This is a pre-existing issue, but could this also trigger an uninitialized
spinlock BUG if fsl_guts_init() fails?

In drivers/soc/fsl/guts.c, if fsl_guts_init() fails during
soc_device_register(), the error path doesn't clean up the global soc.data
and skips initializing soc.rcwcr_lock:

drivers/soc/fsl/guts.c:fsl_guts_init() {
    ...
    soc_dev = soc_device_register(soc_dev_attr);
    if (IS_ERR(soc_dev)) {
        ret = PTR_ERR(soc_dev);
        goto err;
    }

    spin_lock_init(&soc.rcwcr_lock);
    ...
}

Since soc.data is left populated, the readiness check in
fsl_guts_lane_set_mode() will pass. Won't this then proceed to acquire the
uninitialized soc.rcwcr_lock, triggering a SPIN_BUG_ON?

> +		if (err)
> +			goto out;
> +	}
> +
> 	err = lynx_10g_lane_disable_pcvt(lane, lane->mode);
> 	if (err)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612210859.266759-1-vladimir.oltean@nxp.com?part=8

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy