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@xxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
