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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Selects a non-existent Kconfig symbol PHY_QCOM_MIPI_CSI2.
- [High] Hardware PHY state and resources are leaked on early probe and registration error paths.
- [Medium] Missing return value checks for phy_mipi_dphy_get_default_config_for_hsclk() and phy_set_mode() can lead to uninitialized PHY configuration.
--

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

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

Add the ability to use a PHY pointer which interacts with the standard PHY
API. In the first instance the code will try to use the new PHY interface.
If no PHYs are present in the DT then the legacy method will be attempted.

> 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 this select a nonexistent config symbol? Selecting a symbol that isn't
defined anywhere in the kernel causes a silent build dependency failure,
which will leave the required PHY infrastructure unbuilt.

[ ... ]

> 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

[ ... ]

> @@ -306,11 +308,88 @@ static int csiphy_stream_on(struct csiphy_device *csiphy)
> +	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 we check the return values of phy_mipi_dphy_get_default_config_for_hsclk()
and phy_set_mode() here? 

If link_freq evaluates to 0, it appears phy_mipi_dphy_get_default_config_for_hsclk()
could return -EINVAL and leave the output dphy_opts structure zero-filled. Passing
this uninitialized configuration directly to phy_configure() could lead to
erratic hardware behavior.

[ ... ]

> @@ -709,6 +788,56 @@ int msm_csiphy_subdev_init(struct camss *camss,
> +	ret = phy_init(csiphy->phy);
> +	if (ret)
> +		dev_err(dev, "%s init fail %d\n", csiphy->name, ret);
> +
> +done:
> +	return ret;
> +}

[ ... ]

> 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() to acquire the PHY.
If a subsequent iteration of this loop fails, or if a later initialization step
like camss_probe() fails, the code returns immediately or goes to an error label
without rolling back the previously initialized PHYs (which require phy_exit()).

Additionally, in msm_csiphy_register_entity() located in camss-csiphy.c:

	ret = csiphy_init_formats(sd, NULL);
	if (ret < 0) {
		dev_err(dev, "Failed to init format: %d\n", ret);
		return ret;
	}

A failure here returns early without calling phy_exit(). When the calling code
in camss_register_entities() rolls back, it decrements the index and skips this
failing entity, which seems to permanently leak its PHY initialization state.

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

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