Re: [PATCH v14 4/4] 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] Kconfig symbol `VIDEO_QCOM_CAMSS` selects a non-existent configuration `PHY_QCOM_MIPI_CSI2`.
- [Medium] Missing `phy_exit()` cleanup on error paths leading to leaked PHY hardware state.
- [Medium] Unchecked return value of `phy_mipi_dphy_get_default_config_for_hsclk()` leads to uninitialized PHY configuration.
--

commit 23536801ceab899563c72768f20e4c404c373978
Author: Bryan O'Donoghue <bryan.odonoghue@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. In the first instance the code will try to use the new
PHY interface, falling back to the legacy method if no PHYs are present in
the DT.

> diff --git a/drivers/media/platform/qcom/camss/Kconfig b/drivers/media/platform/qcom/camss/Kconfig
> --- 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 config PHY_QCOM_MIPI_CSI2 actually exist?

Selecting a non-existent symbol can leave the required PHY driver unbuilt,
leading to silent dependency failures where the hardware driver will lack its
required PHY counterpart at runtime.

> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> --- 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)
>  {
>  	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;
>  	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 we check the return value of
phy_mipi_dphy_get_default_config_for_hsclk() here in csiphy_stream_on()?

If this calculation helper fails, for example due to an unsupported link
frequency, it returns a negative error code and leaves dphy_cfg
zero-initialized. Because the error isn't checked, the zeroed configuration
would be passed down to phy_configure(), risking an invalid hardware state or
a failure to start the video stream.

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> --- 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: Medium]
If msm_csiphy_subdev_init() fails on a later iteration, does this loop leak
the PHY hardware state of the previously initialized PHYs?

The msm_csiphy_subdev_init() function now calls phy_init(). If we return
early here, we never call phy_exit() for the PHYs that successfully
initialized in prior loop iterations.

Additionally, this initialization state appears to leak on other error paths
too. For example, if camss_register_entities() fails:

> @@ -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;
>  }

If camss_register_entities() fails on the very first PHY (i=0), the cleanup
loop skips msm_csiphy_unregister_entity() for all PHYs because i becomes -1.

Since msm_csiphy_unregister_entity() is what currently calls phy_exit(), this
means all PHYs that were phy_init()ed during camss_init_subdevices() are
never torn down, leaking their initialization state and reference counts.

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