Re: [PATCH] drm/vboxvideo: Unmap iomem on probe-failure and remove

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 10/27/20 2:51 PM, Hans de Goede wrote:
> Add missing pci_iounmap() calls to properly unmap the memory on
> probe-failure and remove.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

For some reason the spam-filter used by Red Hat's email system has eaten
Daniel Vetter's reply to this, so let me copy and paste that from patchwork:

Daniel Vetter wrote:

> I think switching over to devm would be really nice. And for pci all
> you need to do is use pcim_enable_device and delete all the cleanup
> code, and it's all done. Hand rolling device cleanup code really isn't
> a great idea and way too error-prone. Plus you're using lots of devm_
> already.

Good point, so I just checked and the vboxvideo code is already
using pcim_enable_device() so it looks like this is a false-positive
from the lkp@intel.com bot, and Dan Carpenter missed that pcim_enable_device()
makes all subsequent pci-resource acquiring calls behave like devm calls,
when he forwarded the report to me.

Tl;DR: there is no bug / leak and this patch can be dropped.

Is there a place where I can report a bug against the lkp@intel.com bot
for this false-positive ?

Regards,

Hans




> ---
>  drivers/gpu/drm/vboxvideo/vbox_main.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_main.c b/drivers/gpu/drm/vboxvideo/vbox_main.c
> index d68d9bad7674..2eeb1d3be54a 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_main.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_main.c
> @@ -71,6 +71,8 @@ static void vbox_accel_fini(struct vbox_private *vbox)
>  
>  	for (i = 0; i < vbox->num_crtcs; ++i)
>  		vbva_disable(&vbox->vbva_info[i], vbox->guest_pool, i);
> +
> +	pci_iounmap(vbox->ddev.pdev, vbox->vbva_buffers);
>  }
>  
>  /* Do we support the 4.3 plus mode hint reporting interface? */
> @@ -124,19 +126,19 @@ int vbox_hw_init(struct vbox_private *vbox)
>  	vbox->guest_pool = devm_gen_pool_create(vbox->ddev.dev, 4, -1,
>  						"vboxvideo-accel");
>  	if (!vbox->guest_pool)
> -		return -ENOMEM;
> +		goto out_unmap_guest_heap;
>  
>  	ret = gen_pool_add_virt(vbox->guest_pool,
>  				(unsigned long)vbox->guest_heap,
>  				GUEST_HEAP_OFFSET(vbox),
>  				GUEST_HEAP_USABLE_SIZE, -1);
>  	if (ret)
> -		return ret;
> +		goto out_unmap_guest_heap;
>  
>  	ret = hgsmi_test_query_conf(vbox->guest_pool);
>  	if (ret) {
>  		DRM_ERROR("vboxvideo: hgsmi_test_query_conf failed\n");
> -		return ret;
> +		goto out_unmap_guest_heap;
>  	}
>  
>  	/* Reduce available VRAM size to reflect the guest heap. */
> @@ -148,23 +150,30 @@ int vbox_hw_init(struct vbox_private *vbox)
>  
>  	if (!have_hgsmi_mode_hints(vbox)) {
>  		ret = -ENOTSUPP;
> -		return ret;
> +		goto out_unmap_guest_heap;
>  	}
>  
>  	vbox->last_mode_hints = devm_kcalloc(vbox->ddev.dev, vbox->num_crtcs,
>  					     sizeof(struct vbva_modehint),
>  					     GFP_KERNEL);
> -	if (!vbox->last_mode_hints)
> -		return -ENOMEM;
> +	if (!vbox->last_mode_hints) {
> +		ret = -ENOMEM;
> +		goto out_unmap_guest_heap;
> +	}
>  
>  	ret = vbox_accel_init(vbox);
>  	if (ret)
> -		return ret;
> +		goto out_unmap_guest_heap;
>  
>  	return 0;
> +
> +out_unmap_guest_heap:
> +	pci_iounmap(vbox->ddev.pdev, vbox->guest_pool);
> +	return ret;
>  }
>  
>  void vbox_hw_fini(struct vbox_private *vbox)
>  {
>  	vbox_accel_fini(vbox);
> +	pci_iounmap(vbox->ddev.pdev, vbox->guest_pool);
>  }
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel