Re: [PATCH v2 2/2] media: qcom: camss: Add Glymur-specific resources

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] media: qcom: camss: NULL deref in
  msm_csiphy_subdev_init_legacy() on Glymur
- [Low] media: qcom: camss: missing Fixes trailer for commit
  09d6c30d3fae
--

commit 0569a71f44b398cac082b08f3b002bec082fcc8b
Author: Nihal Kumar Gupta <nihal.gupta@oss.qualcomm.com>

> media: qcom: camss: Add Glymur-specific resources
>
> Glymur has three CSI2 PHYs, while X1E80100 has four. Reusing the X1E80100
> CSIPHY resources also exposes CSIPHY2, which is not present on Glymur.
>
> Add Glymur-specific CSIPHY resources for the three available PHYs while
> continuing to reuse the common X1E80100 CAMSS resources.
>
> Signed-off-by: Nihal Kumar Gupta <nihal.gupta@oss.qualcomm.com>

[Severity: Low]
This fixes the mapping of qcom,glymur-camss to x1e80100_resources.  Should
this include:

  Fixes: 09d6c30d3fae ("media: qcom: camss: Add Glymur compatible")

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index f8e5cf9f3ff09..f3cab9ecef042 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -4103,6 +4103,30 @@ static const struct resources_icc icc_res_sa8775p[] = {
>  	},
>  };
>  
> +static const struct camss_subdev_resources csiphy_res_glymur[] = {
> +	/* CSIPHY0 */
> +	{
> +		.csiphy = {
> +			.id = 0,
> +			.formats = &csiphy_formats_sdm845
> +		},
> +	},
> +	/* CSIPHY1 */
> +	{
> +		.csiphy = {
> +			.id = 1,
> +			.formats = &csiphy_formats_sdm845
> +		},
> +	},
> +	/* CSIPHY4 */
> +	{
> +		.csiphy = {
> +			.id = 4,
> +			.formats = &csiphy_formats_sdm845
> +		},
> +	},
> +};

[Severity: High]
Can leaving .csiphy.hw_ops unset in csiphy_res_glymur cause a NULL pointer
dereference during probe?

camss_detect_legacy_phy() defaults camss->legacy_phy to true and only clears
it when it finds an endpoint whose remote port parent node is named "phy":

drivers/media/platform/qcom/camss/camss.c:camss_detect_legacy_phy() {
	camss->legacy_phy = true;

	/* Find first remote-endpoint and determine if its a PHY */
	for_each_endpoint_of_node(camss->dev->of_node, ep) {
		remote = of_graph_get_remote_port_parent(ep);
		if (!remote)
			continue;

		camss->legacy_phy = !of_node_name_eq(remote, "phy");
		of_node_put(remote);
		of_node_put(ep);
		break;
	}
}

Because endpoint and remote-endpoint are optional in the binding (for
example, when no camera is connected or only the internal test pattern
generators are used), camss->legacy_phy stays true when no remote endpoint
is present.  camss_init_subdevices() then calls
msm_csiphy_subdev_init_legacy(), which unconditionally calls
csiphy->res->hw_ops->init(csiphy).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-glymur_camss-v2-0-b1b76509c744@oss.qualcomm.com?part=2