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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 17.05.23 um 19:03 schrieb Thomas Hellström:
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?

I'd prefer to export __drmm_mutex_release() from the module over the current static inline.

Best regards
Thomas


/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




--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux