Re: [PATCH v15 4/4] 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:
- [High] Unchecked return value from `phy_mipi_dphy_get_default_config_for_hsclk` leads to uninitialized PHY configuration and potential divide-by-zero kernel panic.
- [Medium] The Kconfig symbol `PHY_QCOM_MIPI_CSI2` selected by `VIDEO_QCOM_CAMSS` does not exist.
- [Medium] Missing `phy_exit()` on probe error paths leads to a PHY state resource leak.

Pre-existing issues:
- [Critical] Missing bounds check on the device tree port number leads to a heap out-of-bounds write.
--

commit 26d58015dee86032083946c737ad4b615540ce3c
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 Kconfig symbol PHY_QCOM_MIPI_CSI2 exist? Selecting a non-existent
Kconfig symbol has no effect or causes unmet dependency warnings, which can
omit required features from the build.

> 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)
[ ... ]
> +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);

[Severity: High]
If camss_get_link_freq() returns 0, phy_mipi_dphy_get_default_config_for_hsclk()
will return -EINVAL. Does this code ignore the return value, leaving dphy_opts
uninitialized? 

Passing an all-zero struct to phy_configure() could cause the PHY driver to 
attempt a division by hs_clk_rate (which is 0), leading to a kernel panic.

> +
> +	phy_set_mode(csiphy->phy, PHY_MODE_MIPI_DPHY);
> +
> +	ret = phy_configure(csiphy->phy, &dphy_opts);
> +	if (ret) {
> +		dev_err(dev, "failed to configure MIPI D-PHY\n");
> +		goto error;
> +	}
> +
> +	return phy_power_on(csiphy->phy);
> +
> +error:
> +	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
> @@ -4799,8 +4799,43 @@ static int camss_parse_ports(struct camss *camss)
[ ... ]
>  		if (IS_ERR(csd)) {
>  			ret = PTR_ERR(csd);
>  			goto err_cleanup;
>  		}
>  
>  		ret = camss_parse_endpoint_node(dev, ep, csd);

[Severity: Critical]
This is a pre-existing issue, but does camss_parse_endpoint_node() lack bounds
checking on the device tree port number?

When camss_parse_endpoint_node() reads the port number into
csd->interface.csiphy_id, it appears to be used as an index into the
camss->csiphy array in camss_subdev_notifier_bound() without verifying it is
less than camss->res->csiphy_num:

camss_subdev_notifier_bound() {
    u8 id = csd->interface.csiphy_id;
    struct csiphy_device *csiphy = &camss->csiphy[id];
    ...
    csiphy->cfg.csi2 = &csd->interface.csi2;
}

Can a malformed device tree with a large port number cause a heap out-of-bounds
write here?

>  		if (ret < 0)
>  			goto err_cleanup;
>  	}
>  
>  	return 0;
[ ... ]
> @@ -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 subsequent iteration (or if a later probe
step fails and jumps to err_genpd_cleanup in camss_probe), do we leak the
initialized PHY state?

msm_csiphy_subdev_init() explicitly initializes the PHY with phy_init(), which
establishes the requirement for a subsequent phy_exit(). When returning here,
camss_unregister_entities() is skipped, which appears to leak the PHY resources
initialized in earlier loop iterations.

>  		}
>  	}

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

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