Re: [PATCH 3/6] drm: Define a conditional guard for drm_dev_{enter,exit}()

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 18 May 2026 11:16:55 +0200
Christian König <christian.koenig@amd.com> wrote:

> On 5/18/26 10:28, Boris Brezillon wrote:
> > On Thu, 14 May 2026 11:34:52 -0700
> > Chia-I Wu <olvaffe@gmail.com> wrote:
> >   
> >> On Wed, May 13, 2026 at 10:24 AM Boris Brezillon
> >> <boris.brezillon@collabora.com> wrote:  
> >>>
> >>> Define a conditional drm_dev_access guard to automate the
> >>> drm_dev_{enter,exit}() sequence.
> >>>
> >>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> >>> ---
> >>>  include/drm/drm_drv.h | 9 +++++++++
> >>>  1 file changed, 9 insertions(+)
> >>>
> >>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> >>> index 42fc085f986d..79d1958f93e4 100644
> >>> --- a/include/drm/drm_drv.h
> >>> +++ b/include/drm/drm_drv.h
> >>> @@ -490,6 +490,15 @@ void drm_dev_unplug(struct drm_device *dev);
> >>>  int drm_dev_wedged_event(struct drm_device *dev, unsigned long method,
> >>>                          struct drm_wedge_task_info *info);
> >>>
> >>> +/*
> >>> + * Only the conditional drm_dev_access guard is valid. The drm_dev one is
> >>> + * here so we can extend it with a conditional variant.
> >>> + */
> >>> +DEFINE_LOCK_GUARD_1(drm_dev, struct drm_device,
> >>> +                   { WARN_ON("Use cond guards"); _T->idx = -1; },
> >>> +                   drm_dev_exit(_T->idx), int idx);    
> >> If this is ever mis-used, drm_dev_exit(-1) seems to cause OOB access.
> >> Is BUG more appropriate than WARN_ON?  
> > 
> > I actually had
> > 
> > 			if (_T->idx >= 0) drm_dev_exit(_T->idx),
> > 
> > at some point, and I ditched it thinking the WARN_ON_ONCE()
> > in srcu_read_unlock() would cover for that. I can add it back, of
> > course.
> > 
> > I'd be fine with a BUG_ON() too, but every time I tried to add one I've
> > been encouraged to handle the unexpected case instead.
> > 
> > Ideally, we would have a DEFINE_LOCK_GUARD_COND() variant that, instead
> > of expanding a non-conditional one, would define the whole thing so
> > that the non-conditional variant is never exposed.  
> 
> Would it be possible to use BUILD_BUG() here?

Ah, nice! I was searching for this kind of compile-time assert that
would trigger if the code is used, and BUILD_BUG() indeed does what we
want. Thanks for the tip.