Message-ID: <a01f6a7e-c56e-4a78-8605-ab5b253d9c9e@suse.de>
Date: Mon, 15 Jul 2024 11:22:15 +0200
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Tj <tj.iam.tj@...ton.me>
Cc: Marek Olsak <Marek.Olsak@....com>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "1075713@...s.debian.org" <1075713@...s.debian.org>
Subject: Re: Regression: firmware/sysfb.c device path

Hi

Am 15.07.24 um 11:03 schrieb Tj:
> On Monday, 15 July 2024 at 07:44, Thomas Zimmermann <tzimmermann@...e.de> wrote:
>
>>> See hw/xfree86/fbdevhw/fbdevhw.c::fbdev_open()
>>>
>>> https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/hw/xfree86/fbdevhw/fbdevhw.c?ref_type=heads#L381
>>
>> Amazing debugging skills!
>>
>> The patch that causes the regression in X sets the PCI device as parent
>> for the VESA framebuffer. That means that the PCI device won't be
>> unplugged or suspended while the VESA framebuffer is still in use.
>> Without, results are undefined.
>>
>> Therefore, could this be fixed in X' fbdev driver?
> I was in two minds about this one. On the 'perfection' side I agree Xorg should not rely on a symbolic link at all - though that is easy to say; I wasn't the one that had to find a way to do what fbdevhw has to do at the time!
>
> But the other me argues that kernel ought not to break userspace and as it is a pretty common scenario across distros that userspace stays relatively stable (sans security bugs) but kernels can and do move on quite rapidly and frequently the kernel ought to play nice here :)
>
> With this in mind I can foresee many systems that work perfectly fine will break when only the kernel is upgraded and thus it'll have the finger of blame pointed at it.
>
> Since this is more likely to strike in automated test harness scenarios (due to it being relatively obscure) as well there may well be circumstances where changing a userspace library immediately is extremely problematical.
>
> I wonder if there's some half-way deprecation measure here that would allow a period of transition?
>
> I can imagine one way would be a custom udev rule (for systemd-udevd hosts - not sure about non-udevd) that replaces the new symlink with the old - but if a distro does that it's almost as easy to patch and rebuild fbdevhw - and far more brittle.

I agree with the assessment. Luckily not too many systems use fbdevhw 
these days and updating X isn't much more effort than updating the kernel.

And OTOH the kernel patch fixes a real problem. If the PCI device 
suspends or does a hotunplug, the state of the VESA/EFI framebuffers 
becomes undefined (with undefined results). So reverting it is not a 
good option either.

>
>> A fix could look like this:
>>
>> 1) readlink /sys/class/graphics/fb0/device/subsystem
>>   2) strcmp to "bus/pci"
> This works and doesn't cause a regression in v6.8.12:

These device/subsystem paths have been stable since forever. That should 
most likely not change.

>
> ~ # uname -r; grep fbdev /var/log/Xorg.0.log
> 6.9.7-amd64
> [    31.376] (==) Matched fbdev as autoconfigured driver 2
> [    31.377] (II) LoadModule: "fbdev"
> [    31.377] (II) Loading /usr/lib/xorg/modules/drivers/fbdev_drv.so
> [    31.377] (II) Module fbdev: vendor="X.Org Foundation"
> [    31.377] (II) FBDEV: driver for framebuffer: fbdev
> [    31.410] fbdev trace: FBDevPciProbe()
> [    31.410] (II) Loading sub module "fbdevhw"
> [    31.410] (II) LoadModule: "fbdevhw"
> [    31.410] (II) Loading /usr/lib/xorg/modules/libfbdevhw.so
> [    31.410] (II) Module fbdevhw: vendor="X.Org Foundation"
> [    31.410] fbdev trace: FBDevPciProbe() return
> [    31.410] (WW) Falling back to old probe method for fbdev
> [    31.410] fbdev trace: FBDevProbe()
> [    31.410] (II) Loading sub module "fbdevhw"
> [    31.410] (II) LoadModule: "fbdevhw"
> [    31.410] (II) Loading /usr/lib/xorg/modules/libfbdevhw.so
> [    31.410] (II) Module fbdevhw: vendor="X.Org Foundation"
> [    31.410] fbdev: FBDevProbe() for() numDevSection=0
> [    31.410] fbdev: FBDevProbe() isPci0 isISA=0
> [    31.410] fbdev: FBDevProbe() calling fbdevHWProbe(NULL, '(null)', NULL)
> [    31.410] (II) fbdev_open(scrnIndex=-1, dev=(null), namep=(nil))
> [    31.410] (II) fbdev_open() using dev from env FRAMEBUFFER=(null)
> [    31.410] (II) fbdev_open() using default dev=/dev/fb0
> [    31.410] (II) fbdev_open() sysfs_path=/sys/class/graphics/fb0/device/subsystem
> [    31.410] (II) fbdev_open() buf=../../../../bus/platform
> [    31.410] (II) fbdev_open() returning file descriptor 11
> [    31.410] fbdev trace: FBDevProbe() fbdevHWProbe()
> [    31.410] fbdev trace: FBDevProbe() else xf86ClaimFbSlot()
> [    31.410] fbdev trace: FBDevProbe() return
> [    31.410] (II) UnloadModule: "fbdev"
> [    31.410] (II) UnloadSubModule: "fbdevhw"
> [    31.410] fbdev: PreInit 0
> [    31.410] (II) FBDEV(0): fbdev_open(scrnIndex=0, dev=(null), namep=(nil))
> [    31.410] (II) FBDEV(0): fbdev_open() using dev from env FRAMEBUFFER=(null)
> [    31.410] (II) FBDEV(0): fbdev_open() using default dev=/dev/fb0
> [    31.410] (II) FBDEV(0): fbdev_open() sysfs_path=/sys/class/graphics/fb0/device/subsystem
> [    31.410] (II) FBDEV(0): fbdev_open() buf=../../../../bus/platform
> [    31.410] (II) FBDEV(0): fbdev_open() returning file descriptor 12
>
> ~ # uname -r; grep fbdev /var/log/Xorg.0.log
> 6.8.12-amd64
> [    14.225] (==) Matched fbdev as autoconfigured driver 2
> [    14.225] (II) LoadModule: "fbdev"
> [    14.225] (II) Loading /usr/lib/xorg/modules/drivers/fbdev_drv.so
> [    14.225] (II) Module fbdev: vendor="X.Org Foundation"
> [    14.225] (II) FBDEV: driver for framebuffer: fbdev
> [    14.252] fbdev trace: FBDevPciProbe()
> [    14.252] (II) Loading sub module "fbdevhw"
> [    14.252] (II) LoadModule: "fbdevhw"
> [    14.252] (II) Loading /usr/lib/xorg/modules/libfbdevhw.so
> [    14.252] (II) Module fbdevhw: vendor="X.Org Foundation"
> [    14.253] fbdev trace: FBDevPciProbe() return
> [    14.253] (WW) Falling back to old probe method for fbdev
> [    14.253] fbdev trace: FBDevProbe()
> [    14.253] (II) Loading sub module "fbdevhw"
> [    14.253] (II) LoadModule: "fbdevhw"
> [    14.253] (II) Loading /usr/lib/xorg/modules/libfbdevhw.so
> [    14.253] (II) Module fbdevhw: vendor="X.Org Foundation"
> [    14.253] fbdev: FBDevProbe() for() numDevSection=0
> [    14.253] fbdev: FBDevProbe() isPci0 isISA=0
> [    14.253] fbdev: FBDevProbe() calling fbdevHWProbe(NULL, '(null)', NULL)
> [    14.253] (II) fbdev_open(scrnIndex=-1, dev=(null), namep=(nil))
> [    14.253] (II) fbdev_open() using dev from env FRAMEBUFFER=(null)
> [    14.253] (II) fbdev_open() using default dev=/dev/fb0
> [    14.253] (II) fbdev_open() sysfs_path=/sys/class/graphics/fb0/device/subsystem
> [    14.253] (II) fbdev_open() buf=../../../bus/platform
> [    14.253] (II) fbdev_open() returning file descriptor 11
> [    14.253] fbdev trace: FBDevProbe() fbdevHWProbe()
> [    14.253] fbdev trace: FBDevProbe() else xf86ClaimFbSlot()
> [    14.253] fbdev trace: FBDevProbe() return
> [    14.253] (II) UnloadModule: "fbdev"
> [    14.253] (II) UnloadSubModule: "fbdevhw"
> [    14.253] fbdev: PreInit 0
> [    14.253] (II) FBDEV(0): fbdev_open(scrnIndex=0, dev=(null), namep=(nil))
> [    14.253] (II) FBDEV(0): fbdev_open() using dev from env FRAMEBUFFER=(null)
> [    14.253] (II) FBDEV(0): fbdev_open() using default dev=/dev/fb0
> [    14.253] (II) FBDEV(0): fbdev_open() sysfs_path=/sys/class/graphics/fb0/device/subsystem
> [    14.253] (II) FBDEV(0): fbdev_open() buf=../../../bus/platform
> [    14.253] (II) FBDEV(0): fbdev_open() returning file descriptor 12
>
> I'll propose this patch in Debian Xorg for Unstable/Testing but not sure if it'll be accepted for Stable.

It should not be a problem, as only 6.9 is affected. Debian stable does 
update to such newer kernels, right?

We should definitely get your patch into the Xorg upstream.

Best regards
Thomas


-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Powered by blists - more mailing lists