Re: [PATCH v8 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 2 potential issue(s) to consider:
- [Medium] Asynchronous pm_runtime_put() during PHY power off leaves hardware powered on if device is removed.
- [Medium] Unchecked return value of of_property_count_strings() leads to massive allocation attempt and incorrect error masking.
--

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

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

This patch introduces a dedicated QMP PCIe PHY driver for hardware blocks
that support multiple link topologies. It reads the current link mode from
the hardware and instantiates the necessary sub-PHYs, inheriting firmware
configurations.

> 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
> index 0000000000000..739c7c96aeed6
> --- /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]);
> +		if (ret < 0) {
> +			dev_err(qmp->dev, "failed to power on %s domain: %d\n",
> +				cfg->pd_names[i], ret);
> +			goto err_power_off;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_power_off:
> +	while (--i >= 0)
> +		pm_runtime_put(qmp->pd_devs[i]);

[Severity: Medium]
Will using an asynchronous pm_runtime_put() here (and in 
qmp_pcie_pd_power_off) race with device unbind?

If the PHY provider unbinds, device links will cause the consumer to unbind
first, which calls phy_power_off() and phy_exit(), queuing an asynchronous
suspend. The PHY provider's devres cleanup then calls dev_pm_domain_detach(),
which executes pm_runtime_disable() and cancels the pending suspend.

Does this leave the hardware powered on? Should pm_runtime_put_sync() be used
instead to ensure the hardware powers down before the domain detaches?

> +
> +	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: Medium]
As mentioned above, can this asynchronous put race with the devres teardown
during removal?

> +}
[ ... ]
> +static int qmp_pcie_multiphy_register_clocks(struct device *dev,
> +					      struct device_node *np,
> +					      struct qmp_pcie_multiphy *qmp_data)
> +{
> +	int num_pipe_outputs;
> +	int i, ret;
> +
> +	num_pipe_outputs = of_property_count_strings(np, "clock-output-names");
> +
> +	qmp_data->num_pipe_outputs = num_pipe_outputs;
> +	qmp_data->pipe_out_clks = devm_kcalloc(dev, num_pipe_outputs,
> +					       sizeof(*qmp_data->pipe_out_clks),
> +					       GFP_KERNEL);

[Severity: Medium]
Does this code properly handle missing or malformed device tree properties?

If "clock-output-names" is missing, of_property_count_strings() will return
a negative error code (e.g., -EINVAL). This negative value is passed as the
count parameter to devm_kcalloc(), underflowing into a massive size_t. 

While the internal size_mul checks in the allocator safely return NULL, this
causes the driver to fail probe with -ENOMEM rather than the actual device
tree parsing error. Should num_pipe_outputs be checked for a negative value
before attempting the allocation?

> +	if (!qmp_data->pipe_out_clks)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_pipe_outputs; i++) {
> +		ret = __phy_pipe_clk_register(dev, np, i,
> +					      &qmp_data->pipe_out_clks[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return devm_of_clk_add_hw_provider(dev, qmp_pcie_multiphy_clk_hw_get, qmp_data);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-glymur_linkmode_0731-v8-0-a455265ad8bf@oss.qualcomm.com?part=2