Re: [PATCH v12 6/6] media: qcom: camss: Drop legacy PHY descriptions from x1e

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Missing `phy_exit()` cleanup on probe error paths results in PHY resource leaks for non-legacy PHYs.

Pre-existing issues:
- [High] Potential out-of-bounds access in `camss_subdev_notifier_bound()` due to non-contiguous hardware IDs.
--

--- Patch [6]: [PATCH v12 6/6] media: qcom: camss: Drop legacy PHY descriptions from x1e ---
commit 77049628030f3bcd1557363f75216c90c62c80af
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

media: qcom: camss: Drop legacy PHY descriptions from x1e

x1e is the first CAMSS SoC to use the new PHY interface. This patch drops
the redundant legacy CSIPHY descriptions to enable the new interface.

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 146fea438486f..f360b2c99bf89 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
[ ... ]
> @@ -5803,7 +5767,6 @@ static const struct camss_resources sm8650_resources = {
>  static const struct camss_resources x1e80100_resources = {
>  	.version = CAMSS_X1E80100,
>  	.pd_name = "top",
> -	.legacy_phy = true,

[Severity: High]
Does dropping the legacy PHY interface introduce PHY resource leaks on probe
error paths?

If msm_csiphy_subdev_init() fails during camss_init_subdevices(), previously
initialized PHYs do not appear to be rolled back with phy_exit():

drivers/media/platform/qcom/camss/camss.c:camss_init_subdevices() {
...
			ret = msm_csiphy_subdev_init(camss, &camss->csiphy[i],
						     &res->csiphy_res[i],
						     res->csiphy_res[i].csiphy.id);
			if (ret < 0)
				return ret;
...
}

Additionally, if camss_probe() fails after camss_init_subdevices() (for
instance, in dma_set_mask_and_coherent()), the error path jumps to
err_genpd_cleanup, bypassing camss_unregister_entities() and permanently
leaking all initialized PHYs:

drivers/media/platform/qcom/camss/camss.c:camss_probe() {
...
	ret = dma_set_mask_and_coherent(dev, 0xffffffff);
	if (ret)
		goto err_genpd_cleanup;
...
}

Similarly, does camss_register_entities() fail to call phy_exit() for the
failed csiphy[i] and all subsequent PHYs, since the cleanup loop starts at
i - 1?

drivers/media/platform/qcom/camss/camss.c:camss_register_entities() {
...
err_reg_csiphy:
	for (i--; i >= 0; i--) {
		if (camss->csiphy[i].phy)
			msm_csiphy_unregister_entity(&camss->csiphy[i]);
	}
...
}


[Severity: High]
This is a pre-existing issue, but since x1e80100 defines non-contiguous PHY
IDs (0, 1, 2, and 4), does this cause an out-of-bounds access?

The csiphy_num is 4 for x1e80100, meaning the camss->csiphy array only has
4 elements. The endpoint port ID is parsed from the device tree and stored
in csd->interface.csiphy_id:

drivers/media/platform/qcom/camss/camss.c:camss_parse_endpoint_node() {
...
	csd->interface.csiphy_id = vep.base.port;
...
}

If the port number aligns with the physical hardware ID (e.g., port@4 for
CSIPHY4), will this use id = 4 to index the 4-element array in
camss_subdev_notifier_bound(), causing an out-of-bounds access?

drivers/media/platform/qcom/camss/camss.c:camss_subdev_notifier_bound() {
...
	u8 id = csd->interface.csiphy_id;
	struct csiphy_device *csiphy = &camss->csiphy[id];

	csiphy->cfg.csi2 = &csd->interface.csi2;
	subdev->host_priv = csiphy;
...
}

>  	.csiphy_res = csiphy_res_x1e80100,
>  	.tpg_res = tpg_res_x1e80100,
>  	.csid_res = csid_res_x1e80100,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-b4-linux-next-25-03-13-dtsi-x1e80100-camss-v12-0-f8588da41f16@linaro.org?part=6