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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The Kconfig symbol VIDEO_QCOM_CAMSS selects a non-existent symbol PHY_QCOM_MIPI_CSI2.
- [Medium] Missing phy_exit() calls on probe error paths leak initialized PHY hardware states.
- [Medium] The return value of phy_mipi_dphy_get_default_config_for_hsclk() is ignored.

Pre-existing issues:
- [High] Dereferencing csiphy->cfg.csi2 without checking causes a NULL pointer dereference if no sensor is bound.
--

--- Patch [3]: [PATCH v13 3/5] media: qcom: camss: Add support for PHY API devices ---
commit 7ad9edf4c6cb1def82d912d6116e4eddc0da65e4
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: Medium]
Does the PHY_QCOM_MIPI_CSI2 config symbol exist in the tree?

It looks like VIDEO_QCOM_CAMSS selects a non-existent symbol here, which
could lead to unmet dependencies or silent configuration issues where the
feature remains 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;

[Severity: High]
This is a pre-existing issue, but can this dereference csiphy->cfg.csi2
while it is NULL if no remote sensor endpoint is defined in the Device Tree?

Since csiphy->cfg.csi2 is populated exclusively in the async notifier's
bound callback (camss_subdev_notifier_bound), if no sensor is bound, could
an unprivileged user trigger a NULL pointer dereference by calling the
VIDIOC_SUBDEV_S_STREAM ioctl on the /dev/v4l-subdevX node?

> +	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);

[Severity: Medium]
Does phy_mipi_dphy_get_default_config_for_hsclk() return an error code that
should be checked here?

If it fails, dphy_opts might remain zero-initialized when passed to
phy_configure() below.

> +
> +	phy_set_mode(csiphy->phy, PHY_MODE_MIPI_DPHY);
> +
> +	ret = phy_configure(csiphy->phy, &dphy_opts);

[ ... ]

> 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

[ ... ]

> @@ -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: Medium]
Does this error path leak the PHY hardware states?

In camss_init_subdevices(), phy_init() is called for each CSIPHY port.
If camss_register_entities() fails on index 'i', this rollback loop starts
at 'i--', skipping the unregistration (and therefore phy_exit()) for the
failing index 'i' and any other initialized PHYs that hadn't been processed
yet.

Likewise, if probe fails later and jumps to err_genpd_cleanup in
camss_probe(), are the previously initialized PHY states properly cleaned up?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-b4-linux-next-25-03-13-dtsi-x1e80100-camss-v13-0-ae811e2f0799@linaro.org?part=3

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