On Mon, 29 Jul 2024, Nikita Zhandarovich <n.zhandarov...@fintech.ru> wrote:
> On the off chance that clock value ends up being too high (by means
> of skl_ddi_calculate_wrpll() having benn called with big enough
> value of crtc_state->port_clock * 1000), one possible consequence
> may be that the result will not be able to fit into signed int.
>
> Fix this issue by moving conversion of clock parameter from kHz to Hz
> into the body of skl_ddi_calculate_wrpll(), as well as casting the
> same parameter to u64 type while calculating the value for AFE clock.
> This both mitigates the overflow problem and avoids possible erroneous
> integer promotion mishaps.
>
> Found by Linux Verification Center (linuxtesting.org) with static
> analysis tool SVACE.
>
> Fixes: fe70b262e781 ("drm/i915: Move a bunch of stuff into rodata from the
> stack")
I don't think that's right. The code was only shuffled around at that
point. I think the bug's been there since the code was added in commit
82d354370189 ("drm/i915/skl: Implementation of SKL DPLL programming").
Fixed while applying to drm-intel-next, thanks for the patch.
BR,
Jani.
> Cc: sta...@vger.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarov...@fintech.ru>
> ---
> v2: instead of double casting of 'clock' with (u64)(u32), convert
> 'clock' to Hz inside skl_ddi_calculate_wrpll() and cast it only
> to u64 to mitigate the issue. Per Jani's <jani.nik...@linux.intel.com>
> helpful suggestion made here:
> https://lore.kernel.org/all/87ed7gzhin....@intel.com/
> Also, change commit description accordingly.
>
> v1:
> https://lore.kernel.org/all/20240724184911.12250-1-n.zhandarov...@fintech.ru/
>
> drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index 90998b037349..292d163036b1 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -1658,7 +1658,7 @@ static void skl_wrpll_params_populate(struct
> skl_wrpll_params *params,
> }
>
> static int
> -skl_ddi_calculate_wrpll(int clock /* in Hz */,
> +skl_ddi_calculate_wrpll(int clock,
> int ref_clock,
> struct skl_wrpll_params *wrpll_params)
> {
> @@ -1683,7 +1683,7 @@ skl_ddi_calculate_wrpll(int clock /* in Hz */,
> };
> unsigned int dco, d, i;
> unsigned int p0, p1, p2;
> - u64 afe_clock = clock * 5; /* AFE Clock is 5x Pixel clock */
> + u64 afe_clock = (u64)clock * 1000 * 5; /* AFE Clock is 5x Pixel clock,
> in Hz */
>
> for (d = 0; d < ARRAY_SIZE(dividers); d++) {
> for (dco = 0; dco < ARRAY_SIZE(dco_central_freq); dco++) {
> @@ -1808,7 +1808,7 @@ static int skl_ddi_hdmi_pll_dividers(struct
> intel_crtc_state *crtc_state)
> struct skl_wrpll_params wrpll_params = {};
> int ret;
>
> - ret = skl_ddi_calculate_wrpll(crtc_state->port_clock * 1000,
> + ret = skl_ddi_calculate_wrpll(crtc_state->port_clock,
> i915->display.dpll.ref_clks.nssc,
> &wrpll_params);
> if (ret)
> return ret;
--
Jani Nikula, Intel
