Re: [PATCH v2 2/3] media: ccs-pll: Support optional input for VT pixel rate

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] State reuse logic bug: Repurposing the output field `pixel_rate_pixel_array` as an optional input causes subsequent PLL recalculations to reuse the previous achieved rate, breaking dynamic adaptation.
- [High] Missing validation: The DUAL_PLL configuration path bypasses hardware limit checks for FIFO derating and overrating.
--

--- Patch [2]: [PATCH v2 2/3] media: ccs-pll: Support optional input for VT pixel rate ---
commit 61f223eeeb41f300a2760175cdb5d9c15c35bf95
Author: Jai Luthra <[email protected]>

media: ccs-pll: Support optional input for VT pixel rate

This commit adds support for an optional target VT pixel rate input when
the PLL is configured in DUAL_MODE. It utilizes the pixel_rate_pixel_array
field to pass this input target, falling back to pixel_rate_csi if it is
not provided.

> When the PLL is configured in DUAL_MODE (separate VT and OP trees) and
> the sensor's FIFOs allow derating and/or overrating, the VT pixel rate
> might be higher or lower than OP pixel rate.

[Severity: High]
Does this bypass the hardware capability validation for FIFO derating and
overrating?

When CCS_PLL_FLAG_DUAL_PLL is set, ccs_pll_calculate() breaks out of the loop
early and returns successfully after calling ccs_pll_calculate_vt_tree(). This
completely skips the call to check_ext_bounds().

Could this allow arbitrary diverging VT pixel rates to be applied that exceed
the sensor's FIFO capabilities without throwing an error, leading to FIFO
underflows or overflows?

> diff --git a/drivers/media/i2c/ccs-pll.c b/drivers/media/i2c/ccs-pll.c
> index 1605cfa5db19d..9aa164994cc73 100644
> --- a/drivers/media/i2c/ccs-pll.c
> +++ b/drivers/media/i2c/ccs-pll.c

[ ... ]

> @@ -388,6 +388,14 @@ __ccs_pll_calculate_vt_tree(struct device *dev,
>  	return 0;
>  }
>  
> +static inline u32 ccs_pll_vt_pixel_rate(const struct ccs_pll *pll)
> +{
> +	if (pll->pixel_rate_pixel_array)
> +		return pll->pixel_rate_pixel_array;

[Severity: High]
Does this cause a state reuse issue for subsequent PLL recalculations?

This helper treats pixel_rate_pixel_array as an input target. However,
__ccs_pll_calculate_vt_tree() writes the achieved VT rate back into this
same field:

drivers/media/i2c/ccs-pll.c:__ccs_pll_calculate_vt_tree() {
    ...
    pll->pixel_rate_pixel_array =
        pll_bk->pix_clk_freq_hz * pll->vt_lanes;
    ...
}

Callers like ccs_pll_update() in ccs-core.c maintain the struct ccs_pll across
multiple calls (such as during V4L2 link frequency changes) without zeroing
this field.

Since this helper unconditionally prefers pixel_rate_pixel_array if non-zero,
won't subsequent configuration changes silently use the stale achieved rate
from a previous calculation instead of the intended target?

> +
> +	return pll->pixel_rate_csi;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2