Messages in this thread | | | Date | Thu, 12 Jan 2023 08:48:52 +0100 | Subject | Re: [PATCH 11/11] video/aperture: Only remove sysfb on the default vga pci device | From | Thomas Zimmermann <> |
| |
Hi
Am 11.01.23 um 17:37 schrieb Daniel Vetter: > On Wed, Jan 11, 2023 at 05:20:00PM +0100, Thomas Zimmermann wrote: >> Hi >> >> Am 11.01.23 um 16:41 schrieb Daniel Vetter: >>> This fixes a regression introduced by ee7a69aa38d8 ("fbdev: Disable >>> sysfb device registration when removing conflicting FBs"), where we >>> remove the sysfb when loading a driver for an unrelated pci device, >>> resulting in the user loosing their efifb console or similar. >>> >>> Note that in practice this only is a problem with the nvidia blob, >>> because that's the only gpu driver people might install which does not >>> come with an fbdev driver of it's own. For everyone else the real gpu >>> driver will restor a working console. >>> >>> Also note that in the referenced bug there's confusion that this same >>> bug also happens on amdgpu. But that was just another amdgpu specific >>> regression, which just happened to happen at roughly the same time and >>> with the same user-observable symptons. That bug is fixed now, see >>> https://bugzilla.kernel.org/show_bug.cgi?id=216331#c15 >>> >>> For the above reasons the cc: stable is just notionally, this patch >>> will need a backport and that's up to nvidia if they care enough. >>> >>> References: https://bugzilla.kernel.org/show_bug.cgi?id=216303#c28 >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>> Cc: Aaron Plattner <aplattner@nvidia.com> >>> Cc: Javier Martinez Canillas <javierm@redhat.com> >>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>> Cc: Helge Deller <deller@gmx.de> >>> Cc: Sam Ravnborg <sam@ravnborg.org> >>> Cc: Alex Deucher <alexander.deucher@amd.com> >>> Cc: <stable@vger.kernel.org> # v5.19+ (if someone else does the backport) >>> --- >>> drivers/video/aperture.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c >>> index ba565515480d..a1821d369bb1 100644 >>> --- a/drivers/video/aperture.c >>> +++ b/drivers/video/aperture.c >>> @@ -321,15 +321,16 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na >>> primary = pdev == vga_default_device(); >>> + if (primary) >>> + sysfb_disable(); >>> + >> >> There's another sysfb_disable() in aperture_remove_conflicting_devices() >> without the branch but with a long comment. I find this slightly confusing. >> >> I'd rather add a branched sysfb_disable() plus the comment to >> aperture_detach_devices(). And then add a 'primary' parameter to >> aperture_detach_devices(). In aperture_remove_conflicting_devices() the >> parameter would be unconditionally true. > > Yeah I was on the fence, but should be easy to redo with all the prep work > out of the way. It does mean we call sysfb_disable once for every bar, but > that shouldn't matter in any reasonable case :-)
Or leave it as is. It's not so important. The idea of the current design was that aperture_remove_conflicting_device() would be the general implementation and aperture_remove_conflicting_pci_device() would be a helper that only detects the correct PCI BAR. That never really worked in practice.
Best regards Thomas
> -Daniel > >> >> Best regards >> Thomas >> >>> for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) { >>> if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM)) >>> continue; >>> base = pci_resource_start(pdev, bar); >>> size = pci_resource_len(pdev, bar); >>> - ret = aperture_remove_conflicting_devices(base, size, name); >>> - if (ret) >>> - return ret; >>> + aperture_detach_devices(base, size); >>> } >>> if (!primary) >> >> -- >> 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 > > > >
-- 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 [unhandled content-type:application/pgp-signature] | |