Re: [RESEND PATCH 2/3] phy: zynqmp: use read-modify-write for SERDES scrambler bypass

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The error code from clk_prepare_enable() is not propagated, causing a false success return.
- [High] Missing clk_disable_unprepare() on error paths causes a clock resource leak.
--

commit 5cbba5539e34550743b9636376f1a41a65d04bb8
Author: Nava kishore Manne <nava.kishore.manne@amd.com>

phy: zynqmp: use read-modify-write for SERDES scrambler bypass

This commit switches xpsgtr_bypass_scrambler_8b10b() to use 
xpsgtr_clr_set_phy() instead of xpsgtr_write_phy(). This modifies only the
bypass bits while preserving the rest of the register fields.

> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
> index c037d7c13d485..6c56c4df8523c 100644
> --- a/drivers/phy/xilinx/phy-zynqmp.c
> +++ b/drivers/phy/xilinx/phy-zynqmp.c
> @@ -505,8 +505,12 @@ static void xpsgtr_lane_set_protocol(struct xpsgtr_phy *gtr_phy)
>  /* Bypass (de)scrambler and 8b/10b decoder and encoder. */
>  static void xpsgtr_bypass_scrambler_8b10b(struct xpsgtr_phy *gtr_phy)
>  {

[ ... ]

> static int xpsgtr_phy_init(struct phy *phy)
> {
> 	struct xpsgtr_phy *gtr_phy = phy_get_drvdata(phy);
> 	struct xpsgtr_dev *gtr_dev = gtr_phy->dev;
> 	int ret = 0;
> 
> 	mutex_lock(&gtr_dev->gtr_mutex);
> 
> 	/* Configure and enable the clock when peripheral phy_init call */
> 	if (clk_prepare_enable(gtr_dev->clk[gtr_phy->refclk]))
> 		goto out;

[Severity: High]
This is a pre-existing issue, but does this error path fail to set the
return code? 

When clk_prepare_enable() fails, it jumps to 'out' while 'ret'
is still 0, leading to a false success return. This can cause the PHY
framework to assume the PHY is initialized.

> 
> 	/* Skip initialization if not required. */
> 	if (!xpsgtr_phy_init_required(gtr_phy))
> 		goto out;
> 
> 	if (gtr_dev->tx_term_fix) {
> 		ret = xpsgtr_phy_tx_term_fix(gtr_phy);
> 		if (ret < 0)
> 			goto out;

[Severity: High]
This is a pre-existing issue, but does this error path jump to 'out' without
disabling the previously enabled clock?

Because 'ret' is negative here, the PHY framework considers the
initialization failed and won't call xpsgtr_phy_exit(). This results in
a permanent clock resource leak.

> 
> 		gtr_dev->tx_term_fix = false;
> 	}
> 
> 	/* Enable coarse code saturation limiting logic. */
> 	xpsgtr_write_phy(gtr_phy, L0_TM_PLL_DIG_37, L0_TM_COARSE_CODE_LIMIT);
> 
> 	/*
> 	 * Configure the PLL, the lane protocol, and perform protocol-specific
> 	 * initialization.
> 	 */
> 	ret = xpsgtr_configure_pll(gtr_phy);
> 	if (ret)
> 		goto out;

[Severity: High]
This is a pre-existing issue, but does this error path also leak the clock?

Like the tx_term_fix failure above, this jumps to 'out' without calling
clk_disable_unprepare() on the clock enabled at the start of the function.

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

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