Re: [PATCH v8 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 5 potential issue(s) to consider:
- [High] Integer underflow in PHY settle count calculation.
- [High] Missing bounds checking for device tree lane arrays leads to out-of-bounds IO memory writes.
- [High] Unconditional dereference of csi2phy->pd_list can cause a kernel panic if power domains are missing.
- [Medium] Probe ignores -ENODEV from OPP table initialization, but power_on unconditionally fails without it.
- [Medium] qcom_csi2_phy_xlate lacks args_count validation before reading args array.
--

commit 66168a7f8131d0b43bbb6d3d1472182c1beff538
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver

This commit adds a new MIPI CSI2 driver for DPHY mode, importing CAMSS CSI
PHY init sequences for initial support. The driver handles data/clock lane
configuration and replaces the useless interrupt handler with regulator and
clock bulk APIs.

> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
> new file mode 100644
> index 0000000000000..86ec405820e62
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
[ ... ]
> +static u8 phy_qcom_mipi_csi2_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
> +{
> +	u32 t_hs_prepare_max_ps;
> +	u32 timer_period_ps;
> +	u32 t_hs_settle_ps;
> +	u8 settle_cnt;
> +	u32 ui_ps;
> +
> +	if (link_freq <= 0)
> +		return 0;
> +
> +	ui_ps = div_u64(PSEC_PER_SEC, link_freq);
> +	ui_ps /= 2;
> +	t_hs_prepare_max_ps = 85000 + 6 * ui_ps;
> +	t_hs_settle_ps = t_hs_prepare_max_ps;
> +
> +	timer_period_ps = div_u64(PSEC_PER_SEC, timer_clk_rate);
> +	settle_cnt = t_hs_settle_ps / timer_period_ps - 6;

[Severity: High]
Can this underflow if the ratio is less than 6?

For example, with a CSI-2 link frequency of 200 MHz, the ratio could evaluate
to 5. Subtracting 6 would result in an unsigned integer underflow, yielding 255
for the u8 variable. This could cause an inflated settle count to be written
to the PHY registers. Does this calculation need to clamp the result to 0 or
ensure the ratio is greater than or equal to 6?

> +
> +	return settle_cnt;
> +}
[ ... ]
> +static int phy_qcom_mipi_csi2_lanes_enable(struct mipi_csi2phy_device *csi2phy,
> +					   struct mipi_csi2phy_stream_cfg *cfg)
> +{
[ ... ]
> +	/* 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));

[Severity: High]
Does this code perform bounds checking on the device tree lane arrays?

Since the data-lanes and clock-lanes properties from the device tree are
stored in pos without validation, a large pos value could trigger
out-of-bounds IO memory writes via the CSIPHY_2PH_LN_CSI_2PHASE_CTRL9n(pos)
offset and BIT(pos) bitwise shift.

> +		}
> +	}
> +
> +	if (lane_cfg->clk.pol)
> +		writel(BIT(2), csi2phy->base + CSIPHY_2PH_LN_CSI_2PHASE_CTRL9n(lane_cfg->clk.pos));
[ ... ]
> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
> new file mode 100644
> index 0000000000000..dfeff863a406f
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
[ ... ]
> +static int
> +phy_qcom_mipi_csi2_set_clock_rates(struct mipi_csi2phy_device *csi2phy,
> +				   s64 link_freq)
> +{
> +	struct device *dev = csi2phy->dev;
> +	unsigned long opp_rate = link_freq / 4;
> +	struct dev_pm_opp *opp;
> +	long timer_rate;
> +	int i, ret;
> +
> +	opp = dev_pm_opp_find_freq_ceil(dev, &opp_rate);
> +	if (IS_ERR(opp)) {

[Severity: Medium]
Will this fail unconditionally if the OPP table is missing?

In phy_qcom_mipi_csi2_probe(), the -ENODEV error from
devm_pm_opp_of_add_table() is explicitly ignored, allowing the probe to
succeed even if the device tree lacks an OPP table. 

However, dev_pm_opp_find_freq_ceil() is called here unconditionally,
which would return an error and cause the power_on routine to fail. Should
we enforce the OPP table presence during probe or handle its absence
gracefully here?

> +		dev_err(csi2phy->dev, "Couldn't find ceiling for %lld Hz\n",
> +			link_freq);
> +		return PTR_ERR(opp);
> +	}
> +
> +	for (i = 0; i < csi2phy->pd_list->num_pds; i++) {

[Severity: High]
Can this dereference a NULL pointer if power domains are missing?

If the device tree lacks the power-domains property,
devm_pm_domain_attach_list() returns 0 and leaves csi2phy->pd_list as NULL.
Iterating with csi2phy->pd_list->num_pds without a NULL check would cause
a kernel panic.

> +		unsigned int perf = dev_pm_opp_get_required_pstate(opp, i);
[ ... ]
> +static struct phy *qcom_csi2_phy_xlate(struct device *dev,
> +				       const struct of_phandle_args *args)
> +{
> +	struct mipi_csi2phy_device *csi2phy = dev_get_drvdata(dev);
> +
> +	if (args->args[0] != PHY_TYPE_DPHY) {

[Severity: Medium]
Should this check args->args_count before accessing the args array?

If a malformed device tree specifies #phy-cells = <0>, args_count will be 0,
and reading args->args[0] will access uninitialized memory.

> +		dev_err(csi2phy->dev, "mode %d -EOPNOTSUPP\n", args->args[0]);
> +		return ERR_PTR(-EOPNOTSUPP);
> +	}
> +
> +	csi2phy->phy_mode = args->args[0];
> +
> +	return csi2phy->phy;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260523-x1e-csi2-phy-v8-0-a85668459521@linaro.org?part=2

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