Re: [PATCH 2/2] media: qcom: camss: Add Glymur specific resources

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: qcom: camss: NULL dereference from missing csiphy_res in glymur_resources
--

commit 8950cd65e1bb42630c5e3d1c122c6f357c2b384e
Author: Nihal Kumar Gupta <nihal.gupta@oss.qualcomm.com>

media: qcom: camss: Add Glymur specific resources

This commit adds a dedicated camss_resources table for Glymur so that its
standalone CSI2 PHYs do not reuse the X1E80100 CSIPHY resource table.

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index c54185c93a8a9..72541ce3a632e 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -5576,6 +5576,21 @@ static void camss_remove(struct platform_device *pdev)
>  	camss_genpd_cleanup(camss);
>  }
>  
> +static const struct camss_resources glymur_resources = {
> +	.version = CAMSS_X1E80100,
> +	.pd_name = "top",
> +	.tpg_res = tpg_res_x1e80100,
> +	.csid_res = csid_res_x1e80100,
> +	.vfe_res = vfe_res_x1e80100,
> +	.csid_wrapper_res = &csid_wrapper_res_x1e80100,
> +	.icc_res = icc_res_x1e80100,
> +	.icc_path_num = ARRAY_SIZE(icc_res_x1e80100),
> +	.csiphy_num = 5,
> +	.tpg_num = ARRAY_SIZE(tpg_res_x1e80100),
> +	.csid_num = ARRAY_SIZE(csid_res_x1e80100),
> +	.vfe_num = ARRAY_SIZE(vfe_res_x1e80100),
> +};

[Severity: High]
Does leaving csiphy_res as NULL while setting csiphy_num = 5 cause a NULL
pointer dereference when probing qcom,glymur-camss?

In the standalone PHY path, camss_probe() -> camss_init_subdevices() calls
msm_csiphy_subdev_init(camss, i) for each port, which indexes
camss->res->csiphy_res and dereferences res->csiphy.id:

drivers/media/platform/qcom/camss/camss-csiphy.c:msm_csiphy_subdev_init() {
	const struct camss_subdev_resources *res = &camss->res->csiphy_res[port];
	...
	csiphy->camss = camss;
	csiphy->id = res->csiphy.id;
	csiphy->res = &res->csiphy;
}

Even if msm_csiphy_subdev_init() failed early (for example with
-EPROBE_DEFER from devm_phy_get_by_of_node()), or if no remote PHY
endpoints are enabled and camss_detect_legacy_phy() leaves
camss->legacy_phy as true, camss_init_subdevices() also dereferences
res->csiphy_res[i].csiphy.id:

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

Furthermore, the standalone PHY subdev operations in
drivers/media/platform/qcom/camss/camss-csiphy.c (via
msm_csiphy_register_entity() -> csiphy_init_formats() ->
csiphy_set_format() -> csiphy_try_format(), as well as
csiphy_enum_mbus_code() and csiphy_stream_on()) unconditionally dereference
csiphy->res->formats:

drivers/media/platform/qcom/camss/camss-csiphy.c:csiphy_stream_on() {
	u8 bpp = csiphy_get_bpp(csiphy->res->formats->formats, csiphy->res->formats->nformats,
				csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
}

Should glymur_resources provide a csiphy_res table (or should the CSIPHY
init and format paths handle a NULL csiphy_res) so this doesn't crash at
probe or runtime?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-glymur_camss-v1-0-fcb619fc7319@oss.qualcomm.com?part=2