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

[Noralf Trønnes]

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.

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.

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
>