diff mbox series

[v2,10/10] drm/vboxvideo: fix mapping leaks

Message ID 20240123094317.15958-11-pstanner@redhat.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series Make PCI's devres API more consistent | expand

Commit Message

Philipp Stanner Jan. 23, 2024, 9:43 a.m. UTC
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.

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(-)

Comments

Hans de Goede Jan. 29, 2024, 11:15 a.m. UTC | #1
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 ?

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,
Philipp Stanner Jan. 29, 2024, 1:12 p.m. UTC | #2
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,
>
diff mbox series

Patch

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,