Re: [PATCH v17 05/11] media: qcom: camss: csiphy: Add support for v2.4.0 two-phase CSIPHY

From: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>
To: sashiko-reviews@lists.linux.dev
Cc: vkoul@kernel.org, neil.armstrong@linaro.org,
	linux-phy@lists.infradead.org, robh@kernel.org,
	olteanv@gmail.com, media-ci@linuxtv.org,
	devicetree@vger.kernel.org, conor+dt@kernel.org
Subject: Re: [PATCH v17 05/11] media: qcom: camss: csiphy: Add support for v2.4.0 two-phase CSIPHY
Date: Wed, 30 Sep 2026 21:01:18 +0800	[thread overview]
Message-ID: <76154416-bd16-4721-b7e8-035aea1457a0@oss.qualcomm.com> (raw)
In-Reply-To: <20260929061712.083BD1F000FF@smtp.kernel.org>

On 9/29/2026 2:17 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] media: camss: guaranteed NULL pointer dereference in msm_csiphy_subdev_init()
> --
> 
> commit 3c87101a27e8d1c1d3a64a10ed465e1ed433c549
> Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>
> 
> media: qcom: camss: csiphy: Add support for v2.4.0 two-phase CSIPHY
> 
> Add support for the v2.4.0 two-phase CSIPHY found on Kaanapali, where
> the PHY is driven by the standalone phy-qcom-mipi-csi2 DPHY driver
> rather than by CAMSS. Add the mipi_csi2_dphy_3nm_kaanapali configuration,
> which reuses the x1e80100 lane sequence, clocks and supplies but selects
> the v2.4.0 common status offset, adding Kaanapali power domain and
> register the "qcom,kaanapali-csi2-phy" compatible.
> 
>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>> index a528760487bd9..a0cd839c9948a 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -34,6 +34,57 @@
>>   
>>   static const struct parent_dev_ops vfe_parent_dev_ops;
>>   
>> +static const struct camss_subdev_resources csiphy_res_kaanapali[] = {
>> +	/* CSIPHY0 */
>> +	{
>> +		.csiphy = {
>> +			.id = 0,
>> +			.hw_ops = &csiphy_ops_3ph_1_0,
>> +			.formats = &csiphy_formats_sdm845
>> +		},
>> +	},
> 
> [Severity: Critical]
> Can this result in a NULL pointer dereference during device probe?
> 
> Because this Kaanapali resource definition completely omits the .reg array, it
> implicitly initializes to NULL. However, it still binds legacy hardware
> operations (&csiphy_ops_3ph_1_0) that expect MMIO mappings.
> 
> When the CAMSS probe unconditionally iterates over all csiphy_num entries
> and invokes msm_csiphy_subdev_init() for each, it attempts to map the legacy
> register space:
> 
> drivers/media/platform/qcom/camss/camss-csiphy.c:msm_csiphy_subdev_init() {
>      ...
>      csiphy->base = devm_platform_ioremap_resource_byname(pdev, res->reg[0]);
>      ...
> }
> 
> Because .reg was omitted, res->reg[0] is NULL, which propagates down to
> platform_get_resource_byname(). Since Kaanapali defines other named resources,
> evaluating strcmp(r->name, NULL) with a NULL name argument can cause a crash.
> 
> Does the standalone PHY need to be consumed via the kernel's generic PHY
> framework (e.g., devm_phy_get) instead of retaining legacy CAMSS ops that
> assume direct MMIO control?
> 
> [ ... ]
> 

Kaanapali use the re-arch CSIPHY framework so that the resources like 
register, supply are now handled by a separate PHY driver. We don't need 
to declare those redundant resources in CAMSS driver now.

Best Regards,
Hangxiang

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