Re: [PATCH v22 07/14] media: qcom: camss: csiphy: Add support for v2.4.0 two-phase CSIPHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: qcom: camss: NULL hw_ops deref in msm_csiphy_subdev_init_legacy()
- [Low] media: qcom: camss: commit message describes phy driver changes
--

commit 64814811592ee91f0301ddbd08c2ef3cf1f43a07
Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>

media: qcom: camss: csiphy: Add support for v2.4.0 two-phase CSIPHY

This commit adds the csiphy_res_kaanapali subdevice resource table for
CSIPHY0 through CSIPHY5 and wires it into kaanapali_resources in the
Qualcomm CAMSS driver.

>     media: qcom: camss: csiphy: Add support for v2.4.0 two-phase CSIPHY
>     
>     Add support for the v2.4.0 two-phase CSIPHY found on Kaanapali, where
>     the PHY is driven by the standalone phy-qcom-mipi-csi2 DPHY driver
>     rather than by CAMSS. Add the mipi_csi2_dphy_3nm_kaanapali configuration,
>     which reuses the x1e80100 lane sequence, clocks and supplies but selects
>     the v2.4.0 common status offset, adds Kaanapali power domain and register
>     the "qcom,kaanapali-csi2-phy" compatible.
>     
>     Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>     Signed-off-by: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>

[Severity: Low]
This isn't a bug, but does the second sentence of the commit message
describe changes from an earlier patch in the series ("phy: qcom-mipi-csi2:
Add Kaanapali support") rather than this patch?

Those changes to drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c and
drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c were split into that earlier
commit, whereas this commit adds csiphy_res_kaanapali in
drivers/media/platform/qcom/camss/camss.c and wires .csiphy_res and
.csiphy_num into kaanapali_resources. Could the commit message be updated
to describe the CAMSS changes in this patch?

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 98ec23c39fda2..0358ee3840129 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -38,6 +38,51 @@
>  
>  static const struct parent_dev_ops vfe_parent_dev_ops;
>  
> +static const struct camss_subdev_resources csiphy_res_kaanapali[] = {
> +	/* CSIPHY0 */
> +	{
> +		.csiphy = {
> +			.id = 0,
> +			.formats = &csiphy_formats_sdm845
> +		},
> +	},

[ ... ]

> @@ -5606,8 +5651,10 @@ static void camss_remove(struct platform_device *pdev)
>  static const struct camss_resources kaanapali_resources = {
>  	.version = CAMSS_KAANAPALI,
>  	.pd_name = "top",
> +	.csiphy_res = csiphy_res_kaanapali,
>  	.icc_res = icc_res_kaanapali,
>  	.icc_path_num = ARRAY_SIZE(icc_res_kaanapali),
> +	.csiphy_num = ARRAY_SIZE(csiphy_res_kaanapali),
>  };

[Severity: High]
Can this dereference a NULL csiphy->res->hw_ops pointer in
msm_csiphy_subdev_init_legacy() when probing qcom,kaanapali-camss without
any connected external PHY endpoints in the device tree (for example, with
the default empty port nodes in kaanapali.dtsi or when only using internal
TPGs)?

Because Kaanapali CSIPHY blocks are driven by the standalone PHY driver,
csiphy_res_kaanapali leaves .csiphy.hw_ops as NULL.

However, during camss_probe() -> camss_init_subdevices() ->
camss_detect_legacy_phy() in drivers/media/platform/qcom/camss/camss.c,
camss->legacy_phy defaults to true when no endpoint connected to a remote
node named "phy" is found:

	camss->legacy_phy = true;

	/* Find first remote-endpoint and determine if its a PHY */
	for_each_endpoint_of_node(camss->dev->of_node, ep) {
		remote = of_graph_get_remote_port_parent(ep);
		if (!remote)
			continue;

		camss->legacy_phy = !of_node_name_eq(remote, "phy");
		of_node_put(remote);
		of_node_put(ep);
		break;
	}

Then camss_init_subdevices() takes the legacy path:

	for (i = 0; i < camss->res->csiphy_num; i++) {
		if (!camss->legacy_phy) {
			ret = msm_csiphy_subdev_init(camss, i);
		} else {
			ret = msm_csiphy_subdev_init_legacy(camss,
							    &camss->csiphy[i],
							    &res->csiphy_res[i],
							    res->csiphy_res[i].csiphy.id);
		}

and msm_csiphy_subdev_init_legacy() in
drivers/media/platform/qcom/camss/camss-csiphy.c unconditionally
dereferences csiphy->res->hw_ops:

	csiphy->camss = camss;
	csiphy->id = id;
	csiphy->cfg.combo_mode = 0;
	csiphy->res = &res->csiphy;

	ret = csiphy->res->hw_ops->init(csiphy);

Should camss_detect_legacy_phy() or camss_init_subdevices() avoid selecting
legacy PHY mode on platforms where csiphy_res[i].csiphy.hw_ops is NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-kaanapali-camss-v22-0-7da3ea49c8a1@oss.qualcomm.com?part=7

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy