[PATCH v11 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver

[Bryan O'Donoghue]

On 19/07/2026 13:41, sashiko-bot at kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Insufficient validation of the `data-lanes` property allows values up to 7, leading to integer truncation and out-of-bounds MMIO writes into common PHY registers.
> - [Medium] The driver statically configures lanes based on the device tree instead of respecting dynamic `dphy_cfg->lanes` requests, violating the MIPI D-PHY API contract.
> - [Medium] Setting lane polarity unconditionally overwrites the entire `CTRL9` register instead of performing a read-modify-write.
> - [Low] Unmasked assignment of the first byte in `hw_version` may corrupt the final value with undefined upper bits.
> --
> 
> --- Patch [2]: [PATCH v11 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver ---
> commit cff02f6508398df4f254c675b3580c3c848e17c2
> Author: Bryan O'Donoghue <bryan.odonoghue at linaro.org>
> Subject: phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver
> 
> This commit adds a new MIPI CSI2 PHY driver in DPHY mode for Qualcomm
> hardware. It ports logic from existing camss drivers to initialize and
> configure data and clock lanes based on device tree properties.
> 
>> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
>> --- /dev/null
>> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
> 
> [ ... ]
> 
>> +static void phy_qcom_mipi_csi2_hw_version_read(struct mipi_csi2phy_device *csi2phy)
>> +{
>> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
>> +	u32 tmp;
>> +
>> +	writel(CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID, csi2phy->base +
>> +	       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->common_regs_offset, 6));
>> +
>> +	tmp = readl_relaxed(csi2phy->base +
>> +			    CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->common_regs_offset, 12));
>> +	csi2phy->hw_version = tmp;
> 
> [Severity: Low]
> Does this assignment to csi2phy->hw_version need a mask like & 0xFF?
> 
> If the hardware register returns non-zero garbage values in the upper 24
> bits, those bits might remain in hw_version and corrupt the final value
> when the subsequent shifted bytes are OR'ed in.
> 
> [ ... ]

No not validating for "hardware returns garbage" that's a rabbit hole we 
don't go down. The bits out of registers are trusted as correct.
> 
>> +static int phy_qcom_mipi_csi2_lanes_enable(struct mipi_csi2phy_device *csi2phy,
>> +					   struct mipi_csi2phy_stream_cfg *cfg)
>> +{
>> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
>> +	struct mipi_csi2phy_lanes_cfg *lane_cfg = &cfg->lane_cfg;
>> +	u8 settle_cnt;
>> +	u8 val;
>> +	int i;
> 
> [ ... ]
> 
>> +	/*
>> +	 * CSI_COMMON_CTRL5 is a physical lane power-up bitmap:
>> +	 * - Bits [0,2,4,6] → D-PHY data lanes(LN0, LN2, LN4, LN6)
>> +	 * - Bits [1,3,5] → C-PHY trio lanes(LN1, LN3, LN5)
>> +	 * - Bit [7] → D-PHY clock lane(LNCK) dedicated clock enable
>> +	 */
>> +	val = BIT(lane_cfg->clk.pos);
>> +	for (i = 0; i < cfg->num_data_lanes; i++)
>> +		val |= BIT(lane_cfg->data[i].pos * 2);
> 
> [Severity: High]
> Could this cause an integer truncation?
> 
> Because the val variable is defined as a u8, shifting by 8 or more will
> silently truncate to 0. If a lane position is >= 4, lane_cfg->data[i].pos * 2
> becomes >= 8, which means the driver might fail to power up the lane.

Will validate data_lanes[i] against >= MAX_DATA_LANES
> 
>> +	writel(val, csi2phy->base +
>> +	       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->common_regs_offset, 5));
>> +
>> +	/* Lane configuration for polarity @ CSIPHY-base + CTRL9 */
>> +	for (i = 0; i < cfg->num_data_lanes; i++) {
>> +		if (lane_cfg->data[i].pol) {
>> +			u8 pos = lane_cfg->data[i].pos;
>> +
>> +			writel(BIT(2), csi2phy->base + CSIPHY_2PH_LN_CSI_2PHASE_CTRL9n(pos * 2));
> 
> [Severity: High]
> Can this result in an out-of-bounds memory write?
> 
> If pos is >= 4, pos * 2 is >= 8. The CSIPHY_2PH_LN_CSI_2PHASE_CTRL9n macro
> will compute an offset that overflows the physical lane register space and
> aliases directly into the common PHY register block, potentially clobbering
> common registers.


Will validate data_lanes[i] against >= MAX_DATA_LANES

> [Severity: Medium]
> Is it safe to use an absolute writel() here instead of a read-modify-write?
> 
> Writing BIT(2) directly clears any hardware defaults or reserved bits that
> might be present in the rest of the register. This also creates asymmetric
> handling since the register is left untouched if polarity is false.

Overwriting the register is deliberate at this point

>> +		}
>> +	}
>> +
>> +	if (lane_cfg->clk.pol)
>> +		writel(BIT(2), csi2phy->base + CSIPHY_2PH_LN_CSI_2PHASE_CTRL9n(lane_cfg->clk.pos));
> 
> [Severity: Medium]
> Similarly, could this absolute write clobber the clock lane CTRL9 register?

Overwriting the register is deliberate at this point


>> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
>> --- /dev/null
>> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
> 
> [ ... ]
> 
>> +static int phy_qcom_mipi_csi2_configure(struct phy *phy,
>> +					union phy_configure_opts *opts)
>> +{
>> +	struct mipi_csi2phy_device *csi2phy = phy_get_drvdata(phy);
>> +	struct phy_configure_opts_mipi_dphy *dphy_cfg = &opts->mipi_dphy;
>> +	struct mipi_csi2phy_stream_cfg *stream_cfg = &csi2phy->stream_cfg;
>> +	int ret;
>> +
>> +	ret = phy_mipi_dphy_config_validate(dphy_cfg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (dphy_cfg->lanes < 1 || dphy_cfg->lanes > CSI2_MAX_DATA_LANES)
>> +		return -EINVAL;
>> +
>> +	stream_cfg->link_freq = dphy_cfg->hs_clk_rate;
>> +
>> +	return 0;
>> +}
> 
> [Severity: Medium]
> Should the dynamically requested dphy_cfg->lanes value be saved into the
> active configuration here?
> 
> The value is validated but discarded, meaning the driver will always
> statically power up the maximum number of lanes configured in the device
> tree instead of respecting dynamic requests from the consumer. Does this
> violate the MIPI D-PHY API contract for dynamic lane configuration?

We use a fixed configuration right now. Future work may allow for 
dynamic lane configuration - requires way more work defining registers 
and bits plus logic to configure based on requested input.


> [ ... ]
> 
>> +static int phy_qcom_mipi_csi2_parse_routing(struct mipi_csi2phy_device *csi2phy)
>> +{
>> +	struct mipi_csi2phy_stream_cfg *stream_cfg = &csi2phy->stream_cfg;
> 
> [ ... ]
> 
>> +	csi2phy->stream_cfg.lane_cfg.clk.pos = CSI2_DEFAULT_CLK_LANE;
>> +	csi2phy->stream_cfg.lane_cfg.clk.pol = lane_polarities[0];
>> +
>> +	for (i = 0; i < csi2phy->stream_cfg.num_data_lanes; i++) {
>> +		if (data_lanes[i] > CSI2_MAX_LANE_NUM) {
> 
> [Severity: High]
> Is CSI2_MAX_LANE_NUM (7) too large for this bounds check?
> 
> As noted in phy_qcom_mipi_csi2_lanes_enable(), allowing lane numbers >= 4
> results in pos * 2 being >= 8 later on, which causes both truncation in the
> u8 bitmask and out-of-bounds writes into the common PHY register region.

No data-lanes is constrained and will be validated in the next pass.

>> +			dev_err(dev, "Invalid lane %d\n", data_lanes[i]);
>> +			ret = -EINVAL;
>> +			goto out_put;
>> +		}
>> +		csi2phy->stream_cfg.lane_cfg.data[i].pos = data_lanes[i];
>> +		csi2phy->stream_cfg.lane_cfg.data[i].pol = lane_polarities[i + 1];
>> +	}
>