Re: [PATCH v2 10/10] drm/vboxvideo: fix mapping leaks

[Philipp Stanner <pstanner@redhat.com>]

Hi,

On Mon, 2024-01-29 at 12:15 +0100, Hans de Goede wrote:
> Hi Philipp,
> 
> On 1/23/24 10:43, Philipp Stanner wrote:
> > When the PCI devres API was introduced to this driver, it was
> > wrongly
> > assumed that initializing the device with pcim_enable_device()
> > instead
> > of pci_enable_device() will make all PCI functions managed.
> > 
> > This is wrong and was caused by the quite confusing devres API for
> > PCI
> > in which some, but not all, functions become managed that way.
> > 
> > The function pci_iomap_range() is never managed.
> > 
> > Replace pci_iomap_range() with the actually managed function
> > pcim_iomap_range().
> > 
> > Additionally, add a call to pcim_request_region() to ensure
> > exclusive
> > access to BAR 0.
> 
> I'm a bit worried about this last change. There might be
> issues where the pcim_request_region() fails due to
> e.g. a conflict with the simplefb / simpledrm code.
> 
> There is a drm_aperture_remove_conflicting_pci_framebuffers()
> call done before hw_init() gets called, but still this
> has been known to cause issues in the past.
> 
> Can you split out the adding of the pcim_request_region()
> into a separate patch and *not* mark that separate patch
> for stable ?

Yes, that sounds reasonable. I'll split it out and deal with it once
I'll send the other DRM patches from my backlog.

Greetings,
P.

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> > 
> > CC: <stable@kernel.vger.org> # v5.10+
> > Fixes: 8558de401b5f ("drm/vboxvideo: use managed pci functions")
> > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > ---
> >  drivers/gpu/drm/vboxvideo/vbox_main.c | 24 +++++++++++++----------
> > -
> >  1 file changed, 13 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vboxvideo/vbox_main.c
> > b/drivers/gpu/drm/vboxvideo/vbox_main.c
> > index 42c2d8a99509..7f686a0190e6 100644
> > --- a/drivers/gpu/drm/vboxvideo/vbox_main.c
> > +++ b/drivers/gpu/drm/vboxvideo/vbox_main.c
> > @@ -42,12 +42,11 @@ static int vbox_accel_init(struct vbox_private
> > *vbox)
> >         /* Take a command buffer for each screen from the end of
> > usable VRAM. */
> >         vbox->available_vram_size -= vbox->num_crtcs *
> > VBVA_MIN_BUFFER_SIZE;
> >  
> > -       vbox->vbva_buffers = pci_iomap_range(pdev, 0,
> > -                                            vbox-
> > >available_vram_size,
> > -                                            vbox->num_crtcs *
> > -                                            VBVA_MIN_BUFFER_SIZE);
> > -       if (!vbox->vbva_buffers)
> > -               return -ENOMEM;
> > +       vbox->vbva_buffers = pcim_iomap_range(
> > +                       pdev, 0, vbox->available_vram_size,
> > +                       vbox->num_crtcs * VBVA_MIN_BUFFER_SIZE);
> > +       if (IS_ERR(vbox->vbva_buffers))
> > +               return PTR_ERR(vbox->vbva_buffers);
> >  
> >         for (i = 0; i < vbox->num_crtcs; ++i) {
> >                 vbva_setup_buffer_context(&vbox->vbva_info[i],
> > @@ -115,12 +114,15 @@ int vbox_hw_init(struct vbox_private *vbox)
> >  
> >         DRM_INFO("VRAM %08x\n", vbox->full_vram_size);
> >  
> > +       ret = pcim_request_region(pdev, 0, "vboxvideo");
> > +       if (ret)
> > +               return ret;
> > +
> >         /* Map guest-heap at end of vram */
> > -       vbox->guest_heap =
> > -           pci_iomap_range(pdev, 0, GUEST_HEAP_OFFSET(vbox),
> > -                           GUEST_HEAP_SIZE);
> > -       if (!vbox->guest_heap)
> > -               return -ENOMEM;
> > +       vbox->guest_heap = pcim_iomap_range(pdev, 0,
> > +                       GUEST_HEAP_OFFSET(vbox), GUEST_HEAP_SIZE);
> > +       if (IS_ERR(vbox->guest_heap))
> > +               return PTR_ERR(vbox->guest_heap);
> >  
> >         /* Create guest-heap mem-pool use 2^4 = 16 byte chunks */
> >         vbox->guest_pool = devm_gen_pool_create(vbox->ddev.dev, 4,
> > -1,
>