| Message ID | 20220131210552.482606-7-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | some fbcon patches, mostly locking | expand |
Hi Daniel, On Mon, Jan 31, 2022 at 10:05:37PM +0100, Daniel Vetter wrote: > Before > > 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 > > it was possible to load fbcon and fbdev drivers in any order, which > means that fbcon init had to handle the case where fbdev drivers where > already registered. > > This is no longer possible, hence delete that code. > > Note that the exit case is a bit more complex and will be done in a > separate patch. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Helge Deller <deller@gmx.de> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Claudio Suarez <cssk@net-c.es> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Cc: Du Cheng <ducheng2@gmail.com> > --- > drivers/video/fbdev/core/fbcon.c | 13 +------------ > 1 file changed, 1 insertion(+), 12 deletions(-) > > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c > index 8f971de35885..814b648e8f09 100644 > --- a/drivers/video/fbdev/core/fbcon.c > +++ b/drivers/video/fbdev/core/fbcon.c > @@ -942,7 +942,7 @@ static const char *fbcon_startup(void) > return display_desc; > /* > * Instead of blindly using registered_fb[0], we use info_idx, set by > - * fb_console_init(); > + * fbcon_fb_registered(); > */ This comment change looks like it does not belong in this patch. Also, the comment is wrong as info_idx is set in several places. Like set_con2fb_map(), fbcon_remap_all(), and more. Though it is not set by fb_console_init - so partly OK. With the comment adjustment dropped. Acked-by: Sam Ravnborg <sam@ravnborg.org> at least the code deletion looked OK, I failed to follow all the logic. So would be good if someone else could ack it too. Sam > info = registered_fb[info_idx]; > if (!info) > @@ -3316,17 +3316,6 @@ static void fbcon_start(void) > return; > } > #endif > - > - if (num_registered_fb) { > - int i; > - > - for_each_registered_fb(i) { > - info_idx = i; > - break; > - } > - > - do_fbcon_takeover(0); > - } > } > > static void fbcon_exit(void) > -- > 2.33.0
On Thu, Feb 03, 2022 at 09:45:29PM +0100, Sam Ravnborg wrote: > Hi Daniel, > > On Mon, Jan 31, 2022 at 10:05:37PM +0100, Daniel Vetter wrote: > > Before > > > > 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 > > > > it was possible to load fbcon and fbdev drivers in any order, which > > means that fbcon init had to handle the case where fbdev drivers where > > already registered. > > > > This is no longer possible, hence delete that code. > > > > Note that the exit case is a bit more complex and will be done in a > > separate patch. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Helge Deller <deller@gmx.de> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Claudio Suarez <cssk@net-c.es> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > > Cc: Du Cheng <ducheng2@gmail.com> > > --- > > drivers/video/fbdev/core/fbcon.c | 13 +------------ > > 1 file changed, 1 insertion(+), 12 deletions(-) > > > > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c > > index 8f971de35885..814b648e8f09 100644 > > --- a/drivers/video/fbdev/core/fbcon.c > > +++ b/drivers/video/fbdev/core/fbcon.c > > @@ -942,7 +942,7 @@ static const char *fbcon_startup(void) > > return display_desc; > > /* > > * Instead of blindly using registered_fb[0], we use info_idx, set by > > - * fb_console_init(); > > + * fbcon_fb_registered(); > > */ > This comment change looks like it does not belong in this patch. > Also, the comment is wrong as info_idx is set in several places. > Like set_con2fb_map(), fbcon_remap_all(), and more. Yeah I can split this out into a separate patch, but I spotted this wrong comment as part of reviewing this code change here - essentially you have to check how fb_info for fbcon are registered and fbcon init happens to validate that deleting the below code is correct. Ok if I put this explainer into the commit message, or do you want me to split this out fully? -Daniel > > Though it is not set by fb_console_init - so partly OK. > > With the comment adjustment dropped. > Acked-by: Sam Ravnborg <sam@ravnborg.org> > > at least the code deletion looked OK, I failed to follow all the logic. > So would be good if someone else could ack it too. > > Sam > > > > > info = registered_fb[info_idx]; > > if (!info) > > @@ -3316,17 +3316,6 @@ static void fbcon_start(void) > > return; > > } > > #endif > > - > > - if (num_registered_fb) { > > - int i; > > - > > - for_each_registered_fb(i) { > > - info_idx = i; > > - break; > > - } > > - > > - do_fbcon_takeover(0); > > - } > > } > > > > static void fbcon_exit(void) > > -- > > 2.33.0
Hi Daniel, On Tue, Feb 08, 2022 at 02:42:25PM +0100, Daniel Vetter wrote: > On Thu, Feb 03, 2022 at 09:45:29PM +0100, Sam Ravnborg wrote: > > Hi Daniel, > > > > On Mon, Jan 31, 2022 at 10:05:37PM +0100, Daniel Vetter wrote: > > > Before > > > > > > 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 > > > > > > it was possible to load fbcon and fbdev drivers in any order, which > > > means that fbcon init had to handle the case where fbdev drivers where > > > already registered. > > > > > > This is no longer possible, hence delete that code. > > > > > > Note that the exit case is a bit more complex and will be done in a > > > separate patch. > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > Cc: Helge Deller <deller@gmx.de> > > > Cc: Daniel Vetter <daniel@ffwll.ch> > > > Cc: Claudio Suarez <cssk@net-c.es> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > > > Cc: Du Cheng <ducheng2@gmail.com> > > > --- > > > drivers/video/fbdev/core/fbcon.c | 13 +------------ > > > 1 file changed, 1 insertion(+), 12 deletions(-) > > > > > > diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c > > > index 8f971de35885..814b648e8f09 100644 > > > --- a/drivers/video/fbdev/core/fbcon.c > > > +++ b/drivers/video/fbdev/core/fbcon.c > > > @@ -942,7 +942,7 @@ static const char *fbcon_startup(void) > > > return display_desc; > > > /* > > > * Instead of blindly using registered_fb[0], we use info_idx, set by > > > - * fb_console_init(); > > > + * fbcon_fb_registered(); > > > */ > > This comment change looks like it does not belong in this patch. > > Also, the comment is wrong as info_idx is set in several places. > > Like set_con2fb_map(), fbcon_remap_all(), and more. > > Yeah I can split this out into a separate patch, but I spotted this wrong > comment as part of reviewing this code change here - essentially you have > to check how fb_info for fbcon are registered and fbcon init happens to > validate that deleting the below code is correct. > > Ok if I put this explainer into the commit message, or do you want me to > split this out fully? Keep it and add my a-b Sam
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index 8f971de35885..814b648e8f09 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -942,7 +942,7 @@ static const char *fbcon_startup(void) return display_desc; /* * Instead of blindly using registered_fb[0], we use info_idx, set by - * fb_console_init(); + * fbcon_fb_registered(); */ info = registered_fb[info_idx]; if (!info) @@ -3316,17 +3316,6 @@ static void fbcon_start(void) return; } #endif - - if (num_registered_fb) { - int i; - - for_each_registered_fb(i) { - info_idx = i; - break; - } - - do_fbcon_takeover(0); - } } static void fbcon_exit(void)
Before 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 it was possible to load fbcon and fbdev drivers in any order, which means that fbcon init had to handle the case where fbdev drivers where already registered. This is no longer possible, hence delete that code. Note that the exit case is a bit more complex and will be done in a separate patch. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Helge Deller <deller@gmx.de> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Claudio Suarez <cssk@net-c.es> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Du Cheng <ducheng2@gmail.com> --- drivers/video/fbdev/core/fbcon.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-)