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

[sashiko-bot@kernel.org]

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?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-link_retain-v2-0-08ed448b081c@oss.qualcomm.com?part=1

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