[PATCH 11/11] drm/fb-helper: generic: Don't take module ref for fbcon

[Daniel Vetter]

On Mon, Jan 28, 2019 at 03:40:47PM +0100, Noralf Trønnes wrote:
> 
> 
> Den 21.01.2019 10.05, skrev Daniel Vetter:
> > On Sun, Jan 20, 2019 at 12:43:18PM +0100, Noralf Trønnes wrote:
> >> It's now safe to let fbcon unbind automatically on fbdev unregister.
> >> The crash problem was fixed in commit 2122b40580dd
> >> ("fbdev: fbcon: Fix unregister crash when more than one framebuffer")
> >>
> >> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
> >> ---
> >>  drivers/gpu/drm/drm_fb_helper.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >> index 31fcf94bf825..5d0327f603bb 100644
> >> --- a/drivers/gpu/drm/drm_fb_helper.c
> >> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >> @@ -2999,7 +2999,8 @@ static int drm_fbdev_fb_open(struct fb_info *info, int user)
> >>  {
> >>  	struct drm_fb_helper *fb_helper = info->par;
> >>  
> >> -	if (!try_module_get(fb_helper->dev->driver->fops->owner))
> >> +	/* No need to take a ref for fbcon because it unbinds on unregister */
> >> +	if (user && !try_module_get(fb_helper->dev->driver->fops->owner))
> > 
> > Module refcount != driver refcount. You can always unbind a driver through
> > the sysfs unbind file, no matter whether the module is pinned or not. So I
> > think pinng the module is still the right thing to do, just to avoid races
> > and fun stuff.
> > 
> > btw, I looked into making fbdev hotunplug safe (i.e. drm_dev_enter/exit,
> > but for fbdev) over the holidays. Fixing that properly for fbdev userspace
> > clients looks like serious amounts of work, and I think it's impossible
> > for fbcon. Or at least I didn't come up with a workable idea to sprinkle
> > the dev_enter/exit stuff around. For the userspace side the basic
> > infrastructure with get_fb_info() and put_fb_info() is there already.
> > 
> 
> fbdev blocks file operations after unregister through this function:
> 
> static struct fb_info *file_fb_info(struct file *file)
> {
> 	struct inode *inode = file_inode(file);
> 	int fbidx = iminor(inode);
> 	struct fb_info *info = registered_fb[fbidx];
> 
> 	if (info != file->private_data)
> 		info = NULL;
> 	return info;
> }
> 
> static int do_unregister_framebuffer(struct fb_info *fb_info)
> {
> ...
> 	registered_fb[fb_info->node] = NULL;
> ...
> }
> 
> The only way for fbdev userspace to trigger driver actions is through
> mmap and defio. At first I planned to put drm_dev_enter/exit in
> drm_fb_helper_dirty_work(), but realised that the driver would need to
> do the same in it's dirty function to cover DRM userspace anyways, so I
> dropped it. It's the driver's responsibility to protect it's resources.

The above is all about the driver refcounting. Which does exist (but races
badly) for fbdev. The problem is you're changing the module refcount,
which is there to make sure the functions don't disappear. Note that on
the drm side we do the exact same thing, so least for symmetry reasons I
think we should keep it. The refcounting is done by the file open/close
functions since we set fops->owner, not drm code itself, but it's there.
 
> As for fbcon, yes this is all a spaghetti monster, so better safe than
> sorry I guess. And it's just developers that would benefit from this
> anyways, not having to unbind fbcon before unlaoding the driver module.

Hm yeah, make sense. On second thought, on this patch:

Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
-Daniel

> 
> Noralf.
> 
> > Cheers, Daniel
> > 
> >>  		return -ENODEV;
> >>  
> >>  	return 0;
> >> @@ -3009,7 +3010,8 @@ static int drm_fbdev_fb_release(struct fb_info *info, int user)
> >>  {
> >>  	struct drm_fb_helper *fb_helper = info->par;
> >>  
> >> -	module_put(fb_helper->dev->driver->fops->owner);
> >> +	if (user)
> >> +		module_put(fb_helper->dev->driver->fops->owner);
> >>  
> >>  	return 0;
> >>  }
> >> -- 
> >> 2.20.1
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch