Re: [Intel-xe] [PATCH v5 1/7] drm: fix drmm_mutex_init()

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Wed, 2023-05-17 at 17:29 +0100, Matthew Auld wrote:
> On 17/05/2023 17:05, Stanislaw Gruszka wrote:
> > On Wed, May 17, 2023 at 04:22:38PM +0100, Matthew Auld wrote:
> > > In mutex_init() lockdep seems to identify a lock by defining a
> > > static
> > > key for each lock class. However if we wrap the whole thing in a
> > > function the static key will be the same for everything calling
> > > that
> > > function, which looks to be the case for drmm_mutex_init(). This
> > > then
> > > results in impossible lockdep splats since lockdep thinks
> > > completely
> > > unrelated locks are the same lock class. The other issue is that
> > > when
> > > looking at splats we lose the actual lock name, where instead of
> > > seeing
> > > something like xe->mem_access.lock for the name, we just see
> > > something
> > > generic like lock#8.
> > > 
> > > Attempt to fix this by converting drmm_mutex_init() into a macro,
> > > which
> > > should ensure that mutex_init() behaves as expected.
> > 
> > Nice catch :-) we observed lockdep deadlock false alarms too, but I
> > could
> > not spot it out and were adding lockdep_set_class(key) to avoid
> > those.
> > 
> > 
> > > +static inline void __drmm_mutex_release(struct drm_device *dev,
> > > void *res)
> > 
> > Can this be inline if used in drmm_add_action_or_reset() ?
> 
> I think so. Did I missing something here? It at least builds for me.

I think in each file that contains a drmm_mutex_init(), the code will
need a pointer to the function __drmm_mutex_release() and the compiler
will therefore need to emit a non-inlined static version of the
function in that file. Not sure if that's a problem, though. If so
could make it extern?

/Thomas



> 
> > 
> > 
> > > +{
> > > +       struct mutex *lock = res;
> > > +
> > > +       mutex_destroy(lock);
> > > +}
> > > +
> > > +/**
> > > + * drmm_mutex_init - &drm_device-managed mutex_init()
> > > + * @dev: DRM device
> > > + * @lock: lock to be initialized
> > > + *
> > > + * Returns:
> > > + * 0 on success, or a negative errno code otherwise.
> > > + *
> > > + * This is a &drm_device-managed version of mutex_init(). The
> > > initialized
> > > + * lock is automatically destroyed on the final drm_dev_put().
> > > + */
> > > +#define drmm_mutex_init(dev, lock)
> > > ({                                       \
> > > +       mutex_init(lock);                                        
> > >             \
> > > +       drmm_add_action_or_reset(dev, __drmm_mutex_release,
> > > lock);           \
> > > +})                                                              
> > >             \
> > 
> > Regards
> > Stanislaw
> > 
> >