Re: [PATCH v3 1/3] phy: rockchip: phy-rockchip-inno-csidphy: fix rk1808 hsfreq table

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Valid low-frequency configurations are incorrectly rejected because a `cfg_bit` value of `0x00` conflicts with the default 'not found' sentinel.
- [High] The `rk3368_grf_dphy_regs` array is missing an explicit size and only initializes index 0, meaning it is compiled as an array of size 1. `rockchip_inno_csidphy_power_on()` unconditionally indexes into this array at indices 1 and 2, causing an out-of-bounds read and an erroneous write to an undefined GRF register offset.
--

commit 4cc350ff3a3bfdcb6204ddc2357172385fbe4d2a
Author: Gerald Loacker <gerald.loacker@wolfvision.net>

phy: rockchip: phy-rockchip-inno-csidphy: fix rk1808 hsfreq table

The rk1808 hsfreq table previously capped at 2499 Mbps. This commit
extends the final entry to 2500 Mbps to support this exact data rate,
which is essential for the RK3588 platform reusing this array.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c b/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c
> index c79fb53d8ee5c..5281f8dea0ad3 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c
> @@ -170,7 +170,7 @@ static const struct hsfreq_range rk1808_mipidphy_hsfreq_ranges[] = {
>  	{ 299, 0x06}, { 399, 0x08}, { 499, 0x0b}, { 599, 0x0e},
>  	{ 699, 0x10}, { 799, 0x12}, { 999, 0x16}, {1199, 0x1e},
>  	{1399, 0x23}, {1599, 0x2d}, {1799, 0x32}, {1999, 0x37},
> -	{2199, 0x3c}, {2399, 0x41}, {2499, 0x46}
> +	{2199, 0x3c}, {2399, 0x41}, {2500, 0x46}
>  };

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the frequency
tables I noticed a potential problem in how the selected configuration is
evaluated later in the driver.

If a data rate maps to a configuration where the cfg_bit is 0x00, will
rockchip_inno_csidphy_configure() incorrectly reject it?

In rockchip_inno_csidphy_configure():
    if (!hsfreq)
        return -EINVAL;

It seems this check uses 0x00 as a not found sentinel, but valid low
frequency ranges might legitimately use 0x00 as their configuration value.
This same logic is also present in rk_dphy_configure() in
phy-rockchip-dphy-rx0.c. Does this prevent the hardware from operating at
those low frequencies?

[Severity: High]
This is also a pre-existing issue, but I noticed an out-of-bounds array
access in this driver for the RK3368 platform.

The rk3368_grf_dphy_regs array is initialized with only a single element:

static const struct dphy_reg rk3368_grf_dphy_regs[] = {
    [GRF_DPHY_CSIPHY_FORCERXMODE] = PHY_REG(RK3368_GRF_SOC_CON6_OFFSET, 4, 8),
};

However, rockchip_inno_csidphy_power_on() unconditionally accesses
indices beyond this:

    write_grf_reg(priv, GRF_DPHY_CSIPHY_CLKLANE_EN, 0x1);
    write_grf_reg(priv, GRF_DPHY_CSIPHY_DATALANE_EN,
                  GENMASK(priv->config.lanes - 1, 0));

Could this read past the end of the 1-element array and cause an erroneous
write to an undefined register offset if the adjacent memory evaluates as
valid?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630-feature-mipi-csi-dphy-4k60-v3-0-176792ab71fa@wolfvision.net?part=1

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