[Intel-gfx] [PATCH v8 02/12] drm/i915: Add i915 perf infrastructure

[Matthew Auld]

On 31 October 2016 at 16:27, Robert Bragg <robert at sixbynine.org> wrote:
>
>
> On Fri, Oct 28, 2016 at 3:27 PM, Matthew Auld
> <matthew.william.auld at gmail.com> wrote:
>>
>> > +/* Note we copy the properties from userspace outside of the i915 perf
>> > + * mutex to avoid an awkward lockdep with mmap_sem.
>> > + *
>> > + * Note this function only validates properties in isolation it doesn't
>> > + * validate that the combination of properties makes sense or that all
>> > + * properties necessary for a particular kind of stream have been set.
>> > + */
>> > +static int read_properties_unlocked(struct drm_i915_private *dev_priv,
>> > +                                   u64 __user *uprops,
>> > +                                   u32 n_props,
>> > +                                   struct perf_open_properties *props)
>> > +{
>> > +       u64 __user *uprop = uprops;
>> > +       int i;
>> > +
>> > +       memset(props, 0, sizeof(struct perf_open_properties));
>> > +
>> > +       if (!n_props) {
>> > +               DRM_ERROR("No i915 perf properties given");
>> > +               return -EINVAL;
>> > +       }
>> > +
>> > +       if (n_props > DRM_I915_PERF_PROP_MAX) {
>> Ah but DRM_I915_PERF_PROP_MAX is not a property itself.
>
>
> I'm not sure I follow what your implied concern is?
>
> This is just a sanity check for the number properties given by userspace,
> based on the assumption that there's currently no reason for multiple values
> with a particular property id.
>
All I meant was should it not be n_props >= DRM_I915_PERF_PROP_MAX ?

So with that fixed, or if I'm completely mad:
Reviewed-by: Matthew Auld <matthew.auld at intel.com>