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

On 5/13/2026 3:05 AM, [email protected] wrote:
> 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.
> --

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

For this pre-existing issue is it ok to send a separate series and will
include others error handling gaps. So far i have identified two issues.

phy: zynqmp: balance pm_runtime on probe allocation failure
phy: zynqmp: fix xpsgtr_phy_init() clock error handling

Thanks,
Radhey

> 
> commit f38f7c8479703290a0f18153b9a9207f41500566
> Author: Nava kishore Manne <[email protected]>
> 
> 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)?
>