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

On Fri, Jul 31, 2026 at 05:26:39AM +0000, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?

This driver has no .remove callback so there's no unbind path for
qmp_pcie_pd_power_off()'s async put to race with. And even if
dev_pm_domain_detach is somehow called. qmp_pcie_pd_detach()'s
dev_pm_domain_detach(dev, true) handles any still-pending async request
internally (pm_runtime_disable() → __pm_runtime_barrier()).

> 
> > +
> > +	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?

As Konrad commented, clock-output-names is required via bindings, so the
check is not required.

- Qiang Yu

> 
> > +	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/[email protected]?part=2