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
> >
> >
