Re: [PATCH v2 1/4] phy: qcom: qmp-pcie: Skip PHY reset if already up

[Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>]

On 5/21/2026 6:53 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The `skip_reset` condition is evaluated twice using a live hardware register, risking unbalanced reset operations if the PHY status changes between init and power_on.
> --
>
> commit 7deef03fab352fea4d882c35e253738b3fad392c
> Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>
> phy: qcom: qmp-pcie: Skip PHY reset if already up
>
> This patch skips the nocsr reset during initialization if the bootloader
> has already powered up the PCIe PHY and its status indicates it is up.
> This avoids redundant initialization and link retraining delays during
> boot.
>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>> index fed2fc9bb3110..1458ac1478c7e 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> [ ... ]
>
>> @@ -4744,6 +4745,9 @@ static int qmp_pcie_init(struct phy *phy)
>>  		qphy_checkbits(pcs, cfg->regs[QPHY_START_CTRL], SERDES_START | PCS_START) &&
>>  		qphy_checkbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], cfg->pwrdn_ctrl);
>>  
>> +	skip_reset = qmp->skip_init && !qphy_checkbits(pcs, cfg->regs[QPHY_PCS_STATUS],
>> +							    cfg->phy_status);
>> +
>>  	if (!qmp->skip_init && !cfg->tbls.serdes_num) {
>>  		dev_err(qmp->dev, "Init sequence not available\n");
>>  		return -ENODATA;
> [ ... ]
>
>> @@ -4823,8 +4829,11 @@ static int qmp_pcie_power_on(struct phy *phy)
>>  	void __iomem *pcs = qmp->pcs;
>>  	void __iomem *status;
>>  	unsigned int mask, val;
>> +	bool skip_reset;
>>  	int ret;
>>  
>> +	skip_reset = qmp->skip_init && !qphy_checkbits(pcs, cfg->regs[QPHY_PCS_STATUS],
>> +							    cfg->phy_status);
> [Severity: High]
> Since skip_reset is evaluated dynamically in both qmp_pcie_init() and
> qmp_pcie_power_on() by reading the live QPHY_PCS_STATUS hardware register,
> could this lead to an unbalanced reset?
>
> If qmp->skip_init is true but the PHY status bit indicates initialization is
> not complete, skip_reset will be false in qmp_pcie_init(), causing
> nocsr_reset to be asserted.
>
> Once the reset is asserted, the hardware state changes. When
> qmp_pcie_power_on() later re-evaluates skip_reset, reading the status
> register of a PHY held in reset might return 0. This would make skip_reset
> evaluate to true here, skipping the reset_control_deassert() call and leaving
> the PHY permanently stuck in reset.
>
> Should the decision to skip the reset be evaluated exactly once and cached in
> the device context instead of reading the live register twice?
All the resources are voted by this time, so this situation might not happen
at all.

- Krishna Chaitanya.
>>  	/*
>>  	 * Write CSR register for PHY that doesn't support no_csr reset or has not
>>  	 * been initialized.


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