From: Helge Deller <deller@gmx.de> To: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Helge Deller <deller@gmx.de>, DRI Development <dri-devel@lists.freedesktop.org>, Daniel Vetter <daniel.vetter@ffwll.ch>, Linux Fbdev development list <linux-fbdev@vger.kernel.org> Subject: Re: [PATCH v2 3/4] fbmem: Prevent invalid virtual screen sizes in fb_set_var() Date: Sun, 3 Jul 2022 15:53:56 +0200 [thread overview] Message-ID: <YsGfdEO4lU+y2004@p100> (raw) In-Reply-To: <CAMuHMdUTwM+y+yi5ASY9hQLgJD-4pjtStGA9m82853LmbdywOA@mail.gmail.com> * Geert Uytterhoeven <geert@linux-m68k.org>: > > --- a/drivers/video/fbdev/core/fbmem.c > > +++ b/drivers/video/fbdev/core/fbmem.c > > @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) > > if (ret) > > return ret; > > > > + /* make sure virtual resolution >= physical resolution */ > > + if (WARN_ON(var->xres_virtual < var->xres)) { > > WARN_ON_ONCE()? > This does mean we would miss two or more buggy drivers in a single system. > > > + pr_warn("fbcon: Fix up invalid xres %d for %s\n", > > xres_virtual? > > > + var->xres_virtual, info->fix.id); > > + var->xres_virtual = var->xres; > > I think it's better to not fix this up, but return -EINVAL instead. > After all if we get here, we have a buggy driver that needs to be fixed. > > > + } > > + if (WARN_ON(var->yres_virtual < var->yres)) { > > + pr_warn("fbcon: Fix up invalid yres %d for %s\n", > > + var->yres_virtual, info->fix.id); > > + var->yres_virtual = var->yres; > > + } > > Same for yres. Geert, thanks for your valuable feedback! In general I don't like for this case any of the WARN_ON* functions. They don't provide any useful info for us, dumps unneccessarily the stack backtrace and would annoy existing users. We know, that DRM drivers can't change the resolution. If we would leave in any kind of warning, all DRM users will ask back - and we don't have a solution for them. It's also no regression, because it didn't worked before either. But we want to detect fbdev drivers which behave badly - so we should print that info with the driver name. Below is a new patch which addresses this. The search for drm drivers looks somewhat hackish - maybe someone can propose a better approach? Thoughts? Helge From 0f33e2a3730342ab85df372f80b4f61762fb1130 Mon Sep 17 00:00:00 2001 From: Helge Deller <deller@gmx.de> Date: Wed, 29 Jun 2022 15:53:55 +0200 Subject: [PATCH] fbmem: Check virtual screen sizes in fb_set_var() Verify that the fbdev or drm driver correctly adjusted the virtual screen sizes. On failure report (in case of fbdev drivers) the failing driver. Signed-off-by: Helge Deller <deller@gmx.de> Cc: stable@vger.kernel.org # v5.4+ diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 160389365a36..9b75aa4405ee 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1016,6 +1016,21 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) if (ret) return ret; + /* verify that virtual resolution >= physical resolution */ + if (var->xres_virtual < var->xres || + var->yres_virtual < var->yres) { + /* drm drivers don't support mode changes yet. Do not report. */ + if (strstr(info->fix.id, "drm")) + return -ENOTSUPP; + + pr_warn("WARNING: fbcon: Driver %s missed to adjust virtual" + " screen size (%dx%d vs. %dx%d)\n", + info->fix.id, + var->xres_virtual, var->yres_virtual, + var->xres, var->yres); + return -EINVAL; + } + if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW) return 0;
WARNING: multiple messages have this Message-ID (diff)
From: Helge Deller <deller@gmx.de> To: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, Helge Deller <deller@gmx.de>, Linux Fbdev development list <linux-fbdev@vger.kernel.org>, DRI Development <dri-devel@lists.freedesktop.org> Subject: Re: [PATCH v2 3/4] fbmem: Prevent invalid virtual screen sizes in fb_set_var() Date: Sun, 3 Jul 2022 15:53:56 +0200 [thread overview] Message-ID: <YsGfdEO4lU+y2004@p100> (raw) In-Reply-To: <CAMuHMdUTwM+y+yi5ASY9hQLgJD-4pjtStGA9m82853LmbdywOA@mail.gmail.com> * Geert Uytterhoeven <geert@linux-m68k.org>: > > --- a/drivers/video/fbdev/core/fbmem.c > > +++ b/drivers/video/fbdev/core/fbmem.c > > @@ -1016,6 +1016,18 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) > > if (ret) > > return ret; > > > > + /* make sure virtual resolution >= physical resolution */ > > + if (WARN_ON(var->xres_virtual < var->xres)) { > > WARN_ON_ONCE()? > This does mean we would miss two or more buggy drivers in a single system. > > > + pr_warn("fbcon: Fix up invalid xres %d for %s\n", > > xres_virtual? > > > + var->xres_virtual, info->fix.id); > > + var->xres_virtual = var->xres; > > I think it's better to not fix this up, but return -EINVAL instead. > After all if we get here, we have a buggy driver that needs to be fixed. > > > + } > > + if (WARN_ON(var->yres_virtual < var->yres)) { > > + pr_warn("fbcon: Fix up invalid yres %d for %s\n", > > + var->yres_virtual, info->fix.id); > > + var->yres_virtual = var->yres; > > + } > > Same for yres. Geert, thanks for your valuable feedback! In general I don't like for this case any of the WARN_ON* functions. They don't provide any useful info for us, dumps unneccessarily the stack backtrace and would annoy existing users. We know, that DRM drivers can't change the resolution. If we would leave in any kind of warning, all DRM users will ask back - and we don't have a solution for them. It's also no regression, because it didn't worked before either. But we want to detect fbdev drivers which behave badly - so we should print that info with the driver name. Below is a new patch which addresses this. The search for drm drivers looks somewhat hackish - maybe someone can propose a better approach? Thoughts? Helge From 0f33e2a3730342ab85df372f80b4f61762fb1130 Mon Sep 17 00:00:00 2001 From: Helge Deller <deller@gmx.de> Date: Wed, 29 Jun 2022 15:53:55 +0200 Subject: [PATCH] fbmem: Check virtual screen sizes in fb_set_var() Verify that the fbdev or drm driver correctly adjusted the virtual screen sizes. On failure report (in case of fbdev drivers) the failing driver. Signed-off-by: Helge Deller <deller@gmx.de> Cc: stable@vger.kernel.org # v5.4+ diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 160389365a36..9b75aa4405ee 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1016,6 +1016,21 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var) if (ret) return ret; + /* verify that virtual resolution >= physical resolution */ + if (var->xres_virtual < var->xres || + var->yres_virtual < var->yres) { + /* drm drivers don't support mode changes yet. Do not report. */ + if (strstr(info->fix.id, "drm")) + return -ENOTSUPP; + + pr_warn("WARNING: fbcon: Driver %s missed to adjust virtual" + " screen size (%dx%d vs. %dx%d)\n", + info->fix.id, + var->xres_virtual, var->yres_virtual, + var->xres, var->yres); + return -EINVAL; + } + if ((var->activate & FB_ACTIVATE_MASK) != FB_ACTIVATE_NOW) return 0;
next prev parent reply other threads:[~2022-07-03 13:54 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-01 20:23 [PATCH v2 0/4] fbcon: Fixes for screen resolution changes - round 2 Helge Deller 2022-07-01 20:23 ` [PATCH v2 1/4] fbcon: Disallow setting font bigger than screen size Helge Deller 2022-07-01 20:23 ` [PATCH v2 2/4] fbcon: Prevent that screen size is smaller than font size Helge Deller 2022-07-03 8:50 ` Geert Uytterhoeven 2022-07-03 8:50 ` Geert Uytterhoeven 2022-07-01 20:23 ` [PATCH v2 3/4] fbmem: Prevent invalid virtual screen sizes in fb_set_var() Helge Deller 2022-07-03 8:55 ` Geert Uytterhoeven 2022-07-03 8:55 ` Geert Uytterhoeven 2022-07-03 13:53 ` Helge Deller [this message] 2022-07-03 13:53 ` Helge Deller 2022-07-03 14:41 ` Geert Uytterhoeven 2022-07-03 14:41 ` Geert Uytterhoeven 2022-07-04 8:38 ` Helge Deller 2022-07-04 8:38 ` Helge Deller 2022-07-01 20:23 ` [PATCH v2 4/4] fbcon: Use fbcon_info_from_console() in fbcon_modechange_possible() Helge Deller 2022-07-01 20:25 ` [PATCH v2 0/4] fbcon: Fixes for screen resolution changes - round 2 Helge Deller -- strict thread matches above, loose matches on Subject: below -- 2022-07-01 20:22 Helge Deller 2022-07-01 20:22 ` [PATCH v2 3/4] fbmem: Prevent invalid virtual screen sizes in fb_set_var() Helge Deller
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YsGfdEO4lU+y2004@p100 \ --to=deller@gmx.de \ --cc=daniel.vetter@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=geert@linux-m68k.org \ --cc=linux-fbdev@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.