On 7/1/22 16:52, Geert Uytterhoeven wrote: > Hi Helge, > > On Fri, Jul 1, 2022 at 11:30 AM Helge Deller <deller@xxxxxx> wrote: >> On 7/1/22 09:28, Geert Uytterhoeven wrote: >>> On Thu, Jun 30, 2022 at 10:10 PM Helge Deller <deller@xxxxxx> wrote: >>>> On 6/30/22 22:00, Geert Uytterhoeven wrote: >>>>> On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller@xxxxxx> wrote: >>>>>> On 6/30/22 21:36, Geert Uytterhoeven wrote: >>>>>>> On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@xxxxxx> wrote: >>>>>>>> On 6/30/22 21:00, Geert Uytterhoeven wrote: >>>>>>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@xxxxxx> wrote: >>>>>>>>>> The virtual screen size can't be smaller than the physical screen size. >>>>>>>>>> Based on the general rule that we round up user-provided input if >>>>>>>>>> neccessary, adjust the virtual screen size as well if needed. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Helge Deller <deller@xxxxxx> >>>>>>>>>> Cc: stable@xxxxxxxxxxxxxxx # v5.4+ >>>>>>>>> >>>>>>>>> Thanks for your patch! >>>>>>>>> >>>>>>>>>> --- a/drivers/video/fbdev/core/fbmem.c >>>>>>>>>> +++ b/drivers/video/fbdev/core/fbmem.c >>>>>>>>>> @@ -1106,6 +1106,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, >>>>>>>>>> return -EFAULT; >>>>>>>>>> console_lock(); >>>>>>>>>> lock_fb_info(info); >>>>>>>>>> + /* adjust virtual screen size if user missed it */ >>>>>>>>>> + if (var.xres_virtual < var.xres) >>>>>>>>>> + var.xres_virtual = var.xres; >>>>>>>>>> + if (var.yres_virtual < var.yres) >>>>>>>>>> + var.yres_virtual = var.yres; >>>>>>>>>> ret = fb_set_var(info, &var); >>>>>>>>>> if (!ret) >>>>>>>>>> fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL); >>>>>>>>> >>>>>>>>> Given "[PATCH 4/5] fbmem: Prevent invalid virtual screen sizes in >>>>>>>>> fb_set_var", I don't think we need this patch. >>>>>>>> >>>>>>>> We do. >>>>>>> >>>>>>> Why? It will be caught by [PATCH 4/5]. >>>>>> >>>>>> Right, it will be caught by patch #4. >>>>>> But if you drop this part, then everytime a user runs >>>>>> fbset -xres 800 -yres 600 -xvres 200 >>>>>> users will get the KERNEL BUG WARNING (from patch #4) including >>>>>> a kernel backtrace in their syslogs. >>>>> >>>>> No, they will only see that warning if they are using a broken fbdev >>>>> driver that implements .fb_check_var(), but fails to validate or >>>>> update the passed geometry. >>>> >>>> IMHO this argument is mood. >>>> That way you put pressure on and need such simple code in >>>> each single driver to fix it up, instead of cleaning it up at a central >>>> place. >>> >>> Most hardware has restrictions on resolution (e.g. xres must be a >>> multiple of N), so the driver has to round up the resolution to make >>> it fit. And after that the driver has to validate and update the >>> virtual resolution again anyway... >>> >>> If a driver does not support changing the video mode, it can leave >>> out the .fb_check_var() and .fb_set_par() callbacks, so the fbdev >>> core will ignore the userspace-supplied parameters, and reinstate >>> the single supported mode. See e.g. "[PATCH] drm/fb-helper: >>> Remove helpers to change frame buffer config" >>> (https://lore.kernel.org/all/20220629105658.1373770-1-geert@xxxxxxxxxxxxxx). >> >> I implemented all of your suggested changes - from this mail and the others. >> I've committed a new testing tree to the fbcon-fix-testing branch at: >> https://github.com/hdeller/linux/tree/fbcon-fix-testing >> The diff is here: >> https://github.com/torvalds/linux/compare/master...hdeller:linux:fbcon-fix-testing >> >> Although your idea is good since we now would find issues in the drivers, >> I don't think we want to commit it, since the testcase from >> the bug report then immediately crashes the kernel - see below. > > That is expected behavior with panic_on_warn? Oh well. You're right! The kernel config had panic_on_warn enabled, which I didn't noticed. I disabled that option and now it works: [ 147.430332][ T3171] WARNING: CPU: 0 PID: 3171 at drivers/video/fbdev/core/fbmem.c:1025 fb_set_var.cold+0x83/0x1bc .... [ 147.431126][ T3171] ---[ end trace 0000000000000000 ]--- [ 147.431132][ T3171] fbcon: Fix up invalid yres 0 for bochs-drmdrmfb > The right fix is to fix the broken .fb_check_var() implementation. Yep. I'll send this patch series now. Helge
