Alex Deucher alexander.deucher@amd.com writes:
Hello Alex,
In aperture_remove_conflicting_pci_devices(), we currently only call sysfb_disable() on vga class devices. This leads to the following problem when the pimary device is not VGA compatible:
- A PCI device with a non-VGA class is the boot display
- That device is probed first and it is not a VGA device so sysfb_disable() is not called, but the device resources are freed by aperture_detach_platform_device()
- Non-primary GPU has a VGA class and it ends up calling sysfb_disable()
- NULL pointer dereference via sysfb_disable() since the resources have already been freed by aperture_detach_platform_device() when it was called by the other device.
Fix this by passing a device pointer to sysfb_disable() and checking the device to determine if we should execute it or not.
v2: Fix build when CONFIG_SCREEN_INFO is not set
Fixes: 5ae3716cfdcd ("video/aperture: Only remove sysfb on the default vga pci device") 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: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Alex Deucher alexander.deucher@amd.com Cc: stable@vger.kernel.org
The patch looks good to me.
Reviewed-by: Javier Martinez Canillas javierm@redhat.com
I just have to minor comments below:
...
/**
- sysfb_disable() - disable the Generic System Framebuffers support
- @dev: the device to check if non-NULL
- This disables the registration of system framebuffer devices that match the
- generic drivers that make use of the system framebuffer set up by firmware.
@@ -61,8 +64,12 @@ static bool sysfb_unregister(void)
- Context: The function can sleep. A @disable_lock mutex is acquired to serialize
against sysfb_init(), that registers a system framebuffer device.*/ -void sysfb_disable(void) +void sysfb_disable(struct device *dev) {
- struct screen_info *si = &screen_info;
- if (dev && dev != sysfb_parent_dev(si))
return;
Does this need to be protected by the disable_lock mutex? i.e:
mutex_lock(&disable_lock); if (!dev || dev == sysfb_parent_dev(si) { sysfb_unregister(); disabled = true; } mutex_unlock(&disable_lock);
...
@@ -353,8 +353,7 @@ int aperture_remove_conflicting_pci_devices(struct pci_dev *pdev, const char *na if (pdev == vga_default_device()) primary = true;
- if (primary)
sysfb_disable();
- sysfb_disable(&pdev->dev);
After this change the primary variable is only used to determine whether __aperture_remove_legacy_vga_devices(pdev) should be called or not. So I wonder if could just be dropped and instead have:
/* * If this is the primary adapter, there could be a VGA device * that consumes the VGA framebuffer I/O range. Remove this * device as well. */ if (pdev == vga_default_device()) ret = __aperture_remove_legacy_vga_devices(pdev);