| Message ID | 20220707153952.32264-6-tzimmermann@suse.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fbdev: Maintain device ownership with aperture helpers | expand |
On 7/7/22 17:39, Thomas Zimmermann wrote: > Convert fbdev drivers from fbdev's remove_conflicting_framebuffers() to > the framework-independent aperture_remove_conflicting_devices(). Calling > this function will also remove conflicting DRM drivers. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- [...] > static int lynxfb_kick_out_firmware_fb(struct pci_dev *pdev) > { > - struct apertures_struct *ap; > + resource_size_t base = pci_resource_start(pdev, 0); > + resource_size_t size = pci_resource_len(pdev, 0); > bool primary = false; > > - ap = alloc_apertures(1); > - if (!ap) > - return -ENOMEM; > - > - ap->ranges[0].base = pci_resource_start(pdev, 0); > - ap->ranges[0].size = pci_resource_len(pdev, 0); > #ifdef CONFIG_X86 > primary = pdev->resource[PCI_ROM_RESOURCE].flags & > IORESOURCE_ROM_SHADOW; > #endif > - remove_conflicting_framebuffers(ap, "sm750_fb1", primary); > - kfree(ap); > - return 0; > + > + return aperture_remove_conflicting_devices(base, size, primary, "sm750_fb1"); Do you know why this can't just use aperture_remove_conflicting_pci_devices() ? It seems that the driver is open coding part of the logic already in that helper. For example, figuring out if is a primary by checking the IORESOURCE_ROM_SHADOW flag in the PCI_ROM_RESOURCE. But also getting the base and size for PCI BAR 0, since the loop in that helper would already take care of that (it also starts at BAR 0). > } > > static int lynxfb_pci_probe(struct pci_dev *pdev, > diff --git a/drivers/video/fbdev/aty/radeon_base.c b/drivers/video/fbdev/aty/radeon_base.c > index b311c07fe66d..e5e362b8c9da 100644 > --- a/drivers/video/fbdev/aty/radeon_base.c > +++ b/drivers/video/fbdev/aty/radeon_base.c > @@ -54,6 +54,7 @@ > > #include "radeonfb.h" > > +#include <linux/aperture.h> > #include <linux/module.h> > #include <linux/moduleparam.h> > #include <linux/kernel.h> > @@ -2239,20 +2240,10 @@ static const struct bin_attribute edid2_attr = { > > static int radeon_kick_out_firmware_fb(struct pci_dev *pdev) > { > - struct apertures_struct *ap; > + resource_size_t base = pci_resource_start(pdev, 0); > + resource_size_t size = pci_resource_len(pdev, 0); > > - ap = alloc_apertures(1); > - if (!ap) > - return -ENOMEM; > - > - ap->ranges[0].base = pci_resource_start(pdev, 0); > - ap->ranges[0].size = pci_resource_len(pdev, 0); > - > - remove_conflicting_framebuffers(ap, KBUILD_MODNAME, false); > - > - kfree(ap); > - > - return 0; > + return aperture_remove_conflicting_devices(base, size, KBUILD_MODNAME, false); Same for this.
Hi Am 11.07.22 um 13:01 schrieb Javier Martinez Canillas: > On 7/7/22 17:39, Thomas Zimmermann wrote: >> Convert fbdev drivers from fbdev's remove_conflicting_framebuffers() to >> the framework-independent aperture_remove_conflicting_devices(). Calling >> this function will also remove conflicting DRM drivers. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- > > [...] > >> static int lynxfb_kick_out_firmware_fb(struct pci_dev *pdev) >> { >> - struct apertures_struct *ap; >> + resource_size_t base = pci_resource_start(pdev, 0); >> + resource_size_t size = pci_resource_len(pdev, 0); >> bool primary = false; >> >> - ap = alloc_apertures(1); >> - if (!ap) >> - return -ENOMEM; >> - >> - ap->ranges[0].base = pci_resource_start(pdev, 0); >> - ap->ranges[0].size = pci_resource_len(pdev, 0); >> #ifdef CONFIG_X86 >> primary = pdev->resource[PCI_ROM_RESOURCE].flags & >> IORESOURCE_ROM_SHADOW; >> #endif >> - remove_conflicting_framebuffers(ap, "sm750_fb1", primary); >> - kfree(ap); >> - return 0; >> + >> + return aperture_remove_conflicting_devices(base, size, primary, "sm750_fb1"); > > Do you know why this can't just use aperture_remove_conflicting_pci_devices() ? I simply don't want change too much at once, because there was this problem with the PCI helper on ast. At some point we can make a push to really fix this throughout the code base. And that would include an update to fb_is_primary_device(), [1] which doesn't fill well into the new interfaces. Best regards Thomas [1] https://elixir.bootlin.com/linux/latest/source/arch/x86/video/fbdev.c#L14 > > It seems that the driver is open coding part of the logic already in that helper. > For example, figuring out if is a primary by checking the IORESOURCE_ROM_SHADOW > flag in the PCI_ROM_RESOURCE. > > But also getting the base and size for PCI BAR 0, since the loop in that helper > would already take care of that (it also starts at BAR 0). > >> } >> >> static int lynxfb_pci_probe(struct pci_dev *pdev, >> diff --git a/drivers/video/fbdev/aty/radeon_base.c b/drivers/video/fbdev/aty/radeon_base.c >> index b311c07fe66d..e5e362b8c9da 100644 >> --- a/drivers/video/fbdev/aty/radeon_base.c >> +++ b/drivers/video/fbdev/aty/radeon_base.c >> @@ -54,6 +54,7 @@ >> >> #include "radeonfb.h" >> >> +#include <linux/aperture.h> >> #include <linux/module.h> >> #include <linux/moduleparam.h> >> #include <linux/kernel.h> >> @@ -2239,20 +2240,10 @@ static const struct bin_attribute edid2_attr = { >> >> static int radeon_kick_out_firmware_fb(struct pci_dev *pdev) >> { >> - struct apertures_struct *ap; >> + resource_size_t base = pci_resource_start(pdev, 0); >> + resource_size_t size = pci_resource_len(pdev, 0); >> >> - ap = alloc_apertures(1); >> - if (!ap) >> - return -ENOMEM; >> - >> - ap->ranges[0].base = pci_resource_start(pdev, 0); >> - ap->ranges[0].size = pci_resource_len(pdev, 0); >> - >> - remove_conflicting_framebuffers(ap, KBUILD_MODNAME, false); >> - >> - kfree(ap); >> - >> - return 0; >> + return aperture_remove_conflicting_devices(base, size, KBUILD_MODNAME, false); > > Same for this. >
Hello Thomas, On 7/15/22 13:48, Thomas Zimmermann wrote: [...] >>> + >>> + return aperture_remove_conflicting_devices(base, size, primary, "sm750_fb1"); >> >> Do you know why this can't just use aperture_remove_conflicting_pci_devices() ? > > I simply don't want change too much at once, because there was this > problem with the PCI helper on ast. > Makes sense. Feel free to add: Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > At some point we can make a push to really fix this throughout the code > base. And that would include an update to fb_is_primary_device(), [1] > which doesn't fill well into the new interfaces. > Agreed.
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c index dbd1159a2ef0..ce04c38f6afd 100644 --- a/drivers/staging/sm750fb/sm750.c +++ b/drivers/staging/sm750fb/sm750.c @@ -1,4 +1,5 @@ // SPDX-License-Identifier: GPL-2.0 +#include <linux/aperture.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/errno.h> @@ -987,22 +988,16 @@ static int sm750fb_framebuffer_alloc(struct sm750_dev *sm750_dev, int fbidx) static int lynxfb_kick_out_firmware_fb(struct pci_dev *pdev) { - struct apertures_struct *ap; + resource_size_t base = pci_resource_start(pdev, 0); + resource_size_t size = pci_resource_len(pdev, 0); bool primary = false; - ap = alloc_apertures(1); - if (!ap) - return -ENOMEM; - - ap->ranges[0].base = pci_resource_start(pdev, 0); - ap->ranges[0].size = pci_resource_len(pdev, 0); #ifdef CONFIG_X86 primary = pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW; #endif - remove_conflicting_framebuffers(ap, "sm750_fb1", primary); - kfree(ap); - return 0; + + return aperture_remove_conflicting_devices(base, size, primary, "sm750_fb1"); } static int lynxfb_pci_probe(struct pci_dev *pdev, diff --git a/drivers/video/fbdev/aty/radeon_base.c b/drivers/video/fbdev/aty/radeon_base.c index b311c07fe66d..e5e362b8c9da 100644 --- a/drivers/video/fbdev/aty/radeon_base.c +++ b/drivers/video/fbdev/aty/radeon_base.c @@ -54,6 +54,7 @@ #include "radeonfb.h" +#include <linux/aperture.h> #include <linux/module.h> #include <linux/moduleparam.h> #include <linux/kernel.h> @@ -2239,20 +2240,10 @@ static const struct bin_attribute edid2_attr = { static int radeon_kick_out_firmware_fb(struct pci_dev *pdev) { - struct apertures_struct *ap; + resource_size_t base = pci_resource_start(pdev, 0); + resource_size_t size = pci_resource_len(pdev, 0); - ap = alloc_apertures(1); - if (!ap) - return -ENOMEM; - - ap->ranges[0].base = pci_resource_start(pdev, 0); - ap->ranges[0].size = pci_resource_len(pdev, 0); - - remove_conflicting_framebuffers(ap, KBUILD_MODNAME, false); - - kfree(ap); - - return 0; + return aperture_remove_conflicting_devices(base, size, KBUILD_MODNAME, false); } static int radeonfb_pci_register(struct pci_dev *pdev, diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c index 886c564787f1..a944a6620527 100644 --- a/drivers/video/fbdev/hyperv_fb.c +++ b/drivers/video/fbdev/hyperv_fb.c @@ -45,6 +45,7 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt +#include <linux/aperture.h> #include <linux/module.h> #include <linux/kernel.h> #include <linux/vmalloc.h> @@ -1074,8 +1075,9 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info) info->screen_size = dio_fb_size; getmem_done: - remove_conflicting_framebuffers(info->apertures, - KBUILD_MODNAME, false); + aperture_remove_conflicting_devices(info->apertures->ranges[0].base, + info->apertures->ranges[0].size, + KBUILD_MODNAME, false); if (gen2vm) { /* framebuffer is reallocated, clear screen_info to avoid misuse from kexec */
Convert fbdev drivers from fbdev's remove_conflicting_framebuffers() to the framework-independent aperture_remove_conflicting_devices(). Calling this function will also remove conflicting DRM drivers. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/staging/sm750fb/sm750.c | 15 +++++---------- drivers/video/fbdev/aty/radeon_base.c | 17 ++++------------- drivers/video/fbdev/hyperv_fb.c | 6 ++++-- 3 files changed, 13 insertions(+), 25 deletions(-)