| Message ID | 20240819165341.799848-1-alexander.deucher@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] video/aperture: match the pci device when calling sysfb_disable() | expand |
I forgot to update the patch title but it should probably be something like: video/aperture: optionally match the device in sysfb_disable() Alex On Mon, Aug 19, 2024 at 1:00 PM Alex Deucher <alexander.deucher@amd.com> wrote: > > 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: > > 1. A PCI device with a non-VGA class is the boot display > 2. 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() > 3. Non-primary GPU has a VGA class and it ends up calling sysfb_disable() > 4. 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 > --- > drivers/firmware/sysfb.c | 11 +++++++++-- > drivers/of/platform.c | 2 +- > drivers/video/aperture.c | 5 ++--- > include/linux/sysfb.h | 4 ++-- > 4 files changed, 14 insertions(+), 8 deletions(-) > > diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c > index 880ffcb500887..033a044af2646 100644 > --- a/drivers/firmware/sysfb.c > +++ b/drivers/firmware/sysfb.c > @@ -39,6 +39,8 @@ static struct platform_device *pd; > static DEFINE_MUTEX(disable_lock); > static bool disabled; > > +static struct device *sysfb_parent_dev(const struct screen_info *si); > + > static bool sysfb_unregister(void) > { > if (IS_ERR_OR_NULL(pd)) > @@ -52,6 +54,7 @@ static bool sysfb_unregister(void) > > /** > * 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; > mutex_lock(&disable_lock); > sysfb_unregister(); > disabled = true; > @@ -93,7 +100,7 @@ static __init bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev) > } > #endif > > -static __init struct device *sysfb_parent_dev(const struct screen_info *si) > +static struct device *sysfb_parent_dev(const struct screen_info *si) > { > struct pci_dev *pdev; > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index 389d4ea6bfc15..ef622d41eb5b2 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -592,7 +592,7 @@ static int __init of_platform_default_populate_init(void) > * This can happen for example on DT systems that do EFI > * booting and may provide a GOP handle to the EFI stub. > */ > - sysfb_disable(); > + sysfb_disable(NULL); > of_platform_device_create(node, NULL, NULL); > of_node_put(node); > } > diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c > index 561be8feca96c..b23d85ceea104 100644 > --- a/drivers/video/aperture.c > +++ b/drivers/video/aperture.c > @@ -293,7 +293,7 @@ int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t si > * ask for this, so let's assume that a real driver for the display > * was already probed and prevent sysfb to register devices later. > */ > - sysfb_disable(); > + sysfb_disable(NULL); > > aperture_detach_devices(base, size); > > @@ -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); > > for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) { > if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM)) > diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h > index c9cb657dad08a..bef5f06a91de6 100644 > --- a/include/linux/sysfb.h > +++ b/include/linux/sysfb.h > @@ -58,11 +58,11 @@ struct efifb_dmi_info { > > #ifdef CONFIG_SYSFB > > -void sysfb_disable(void); > +void sysfb_disable(struct device *dev); > > #else /* CONFIG_SYSFB */ > > -static inline void sysfb_disable(void) > +static inline void sysfb_disable(struct device *dev) > { > } > > -- > 2.46.0 >
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: > > 1. A PCI device with a non-VGA class is the boot display > 2. 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() > 3. Non-primary GPU has a VGA class and it ends up calling sysfb_disable() > 4. 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);
Hi Alex,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-tip/drm-tip]
[also build test WARNING on robh/for-next drm-misc/drm-misc-next linus/master v6.11-rc4 next-20240820]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Alex-Deucher/video-aperture-match-the-pci-device-when-calling-sysfb_disable/20240820-005528
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link: https://lore.kernel.org/r/20240819165341.799848-1-alexander.deucher%40amd.com
patch subject: [PATCH V2] video/aperture: match the pci device when calling sysfb_disable()
config: i386-randconfig-054-20240820 (https://download.01.org/0day-ci/archive/20240821/202408210620.nTCwLpCO-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240821/202408210620.nTCwLpCO-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408210620.nTCwLpCO-lkp@intel.com/
All warnings (new ones prefixed by >>, old ones prefixed by <<):
>> WARNING: modpost: vmlinux: section mismatch in reference: sysfb_disable+0x7a (section: .text) -> sysfb_pci_dev_is_enabled (section: .init.text)
WARNING: modpost: missing MODULE_DESCRIPTION() in lib/test_objpool.o
WARNING: modpost: missing MODULE_DESCRIPTION() in lib/asn1_decoder.o
diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c index 880ffcb500887..033a044af2646 100644 --- a/drivers/firmware/sysfb.c +++ b/drivers/firmware/sysfb.c @@ -39,6 +39,8 @@ static struct platform_device *pd; static DEFINE_MUTEX(disable_lock); static bool disabled; +static struct device *sysfb_parent_dev(const struct screen_info *si); + static bool sysfb_unregister(void) { if (IS_ERR_OR_NULL(pd)) @@ -52,6 +54,7 @@ static bool sysfb_unregister(void) /** * 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; mutex_lock(&disable_lock); sysfb_unregister(); disabled = true; @@ -93,7 +100,7 @@ static __init bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev) } #endif -static __init struct device *sysfb_parent_dev(const struct screen_info *si) +static struct device *sysfb_parent_dev(const struct screen_info *si) { struct pci_dev *pdev; diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 389d4ea6bfc15..ef622d41eb5b2 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -592,7 +592,7 @@ static int __init of_platform_default_populate_init(void) * This can happen for example on DT systems that do EFI * booting and may provide a GOP handle to the EFI stub. */ - sysfb_disable(); + sysfb_disable(NULL); of_platform_device_create(node, NULL, NULL); of_node_put(node); } diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c index 561be8feca96c..b23d85ceea104 100644 --- a/drivers/video/aperture.c +++ b/drivers/video/aperture.c @@ -293,7 +293,7 @@ int aperture_remove_conflicting_devices(resource_size_t base, resource_size_t si * ask for this, so let's assume that a real driver for the display * was already probed and prevent sysfb to register devices later. */ - sysfb_disable(); + sysfb_disable(NULL); aperture_detach_devices(base, size); @@ -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); for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) { if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM)) diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h index c9cb657dad08a..bef5f06a91de6 100644 --- a/include/linux/sysfb.h +++ b/include/linux/sysfb.h @@ -58,11 +58,11 @@ struct efifb_dmi_info { #ifdef CONFIG_SYSFB -void sysfb_disable(void); +void sysfb_disable(struct device *dev); #else /* CONFIG_SYSFB */ -static inline void sysfb_disable(void) +static inline void sysfb_disable(struct device *dev) { }
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: 1. A PCI device with a non-VGA class is the boot display 2. 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() 3. Non-primary GPU has a VGA class and it ends up calling sysfb_disable() 4. 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 --- drivers/firmware/sysfb.c | 11 +++++++++-- drivers/of/platform.c | 2 +- drivers/video/aperture.c | 5 ++--- include/linux/sysfb.h | 4 ++-- 4 files changed, 14 insertions(+), 8 deletions(-)