Re: [igt-dev] [PATCH 1/7] drm/leases: Drop object_id validation for negative ids

From: Daniel Vetter <daniel@ffwll.ch>
To: Dave Airlie <airlied@gmail.com>
Cc: Keith Packard <keithp@keithp.com>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	IGT development <igt-dev@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [igt-dev] [PATCH 1/7] drm/leases: Drop object_id validation for negative ids
Date: Fri, 29 Mar 2019 09:28:32 +0100	[thread overview]
Message-ID: <20190329082832.GB2665@phenom.ffwll.local> (raw)
In-Reply-To: <CAPM=9txUBdYvGiakPzsTHuqSpPPCex2FVfNpGzA_bb3P1oev_w@mail.gmail.com>

On Fri, Mar 29, 2019 at 02:46:55PM +1000, Dave Airlie wrote:
> commit 2e1c9b2867656ff9a469d23e1dfe90cf77ec0c72
> Author: Tejun Heo <tj@kernel.org>
> Date:   Fri Mar 8 12:43:30 2013 -0800
> 
>     idr: remove WARN_ON_ONCE() on negative IDs
> 
> We used to WARN_ON if we hit a negative id, it appears we don't
> anymore, so just update the commit msg to reflect that info on where
> the code came from originally.
> 
> You had me wondering if I'd been dreaming up reasons for Keith to add code :-P

Hm right, looking around I think another relevant commit is

commit 322d884ba731e05ca79ae58e9dee1ef7dc4de504
Author: Matthew Wilcox <mawilcox@microsoft.com>
Date:   Tue Nov 28 10:01:24 2017 -0500

    idr: Delete idr_find_ext function
    
    Simply changing idr_remove's 'id' argument to 'unsigned long' works
    for all callers.
    
    Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>

I don't remember any such checks for the kms or gem idr lookups, so still
no idea why Keith typed this. The warning is gone since 5+ years after all
(and drm lease isn't that old).
-Daniel

> 
> Dave.
> 
> On Thu, 14 Mar 2019 at 17:54, Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > On Thu, 28 Feb 2019 15:49:04 +0100
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > > Not exactly sure what's the aim here, but the canonical nil object has
> > > id == 0, we don't use negative object ids for anything. Plus all
> > > object_id are valided by the object_idr, there's nothing we need to do
> > > on top of that ENOENT check a bit further down.
> > >
> > > Spotted while typing exhaustive igt coverage for all these
> > > corner-cases.
> > >
> > > Cc: Keith Packard <keithp@keithp.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >
> > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> >
> > > ---
> > >  drivers/gpu/drm/drm_lease.c | 5 -----
> > >  1 file changed, 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> > > index 603b0bd9c5ce..1176d814cf7f 100644
> > > --- a/drivers/gpu/drm/drm_lease.c
> > > +++ b/drivers/gpu/drm/drm_lease.c
> > > @@ -403,11 +403,6 @@ static int fill_object_idr(struct drm_device *dev,
> > >       /* step one - get references to all the mode objects
> > >          and check for validity. */
> > >       for (o = 0; o < object_count; o++) {
> > > -             if ((int) object_ids[o] < 0) {
> > > -                     ret = -EINVAL;
> > > -                     goto out_free_objects;
> > > -             }
> > > -
> > >               objects[o] = drm_mode_object_find(dev, lessor_priv,
> > >                                                 object_ids[o],
> > >                                                 DRM_MODE_OBJECT_ANY);
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev