Den 12.11.2019 18.50, skrev Daniel Vetter:
> There's a pile of things protecting against racing hotunplugs:
> - drm_dev_enter/exit against the underlying device disappearing
> - drm_dev_get/put against drm_device disappearing
> - we unregister everything in drm_dev_unregister to stop userspace from
> creating new open files to access the drm_device
> - fbcon gets synchronously torn down (the console_lock is huge, so there's
> an upside to monolithic locking here).
> - and for fbdev there's get_fb_info and put_fb_info
>
> And I think we've almost got this right, except for a tiny thing: An fbdev
> ioctl might hold an elevate fb_info reference, which prevents us from
> doing the final cleanup in drm_fbdev_release. But it doesn't prevent
> the final cleanup of fbhelper->dev, which means even if the driver protects
> all hw access with drm_dev_enter/exit, we might oops way before that when
> trying to access freed memory.
>
> Fix this by holding a drm_device reference for fbdev, which we drop from
> drm_fbdev_release.
>
> This might lead to a reference loop, where the drm_device won't get cleaned
> up because of the fbdev reference, and fbdev isn't torn down yet because
> the drm_device cleanup code is written the wrong way round. But I think
> all drivers using the generic fbdev code get this right (because they use
> drm_client, so really can't get it wrong), hence this should be ok.
>
> Also, more reasons to switch drivers over to generic fbdev support.
>
> Since I had to re-review a pile of code also add a comment about why
> drm_fbdev_client_unregister can't race.
>
> Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> Cc: Noralf Trønnes <noralf@xxxxxxxxxxx>
> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> --
> Entirely untested (aside from it compiles), but I think we need
> something like this to completely plug all fbdev related hotunplug
> issues. No need to roll out some fancy srcu scheme to fbdev, this
> seems to be solved with refcounting already.
We already hold a ref on drm_device taken in drm_client_init().
Hotunplug is working for generic fbdev, fbcon is automatically unbound
and if userspace is open teardown is delayed until the fd is closed.
This comment here gives some hints:
/*
* fb_ops.fb_destroy is called by the last put_fb_info() call at the end of
* unregister_framebuffer() or fb_release().
*/
static void drm_fbdev_fb_destroy(struct fb_info *info)
{
drm_fbdev_release(info->par);
}
Noralf.
> -Daniel
> ---
> drivers/gpu/drm/drm_fb_helper.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 0ec98e046b59..62a1981d369e 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1992,6 +1992,7 @@ static void drm_fbdev_cleanup(struct drm_fb_helper *fb_helper)
>
> static void drm_fbdev_release(struct drm_fb_helper *fb_helper)
> {
> + drm_dev_put(fb_helper->dev);
> drm_fbdev_cleanup(fb_helper);
> drm_client_release(&fb_helper->client);
> kfree(fb_helper);
> @@ -2123,6 +2124,7 @@ static void drm_fbdev_client_unregister(struct drm_client_dev *client)
> {
> struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
>
> + /* Protected against races by the clientlist_mutex. */
> if (fb_helper->fbdev)
> /* drm_fbdev_fb_destroy() takes care of cleanup */
> drm_fb_helper_unregister_fbi(fb_helper);
> @@ -2243,6 +2245,8 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
> preferred_bpp = 32;
> fb_helper->preferred_bpp = preferred_bpp;
>
> + drm_dev_get(dev);
> +
> ret = drm_fbdev_client_hotplug(&fb_helper->client);
> if (ret)
> DRM_DEV_DEBUG(dev->dev, "client hotplug ret=%d\n", ret);
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
