Re: [PATCH v12 5/6] 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:
- [High] Missing `phy_exit()` cleanup on multiple probe error paths, leaking PHY initialization state.
- [High] Missing PHY node availability check in `camss_parse_ports()` causes a WARN_ON crash during link setup if a PHY is disabled in DT.
- [Medium] The commit unconditionally selects a non-existent Kconfig symbol `PHY_QCOM_MIPI_CSI2`, causing unmet dependencies and build failures.
- [Medium] Ignored return value from `phy_mipi_dphy_get_default_config_for_hsclk()` may lead to invalid PHY configuration.
--

--- Patch [5]: [PATCH v12 5/6] media: qcom: camss: Add support for PHY API devices ---
commit 6ca141cbb4ec0862b23ea5f00a1fb741065d4818
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]
Is PHY_QCOM_MIPI_CSI2 a valid configuration symbol in the kernel tree?

Selecting a non-existent symbol can lead to unmet dependency warnings or
build failures since the intended driver won't be enabled.

> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> index 539ac4888b608..b740edd0c67e6 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
[ ... ]
> @@ -306,11 +308,89 @@ static int csiphy_stream_on(struct csiphy_device *csiphy)
[ ... ]
> +	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 value of phy_mipi_dphy_get_default_config_for_hsclk() be
checked here?

If link_freq is zero or calculation fails, it returns a negative error code
and leaves dphy_opts unpopulated. Passing an uninitialized struct to
phy_configure() might cause invalid configurations or division-by-zero errors.

[ ... ]
> @@ -709,6 +789,60 @@ int msm_csiphy_subdev_init(struct camss *camss,
[ ... ]
> +	ret = phy_init(csiphy->phy);
> +	if (ret)
> +		dev_err(dev, "phy %s init fail %d\n", csiphy->name, ret);
> +
> +put_np:
> +	of_node_put(args.np);
> +
> +	return ret;
> +}

[Severity: High]
Since phy_init() is called here without a devres wrapper, could this leak the
PHY initialization state if probe fails later?

For example, if camss_init_subdevices() fails after this point, the error
path jumps to err_genpd_cleanup and bypasses the camss_unregister_entities()
call needed to invoke phy_exit().

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index e814a96953b1b..146fea438486f 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -4794,14 +4794,51 @@ static int camss_parse_endpoint_node(struct device *dev,
>  static int camss_parse_ports(struct camss *camss)
>  {
>  	struct device *dev = camss->dev;
> +	const struct camss_resources *res = camss->res;
>  	struct fwnode_handle *fwnode = dev_fwnode(dev), *ep;
>  	int ret;
>  
>  	fwnode_graph_for_each_endpoint(fwnode, ep) {
>  		struct camss_async_subdev *csd;
> +		struct fwnode_handle *remote;
> +
> +		if (!fwnode_device_is_available(ep))
> +			continue;
> +
> +		if (res->legacy_phy) {
> +			csd = v4l2_async_nf_add_fwnode_remote(&camss->notifier, ep,
> +							      typeof(*csd));
> +		} else {
> +			struct fwnode_handle *phy_out, *phy_node, *phy_in, *sensor_ep;
> +
> +			phy_out = fwnode_graph_get_remote_endpoint(ep);
> +			if (!phy_out)
> +				continue;
> +
> +			phy_node = fwnode_graph_get_port_parent(phy_out);
> +			fwnode_handle_put(phy_out);
> +			if (!phy_node)
> +				continue;
> +
> +			phy_in = fwnode_graph_get_endpoint_by_id(phy_node, 0, 0, 0);

[Severity: High]
Is it possible to encounter a crash during link setup if a PHY is disabled
in the device tree?

This code retrieves phy_node but does not check if it is available using
fwnode_device_is_available() before adding the remote sensor endpoint to
the notifier.

Later, msm_csiphy_subdev_init() correctly skips the disabled PHY, leaving
the csiphy entity uninitialized with 0 pads. When the sensor binds,
camss_subdev_notifier_complete() attempts to link the sensor to this
empty entity, which triggers a WARN_ON crash inside media_create_pad_link()
due to sink_pad >= sink->num_pads.

[ ... ]
> @@ -5132,8 +5186,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 (camss->csiphy[i].phy)
> +			msm_csiphy_unregister_entity(&camss->csiphy[i]);
> +	}
>  
>  	return ret;
>  }

[Severity: High]
If msm_csiphy_register_entity() fails at index i, does this rollback loop
miss cleaning up the PHY at index i?

The loop decrements i before the check, so it only cleans up indices
strictly less than the failing index, permanently leaking the initialized
PHY for the failing index itself.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-b4-linux-next-25-03-13-dtsi-x1e80100-camss-v12-0-f8588da41f16@linaro.org?part=5