All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Danilo Krummrich" <dakr@kernel.org>
To: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
Cc: "Alice Ryhl" <aliceryhl@google.com>,
	"Matthew Brost" <matthew.brost@intel.com>,
	"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	<dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] drm/gpuvm: take refcount on DRM device
Date: Fri, 17 Apr 2026 21:33:31 +0200	[thread overview]
Message-ID: <DHVOJ6ILGSLK.3OWKA4RV2HB5C@kernel.org> (raw)
In-Reply-To: <544c97fe296f39da35e5349ba1fc0af05f2ff643.camel@linux.intel.com>

On Fri Apr 17, 2026 at 4:41 PM CEST, Thomas Hellström wrote:
> This is problematic since typically you also need a module reference
> when taking a drm device reference.
>
> The reason for this is that the devres reference on the drm device
> expects to be the last one, since it might be called from the module
> exit function of the driver.

No, this is not how it works; if this would be true then drmm_* would be pretty
pointless in the first place, as one could just use devm_* for everything.

Citing the commit introducing drmm_* APIs:

	"The biggest wrong pattern is that developers use devm_, which ties the
	release action to the underlying struct device, whereas all the
	userspace visible stuff attached to a drm_device can long outlive that
	one (e.g. after a hotunplug while userspace has open files and mmap'ed
	buffers)."

> Now if there is an additional reference held at that point the driver module
> can be unloaded with a dangling reference to the drm device.
>
> On the other hand, if you in addition take a module reference then that
> blocks the driver module from being unloaded while held, just like a
> drm file reference. This leads to complicated module release schemes
> like the one in drm_pagemap where the module refcount is released from
> a work item that is waited on in the drm_pagemap exit function.
>
> I'm working to lift the module refcount requirement, but meanwhile I'd
> recommend that in the file close callback, we'd make sure all
> drm_gpuvms have called their drm_gpuvm_free() function, because then we
> are sure that the drm_device is still alive and the module still
> pinned.

If GPUVM has a pointer to the DRM device, it implies shared ownership and hence
GPUVM should account for this shared ownership and take a reference count.

The fact that GPUVM must not outlive module unload when it has driver callbacks
attached is an orthogonal requirement.

The module lifetime / callback issue is a separate problem that exists
regardless of whether you hold a device refcount. Not taking the refcount
doesn't make the module problem go away, it just adds a second, independent bug.

If struct drm_device itself, e.g. due to drm_dev_release() requires a module
refcount, then this is on struct drm_device to ensure this constraint (or remove
the requirement).

IOW, if I get to choose between a DRM component that has a pointer to a DRM
device stalls module unload and a DRM component that has a pointer to a DRM
device oopses the kernel when used wrongly, I prefer the former.

- Danilo

  reply	other threads:[~2026-04-17 19:33 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-16 13:10 [PATCH] drm/gpuvm: take refcount on DRM device Alice Ryhl
2026-04-16 15:26 ` Danilo Krummrich
2026-04-17 14:41 ` Thomas Hellström
2026-04-17 19:33   ` Danilo Krummrich [this message]
2026-04-20  9:28     ` Thomas Hellström
2026-04-20 15:08       ` Danilo Krummrich
2026-04-20 16:19         ` Thomas Hellström
2026-04-27  7:34           ` Alice Ryhl
2026-04-27 11:44             ` Thomas Hellström
2026-05-05 11:06 ` Danilo Krummrich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DHVOJ6ILGSLK.3OWKA4RV2HB5C@kernel.org \
    --to=dakr@kernel.org \
    --cc=aliceryhl@google.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=matthew.brost@intel.com \
    --cc=mripard@kernel.org \
    --cc=thomas.hellstrom@linux.intel.com \
    --cc=tzimmermann@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.