Re: [Xen-devel] [PATCH 04/52] drm: Set final_kfree in drm_dev_alloc

[Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>]

Hi Daniel,

Thank you for the patch.

On Wed, Feb 19, 2020 at 11:20:34AM +0100, Daniel Vetter wrote:
> I also did a full review of all callers, and only the xen driver
> forgot to call drm_dev_put in the failure path. Fix that up too.

I'd split this patch in two then, with the Xen first coming first, and
with an explanation in the commit message of the second patch about why
you call drmm_add_final_kfree() in drm_dev_alloc().

> v2: I noticed that xen has a drm_driver.release hook, and uses
> drm_dev_alloc(). We need to remove the kfree from
> xen_drm_drv_release().
> 
> bochs also has a release hook, but leaked the drm_device ever since
> 
> commit 0a6659bdc5e8221da99eebb176fd9591435e38de
> Author: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> Date:   Tue Dec 17 18:04:46 2013 +0100
> 
>     drm/bochs: new driver
> 
> This patch here fixes that leak.
> 
> Same for virtio, started leaking with
> 
> commit b1df3a2b24a917f8853d43fe9683c0e360d2c33a
> Author: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> Date:   Tue Feb 11 14:58:04 2020 +0100
> 
>     drm/virtio: add drm_driver.release callback.
> 
> Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> Cc: David Airlie <airlied@xxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
> ---
>  drivers/gpu/drm/drm_drv.c           | 3 +++
>  drivers/gpu/drm/xen/xen_drm_front.c | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 3e5627d6eba6..9e62e28bbc62 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -39,6 +39,7 @@
>  #include <drm/drm_color_mgmt.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_file.h>
> +#include <drm/drm_managed.h>
>  #include <drm/drm_mode_object.h>
>  #include <drm/drm_print.h>
>  
> @@ -819,6 +820,8 @@ struct drm_device *drm_dev_alloc(struct drm_driver 
> *driver,
>               return ERR_PTR(ret);
>       }
>  
> +     drmm_add_final_kfree(dev, dev);

drmm_add_final_kfree() can only be called once. Does this mean that a
driver using drm_dev_alloc() isn't allowed to use drmm_add_final_kfree()
to tract its own private structure ?

> +
>       return dev;
>  }
>  EXPORT_SYMBOL(drm_dev_alloc);
> diff --git a/drivers/gpu/drm/xen/xen_drm_front.c 
> b/drivers/gpu/drm/xen/xen_drm_front.c
> index 4be49c1aef51..d22b5da38935 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front.c
> @@ -461,7 +461,6 @@ static void xen_drm_drv_release(struct drm_device *dev)
>       drm_mode_config_cleanup(dev);
>  
>       drm_dev_fini(dev);
> -     kfree(dev);
>  
>       if (front_info->cfg.be_alloc)
>               xenbus_switch_state(front_info->xb_dev,
> @@ -561,6 +560,7 @@ static int xen_drm_drv_init(struct xen_drm_front_info 
> *front_info)
>  fail_modeset:
>       drm_kms_helper_poll_fini(drm_dev);
>       drm_mode_config_cleanup(drm_dev);
> +     drm_dev_put(drm_dev);
>  fail:
>       kfree(drm_info);
>       return ret;

-- 
Regards,

Laurent Pinchart

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel