Re: [RESEND PATCH 3/3] phy: zynqmp: keep SERDES scrambler and 8b/10b enabled for USB

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] PCIe and DisplayPort (DP) protocols lack explicit initialization of the scrambler/encoder bypass state, leaving them vulnerable to state pollution.
- [High] Silently ignores clock enable failure, masking the error and causing unbalanced clock disables later.
- [High] Bus width registers modified by SGMII are not reset by other protocols, risking state inheritance bugs.
--

commit e6d5f06ce31b70c624ec3e6a69c5b7e7ef55746a
Author: Nava kishore Manne <nava.kishore.manne@amd.com>
Subject: phy: zynqmp: keep SERDES scrambler and 8b/10b enabled for USB

This commit ensures that the scrambler and 8b/10b encoder/decoder are not
bypassed for USB operation, as mandated by the USB 3.x specification. The
bypass functionality is restricted to SATA and SGMII modes where encoding is
handled in the controller.

Cc: stable@vger.kernel.org

> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
> index 6c56c4df8523c..087fe402e4e20 100644
> --- a/drivers/phy/xilinx/phy-zynqmp.c
> +++ b/drivers/phy/xilinx/phy-zynqmp.c
[ ... ]
> @@ -547,7 +562,7 @@ static void xpsgtr_phy_init_sgmii(struct xpsgtr_phy *gtr_phy)
>  	xpsgtr_clr_set(gtr_dev, TX_PROT_BUS_WIDTH, mask, val);
>  	xpsgtr_clr_set(gtr_dev, RX_PROT_BUS_WIDTH, mask, val);

[Severity: High]
This isn't a bug introduced by this patch, but SGMII alters the global bus
width registers here without other protocols guaranteeing they are restored to
default states.

If a lane previously used for SGMII is reassigned to another protocol (e.g.,
via warm reboot or bootloader handoff), will the 10-bit bus width setting
persist in the hardware and silently break other protocols (like USB or PCIe)
that expect their default bus widths?

>  
> -	xpsgtr_bypass_scrambler_8b10b(gtr_phy);
> +	xpsgtr_bypass_scrambler_8b10b(gtr_phy, true);
>  }
[ ... ]
> @@ -707,6 +722,10 @@ static int xpsgtr_phy_init(struct phy *phy)
>  	case ICM_PROTOCOL_SGMII:
>  		xpsgtr_phy_init_sgmii(gtr_phy);
>  		break;
> +
> +	case ICM_PROTOCOL_USB:
> +		xpsgtr_bypass_scrambler_8b10b(gtr_phy, false);
> +		break;

[Severity: High]
This isn't a bug introduced by this patch, but the switch statement in
xpsgtr_phy_init() misses explicit bypass configuration for DP and PCIe.

If a lane is dynamically reassigned or left bypassed by previous boot stages,
will DP and PCIe silently fail as they inherit a stale bypass = true state?

>  	}
>  
>  out:

[Severity: High]
This isn't a bug introduced by this patch, but looking at xpsgtr_phy_init(),
could the clock error path cause issues here?

drivers/phy/xilinx/phy-zynqmp.c:xpsgtr_phy_init() {
    int ret = 0;
    ...
    if (clk_prepare_enable(gtr_dev->clk[gtr_phy->refclk]))
        goto out;
    ...
out:
    mutex_unlock(&gtr_dev->gtr_mutex);
    return ret;
}

If clk_prepare_enable() fails, we branch to 'out', but 'ret' remains 0. Will
this silently ignore the clock enable failure, mask the error, and cause
unbalanced clock disables later when xpsgtr_phy_exit() calls
clk_disable_unprepare() on a clock that was never successfully enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627155229.2791113-1-radhey.shyam.pandey@amd.com?part=3

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