Re: [PATCH 1/3] phy: zynqmp: fix L0_TM_DISABLE_SCRAMBLE_ENCODER mask

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unhandled Error Path in `xpsgtr_phy_init()` masks clock initialization failures.
--

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

phy: zynqmp: fix L0_TM_DISABLE_SCRAMBLE_ENCODER mask

The L0_TX_DIG_61 register bit 2 is a reserved read-only field.
The previous mask value 0x0f incorrectly included bit 2, causing
unintended writes to a reserved bit on every scrambler bypass
operation.

Correct the mask to (BIT(3) | GENMASK(1, 0)) to cover only the
valid scramble bypass control bits.

> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
> index fe6b4925d1662..c037d7c13d485 100644
> --- a/drivers/phy/xilinx/phy-zynqmp.c
> +++ b/drivers/phy/xilinx/phy-zynqmp.c
> @@ -53,7 +53,7 @@
>  #define L0_TM_DIG_6			0x106c
>  #define L0_TM_DIS_DESCRAMBLE_DECODER	0x0f
>  #define L0_TX_DIG_61			0x00f4
> -#define L0_TM_DISABLE_SCRAMBLE_ENCODER	0x0f
> +#define L0_TM_DISABLE_SCRAMBLE_ENCODER	(BIT(3) | GENMASK(1, 0))
>  
>  /* PLL Test Mode register parameters */
>  #define L0_TM_PLL_DIG_37		0x2094

This isn't a bug introduced by this patch, but there is a pre-existing
issue in xpsgtr_phy_init().

If clk_prepare_enable() fails, the error path appears to mask the failure:

drivers/phy/xilinx/phy-zynqmp.c:xpsgtr_phy_init() {
	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;
...
out:
	mutex_unlock(&gtr_dev->gtr_mutex);
	return ret;
}

Since ret is initialized to 0, will this return 0 instead of the actual error
code from clk_prepare_enable()?

If the function falsely reports success, the PHY framework will consider the
device initialized.

During teardown, could xpsgtr_phy_exit() unconditionally call
clk_disable_unprepare() on a clock that was never enabled, causing a clock
reference count underflow and a kernel WARN_ON?

Furthermore, would allowing the upper layer to access PHY registers without a
running clock risk hanging the AXI bus on ZynqMP, potentially leading to an
asynchronous external abort (system panic)?

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

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