From patchwork Wed Jul 4 15:31:19 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Noralf_Tr=C3=B8nnes?= X-Patchwork-Id: 10507257 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 66918603D7 for ; Wed, 4 Jul 2018 15:31:27 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5633228D2E for ; Wed, 4 Jul 2018 15:31:27 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4A70E28DC1; Wed, 4 Jul 2018 15:31:27 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id BDFD128DBB for ; Wed, 4 Jul 2018 15:31:26 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id CD2E66EC02; Wed, 4 Jul 2018 15:31:25 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from smtp.domeneshop.no (smtp.domeneshop.no [IPv6:2a01:5b40:0:3005::1]) by gabe.freedesktop.org (Postfix) with ESMTPS id 141AA6EC02 for ; Wed, 4 Jul 2018 15:31:25 +0000 (UTC) Received: from 211.81-166-168.customer.lyse.net ([81.166.168.211]:52662 helo=[192.168.10.173]) by smtp.domeneshop.no with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from ) id 1fajkZ-0002Ij-3W; Wed, 04 Jul 2018 17:31:23 +0200 Subject: Re: [PATCH] fb: fix lost console when the user unplugs a USB adapter To: Mikulas Patocka , Bartlomiej Zolnierkiewicz References: <5091035.TYlAUG7jeO@amdc3058> From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Message-ID: <7861c063-6494-7cd8-683d-02c61f9faa1f@tronnes.org> Date: Wed, 4 Jul 2018 17:31:19 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Dave Airlie , linux-fbdev@vger.kernel.org, Ladislav Michl , Bernie Thompson , dri-devel@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP Den 03.07.2018 19.18, skrev Mikulas Patocka: > > On Tue, 3 Jul 2018, Bartlomiej Zolnierkiewicz wrote: > >> Hi, >> >> On Sunday, June 03, 2018 11:46:29 AM Mikulas Patocka wrote: >>> I have a USB display adapter using the udlfb driver and I use it on an ARM >>> board that doesn't have any graphics card. When I plug the adapter in, the >>> console is properly displayed, however when I unplug and re-plug the >>> adapter, the console is not displayed and I can't access it until I reboot >>> the board. >>> >>> The reason is this: >>> When the adapter is unplugged, dlfb_usb_disconnect calls >>> unlink_framebuffer, then it waits until the reference count drops to zero >>> and then it deallocates the framebuffer. However, the console that is >>> attached to the framebuffer device keeps the reference count non-zero, so >>> the framebuffer device is never destroyed. When the USB adapter is plugged >>> again, it creates a new device /dev/fb1 and the console is not attached to >>> it. >>> >>> This patch fixes the bug by unbinding the console from unlink_framebuffer. >>> The code to unbind the console is moved from do_unregister_framebuffer to >>> a function unbind_console. When the console is unbound, the reference >>> count drops to zero and the udlfb driver frees the framebuffer. When the >>> adapter is plugged back, a new framebuffer is created and the console is >>> attached to it. >>> >>> Signed-off-by: Mikulas Patocka >>> Cc: stable@vger.kernel.org >> After this change unbind_console() will be called twice in the standard >> framebuffer unregister path: >> >> - first time, directly by do_unregister_framebuffer() >> >> - second time, indirectly by do_unregister_framebuffer()->unlink_framebuffer() >> >> This doesn't look correctly. > unbind_console calls the FB_EVENT_FB_UNBIND notifier, FB_EVENT_FB_UNBIND > goes to the function fbcon_fb_unbind and fbcon_fb_unbind checks if the > console is bound to the framebuffer for which unbind is requested. So a > double call won't cause any trouble. > >> Also why can't udlfb just use unregister_framebuffer() like all other >> drivers (it uses unlink_framebuffer() and it is the only user of this >> helper)? > It uses unregister_framebuffer() - but - unregister_framebuffer() may only > be called when the open count of the framebuffer is zero. AFAIU calling unregister_framebuffer() with open fd's is just fine as long as fb_info with buffers stay intact. All it does is to remove the fbX from userspace. Cleanup can be done in fb_ops->fb_destroy. I have been working on generic fbdev emulation for DRM [1] and I did a test now to see what would happen if I did unbind the driver from the device. It worked as expected if I didn't have another fbdev present, but if there is an fb0 and I remove fb1 with a console on it, I would sometimes get crashes, often with a call to cursor_timer_handler() in the traceback. I think there's index mixup in fbcon_fb_unbind(), at least this change seems to solve the immediate problem: I haven't got time to follow up on this now, but making sure DRM generic fbdev emulation device unplug works is on my TODO. BTW, I believe the udl drm driver should be able to use the generic fbdev emulation if it had a drm_driver->gem_prime_vmap hook. It uses a shadow buffer which would also make fbdev mmap work for udl. (shmem buffers and fbdev deferred I/O doesn't work together since they both use page->lru/mapping) Noralf. [1] https://patchwork.freedesktop.org/series/45847/ > So, the udlfb > driver waits until the open count drops to zero and then calls > unregister_framebuffer(). > > But the console subsystem keeps the framebuffer open - which means that if > user use unplugs the USB adapter, the open count won't drop to zero > (because the console is bound to it) - which means that > unregister_framebuffer() will not be called. > > You must unbind the console before calling unregister_framebuffer(). The > PCI framebuffer drivers don't have this problem because the user is not > expected to just unplug the PCI card while it is being used by the > console. > > Mikulas > >>> --- >>> drivers/video/fbdev/core/fbmem.c | 21 +++++++++++++++++---- >>> 1 file changed, 17 insertions(+), 4 deletions(-) >>> >>> Index: linux-4.16.12/drivers/video/fbdev/core/fbmem.c >>> =================================================================== >>> --- linux-4.16.12.orig/drivers/video/fbdev/core/fbmem.c 2018-05-26 06:13:20.000000000 +0200 >>> +++ linux-4.16.12/drivers/video/fbdev/core/fbmem.c 2018-05-26 06:13:20.000000000 +0200 >>> @@ -1805,12 +1805,12 @@ static int do_register_framebuffer(struc >>> return 0; >>> } >>> >>> -static int do_unregister_framebuffer(struct fb_info *fb_info) >>> +static int unbind_console(struct fb_info *fb_info) >>> { >>> struct fb_event event; >>> - int i, ret = 0; >>> + int ret; >>> + int i = fb_info->node; >>> >>> - i = fb_info->node; >>> if (i < 0 || i >= FB_MAX || registered_fb[i] != fb_info) >>> return -EINVAL; >>> >>> @@ -1825,6 +1825,16 @@ static int do_unregister_framebuffer(str >>> unlock_fb_info(fb_info); >>> console_unlock(); >>> >>> + return ret; >>> +} >>> + >>> +static int do_unregister_framebuffer(struct fb_info *fb_info) >>> +{ >>> + struct fb_event event; >>> + int ret; >>> + >>> + ret = unbind_console(fb_info); >>> + >>> if (ret) >>> return -EINVAL; >>> >>> @@ -1835,7 +1845,7 @@ static int do_unregister_framebuffer(str >>> (fb_info->pixmap.flags & FB_PIXMAP_DEFAULT)) >>> kfree(fb_info->pixmap.addr); >>> fb_destroy_modelist(&fb_info->modelist); >>> - registered_fb[i] = NULL; >>> + registered_fb[fb_info->node] = NULL; >>> num_registered_fb--; >>> fb_cleanup_device(fb_info); >>> event.info = fb_info; >>> @@ -1860,6 +1870,9 @@ int unlink_framebuffer(struct fb_info *f >>> device_destroy(fb_class, MKDEV(FB_MAJOR, i)); >>> fb_info->dev = NULL; >>> } >>> + >>> + unbind_console(fb_info); >>> + >>> return 0; >>> } >>> EXPORT_SYMBOL(unlink_framebuffer); >> Best regards, >> -- >> Bartlomiej Zolnierkiewicz >> Samsung R&D Institute Poland >> Samsung Electronics >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > Reviewed-by: Mikulas Patocka diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 5fb156bdcf4e..271b9b988b73 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -3066,7 +3072,7 @@ static int fbcon_fb_unbind(int idx)         for (i = first_fb_vc; i <= last_fb_vc; i++) {                 if (con2fb_map[i] != idx &&                     con2fb_map[i] != -1) { -                       new_idx = i; +                       new_idx = con2fb_map[i];                         break;                 }         }