Re: [PATCH v17 5/5] media: qcom: camss: Use data-lanes starting at 1 for new CSIPHY mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect bounds check for physical lane index against the active lane count breaks sparse lane configurations.
- [Medium] Missing `phy_exit()` calls for successfully initialized PHYs on error paths.
--

commit 9ffe4c10c666d01e0522334082dfc8cfc1e9b25b
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

media: qcom: camss: Use data-lanes starting at 1 for new CSIPHY mode

This patch updates the camss driver to support indexing data-lanes starting at
1 for the new CSIPHY mode. It introduces a flag to differentiate between the
legacy 0-based indexing and the new 1-based indexing, normalizing the inputs
for the CSID decoder.

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 84097d82d99c9..07b3cddca5840 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -4777,7 +4778,14 @@ static int camss_parse_endpoint_node(struct device *dev,
>  		return -ENOMEM;
>  
>  	for (i = 0; i < lncfg->num_data; i++) {
> -		lncfg->data[i].pos = mipi_csi2->data_lanes[i];
> +		u8 lane = mipi_csi2->data_lanes[i];
> +
> +		if (lane < lane_base || lane - lane_base >= lncfg->num_data) {
[Severity: High]
Does this bounds check erroneously reject valid sparse data-lane
configurations?

If a device tree specifies non-contiguous lanes, the physical lane index
can be greater than or equal to the total count of active lanes. For example,
if data-lanes = <0 2>, num_data is 2, but the lane index is 2, which triggers
the 2 >= 2 condition.

Will this unconditionally return -EINVAL and break the device probe for valid
sparse configurations?

> +			dev_err(dev, "invalid data-lane %u\n", lane);
> +			return -EINVAL;
> +		}
> +
> +		lncfg->data[i].pos = mipi_csi2->data_lanes[i] - lane_base;
>  		lncfg->data[i].pol = mipi_csi2->lane_polarities[i + 1];
>  	}

[ ... ]

Can initialized PHY clocks and regulators leak on error paths?

Looking at camss_init_subdevices():

drivers/media/platform/qcom/camss/camss.c:camss_init_subdevices() {
    ...
	for (i = 0; i < camss->res->csiphy_num; i++) {
        ...
		if (ret < 0) {
			dev_err(camss->dev, "csiphy %d init fail\n",
				res->csiphy_res[i].csiphy.id);
			return ret;
		}
	}
    ...
}

[Severity: Medium]
If msm_csiphy_subdev_init() fails for a CSIPHY instance after the first, does
this error path return immediately without calling phy_exit() to clean up the
clocks and regulators for PHYs that were successfully initialized (from index
0 to i-1)?

This also appears to happen during registration fallback:

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

[Severity: Medium]
When CSIPHY registration fails, this rollback loop only cleans up the PHYs
that were successfully registered to V4L2. 

Because all PHYs were previously initialized by camss_init_subdevices(), do
the PHYs from the failing index onwards leak their clocks and regulators since
their phy_exit() is never called?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-b4-linux-next-25-03-13-dtsi-x1e80100-camss-v17-0-e2197a3e2551@linaro.org?part=5