Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 25 Nov 2020 14:45:57 -0500
From:   Jeremy Cline <jcline@redhat.com>
To:     Lyude Paul <lyude@redhat.com>
Cc:     Ben Skeggs <bskeggs@redhat.com>, Karol Herbst <kherbst@redhat.com>,
        David Airlie <airlied@linux.ie>, nouveau@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] drm/nouveau: Add a dedicated mutex for the clients
 list
Message-ID: <20201125194557.GC20747@xps13>
References: <20201103194912.184413-1-jcline@redhat.com>
 <20201103194912.184413-3-jcline@redhat.com>
 <505be3af57c36222564d0790aa8a992b1ea4d287.camel@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <505be3af57c36222564d0790aa8a992b1ea4d287.camel@redhat.com>
Precedence: bulk

On Wed, Nov 25, 2020 at 01:37:06PM -0500, Lyude Paul wrote:
> On Tue, 2020-11-03 at 14:49 -0500, Jeremy Cline wrote:
> > Rather than protecting the nouveau_drm clients list with the lock within
> > the "client" nouveau_cli, add a dedicated lock to serialize access to
> > the list. This is both clearer and necessary to avoid lockdep being
> > upset with us when we need to iterate through all the clients in the
> > list and potentially lock their mutex, which is the same class as the
> > lock protecting the entire list.
> > 
> > Signed-off-by: Jeremy Cline <jcline@redhat.com>
> > ---
> >  drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +++++----
> >  drivers/gpu/drm/nouveau/nouveau_drv.h | 5 +++++
> >  2 files changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index 4fe4d664c5f2..d182b877258a 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -557,6 +557,7 @@ nouveau_drm_device_init(struct drm_device *dev)
> >                 nvkm_dbgopt(nouveau_debug, "DRM");
> >  
> >         INIT_LIST_HEAD(&drm->clients);
> > +       mutex_init(&drm->clients_lock);
> 
> Looks like you forgot to hook up mutex_destroy() somewhere. Note there's
> actually plenty of code in nouveau right now that forgets to do this, but at
> some point we should probably fix that and avoid adding more spots where there's
> no mutex_destroy().
> 

I'm guilty of having looked at the existing locking init code in nouveau
and modeling this work accordingly. I'll send out a fix for this shortly
and look at tidying up the rest of the locks in a separate series.
Thanks!

> >         spin_lock_init(&drm->tile.lock);
> >  
> >         /* workaround an odd issue on nvc1 by disabling the device's
> > @@ -1089,9 +1090,9 @@ nouveau_drm_open(struct drm_device *dev, struct drm_file
> > *fpriv)
> >  
> >         fpriv->driver_priv = cli;
> >  
> > -       mutex_lock(&drm->client.mutex);
> > +       mutex_lock(&drm->clients_lock);
> >         list_add(&cli->head, &drm->clients);
> > -       mutex_unlock(&drm->client.mutex);
> > +       mutex_unlock(&drm->clients_lock);
> >  
> >  done:
> >         if (ret && cli) {
> > @@ -1117,9 +1118,9 @@ nouveau_drm_postclose(struct drm_device *dev, struct
> > drm_file *fpriv)
> >                 nouveau_abi16_fini(cli->abi16);
> >         mutex_unlock(&cli->mutex);
> >  
> > -       mutex_lock(&drm->client.mutex);
> > +       mutex_lock(&drm->clients_lock);
> >         list_del(&cli->head);
> > -       mutex_unlock(&drm->client.mutex);
> > +       mutex_unlock(&drm->clients_lock);
> >  
> >         nouveau_cli_fini(cli);
> >         kfree(cli);
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
> > b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > index 9d04d1b36434..550e5f335146 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > @@ -141,6 +141,11 @@ struct nouveau_drm {
> >  
> >         struct list_head clients;
> >  
> > +       /**
> > +        * @clients_lock: Protects access to the @clients list of &struct
> > nouveau_cli.
> > +        */
> > +       struct mutex clients_lock;
> > +
> >         u8 old_pm_cap;
> >  
> >         struct {
> 
> -- 
> Sincerely,
>    Lyude Paul (she/her)
>    Software Engineer at Red Hat
>    
> Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
> asked me a question, are waiting for a review/merge on a patch, etc. and I
> haven't responded in a while, please feel free to send me another email to check
> on my status. I don't bite!
>