Re: [Intel-gfx] [PATCH 02/21] drm/i915/tgl: Fix macros for TGL SOC based WA

From: Jani Nikula <jani.nikula@intel.com>
To: Lucas De Marchi <lucas.demarchi@intel.com>,
	Aditya Swarup <aditya.swarup@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 02/21] drm/i915/tgl: Fix macros for TGL SOC based WA
Date: Wed, 18 Nov 2020 11:18:08 +0200	[thread overview]
Message-ID: <87mtzfowfj.fsf@intel.com> (raw)
In-Reply-To: <20201117193114.ujqf4mgu3z2pzkab@ldmartin-desk1>

On Tue, 17 Nov 2020, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
> On Tue, Nov 17, 2020 at 10:50:10AM -0800, Aditya Swarup wrote:
>>@@ -1579,9 +1579,9 @@ static inline const struct i915_rev_steppings *
>> tgl_revids_get(struct drm_i915_private *dev_priv)
>> {
>> 	if (IS_TGL_U(dev_priv) || IS_TGL_Y(dev_priv))
>>-		return tgl_uy_revids;
>>+		return tgl_uy_revids + INTEL_REVID(dev_priv);
>
> oohh, no. You have to at least check you are not accessing out of
> bounds. New HW running on old kernel should not access create invalid
> accesses like this.

And this is just one reason why exposing arrays directly as an interface
to the rest of the driver is a bad idea. Basically I look at *all*
externs in the driver with suspicion, and they're all exceptions that
should not be repeated. The revid arrays are a direct invitation to keep
adding more and more extern arrays. And more ways to go out of bounds.

I'd rather we seek for ways to either nuke the revid arrays altogether,
or encapsulate them within a .c file with static scope.

And for that .c file... the arrays are now in gt/intel_workarounds.c
which is a really weird place for stuff that's used for generic stepping
info, and particularly for *display* stepping info.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx