Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190520082216.26273-10-daniel.vetter@ffwll.ch>
 <CGME20190521142317epcas2p44d184ead3ec7d514a8fa6784abf30747@epcas2p4.samsung.com>
 <20190521142304.9652-1-daniel.vetter@ffwll.ch> <6b9747cf-8845-0eb9-878e-f2953665fcec@samsung.com>
In-Reply-To: <6b9747cf-8845-0eb9-878e-f2953665fcec@samsung.com>
From:   Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Wed, 22 May 2019 12:38:18 +0200
Message-ID: <CAKMK7uGKxny2cM9wPBvu9jRUKR0KP2OOHa8qAsReNsKWTvscAA@mail.gmail.com>
Subject: Re: [PATCH] fbcon: Remove fbcon_has_exited
To:     Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc:     DRI Development <dri-devel@lists.freedesktop.org>,
        Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Daniel Vetter <daniel.vetter@intel.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Hans de Goede <hdegoede@redhat.com>,
        =?UTF-8?Q?Noralf_Tr=C3=B8nnes?= <noralf@tronnes.org>,
        Yisheng Xie <ysxie@foxmail.com>,
        Konstantin Khorenko <khorenko@virtuozzo.com>,
        Prarit Bhargava <prarit@redhat.com>,
        Kees Cook <keescook@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, May 22, 2019 at 12:04 PM Bartlomiej Zolnierkiewicz
<b.zolnierkie@samsung.com> 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@ffwll.ch>
> > 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 underflo=
w,
> > 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 =3D=3D -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@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Cc: "Noralf Tr=C3=B8nnes" <noralf@tronnes.org>
> > Cc: Yisheng Xie <ysxie@foxmail.com>
> > Cc: Konstantin Khorenko <khorenko@virtuozzo.com>
> > Cc: Prarit Bhargava <prarit@redhat.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > ---
> >  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


--=20
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch