Re: [PATCH v9 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `pd_data` struct is allocated on the stack but not zero-initialized, causing garbage `pd_flags` to be passed to `devm_pm_domain_attach_list()`.
- [High] Missing resource cleanup on the error path of `phy_qcom_mipi_csi2_power_on` if `ops->lanes_enable()` fails.
- [High] The power-off sequence incorrectly drops the device's voltage (performance state) before its frequency (OPP rate), causing improper power sequencing.
- [Medium] `phy_qcom_mipi_csi2_probe` falsely returns success (0) when PM domains are not attached.
--

--- Patch [2]: [PATCH v9 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver ---
commit c5be1bcc08e40b760c1de3b49fbb5eda1354c97c
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver

Add a new MIPI CSI2 driver in DPHY mode initially. The entire set of
existing CAMSS CSI PHY init sequences are imported in order to save time
and effort in later patches.

> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
[ ... ]
> @@ -118,6 +118,78 @@ static int phy_qcom_mipi_csi2_power_on(struct phy *phy)
>  	struct mipi_csi2phy_device *csi2phy = phy_get_drvdata(phy);
>  	const struct mipi_csi2phy_hw_ops *ops = csi2phy->soc_cfg->ops;
>  	int i, ret;
[ ... ]
>  	ops->reset(csi2phy);
>  
>  	ops->hw_version_read(csi2phy);
>  
>  	return ops->lanes_enable(csi2phy, &csi2phy->stream_cfg);

[Severity: High]
If ops->lanes_enable() fails, does this return directly and bypass the
cleanup labels (unset_rate, poweroff_phy, disable_regulators)?

Will this leak the regulator enable counts and PM runtime usage counts,
leaving the hardware permanently powered on? 

Also, should the clocks be disabled in an error path here since they were
enabled earlier in the function?

>  
>  unset_rate:
>  	for (i = 0; i < csi2phy->pd_list->num_pds; i++) {
>  		if (!csi2phy->soc_cfg->genpds[i].scaled)
>  			continue;
>  
>  		dev_pm_genpd_set_performance_state(csi2phy->pd_list->pd_devs[i], 0);
>  	}
>  
>  	dev_pm_opp_set_rate(csi2phy->dev, 0);

[Severity: High]
Does this error path sequence drop the performance state (voltage) to 0
before dropping the OPP rate (frequency) to 0?

Could lowering the voltage while the hardware is still running at a high
clock frequency cause hardware faults or system instability?

[ ... ]
> @@ -168,6 +168,26 @@ static int phy_qcom_mipi_csi2_power_off(struct phy *phy)
>  
>  	clk_bulk_disable_unprepare(csi2phy->soc_cfg->num_clk,
>  				   csi2phy->clks);
>  
>  	for (i = 0; i < csi2phy->pd_list->num_pds; i++) {
>  		if (!csi2phy->soc_cfg->genpds[i].scaled)
>  			continue;
>  
>  		dev_pm_genpd_set_performance_state(csi2phy->pd_list->pd_devs[i], 0);
>  	}
>  
>  	dev_pm_opp_set_rate(csi2phy->dev, 0);

[Severity: High]
Does this similarly drop the performance state before lowering the OPP rate
during normal power off?

[ ... ]
> @@ -210,6 +210,23 @@ static int phy_qcom_mipi_csi2_attach_pm_domains(struct mipi_csi2phy_device *csi2
>  {
>  	struct dev_pm_domain_attach_data pd_data;
>  	const char **pd_names;
>  	int i;
>  
>  	pd_names = devm_kzalloc(csi2phy->dev,
>  				sizeof(char *) * csi2phy->soc_cfg->num_genpds,
>  				GFP_KERNEL);
[ ... ]
>  	pd_data.pd_names = pd_names;
>  	pd_data.num_pd_names = csi2phy->soc_cfg->num_genpds;
>  
>  	return devm_pm_domain_attach_list(csi2phy->dev, &pd_data,
>  					  &csi2phy->pd_list);
>  }

[Severity: High]
Is the pd_data struct fully zero-initialized before being passed to
devm_pm_domain_attach_list()?

Since pd_data is declared on the stack and only pd_names and num_pd_names
are populated, could random garbage on the stack cause unexpected pd_flags
to be set?

If bits like PD_FLAG_NO_DEV_LINK or PD_FLAG_ATTACH_POWER_ON are set by
uninitialized memory, it might skip creating device links and cause silent
hardware failures.

[ ... ]
> @@ -331,6 +331,90 @@ static int phy_qcom_mipi_csi2_probe(struct platform_device *pdev)
>  	ret = phy_qcom_mipi_csi2_parse_routing(csi2phy);
>  	if (ret)
>  		return ret;
>  
>  	ret = phy_qcom_mipi_csi2_attach_pm_domains(csi2phy);
>  	if (ret < 0 || csi2phy->pd_list == NULL)
>  		return dev_err_probe(dev, ret, "Failed to attach power-domain list\n");
>  
>  	devm_pm_runtime_enable(dev);

[Severity: Medium]
If devm_pm_domain_attach_list() returns 0 but csi2phy->pd_list is NULL,
will dev_err_probe() return 0 here?

Could this falsely report a successful probe to the driver core while
leaving the device half-initialized?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-x1e-csi2-phy-v9-0-0210b90c04cf@linaro.org?part=2