Messages in this thread | | | Date | Tue, 7 Feb 2023 12:29:52 +0100 | Subject | Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt() | From | Thomas Zimmermann <> |
| |
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] | |