lkml.org 
[lkml]   [2023]   [Feb]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()
From
Hi

Am 05.02.23 um 13:51 schrieb Asahi Lina:
> Other functions touching shmem->sgt take the pages lock, so do that here

Really? I was just locking at the Lima driver and it apparently access
sgt without locking in [1]. Not that I disagree with the patch in general.

Best regards
Thomas

[1]
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/lima/lima_gem.c#L21

> too. drm_gem_shmem_get_pages() & co take the same lock, so move to the
> _locked() variants to avoid recursive locking.
>
> Discovered while auditing locking to write the Rust abstractions.
>
> Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects")
> Fixes: 4fa3d66f132b ("drm/shmem: Do dma_unmap_sg before purging pages")
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c | 54 ++++++++++++++++----------
> 1 file changed, 34 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index b602cd72a120..2c559b310cad 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -681,23 +681,7 @@ struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem)
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_get_sg_table);
>
> -/**
> - * drm_gem_shmem_get_pages_sgt - Pin pages, dma map them, and return a
> - * scatter/gather table for a shmem GEM object.
> - * @shmem: shmem GEM object
> - *
> - * This function returns a scatter/gather table suitable for driver usage. If
> - * the sg table doesn't exist, the pages are pinned, dma-mapped, and a sg
> - * table created.
> - *
> - * This is the main function for drivers to get at backing storage, and it hides
> - * and difference between dma-buf imported and natively allocated objects.
> - * drm_gem_shmem_get_sg_table() should not be directly called by drivers.
> - *
> - * Returns:
> - * A pointer to the scatter/gather table of pinned pages or errno on failure.
> - */
> -struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
> +static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_object *shmem)
> {
> struct drm_gem_object *obj = &shmem->base;
> int ret;
> @@ -708,7 +692,7 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
>
> WARN_ON(obj->import_attach);
>
> - ret = drm_gem_shmem_get_pages(shmem);
> + ret = drm_gem_shmem_get_pages_locked(shmem);
> if (ret)
> return ERR_PTR(ret);
>
> @@ -730,10 +714,40 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
> sg_free_table(sgt);
> kfree(sgt);
> err_put_pages:
> - drm_gem_shmem_put_pages(shmem);
> + drm_gem_shmem_put_pages_locked(shmem);
> return ERR_PTR(ret);
> }
> -EXPORT_SYMBOL_GPL(drm_gem_shmem_get_pages_sgt);
> +
> +/**
> + * drm_gem_shmem_get_pages_sgt - Pin pages, dma map them, and return a
> + * scatter/gather table for a shmem GEM object.
> + * @shmem: shmem GEM object
> + *
> + * This function returns a scatter/gather table suitable for driver usage. If
> + * the sg table doesn't exist, the pages are pinned, dma-mapped, and a sg
> + * table created.
> + *
> + * This is the main function for drivers to get at backing storage, and it hides
> + * and difference between dma-buf imported and natively allocated objects.
> + * drm_gem_shmem_get_sg_table() should not be directly called by drivers.
> + *
> + * Returns:
> + * A pointer to the scatter/gather table of pinned pages or errno on failure.
> + */
> +struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
> +{
> + int ret;
> + struct sg_table *sgt;
> +
> + ret = mutex_lock_interruptible(&shmem->pages_lock);
> + if (ret)
> + return ERR_PTR(ret);
> + sgt = drm_gem_shmem_get_pages_sgt_locked(shmem);
> + mutex_unlock(&shmem->pages_lock);
> +
> + return sgt;
> +}
> +EXPORT_SYMBOL(drm_gem_shmem_get_pages_sgt);
>
> /**
> * drm_gem_shmem_prime_import_sg_table - Produce a shmem GEM object from

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2023-03-27 00:12    [W:0.139 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site