From: Daniel Vetter <daniel.vetter@ffwll.ch> To: DRI Development <dri-devel@lists.freedesktop.org> Cc: David Airlie <airlied@linux.ie>, Daniel Vetter <daniel.vetter@ffwll.ch>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>, Chris Wilson <chris@chris-wilson.co.uk>, Thomas Zimmermann <tzimmermann@suse.de>, Daniel Vetter <daniel.vetter@intel.com> Subject: [PATCH] drm/fb-helper: Try to protect cleanup against delayed setup Date: Tue, 13 Jul 2021 15:59:22 +0200 [thread overview] Message-ID: <20210713135922.1384264-1-daniel.vetter@ffwll.ch> (raw) Some vague evidences suggests this can go wrong. Try to prevent it by holding the right mutex and clearing ->deferred_setup to make sure we later on don't accidentally try to re-register the fbdev when the driver thought it had it all cleaned up already. v2: I realized that this is fundamentally butchered, and CI complained about lockdep splats. So limit the critical section again and just add a few notes what the proper fix is. References: https://intel-gfx-ci.01.org/tree/linux-next/next-20201215/fi-byt-j1900/igt@i915_pm_rpm@module-reload.html Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> --- drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 9d82fda274eb..8f11e5abb222 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -598,6 +598,9 @@ EXPORT_SYMBOL(drm_fb_helper_alloc_fbi); * A wrapper around unregister_framebuffer, to release the fb_info * framebuffer device. This must be called before releasing all resources for * @fb_helper by calling drm_fb_helper_fini(). + * + * Note that this is fundamentally racy on hotunload because it doen't handle + * open fbdev file descriptors at all. Use drm_fbdev_generic_setup() instead. */ void drm_fb_helper_unregister_fbi(struct drm_fb_helper *fb_helper) { @@ -611,6 +614,9 @@ EXPORT_SYMBOL(drm_fb_helper_unregister_fbi); * @fb_helper: driver-allocated fbdev helper, can be NULL * * This cleans up all remaining resources associated with @fb_helper. + * + * Note that this is fundamentally racy on hotunload because it doen't handle + * open fbdev file descriptors at all. Use drm_fbdev_generic_setup() instead. */ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper) { @@ -2382,6 +2388,10 @@ static void drm_fbdev_client_unregister(struct drm_client_dev *client) { struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client); + mutex_lock(&fb_helper->lock); + fb_helper->deferred_setup = false; + mutex_unlock(&fb_helper->lock); + if (fb_helper->fbdev) /* drm_fbdev_fb_destroy() takes care of cleanup */ drm_fb_helper_unregister_fbi(fb_helper); -- 2.32.0
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel.vetter@ffwll.ch> To: DRI Development <dri-devel@lists.freedesktop.org> Cc: David Airlie <airlied@linux.ie>, Daniel Vetter <daniel.vetter@ffwll.ch>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>, Maxime Ripard <mripard@kernel.org>, Chris Wilson <chris@chris-wilson.co.uk>, Thomas Zimmermann <tzimmermann@suse.de>, Daniel Vetter <daniel.vetter@intel.com> Subject: [Intel-gfx] [PATCH] drm/fb-helper: Try to protect cleanup against delayed setup Date: Tue, 13 Jul 2021 15:59:22 +0200 [thread overview] Message-ID: <20210713135922.1384264-1-daniel.vetter@ffwll.ch> (raw) Some vague evidences suggests this can go wrong. Try to prevent it by holding the right mutex and clearing ->deferred_setup to make sure we later on don't accidentally try to re-register the fbdev when the driver thought it had it all cleaned up already. v2: I realized that this is fundamentally butchered, and CI complained about lockdep splats. So limit the critical section again and just add a few notes what the proper fix is. References: https://intel-gfx-ci.01.org/tree/linux-next/next-20201215/fi-byt-j1900/igt@i915_pm_rpm@module-reload.html Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> --- drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 9d82fda274eb..8f11e5abb222 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -598,6 +598,9 @@ EXPORT_SYMBOL(drm_fb_helper_alloc_fbi); * A wrapper around unregister_framebuffer, to release the fb_info * framebuffer device. This must be called before releasing all resources for * @fb_helper by calling drm_fb_helper_fini(). + * + * Note that this is fundamentally racy on hotunload because it doen't handle + * open fbdev file descriptors at all. Use drm_fbdev_generic_setup() instead. */ void drm_fb_helper_unregister_fbi(struct drm_fb_helper *fb_helper) { @@ -611,6 +614,9 @@ EXPORT_SYMBOL(drm_fb_helper_unregister_fbi); * @fb_helper: driver-allocated fbdev helper, can be NULL * * This cleans up all remaining resources associated with @fb_helper. + * + * Note that this is fundamentally racy on hotunload because it doen't handle + * open fbdev file descriptors at all. Use drm_fbdev_generic_setup() instead. */ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper) { @@ -2382,6 +2388,10 @@ static void drm_fbdev_client_unregister(struct drm_client_dev *client) { struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client); + mutex_lock(&fb_helper->lock); + fb_helper->deferred_setup = false; + mutex_unlock(&fb_helper->lock); + if (fb_helper->fbdev) /* drm_fbdev_fb_destroy() takes care of cleanup */ drm_fb_helper_unregister_fbi(fb_helper); -- 2.32.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next reply other threads:[~2021-07-13 13:59 UTC|newest] Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-13 13:59 Daniel Vetter [this message] 2021-07-13 13:59 ` [Intel-gfx] [PATCH] drm/fb-helper: Try to protect cleanup against delayed setup Daniel Vetter 2021-07-13 16:43 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork 2021-07-13 17:09 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 2021-07-13 20:21 ` [PATCH] " Sam Ravnborg 2021-07-13 20:21 ` [Intel-gfx] " Sam Ravnborg 2021-07-13 23:25 ` [Intel-gfx] ✗ Fi.CI.IGT: failure for " Patchwork 2021-07-15 14:18 ` [PATCH] " Thomas Zimmermann 2021-07-15 14:18 ` [Intel-gfx] " Thomas Zimmermann 2022-11-15 9:30 ` Andrzej Hajda 2022-11-15 9:30 ` Andrzej Hajda 2022-11-16 9:31 ` Daniel Vetter 2022-11-16 9:31 ` Daniel Vetter 2023-02-17 19:47 Daniel Vetter 2023-02-20 12:43 ` Thomas Zimmermann
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=20210713135922.1384264-1-daniel.vetter@ffwll.ch \ --to=daniel.vetter@ffwll.ch \ --cc=airlied@linux.ie \ --cc=chris@chris-wilson.co.uk \ --cc=daniel.vetter@intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --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: linkBe 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.