linux-kernel - Re: [PATCH] drm/nouveau: add lockdep annotations

Open Source and information security mailing list archives

Date:	Wed, 6 Feb 2013 06:52:55 +1000
From:	Ben Skeggs <skeggsb@...il.com>
To:	Maarten Lankhorst <maarten.lankhorst@...onical.com>
Cc:	Marcin Slusarz <marcin.slusarz@...il.com>,
	Peter Hurley <peter@...leysoftware.com>,
	nouveau@...ts.freedesktop.org,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	dri-devel@...ts.freedesktop.org,
	Daniel J Blueman <daniel@...ra.org>,
	Ben Skeggs <bskeggs@...hat.com>,
	Arend van Spriel <arend@...adcom.com>
Subject: Re: [PATCH] drm/nouveau: add lockdep annotations

On Mon, Feb 04, 2013 at 10:59:28PM +0100, Maarten Lankhorst wrote:
> Op 04-02-13 22:30, Marcin Slusarz schreef:
> > 1) Lockdep thinks all nouveau subdevs belong to the same class and can be
> > locked in arbitrary order, which is not true (at least in general case).
> > Tell it to distinguish subdevs by (o)class type.
> Apart from this specific case, is there any other reason why we require being able to nest 2 subdev locks?
I think I tend to prefer Marcin's fix for this actually.  The subdev's
are completely separate classes of objects and as interaction between
them increases (PM will be very much like this), we may very well
require holding multiple subdev mutexes at once.

Ben.

> 
> Add a NVOBJ_FLAG_CREATE_EXCLUSIVE flag to nouveau_engctx_create and return -EBUSY if there is any existing object. Problem solved, and lockdep is still generic.
> 
> > 2) DRM client can be locked under user client lock - tell lockdep to put DRM
> > client lock in a separate class.
> Can you copy paste a typical lockdep splat for this? Also this should be a separate patch. :-)
> 
> 
> > Reported-by: Arend van Spriel <arend@...adcom.com>
> > Reported-by: Peter Hurley <peter@...leysoftware.com>
> > Reported-by: Maarten Lankhorst <maarten.lankhorst@...onical.com>
> > Reported-by: Daniel J Blueman <daniel@...ra.org>
> > Signed-off-by: Marcin Slusarz <marcin.slusarz@...il.com>
> > Cc: stable@...r.kernel.org [3.7, but needs s/const ofuncs/ofuncs/ to build]
> > ---
> > Lightly tested, only on NV4B and NVC1.
> > ---
> >  drivers/gpu/drm/nouveau/core/core/subdev.c         | 2 +-
> >  drivers/gpu/drm/nouveau/core/include/core/object.h | 7 +++++--
> >  drivers/gpu/drm/nouveau/nouveau_drm.c              | 3 +++
> >  3 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/core/core/subdev.c b/drivers/gpu/drm/nouveau/core/core/subdev.c
> > index f74c30a..48f0637 100644
> > --- a/drivers/gpu/drm/nouveau/core/core/subdev.c
> > +++ b/drivers/gpu/drm/nouveau/core/core/subdev.c
> > @@ -99,7 +99,7 @@ nouveau_subdev_create_(struct nouveau_object *parent,
> >  	if (ret)
> >  		return ret;
> >  
> > -	mutex_init(&subdev->mutex);
> > +	__mutex_init(&subdev->mutex, subname, &oclass->lock_class_key);
> >  	subdev->name = subname;
> >  
> >  	if (parent) {
> > diff --git a/drivers/gpu/drm/nouveau/core/include/core/object.h b/drivers/gpu/drm/nouveau/core/include/core/object.h
> > index 6a90267..62e68ba 100644
> > --- a/drivers/gpu/drm/nouveau/core/include/core/object.h
> > +++ b/drivers/gpu/drm/nouveau/core/include/core/object.h
> > @@ -50,10 +50,13 @@ int  nouveau_object_fini(struct nouveau_object *, bool suspend);
> >  
> >  extern struct nouveau_ofuncs nouveau_object_ofuncs;
> >  
> > +/* Don't allocate dynamically, because lockdep needs lock_class_keys to be in
> > + * ".data". */
> >  struct nouveau_oclass {
> >  	u32 handle;
> > -	struct nouveau_ofuncs *ofuncs;
> > -	struct nouveau_omthds *omthds;
> > +	struct nouveau_ofuncs * const ofuncs;
> > +	struct nouveau_omthds * const omthds;
> > +	struct lock_class_key lock_class_key;
> >  };
> >  
> >  #define nv_oclass(o)    nv_object(o)->oclass
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index ef1ad21..bc00587 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -245,6 +245,8 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> >  	return 0;
> >  }
> >  
> > +static struct lock_class_key drm_client_lock_class_key;
> > +
> >  static int
> >  nouveau_drm_load(struct drm_device *dev, unsigned long flags)
> >  {
> > @@ -256,6 +258,7 @@ nouveau_drm_load(struct drm_device *dev, unsigned long flags)
> >  	ret = nouveau_cli_create(pdev, "DRM", sizeof(*drm), (void**)&drm);
> >  	if (ret)
> >  		return ret;
> > +	lockdep_set_class(&drm->client.mutex, &drm_client_lock_class_key);
> >  
> >  	dev->dev_private = drm;
> >  	drm->dev = dev;
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives