From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id DAADCC433EF for ; Tue, 5 Apr 2022 08:46:00 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id BE8D510EE60; Tue, 5 Apr 2022 08:45:57 +0000 (UTC) Received: from mail-ej1-x644.google.com (mail-ej1-x644.google.com [IPv6:2a00:1450:4864:20::644]) by gabe.freedesktop.org (Postfix) with ESMTPS id F3E9610EE58 for ; Tue, 5 Apr 2022 08:45:56 +0000 (UTC) Received: by mail-ej1-x644.google.com with SMTP id bh17so25191087ejb.8 for ; Tue, 05 Apr 2022 01:45:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=kLwOSJh4JkUAu8bAVMQPem4swoCfWHajhhsSaSbd3wA=; b=bMbT3uGp3dFxXv5749rP/YEH0fwf2ipBnL4qQcoZffJmqP63odGiG48c6lrEQDoOuE 2Io2E+FIcefJqaP5UzCFcWumnT1yERlYhCUDpyrunp/zi7DwDGmp9jA4EjrBmXZPUbxe m4RBFNb29e21bIjfvB3+04nX0j65o9B8vbaPY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :content-transfer-encoding:in-reply-to; bh=kLwOSJh4JkUAu8bAVMQPem4swoCfWHajhhsSaSbd3wA=; b=p2QOPY3lW0g2o4yv8H7B7Xd5EgXHhliE+f3BCW9ZXoDhIQK2m11kfUpBUcHBRLbQ+O 7QV/eWalU2GXoKNLfI4dgR3aVohkmqlyX43mdlIBICc90tdh0Yt8mszJ4mUIX96ptqdE uccEK8LqOS+xilw4GWuq2j4dDvfkAX7GyHP752xFbXkxhD/J8e9/s4AfISKK6a9TdAEY Mb2cARshRIDgnAIz0VM/Xg1+PCaAKpI9fLwO4hqArdqYEQj3qwCB7WCCEaN+eGfDY0dV qosQLfxW2evCtOlytax72QvIe0CUSCd2r/SW5ZlLB/SkYlQll96jyLE3NUKs3cyuvOzl wboA== X-Gm-Message-State: AOAM531MLmdcK2pVmkBINm+dg15LgZFPnuEgqaNPz9Dh44CS7i4ZkQBc Yg7Nmod7lMS6IinDfDFsalzmTw== X-Google-Smtp-Source: ABdhPJylM+dFHXlrCCet5bi78ssTO3yLx8DQZUwNTohynsB3hLO/O0NXGESroauDwjtoDMbXczTEkA== X-Received: by 2002:a17:906:300f:b0:6e0:b38d:777d with SMTP id 15-20020a170906300f00b006e0b38d777dmr2418845ejz.189.1649148355361; Tue, 05 Apr 2022 01:45:55 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id a22-20020a50ff16000000b00410d029ea5csm6312065edu.96.2022.04.05.01.45.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Apr 2022 01:45:54 -0700 (PDT) Date: Tue, 5 Apr 2022 10:45:53 +0200 From: Daniel Vetter To: Thomas Zimmermann Subject: Re: [PATCH v2 09/19] fbcon: Extract fbcon_open/release helpers Message-ID: Mail-Followup-To: Thomas Zimmermann , DRI Development , linux-fbdev@vger.kernel.org, Du Cheng , Tetsuo Handa , Intel Graphics Development , LKML , Claudio Suarez , Greg Kroah-Hartman , Daniel Vetter , Sam Ravnborg References: <20220208210824.2238981-1-daniel.vetter@ffwll.ch> <20220208210824.2238981-10-daniel.vetter@ffwll.ch> <0879a2ff-37df-a9ae-0ce1-2bfcb2d1b322@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <0879a2ff-37df-a9ae-0ce1-2bfcb2d1b322@suse.de> X-Operating-System: Linux phenom 5.10.0-8-amd64 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-fbdev@vger.kernel.org, Du Cheng , Tetsuo Handa , Daniel Vetter , Intel Graphics Development , LKML , DRI Development , Claudio Suarez , Greg Kroah-Hartman , Daniel Vetter , Sam Ravnborg Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Thu, Feb 10, 2022 at 12:46:32PM +0100, Thomas Zimmermann wrote: > Hi > > Am 08.02.22 um 22:08 schrieb Daniel Vetter: > > There's two minor behaviour changes in here: > > - in error paths we now consistently call fb_ops->fb_release > > - fb_release really can't fail (fbmem.c ignores it too) and there's no > > reasonable cleanup we can do anyway. > > > > Note that everything in fbcon.c is protected by the big console_lock() > > lock (especially all the global variables), so the minor changes in > > ordering of setup/cleanup do not matter. > > > > v2: Explain a bit better why this is all correct (Sam) > > > > Acked-by: Sam Ravnborg > > Signed-off-by: Daniel Vetter > > Cc: Daniel Vetter > > Cc: Claudio Suarez > > Cc: Greg Kroah-Hartman > > Cc: Tetsuo Handa > > Cc: Du Cheng > > --- > > drivers/video/fbdev/core/fbcon.c | 107 +++++++++++++++---------------- > > 1 file changed, 53 insertions(+), 54 deletions(-) > > > > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c > > index 058e885d24f6..3e1a3e7bf527 100644 > > --- a/drivers/video/fbdev/core/fbcon.c > > +++ b/drivers/video/fbdev/core/fbcon.c > > @@ -682,19 +682,37 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount) > > #endif /* CONFIG_MISC_TILEBLITTING */ > > +static int fbcon_open(struct fb_info *info) > > +{ > > + if (!try_module_get(info->fbops->owner)) > > + return -ENODEV; > > + > > + if (info->fbops->fb_open && > > + info->fbops->fb_open(info, 0)) { > > + module_put(info->fbops->owner); > > + return -ENODEV; > > + } > > + > > + return 0; > > +} > > + > > +static void fbcon_release(struct fb_info *info) > > +{ > > + if (info->fbops->fb_release) > > + info->fbops->fb_release(info, 0); > > + > > + module_put(info->fbops->owner); > > +} > > static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, > > int unit, int oldidx) > > { > > struct fbcon_ops *ops = NULL; > > - int err = 0; > > - > > - if (!try_module_get(info->fbops->owner)) > > - err = -ENODEV; > > + int err; > > - if (!err && info->fbops->fb_open && > > - info->fbops->fb_open(info, 0)) > > - err = -ENODEV; > > + err = fbcon_open(info); > > + if (err) > > + return err; > > if (!err) { > > ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); > > @@ -715,7 +733,7 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, > > if (err) { > > con2fb_map[unit] = oldidx; > > - module_put(info->fbops->owner); > > + fbcon_release(info); > > } > > return err; > > @@ -726,45 +744,34 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo, > > int oldidx, int found) > > { > > struct fbcon_ops *ops = oldinfo->fbcon_par; > > - int err = 0, ret; > > + int ret; > > - if (oldinfo->fbops->fb_release && > > - oldinfo->fbops->fb_release(oldinfo, 0)) { > > - con2fb_map[unit] = oldidx; > > We don't need oldidx any longer? > > There's some logic wrt to the parameter 'found' here and in set_con2fb_map() > that appears to be relevant. Yeah further patches clean this up more. Or did you see a potential bug here? I did ditch the fb_release error handling, simply because there's not really much we can do anyway, it shouldn't ever fail (that's a driver bug) and it was convoluting the code for no gain. But I might have missed something in this cargo-cult. -Daniel > > Best regards > Thomas > > > > - if (!found && newinfo->fbops->fb_release) > > - newinfo->fbops->fb_release(newinfo, 0); > > - if (!found) > > - module_put(newinfo->fbops->owner); > > - err = -ENODEV; > > - } > > + fbcon_release(oldinfo); > > - if (!err) { > > - fbcon_del_cursor_work(oldinfo); > > - kfree(ops->cursor_state.mask); > > - kfree(ops->cursor_data); > > - kfree(ops->cursor_src); > > - kfree(ops->fontbuffer); > > - kfree(oldinfo->fbcon_par); > > - oldinfo->fbcon_par = NULL; > > - module_put(oldinfo->fbops->owner); > > - /* > > - If oldinfo and newinfo are driving the same hardware, > > - the fb_release() method of oldinfo may attempt to > > - restore the hardware state. This will leave the > > - newinfo in an undefined state. Thus, a call to > > - fb_set_par() may be needed for the newinfo. > > - */ > > - if (newinfo && newinfo->fbops->fb_set_par) { > > - ret = newinfo->fbops->fb_set_par(newinfo); > > + fbcon_del_cursor_work(oldinfo); > > + kfree(ops->cursor_state.mask); > > + kfree(ops->cursor_data); > > + kfree(ops->cursor_src); > > + kfree(ops->fontbuffer); > > + kfree(oldinfo->fbcon_par); > > + oldinfo->fbcon_par = NULL; > > + /* > > + If oldinfo and newinfo are driving the same hardware, > > + the fb_release() method of oldinfo may attempt to > > + restore the hardware state. This will leave the > > + newinfo in an undefined state. Thus, a call to > > + fb_set_par() may be needed for the newinfo. > > + */ > > + if (newinfo && newinfo->fbops->fb_set_par) { > > + ret = newinfo->fbops->fb_set_par(newinfo); > > - if (ret) > > - printk(KERN_ERR "con2fb_release_oldinfo: " > > - "detected unhandled fb_set_par error, " > > - "error code %d\n", ret); > > - } > > + if (ret) > > + printk(KERN_ERR "con2fb_release_oldinfo: " > > + "detected unhandled fb_set_par error, " > > + "error code %d\n", ret); > > } > > - return err; > > + return 0; > > } > > static void con2fb_init_display(struct vc_data *vc, struct fb_info *info, > > @@ -919,7 +926,6 @@ static const char *fbcon_startup(void) > > struct fbcon_display *p = &fb_display[fg_console]; > > struct vc_data *vc = vc_cons[fg_console].d; > > const struct font_desc *font = NULL; > > - struct module *owner; > > struct fb_info *info = NULL; > > struct fbcon_ops *ops; > > int rows, cols; > > @@ -938,17 +944,12 @@ static const char *fbcon_startup(void) > > if (!info) > > return NULL; > > > > - owner = info->fbops->owner; > > - if (!try_module_get(owner)) > > + if (fbcon_open(info)) > > return NULL; > > - if (info->fbops->fb_open && info->fbops->fb_open(info, 0)) { > > - module_put(owner); > > - return NULL; > > - } > > ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); > > if (!ops) { > > - module_put(owner); > > + fbcon_release(info); > > return NULL; > > } > > @@ -3314,10 +3315,6 @@ static void fbcon_exit(void) > > } > > if (mapped) { > > - if (info->fbops->fb_release) > > - info->fbops->fb_release(info, 0); > > - module_put(info->fbops->owner); > > - > > if (info->fbcon_par) { > > struct fbcon_ops *ops = info->fbcon_par; > > @@ -3327,6 +3324,8 @@ static void fbcon_exit(void) > > kfree(info->fbcon_par); > > info->fbcon_par = NULL; > > } > > + > > + fbcon_release(info); > > } > > } > > } > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2C63DC433EF for ; Tue, 5 Apr 2022 08:45:58 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 7AF1510EE58; Tue, 5 Apr 2022 08:45:57 +0000 (UTC) Received: from mail-ej1-x642.google.com (mail-ej1-x642.google.com [IPv6:2a00:1450:4864:20::642]) by gabe.freedesktop.org (Postfix) with ESMTPS id D988910EE53 for ; Tue, 5 Apr 2022 08:45:56 +0000 (UTC) Received: by mail-ej1-x642.google.com with SMTP id yy13so25239221ejb.2 for ; Tue, 05 Apr 2022 01:45:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=kLwOSJh4JkUAu8bAVMQPem4swoCfWHajhhsSaSbd3wA=; b=bMbT3uGp3dFxXv5749rP/YEH0fwf2ipBnL4qQcoZffJmqP63odGiG48c6lrEQDoOuE 2Io2E+FIcefJqaP5UzCFcWumnT1yERlYhCUDpyrunp/zi7DwDGmp9jA4EjrBmXZPUbxe m4RBFNb29e21bIjfvB3+04nX0j65o9B8vbaPY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :content-transfer-encoding:in-reply-to; bh=kLwOSJh4JkUAu8bAVMQPem4swoCfWHajhhsSaSbd3wA=; b=PSWv0GddgYkAzuAOCYz7Xm9MewX+mRf+/RNlq0qIwRDQBzmXqQQEv+nT/4uPkcyE49 f3OkqncBdZ0iQWbZYtKXcBOs1sId878nqTX5RQEW/ZxpyM3UACHA7DpRfJTldO/2b0MO sAwp2O8tEN87JV/nbeR8h5oNZ3D+1ZfLstZhBiJUoqz/pVAdR+XfaXq7I0r6eVrPypf7 4z33JYO53MSm37tdk1eMVMP0IYBG4QRJVmiK9lfHbxlk0fjdfFWZbI776CobxGiZm2XH AuMKbyoMglMb2fvZQrTed1U6bKuxulw02ya7UpJhLDl6pD63dAjoVWxe7l1ookAG7Tm7 SQwg== X-Gm-Message-State: AOAM532hZqL5FPiyGP+gHS3qsPjy3GwA5Dd0TWWpNV5563bDiLOXPz6M UwF/w5iE3cuNCDvgoD2p+sM8hQ== X-Google-Smtp-Source: ABdhPJylM+dFHXlrCCet5bi78ssTO3yLx8DQZUwNTohynsB3hLO/O0NXGESroauDwjtoDMbXczTEkA== X-Received: by 2002:a17:906:300f:b0:6e0:b38d:777d with SMTP id 15-20020a170906300f00b006e0b38d777dmr2418845ejz.189.1649148355361; Tue, 05 Apr 2022 01:45:55 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id a22-20020a50ff16000000b00410d029ea5csm6312065edu.96.2022.04.05.01.45.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Apr 2022 01:45:54 -0700 (PDT) Date: Tue, 5 Apr 2022 10:45:53 +0200 From: Daniel Vetter To: Thomas Zimmermann Message-ID: Mail-Followup-To: Thomas Zimmermann , DRI Development , linux-fbdev@vger.kernel.org, Du Cheng , Tetsuo Handa , Intel Graphics Development , LKML , Claudio Suarez , Greg Kroah-Hartman , Daniel Vetter , Sam Ravnborg References: <20220208210824.2238981-1-daniel.vetter@ffwll.ch> <20220208210824.2238981-10-daniel.vetter@ffwll.ch> <0879a2ff-37df-a9ae-0ce1-2bfcb2d1b322@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <0879a2ff-37df-a9ae-0ce1-2bfcb2d1b322@suse.de> X-Operating-System: Linux phenom 5.10.0-8-amd64 Subject: Re: [Intel-gfx] [PATCH v2 09/19] fbcon: Extract fbcon_open/release helpers X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-fbdev@vger.kernel.org, Du Cheng , Tetsuo Handa , Daniel Vetter , Intel Graphics Development , LKML , DRI Development , Greg Kroah-Hartman , Daniel Vetter , Sam Ravnborg Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Thu, Feb 10, 2022 at 12:46:32PM +0100, Thomas Zimmermann wrote: > Hi > > Am 08.02.22 um 22:08 schrieb Daniel Vetter: > > There's two minor behaviour changes in here: > > - in error paths we now consistently call fb_ops->fb_release > > - fb_release really can't fail (fbmem.c ignores it too) and there's no > > reasonable cleanup we can do anyway. > > > > Note that everything in fbcon.c is protected by the big console_lock() > > lock (especially all the global variables), so the minor changes in > > ordering of setup/cleanup do not matter. > > > > v2: Explain a bit better why this is all correct (Sam) > > > > Acked-by: Sam Ravnborg > > Signed-off-by: Daniel Vetter > > Cc: Daniel Vetter > > Cc: Claudio Suarez > > Cc: Greg Kroah-Hartman > > Cc: Tetsuo Handa > > Cc: Du Cheng > > --- > > drivers/video/fbdev/core/fbcon.c | 107 +++++++++++++++---------------- > > 1 file changed, 53 insertions(+), 54 deletions(-) > > > > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c > > index 058e885d24f6..3e1a3e7bf527 100644 > > --- a/drivers/video/fbdev/core/fbcon.c > > +++ b/drivers/video/fbdev/core/fbcon.c > > @@ -682,19 +682,37 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount) > > #endif /* CONFIG_MISC_TILEBLITTING */ > > +static int fbcon_open(struct fb_info *info) > > +{ > > + if (!try_module_get(info->fbops->owner)) > > + return -ENODEV; > > + > > + if (info->fbops->fb_open && > > + info->fbops->fb_open(info, 0)) { > > + module_put(info->fbops->owner); > > + return -ENODEV; > > + } > > + > > + return 0; > > +} > > + > > +static void fbcon_release(struct fb_info *info) > > +{ > > + if (info->fbops->fb_release) > > + info->fbops->fb_release(info, 0); > > + > > + module_put(info->fbops->owner); > > +} > > static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, > > int unit, int oldidx) > > { > > struct fbcon_ops *ops = NULL; > > - int err = 0; > > - > > - if (!try_module_get(info->fbops->owner)) > > - err = -ENODEV; > > + int err; > > - if (!err && info->fbops->fb_open && > > - info->fbops->fb_open(info, 0)) > > - err = -ENODEV; > > + err = fbcon_open(info); > > + if (err) > > + return err; > > if (!err) { > > ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); > > @@ -715,7 +733,7 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, > > if (err) { > > con2fb_map[unit] = oldidx; > > - module_put(info->fbops->owner); > > + fbcon_release(info); > > } > > return err; > > @@ -726,45 +744,34 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo, > > int oldidx, int found) > > { > > struct fbcon_ops *ops = oldinfo->fbcon_par; > > - int err = 0, ret; > > + int ret; > > - if (oldinfo->fbops->fb_release && > > - oldinfo->fbops->fb_release(oldinfo, 0)) { > > - con2fb_map[unit] = oldidx; > > We don't need oldidx any longer? > > There's some logic wrt to the parameter 'found' here and in set_con2fb_map() > that appears to be relevant. Yeah further patches clean this up more. Or did you see a potential bug here? I did ditch the fb_release error handling, simply because there's not really much we can do anyway, it shouldn't ever fail (that's a driver bug) and it was convoluting the code for no gain. But I might have missed something in this cargo-cult. -Daniel > > Best regards > Thomas > > > > - if (!found && newinfo->fbops->fb_release) > > - newinfo->fbops->fb_release(newinfo, 0); > > - if (!found) > > - module_put(newinfo->fbops->owner); > > - err = -ENODEV; > > - } > > + fbcon_release(oldinfo); > > - if (!err) { > > - fbcon_del_cursor_work(oldinfo); > > - kfree(ops->cursor_state.mask); > > - kfree(ops->cursor_data); > > - kfree(ops->cursor_src); > > - kfree(ops->fontbuffer); > > - kfree(oldinfo->fbcon_par); > > - oldinfo->fbcon_par = NULL; > > - module_put(oldinfo->fbops->owner); > > - /* > > - If oldinfo and newinfo are driving the same hardware, > > - the fb_release() method of oldinfo may attempt to > > - restore the hardware state. This will leave the > > - newinfo in an undefined state. Thus, a call to > > - fb_set_par() may be needed for the newinfo. > > - */ > > - if (newinfo && newinfo->fbops->fb_set_par) { > > - ret = newinfo->fbops->fb_set_par(newinfo); > > + fbcon_del_cursor_work(oldinfo); > > + kfree(ops->cursor_state.mask); > > + kfree(ops->cursor_data); > > + kfree(ops->cursor_src); > > + kfree(ops->fontbuffer); > > + kfree(oldinfo->fbcon_par); > > + oldinfo->fbcon_par = NULL; > > + /* > > + If oldinfo and newinfo are driving the same hardware, > > + the fb_release() method of oldinfo may attempt to > > + restore the hardware state. This will leave the > > + newinfo in an undefined state. Thus, a call to > > + fb_set_par() may be needed for the newinfo. > > + */ > > + if (newinfo && newinfo->fbops->fb_set_par) { > > + ret = newinfo->fbops->fb_set_par(newinfo); > > - if (ret) > > - printk(KERN_ERR "con2fb_release_oldinfo: " > > - "detected unhandled fb_set_par error, " > > - "error code %d\n", ret); > > - } > > + if (ret) > > + printk(KERN_ERR "con2fb_release_oldinfo: " > > + "detected unhandled fb_set_par error, " > > + "error code %d\n", ret); > > } > > - return err; > > + return 0; > > } > > static void con2fb_init_display(struct vc_data *vc, struct fb_info *info, > > @@ -919,7 +926,6 @@ static const char *fbcon_startup(void) > > struct fbcon_display *p = &fb_display[fg_console]; > > struct vc_data *vc = vc_cons[fg_console].d; > > const struct font_desc *font = NULL; > > - struct module *owner; > > struct fb_info *info = NULL; > > struct fbcon_ops *ops; > > int rows, cols; > > @@ -938,17 +944,12 @@ static const char *fbcon_startup(void) > > if (!info) > > return NULL; > > > > - owner = info->fbops->owner; > > - if (!try_module_get(owner)) > > + if (fbcon_open(info)) > > return NULL; > > - if (info->fbops->fb_open && info->fbops->fb_open(info, 0)) { > > - module_put(owner); > > - return NULL; > > - } > > ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); > > if (!ops) { > > - module_put(owner); > > + fbcon_release(info); > > return NULL; > > } > > @@ -3314,10 +3315,6 @@ static void fbcon_exit(void) > > } > > if (mapped) { > > - if (info->fbops->fb_release) > > - info->fbops->fb_release(info, 0); > > - module_put(info->fbops->owner); > > - > > if (info->fbcon_par) { > > struct fbcon_ops *ops = info->fbcon_par; > > @@ -3327,6 +3324,8 @@ static void fbcon_exit(void) > > kfree(info->fbcon_par); > > info->fbcon_par = NULL; > > } > > + > > + fbcon_release(info); > > } > > } > > } > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6EBD8C433EF for ; Tue, 5 Apr 2022 12:16:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1382504AbiDEMPh (ORCPT ); Tue, 5 Apr 2022 08:15:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51010 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S244903AbiDEIwq (ORCPT ); Tue, 5 Apr 2022 04:52:46 -0400 Received: from mail-ej1-x641.google.com (mail-ej1-x641.google.com [IPv6:2a00:1450:4864:20::641]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CFA8F2458C for ; Tue, 5 Apr 2022 01:45:56 -0700 (PDT) Received: by mail-ej1-x641.google.com with SMTP id k23so21738070ejd.3 for ; Tue, 05 Apr 2022 01:45:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=kLwOSJh4JkUAu8bAVMQPem4swoCfWHajhhsSaSbd3wA=; b=bMbT3uGp3dFxXv5749rP/YEH0fwf2ipBnL4qQcoZffJmqP63odGiG48c6lrEQDoOuE 2Io2E+FIcefJqaP5UzCFcWumnT1yERlYhCUDpyrunp/zi7DwDGmp9jA4EjrBmXZPUbxe m4RBFNb29e21bIjfvB3+04nX0j65o9B8vbaPY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :content-transfer-encoding:in-reply-to; bh=kLwOSJh4JkUAu8bAVMQPem4swoCfWHajhhsSaSbd3wA=; b=jrJ4CRoGI/THTp1O8nP+KhfA/x0Rq4eGzJ8+2w93iKRLFo1RdGt80MRBKGCKN+XCuB QuAdTKtFLT6GcFEK2/1nS8W71I30QuDj8dLcSLMPC0NzzWV1nM95iJHyXI7dr8NYNnVf OsHPq55PugaGHbiCGWsFGcgd6jo9cUbKYjw4R01i/uWMpMdIZ4xBXRyBDXStSPF8OfqW IBw2dS3yzybVZhAb+Gbd2HJqSy67DdtColzZ4puVnBbvGYvM7hYe1Xj9pqGImTbYMC7U j7mSsnmXoziwWYQtxM4ISB+PhQsdHbNvh223gfIQPiVkAruNgrYGGD6H/EG8vTheYjJF Fm0g== X-Gm-Message-State: AOAM5318AcDWbFwD0FjZd8goDH66jKg2MTOTG7HoQMZXE9qkDxr60ApL TUe5Kxn8+oXAGD9FqSjkJrbrqQ== X-Google-Smtp-Source: ABdhPJylM+dFHXlrCCet5bi78ssTO3yLx8DQZUwNTohynsB3hLO/O0NXGESroauDwjtoDMbXczTEkA== X-Received: by 2002:a17:906:300f:b0:6e0:b38d:777d with SMTP id 15-20020a170906300f00b006e0b38d777dmr2418845ejz.189.1649148355361; Tue, 05 Apr 2022 01:45:55 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id a22-20020a50ff16000000b00410d029ea5csm6312065edu.96.2022.04.05.01.45.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Apr 2022 01:45:54 -0700 (PDT) Date: Tue, 5 Apr 2022 10:45:53 +0200 From: Daniel Vetter To: Thomas Zimmermann Cc: Daniel Vetter , DRI Development , linux-fbdev@vger.kernel.org, Du Cheng , Tetsuo Handa , Intel Graphics Development , LKML , Claudio Suarez , Greg Kroah-Hartman , Daniel Vetter , Sam Ravnborg Subject: Re: [PATCH v2 09/19] fbcon: Extract fbcon_open/release helpers Message-ID: Mail-Followup-To: Thomas Zimmermann , DRI Development , linux-fbdev@vger.kernel.org, Du Cheng , Tetsuo Handa , Intel Graphics Development , LKML , Claudio Suarez , Greg Kroah-Hartman , Daniel Vetter , Sam Ravnborg References: <20220208210824.2238981-1-daniel.vetter@ffwll.ch> <20220208210824.2238981-10-daniel.vetter@ffwll.ch> <0879a2ff-37df-a9ae-0ce1-2bfcb2d1b322@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <0879a2ff-37df-a9ae-0ce1-2bfcb2d1b322@suse.de> X-Operating-System: Linux phenom 5.10.0-8-amd64 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Feb 10, 2022 at 12:46:32PM +0100, Thomas Zimmermann wrote: > Hi > > Am 08.02.22 um 22:08 schrieb Daniel Vetter: > > There's two minor behaviour changes in here: > > - in error paths we now consistently call fb_ops->fb_release > > - fb_release really can't fail (fbmem.c ignores it too) and there's no > > reasonable cleanup we can do anyway. > > > > Note that everything in fbcon.c is protected by the big console_lock() > > lock (especially all the global variables), so the minor changes in > > ordering of setup/cleanup do not matter. > > > > v2: Explain a bit better why this is all correct (Sam) > > > > Acked-by: Sam Ravnborg > > Signed-off-by: Daniel Vetter > > Cc: Daniel Vetter > > Cc: Claudio Suarez > > Cc: Greg Kroah-Hartman > > Cc: Tetsuo Handa > > Cc: Du Cheng > > --- > > drivers/video/fbdev/core/fbcon.c | 107 +++++++++++++++---------------- > > 1 file changed, 53 insertions(+), 54 deletions(-) > > > > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c > > index 058e885d24f6..3e1a3e7bf527 100644 > > --- a/drivers/video/fbdev/core/fbcon.c > > +++ b/drivers/video/fbdev/core/fbcon.c > > @@ -682,19 +682,37 @@ static int fbcon_invalid_charcount(struct fb_info *info, unsigned charcount) > > #endif /* CONFIG_MISC_TILEBLITTING */ > > +static int fbcon_open(struct fb_info *info) > > +{ > > + if (!try_module_get(info->fbops->owner)) > > + return -ENODEV; > > + > > + if (info->fbops->fb_open && > > + info->fbops->fb_open(info, 0)) { > > + module_put(info->fbops->owner); > > + return -ENODEV; > > + } > > + > > + return 0; > > +} > > + > > +static void fbcon_release(struct fb_info *info) > > +{ > > + if (info->fbops->fb_release) > > + info->fbops->fb_release(info, 0); > > + > > + module_put(info->fbops->owner); > > +} > > static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, > > int unit, int oldidx) > > { > > struct fbcon_ops *ops = NULL; > > - int err = 0; > > - > > - if (!try_module_get(info->fbops->owner)) > > - err = -ENODEV; > > + int err; > > - if (!err && info->fbops->fb_open && > > - info->fbops->fb_open(info, 0)) > > - err = -ENODEV; > > + err = fbcon_open(info); > > + if (err) > > + return err; > > if (!err) { > > ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); > > @@ -715,7 +733,7 @@ static int con2fb_acquire_newinfo(struct vc_data *vc, struct fb_info *info, > > if (err) { > > con2fb_map[unit] = oldidx; > > - module_put(info->fbops->owner); > > + fbcon_release(info); > > } > > return err; > > @@ -726,45 +744,34 @@ static int con2fb_release_oldinfo(struct vc_data *vc, struct fb_info *oldinfo, > > int oldidx, int found) > > { > > struct fbcon_ops *ops = oldinfo->fbcon_par; > > - int err = 0, ret; > > + int ret; > > - if (oldinfo->fbops->fb_release && > > - oldinfo->fbops->fb_release(oldinfo, 0)) { > > - con2fb_map[unit] = oldidx; > > We don't need oldidx any longer? > > There's some logic wrt to the parameter 'found' here and in set_con2fb_map() > that appears to be relevant. Yeah further patches clean this up more. Or did you see a potential bug here? I did ditch the fb_release error handling, simply because there's not really much we can do anyway, it shouldn't ever fail (that's a driver bug) and it was convoluting the code for no gain. But I might have missed something in this cargo-cult. -Daniel > > Best regards > Thomas > > > > - if (!found && newinfo->fbops->fb_release) > > - newinfo->fbops->fb_release(newinfo, 0); > > - if (!found) > > - module_put(newinfo->fbops->owner); > > - err = -ENODEV; > > - } > > + fbcon_release(oldinfo); > > - if (!err) { > > - fbcon_del_cursor_work(oldinfo); > > - kfree(ops->cursor_state.mask); > > - kfree(ops->cursor_data); > > - kfree(ops->cursor_src); > > - kfree(ops->fontbuffer); > > - kfree(oldinfo->fbcon_par); > > - oldinfo->fbcon_par = NULL; > > - module_put(oldinfo->fbops->owner); > > - /* > > - If oldinfo and newinfo are driving the same hardware, > > - the fb_release() method of oldinfo may attempt to > > - restore the hardware state. This will leave the > > - newinfo in an undefined state. Thus, a call to > > - fb_set_par() may be needed for the newinfo. > > - */ > > - if (newinfo && newinfo->fbops->fb_set_par) { > > - ret = newinfo->fbops->fb_set_par(newinfo); > > + fbcon_del_cursor_work(oldinfo); > > + kfree(ops->cursor_state.mask); > > + kfree(ops->cursor_data); > > + kfree(ops->cursor_src); > > + kfree(ops->fontbuffer); > > + kfree(oldinfo->fbcon_par); > > + oldinfo->fbcon_par = NULL; > > + /* > > + If oldinfo and newinfo are driving the same hardware, > > + the fb_release() method of oldinfo may attempt to > > + restore the hardware state. This will leave the > > + newinfo in an undefined state. Thus, a call to > > + fb_set_par() may be needed for the newinfo. > > + */ > > + if (newinfo && newinfo->fbops->fb_set_par) { > > + ret = newinfo->fbops->fb_set_par(newinfo); > > - if (ret) > > - printk(KERN_ERR "con2fb_release_oldinfo: " > > - "detected unhandled fb_set_par error, " > > - "error code %d\n", ret); > > - } > > + if (ret) > > + printk(KERN_ERR "con2fb_release_oldinfo: " > > + "detected unhandled fb_set_par error, " > > + "error code %d\n", ret); > > } > > - return err; > > + return 0; > > } > > static void con2fb_init_display(struct vc_data *vc, struct fb_info *info, > > @@ -919,7 +926,6 @@ static const char *fbcon_startup(void) > > struct fbcon_display *p = &fb_display[fg_console]; > > struct vc_data *vc = vc_cons[fg_console].d; > > const struct font_desc *font = NULL; > > - struct module *owner; > > struct fb_info *info = NULL; > > struct fbcon_ops *ops; > > int rows, cols; > > @@ -938,17 +944,12 @@ static const char *fbcon_startup(void) > > if (!info) > > return NULL; > > > > - owner = info->fbops->owner; > > - if (!try_module_get(owner)) > > + if (fbcon_open(info)) > > return NULL; > > - if (info->fbops->fb_open && info->fbops->fb_open(info, 0)) { > > - module_put(owner); > > - return NULL; > > - } > > ops = kzalloc(sizeof(struct fbcon_ops), GFP_KERNEL); > > if (!ops) { > > - module_put(owner); > > + fbcon_release(info); > > return NULL; > > } > > @@ -3314,10 +3315,6 @@ static void fbcon_exit(void) > > } > > if (mapped) { > > - if (info->fbops->fb_release) > > - info->fbops->fb_release(info, 0); > > - module_put(info->fbops->owner); > > - > > if (info->fbcon_par) { > > struct fbcon_ops *ops = info->fbcon_par; > > @@ -3327,6 +3324,8 @@ static void fbcon_exit(void) > > kfree(info->fbcon_par); > > info->fbcon_par = NULL; > > } > > + > > + fbcon_release(info); > > } > > } > > } > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch