Re: [PATCH v3 5/5] phy: qcom: qmp-pcie: Add Nord Gen5x16 PCIe multi-PHY support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The `skip_init` logic is flawed for the existing Glymur PHY platform, leading to a guaranteed NULL pointer dereference and unintended register corruption during initialization.
- [High] Accessing PHY PCS registers in `qmp_pcie_init()` before explicitly enabling the required AHB clocks leads to a synchronous external abort.
--

commit e58c7fedd872d7c03646f577c8b3832e397473b6
Author: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

phy: qcom: qmp-pcie: Add Nord Gen5x16 PCIe multi-PHY support

This commit adds support for the Nord Gen5x16 bifurcated QMP PCIe PHY.
It implements the PHY initialization sequencing including PLL, TXRXZ,
and PCS tables, handling PHY power domains against their own device.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie-multiphy.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie-multiphy.c
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie-multiphy.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie-multiphy.c
[ ... ]
> +static void qmp_pcie_init_port_registers(struct qmp_pcie *qmp)
> +{
> +	const struct qmp_phy_cfg *cfg = qmp->cfg;
> +	const struct qmp_pcie_offsets *offs = cfg->offsets;
> +	const struct qmp_phy_cfg_tbls *tbls = &cfg->tbls;
> +	int i;
> +
> +	for (i = 0; i < cfg->num_regs; i++) {
> +		void __iomem *port = qmp->base[i];
[ ... ]
> +		qmp_configure(qmp->dev, port + offs->pll,
> +			      tbls->pll_common, tbls->pll_common_num);
> +		qmp_configure(qmp->dev, port + offs->pll,
> +			      cfg->pll_tbls[i], cfg->pll_nums[i]);

[Severity: Critical]
If the existing Glymur platform calls qmp_pcie_init_port_registers(), could
this dereference a null pointer? Looking at glymur_qmp_gen5x8_pciephy_cfg,
cfg->pll_tbls is not initialized and remains null.

[ ... ]
> @@ -953,6 +953,30 @@ static int qmp_pcie_init(struct phy *phy)
>  {
>  	struct qmp_pcie *qmp = phy_get_drvdata(phy);
>  	const struct qmp_phy_cfg *cfg = qmp->cfg;
> +	const struct qmp_pcie_offsets *offs = cfg->offsets;
> +	int i, ret;
>  
>  	ret = qmp_pcie_pd_power_on(qmp);
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * We can skip PHY initialization if all of the following conditions
> +	 * are met:
> +	 *  1. The PHY supports the nocsr_reset that preserves the PHY config.
> +	 *  2. The PHY was started (and not powered down again) by the
> +	 *     bootloader, with all of the expected bits set correctly.
> +	 * In this case, we can continue without having the init sequence
> +	 * defined in the driver.
> +	 */
> +	qmp->skip_init = cfg->pll_tbls && qmp->nocsr_resets;

[Severity: Critical]
Because cfg->pll_tbls is null for the Glymur platform, qmp->skip_init will
always evaluate to false. Does this unintentionally force the Glymur
platform to attempt the port register initialization in qmp_pcie_power_on()?

> +	for (i = 0; qmp->skip_init && i < cfg->num_regs; i++) {
> +		void __iomem *port = qmp->base[i];
> +
> +		if (!qphy_checkbits(port + offs->pcs, cfg->regs[QPHY_PCS_START_CONTROL],
> +				     SERDES_START | PCS_START) ||
> +		    !qphy_checkbits(port + offs->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> +				     SW_PWRDN | REFCLK_DRV_DSBL))
> +			qmp->skip_init = false;
> +	}

[Severity: High]
Does reading QPHY_PCS_START_CONTROL here happen before the required AHB/APB
clocks are enabled? The call to clk_bulk_prepare_enable() occurs further
down in qmp_pcie_init().

If the clocks are currently disabled, such as during boot or after a suspend 
cycle, will accessing these memory-mapped registers trigger a synchronous 
external abort?

[ ... ]
> @@ -1067,6 +1067,20 @@ static int qmp_pcie_power_on(struct phy *phy)
>  		goto err_disable_pipe_clk;
>  	}
>  
> +	if (!qmp->skip_init) {
> +		qmp_pcie_init_port_registers(qmp);
> +
> +		for (i = 0; i < cfg->num_regs; i++) {
> +			qphy_clrbits(qmp->base[i] + offs->pcs,
> +				     cfg->regs[QPHY_PCS_SW_RESET], SW_RESET);

[Severity: Critical]
If Glymur falls into this block due to skip_init being false, it reads
from cfg->regs[QPHY_PCS_SW_RESET]. The Glymur register layout
(pciephy_v8_50_regs_layout) does not define QPHY_PCS_SW_RESET.

Can this result in reading index 0 (which maps to QPHY_PCS_STATUS) and
writing to an undefined offset, thereby corrupting the PHY register at
offset 0x0?

> +			qphy_setbits(qmp->base[i] + offs->pcs,
> +				     cfg->regs[QPHY_PCS_START_CONTROL],
> +				     SERDES_START | PCS_START);
> +		}
> +
> +		usleep_range(2000, 2500);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-nord-v3-0-b0a82721c547@oss.qualcomm.com?part=5