Re: [PATCH v23 08/15] 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 deref in msm_csiphy_subdev_init_legacy()
- [Medium] media: qcom: camss: commit message describes phy driver changes
--

commit 746e86a20142c7ce6cfd5e7633a02e7c5b459ec6
Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>

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

This patch defines the csiphy subdevice resource table for Kaanapali and
wires it into kaanapali_resources in camss.

>     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: Medium]
Does this commit message match the changes in this patch?

The commit message states that this patch adds the
mipi_csi2_dphy_3nm_kaanapali configuration and registers the
"qcom,kaanapali-csi2-phy" compatible, which were added in commit 09220cf48f3f
("phy: qcom-mipi-csi2: Add Kaanapali support") in drivers/phy/qualcomm/,
whereas this patch only populates csiphy_res_kaanapali and wires it into
kaanapali_resources in drivers/media/platform/qcom/camss/camss.c.

In addition, the subject and commit description refer to a "v2.4.0 two-phase"
phy, while Kaanapali uses a 3-phase v2.4.0 phy. Could the commit subject and
description be updated to reflect the camss changes here?

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index a64242c66cf06..b0b63dd3d958e 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
> +		},
> +	},

[ ... ]

> @@ -5607,8 +5652,10 @@ static const struct camss_resources kaanapali_resources = {
>  	.version = CAMSS_KAANAPALI,
>  	.pd_name = "top",
>  	.pm_clks = { "cpas_fast_ahb", "cpas_ahb", "gcc_axi_hf" },
> +	.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 trigger a null pointer dereference in msm_csiphy_subdev_init_legacy()
when probing camss on Kaanapali without a remote "phy" endpoint in the device
tree?

Each entry in csiphy_res_kaanapali[] only initializes .csiphy.id and
.csiphy.formats, leaving .csiphy.hw_ops as null because the Kaanapali phy is
driven by the standalone phy-qcom-mipi-csi2 driver.

However, during camss_probe() -> camss_init_subdevices() ->
camss_detect_legacy_phy(), camss->legacy_phy defaults to true:

drivers/media/platform/qcom/camss/camss.c:camss_detect_legacy_phy() {
	...
	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;
	}
}

If camss is enabled without an active endpoint connected to a node named "phy"
(for example, with unconnected ports in a base board device tree or when
using only the internal test pattern generators csitpg0..2),
camss->legacy_phy remains true.

camss_init_subdevices() then takes the legacy initialization path for each of
the 6 csiphy_res_kaanapali entries:

drivers/media/platform/qcom/camss/camss.c:camss_init_subdevices() {
	...
	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() unconditionally dereferences
csiphy->res->hw_ops:

drivers/media/platform/qcom/camss/camss-csiphy.c:msm_csiphy_subdev_init_legacy() {
	...
	csiphy->res = &res->csiphy;

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

Should camss_init_subdevices() or camss_detect_legacy_phy() guard against
taking the legacy path when res->csiphy_res[i].csiphy.hw_ops is null?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-kaanapali-camss-v23-0-95f250b18612@oss.qualcomm.com?part=8

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