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

[sashiko-bot@kernel.org]

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@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.

[ ... ]

> +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.

> +	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.

[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.

> +		}
> +	}
> +
> +	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?

> 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?

[ ... ]

> +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.

> +			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];
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260719-x1e-csi2-phy-v11-0-9d0a1ed0632d@linaro.org?part=2