Re: [PATCH] drm/i915/pmu: Fix enable count array size and bounds checking

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Tvrtko Ursulin (2019-02-05 10:29:05)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Enable count array is supposed to have one counter for each possible
> engine sampler. As such, array sizing and bounds checking is not correct
> and would blow up the asserts if more samplers were added.
> 
> No ill-effect in the current code base but lets fix it for correctness.
> 
> At the same time tidy the assert for readability and robustness.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Fixes: b46a33e271ed ("drm/i915/pmu: Expose a PMU interface for perf queries")
> ---
>  drivers/gpu/drm/i915/i915_pmu.c         | 22 +++++++++++++++-------
>  drivers/gpu/drm/i915/i915_pmu.h         |  2 ++
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  9 +++++----
>  3 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index b1cb2d3cae16..44a14ef1035c 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -599,7 +599,8 @@ static void i915_pmu_enable(struct perf_event *event)
>          * Update the bitmask of enabled events and increment
>          * the event reference counter.
>          */
> -       GEM_BUG_ON(bit >= I915_PMU_MASK_BITS);
> +       BUILD_BUG_ON(ARRAY_SIZE(i915->pmu.enable_count) != I915_PMU_MASK_BITS);
> +       GEM_BUG_ON(bit >= ARRAY_SIZE(i915->pmu.enable_count));
>         GEM_BUG_ON(i915->pmu.enable_count[bit] == ~0);
	GEM_BUG_ON(i915->pmu.enable_count[bit] == ~0); /* !overflow! */

Took me a moment to realise what you were checking. I was thinking
around the lines of an enable mask before I saw the count.

>         i915->pmu.enable |= BIT_ULL(bit);
>         i915->pmu.enable_count[bit]++;
> @@ -620,11 +621,16 @@ static void i915_pmu_enable(struct perf_event *event)
>                 engine = intel_engine_lookup_user(i915,
>                                                   engine_event_class(event),
>                                                   engine_event_instance(event));
> -               GEM_BUG_ON(!engine);
> -               engine->pmu.enable |= BIT(sample);
>  
> -               GEM_BUG_ON(sample >= I915_PMU_SAMPLE_BITS);
> +               BUILD_BUG_ON(ARRAY_SIZE(engine->pmu.enable_count) !=
> +                            I915_ENGINE_SAMPLE_COUNT ||
> +                            ARRAY_SIZE(engine->pmu.sample) !=
> +                            I915_ENGINE_SAMPLE_COUNT);

Ugh, a bit of a mouthful. Split into two at least it is less of a
towering cliff (and will be easier to understand should it ever fire).

Can apply the same rationale to split up the GEM_BUG_ON.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx