Re: [PATCH v6 2/6] phy: qcom: qmp-pcie: Add QMP PCIe Multi-PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing NULL check for power domain devices causes a kernel panic if a power domain is missing in the device tree.
--

--- Patch [2]: [PATCH v6 2/6] phy: qcom: qmp-pcie: Add QMP PCIe Multi-PHY driver ---
commit 7fe1c90cb97487fff90a2cef8d97abd7bb798202
Author: Qiang Yu <qiang.yu@oss.qualcomm.com>

phy: qcom: qmp-pcie: Add QMP PCIe Multi-PHY driver

This commit adds a dedicated driver for QMP PCIe PHY hardware blocks that
support multiple link topologies. It uses a per-mode config table to
instantiate one qmp_pcie per sub-PHY required by the link mode.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie-multiphy.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie-multiphy.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie-multiphy.c

[ ... ]

> +static int qmp_pcie_pd_power_on(struct qmp_pcie *qmp)
> +{
> +	const struct qmp_phy_cfg *cfg = qmp->cfg;
> +	int i, ret;
> +
> +	for (i = 0; i < cfg->num_pds; i++) {
> +		ret = pm_runtime_resume_and_get(qmp->pd_devs[i]);

[Severity: High]
If dev_pm_domain_attach_by_name() returns NULL during initialization, could
passing that NULL to pm_runtime_resume_and_get() here cause a kernel panic?

> +		if (ret < 0) {
> +			dev_err(qmp->dev, "failed to power on %s domain\n",
> +				cfg->pd_names[i]);
> +			goto err_power_off;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_power_off:
> +	while (--i >= 0)
> +		pm_runtime_put(qmp->pd_devs[i]);
> +
> +	return ret;
> +}
> +
> +static void qmp_pcie_pd_power_off(struct qmp_pcie *qmp)
> +{
> +	const struct qmp_phy_cfg *cfg = qmp->cfg;
> +	int i;
> +
> +	for (i = cfg->num_pds - 1; i >= 0; i--)
> +		pm_runtime_put(qmp->pd_devs[i]);

[Severity: High]
Similarly, could this pm_runtime_put() panic if qmp->pd_devs[i] is NULL?

[ ... ]

> +static int qmp_pcie_pd_init(struct qmp_pcie *qmp)
> +{
> +	const struct qmp_phy_cfg *cfg = qmp->cfg;
> +	struct device *dev = qmp->dev;
> +	int i, ret;
> +
> +	if (!cfg->num_pds)
> +		return 0;
> +
> +	qmp->pd_devs = devm_kcalloc(dev, cfg->num_pds, sizeof(*qmp->pd_devs),
> +				    GFP_KERNEL);
> +	if (!qmp->pd_devs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < cfg->num_pds; i++) {
> +		qmp->pd_devs[i] = dev_pm_domain_attach_by_name(dev,
> +							       cfg->pd_names[i]);
> +		if (IS_ERR(qmp->pd_devs[i])) {

[Severity: High]
dev_pm_domain_attach_by_name() returns NULL if the power domain is missing
in the device tree. Because this only checks IS_ERR(), a NULL pointer will
be stored in qmp->pd_devs[i] in that scenario.

Should this check be IS_ERR_OR_NULL(), or should the power on/off functions
check for NULL before calling the runtime PM APIs?

I noticed that qmp_pcie_pd_detach() correctly uses !IS_ERR_OR_NULL() when
cleaning up, so it seems like NULL might be an expected value to handle.

> +			ret = PTR_ERR(qmp->pd_devs[i]);
> +			goto err_detach;
> +		}
> +	}
> +
> +	return devm_add_action_or_reset(dev, qmp_pcie_pd_detach, qmp);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-glymur_linkmode_0722-v6-0-bba8156b8cbb@oss.qualcomm.com?part=2

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