| Message ID | 20190205102905.22716-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/pmu: Fix enable count array size and bounds checking | expand |
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
On 05/02/2019 16:44, Patchwork wrote: > == Series Details == > > Series: drm/i915/pmu: Fix enable count array size and bounds checking (rev2) > URL : https://patchwork.freedesktop.org/series/56220/ > State : success > > == Summary == > > CI Bug Log - changes from CI_DRM_5541 -> Patchwork_12140 > ==================================================== > > Summary > ------- > > **SUCCESS** > > No regressions found. > > External URL: https://patchwork.freedesktop.org/api/1.0/series/56220/revisions/2/mbox/ > > Known issues > ------------ > > Here are the changes found in Patchwork_12140 that come from known issues: > > ### IGT changes ### > > #### Issues hit #### > > * igt@kms_busy@basic-flip-a: > - fi-gdg-551: PASS -> FAIL [fdo#103182] > > * igt@kms_frontbuffer_tracking@basic: > - fi-byt-clapper: PASS -> FAIL [fdo#103167] > > * igt@kms_pipe_crc_basic@hang-read-crc-pipe-a: > - fi-byt-clapper: PASS -> FAIL [fdo#103191] / [fdo#107362] > > * igt@pm_rpm@basic-rte: > - fi-bsw-kefka: PASS -> FAIL [fdo#108800] > > > #### Possible fixes #### > > * igt@i915_module_load@reload: > - fi-blb-e6850: INCOMPLETE [fdo#107718] -> PASS > > * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence: > - fi-byt-clapper: FAIL [fdo#103191] / [fdo#107362] -> PASS > > * igt@pm_rpm@basic-pci-d3-state: > - fi-byt-j1900: {SKIP} [fdo#109271] -> PASS > > * igt@pm_rpm@basic-rte: > - fi-byt-j1900: FAIL [fdo#108800] -> PASS > > * igt@pm_rpm@module-reload: > - fi-skl-iommu: DMESG-WARN [fdo#109513] -> PASS > > > {name}: This element is suppressed. This means it is ignored when computing > the status of the difference (SUCCESS, WARNING, or FAILURE). > > [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167 > [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182 > [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191 > [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362 > [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718 > [fdo#108800]: https://bugs.freedesktop.org/show_bug.cgi?id=108800 > [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 > [fdo#109513]: https://bugs.freedesktop.org/show_bug.cgi?id=109513 > > > Participating hosts (50 -> 45) > ------------------------------ > > Additional (1): fi-skl-gvtdvm > Missing (6): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-bdw-samus > > > Build changes > ------------- > > * Linux: CI_DRM_5541 -> Patchwork_12140 > > CI_DRM_5541: 308fc30e2da36a06fd1732e2cb17e158b2df95bd @ git://anongit.freedesktop.org/gfx-ci/linux > IGT_4810: 1f89f1a04016cef20aa278ad05508cafdb9976f5 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > Patchwork_12140: 98d5212397e8ca115f9757b7cf20005094417c15 @ git://anongit.freedesktop.org/gfx-ci/linux > > > == Linux commits == > > 98d5212397e8 drm/i915/pmu: Fix enable count array size and bounds checking Pushed, thanks for the review! Regards, Tvrtko
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); 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); + GEM_BUG_ON(sample >= ARRAY_SIZE(engine->pmu.enable_count) || + sample >= ARRAY_SIZE(engine->pmu.sample)); GEM_BUG_ON(engine->pmu.enable_count[sample] == ~0); + + engine->pmu.enable |= BIT(sample); engine->pmu.enable_count[sample]++; } @@ -654,9 +660,11 @@ static void i915_pmu_disable(struct perf_event *event) engine = intel_engine_lookup_user(i915, engine_event_class(event), engine_event_instance(event)); - GEM_BUG_ON(!engine); - GEM_BUG_ON(sample >= I915_PMU_SAMPLE_BITS); + + GEM_BUG_ON(sample >= ARRAY_SIZE(engine->pmu.enable_count) || + sample >= ARRAY_SIZE(engine->pmu.sample)); GEM_BUG_ON(engine->pmu.enable_count[sample] == 0); + /* * Decrement the reference count and clear the enabled * bitmask when the last listener on an event goes away. @@ -665,7 +673,7 @@ static void i915_pmu_disable(struct perf_event *event) engine->pmu.enable &= ~BIT(sample); } - GEM_BUG_ON(bit >= I915_PMU_MASK_BITS); + GEM_BUG_ON(bit >= ARRAY_SIZE(i915->pmu.enable_count)); GEM_BUG_ON(i915->pmu.enable_count[bit] == 0); /* * Decrement the reference count and clear the enabled diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h index 7f164ca3db12..b3728c5f13e7 100644 --- a/drivers/gpu/drm/i915/i915_pmu.h +++ b/drivers/gpu/drm/i915/i915_pmu.h @@ -31,6 +31,8 @@ enum { ((1 << I915_PMU_SAMPLE_BITS) + \ (I915_PMU_LAST + 1 - __I915_PMU_OTHER(0))) +#define I915_ENGINE_SAMPLE_COUNT (I915_SAMPLE_SEMA + 1) + struct i915_pmu_sample { u64 cur; }; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 1398eb81dee6..4d4ea6963a72 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -403,16 +403,17 @@ struct intel_engine_cs { /** * @enable_count: Reference count for the enabled samplers. * - * Index number corresponds to the bit number from @enable. + * Index number corresponds to @enum drm_i915_pmu_engine_sample. */ - unsigned int enable_count[I915_PMU_SAMPLE_BITS]; + unsigned int enable_count[I915_ENGINE_SAMPLE_COUNT]; /** * @sample: Counter values for sampling events. * * Our internal timer stores the current counters in this field. + * + * Index number corresponds to @enum drm_i915_pmu_engine_sample. */ -#define I915_ENGINE_SAMPLE_MAX (I915_SAMPLE_SEMA + 1) - struct i915_pmu_sample sample[I915_ENGINE_SAMPLE_MAX]; + struct i915_pmu_sample sample[I915_ENGINE_SAMPLE_COUNT]; } pmu; /*