[PATCH v16 4/5] media: qcom: camss: Add support for PHY API devices

[sashiko-bot at kernel.org]

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

New issues:
- [High] The Kconfig selects a non-existent symbol `PHY_QCOM_MIPI_CSI2`.
- [High] Missing `phy_exit()` cleanup for successfully initialized PHYs on probe and registration error paths.
- [Medium] Ignored return values from PHY API configuration functions.

Pre-existing issues:
- [High] Potential NULL pointer dereference of `csiphy->cfg.csi2` if streaming is started before the sensor subdevice is bound.
--

commit 55545599c66ba86b7d549259aecbc5a1c20f76a5
Author: Bryan O'Donoghue <bryan.odonoghue at linaro.org>

media: qcom: camss: Add support for PHY API devices

This commit adds the ability to use a PHY pointer which interacts with the
standard PHY API. If no PHYs are present in the DT, it falls back to the
legacy configuration method.

> diff --git a/drivers/media/platform/qcom/camss/Kconfig b/drivers/media/platform/qcom/camss/Kconfig
> index 4eda48cb1adf0..1edc5e5a1829e 100644
> --- a/drivers/media/platform/qcom/camss/Kconfig
> +++ b/drivers/media/platform/qcom/camss/Kconfig
> @@ -7,3 +7,4 @@ config VIDEO_QCOM_CAMSS
>  	select VIDEO_V4L2_SUBDEV_API
>  	select VIDEOBUF2_DMA_SG
>  	select V4L2_FWNODE
> +	select PHY_QCOM_MIPI_CSI2

[Severity: High]
Does the symbol PHY_QCOM_MIPI_CSI2 exist in the kernel tree?

Selecting a non-existent Kconfig symbol can cause unmet direct dependency
warnings and potentially silent build failures, leaving required features
disabled.

[ ... ]

> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> index 539ac4888b608..e00748dd83b02 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
[ ... ]
> +static int csiphy_stream_on(struct csiphy_device *csiphy)
> +{
> +	u8 bpp = csiphy_get_bpp(csiphy->res->formats->formats, csiphy->res->formats->nformats,
> +				csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
> +	struct csiphy_lanes_cfg *lncfg = &csiphy->cfg.csi2->lane_cfg;
> +	struct phy_configure_opts_mipi_dphy *dphy_cfg;
> +	union phy_configure_opts dphy_opts = { 0 };
> +	struct device *dev = csiphy->camss->dev;
> +	u8 num_lanes = lncfg->num_data;

[Severity: High]
This isn't a bug introduced by this patch, but could this code dereference
csiphy->cfg.csi2 while it is NULL?

If userspace opens a video node and issues VIDIOC_STREAMON before the sensor
subdevice binds (or if the sensor never binds), csiphy->cfg.csi2 will remain
uninitialized. The code here dereferences it to read lncfg->num_data before
validating if a sensor is actually present.

> +	s64 link_freq;
> +	int ret;
> +
> +	dphy_cfg = &dphy_opts.mipi_dphy;
> +
> +	link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp, num_lanes);
> +
> +	if (link_freq < 0) {
> +		dev_err(dev,
> +			"Cannot get CSI2 transmitter's link frequency\n");
> +		return -EINVAL;
> +	}
> +
> +	phy_mipi_dphy_get_default_config_for_hsclk(link_freq, num_lanes, dphy_cfg);
> +
> +	phy_set_mode(csiphy->phy, PHY_MODE_MIPI_DPHY);
> +
> +	ret = phy_configure(csiphy->phy, &dphy_opts);

[Severity: Medium]
Should the return values of phy_mipi_dphy_get_default_config_for_hsclk() and
phy_set_mode() be checked?

If phy_mipi_dphy_get_default_config_for_hsclk() fails (for example, with a
bad link_freq), dphy_opts will remain untouched. Passing this zero-initialized
structure to phy_configure() could then misconfigure the hardware.

[ ... ]

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 2123f6388e3d7..84097d82d99c9 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
[ ... ]
> @@ -4832,14 +4890,21 @@ static int camss_init_subdevices(struct camss *camss)
>  	unsigned int i;
>  	int ret;
>  
> +	camss_detect_legacy_phy(camss);
> +
>  	for (i = 0; i < camss->res->csiphy_num; i++) {
> -		ret = msm_csiphy_subdev_init(camss, &camss->csiphy[i],
> -					     &res->csiphy_res[i],
> -					     res->csiphy_res[i].csiphy.id);
> +		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);
> +		}
> +
>  		if (ret < 0) {
> -			dev_err(camss->dev,
> -				"Failed to init csiphy%d sub-device: %d\n",
> -				i, ret);
> +			dev_err(camss->dev, "csiphy %d init fail\n",
> +				res->csiphy_res[i].csiphy.id);
>  			return ret;
>  		}
>  	}

[Severity: High]
Does this error path leak the PHY initialization state?

When msm_csiphy_subdev_init() succeeds, it calls phy_init(). If a later
csiphy iteration fails, or if a subsequent subdevice like vfe fails to
initialize, camss_init_subdevices() returns early. This jumps to the
err_genpd_cleanup path in camss_probe(), completely bypassing
camss_unregister_entities(), and leaving earlier successfully initialized
PHYs without a balancing phy_exit().

[ ... ]

> @@ -5131,8 +5207,10 @@ static int camss_register_entities(struct camss *camss)
>  
>  	i = camss->res->csiphy_num;
>  err_reg_csiphy:
> -	for (i--; i >= 0; i--)
> -		msm_csiphy_unregister_entity(&camss->csiphy[i]);
> +	for (i--; i >= 0; i--) {
> +		if (csiphy_enabled(camss, &camss->csiphy[i]))
> +			msm_csiphy_unregister_entity(&camss->csiphy[i]);
> +	}
>  
>  	return ret;
>  }

[Severity: High]
Does this error path also miss calling phy_exit() for some initialized PHYs?

Because this registration cleanup loop only iterates backward from the failing
index `i`, any csiphys from `i` onward (which were initialized during the
earlier camss_init_subdevices() phase but had not reached registration yet)
will not have their msm_csiphy_unregister_entity() cleanup called. This leaves
their PHY references leaked.

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