On Wed, May 22, 2019 at 12:04 PM Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx> wrote: > > > Hi Daniel, > > On 5/21/19 4:23 PM, Daniel Vetter wrote: > > This is unused code since > > > > commit 6104c37094e729f3d4ce65797002112735d49cd1 > > Author: Daniel Vetter <daniel.vetter@xxxxxxxx> > > Date: Tue Aug 1 17:32:07 2017 +0200 > > > > fbcon: Make fbcon a built-time depency for fbdev > > > > when fbcon was made a compile-time static dependency of fbdev. We > > can't exit fbcon anymore without exiting fbdev first, which only works > > if all fbdev drivers have unloaded already. Hence this is all dead > > code. > > > > v2: I missed that fbcon_exit is also called from con_deinit stuff, and > > there fbcon_has_exited prevents double-cleanup. But we can fix that > > by properly resetting con2fb_map[] to all -1, which is used everywhere > > else to indicate "no fb_info allocate to this console". With that > > change the double-cleanup (which resulted in a module refcount underflow, > > among other things) is prevented. > > > > Aside: con2fb_map is a signed char, so don't register more than 128 fb_info > > or hilarity will ensue. > > > > v3: CI showed me that I still didn't fully understand what's going on > > here. The leaked references in con2fb_map have been used upon > > rebinding the fb console in fbcon_init. It worked because fbdev > > unregistering still cleaned out con2fb_map, and reset it to info_idx. > > If the last fbdev driver unregistered, then it also reset info_idx, > > and unregistered the fbcon driver. > > > > Imo that's all a bit fragile, so let's keep the con2fb_map reset to > > -1, and in fbcon_init pick info_idx if we're starting fresh. That > > means unbinding and rebinding will cleanse the mapping, but why are > > you doing that if you want to retain the mapping, so should be fine. > > > > Also, I think info_idx == -1 is impossible in fbcon_init - we > > unregister the fbcon in that case. So catch&warn about that. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> > > Cc: Hans de Goede <hdegoede@xxxxxxxxxx> > > Cc: "Noralf Trønnes" <noralf@xxxxxxxxxxx> > > Cc: Yisheng Xie <ysxie@xxxxxxxxxxx> > > Cc: Konstantin Khorenko <khorenko@xxxxxxxxxxxxx> > > Cc: Prarit Bhargava <prarit@xxxxxxxxxx> > > Cc: Kees Cook <keescook@xxxxxxxxxxxx> > > --- > > drivers/video/fbdev/core/fbcon.c | 39 +++++--------------------------- > > 1 file changed, 6 insertions(+), 33 deletions(-) > This patch was #09/33 in your patch series, now it is independent change. > > Do you want me to apply it now or should I wait for the new version of > the whole series? It's an in-reply-to replacement for the totally broken one, so that patchwork picks things up correctly (and therefore our CI). I'm not sure how far that convention is used beyond dri-devel ... I did fix up all the issues pointed out thus far, but I haven't fully appeased our CI just yet. Once that's done I'll resend. Thanks, Daniel > [ I looked at all patches in the series and they look fine to me. > After outstanding issues are fixed I'll be happy to apply them all > to fbdev-for-next (I can create immutable branch if needed). ] > > Best regards, > -- > Bartlomiej Zolnierkiewicz > Samsung R&D Institute Poland > Samsung Electronics -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
