On Wed, 29 May 2019 at 11:18, Boris Brezillon boris.brezillon@collabora.com wrote:

mmu_ops->unmap() will fail when called on a BO that has not been previously mapped, and the error path in panfrost_ioctl_create_bo() can call drm_gem_object_put_unlocked() (which in turn calls panfrost_mmu_unmap()) on a BO that has not been mapped yet.

Keep track of the mapped/unmapped state to avoid such issues.

Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver") Cc: stable@vger.kernel.org Signed-off-by: Boris Brezillon boris.brezillon@collabora.com

---

drivers/gpu/drm/panfrost/panfrost_gem.h | 1 + drivers/gpu/drm/panfrost/panfrost_mmu.c | 8 ++++++++ 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h index 045000eb5fcf..6dbcaba020fc 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.h +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h @@ -11,6 +11,7 @@ struct panfrost_gem_object { struct drm_gem_shmem_object base;

    struct drm_mm_node node;

•   bool is_mapped;
  

};

static inline diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index 762b1bd2a8c2..fb556aa89203 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -156,6 +156,9 @@ int panfrost_mmu_map(struct panfrost_gem_object *bo) struct sg_table *sgt; int ret;

•   if (bo->is_mapped)
  

•           return 0;
  

In what circumstances we want to silently go on? I would expect that for this function to be called when the BO has been mapped already would mean that we have a bug in the kernel, so why not a WARN?

•   sgt = drm_gem_shmem_get_pages_sgt(obj);
    if (WARN_ON(IS_ERR(sgt)))
            return PTR_ERR(sgt);
  

@@ -189,6 +192,7 @@ int panfrost_mmu_map(struct panfrost_gem_object *bo)

    pm_runtime_mark_last_busy(pfdev->dev);
    pm_runtime_put_autosuspend(pfdev->dev);

•   bo->is_mapped = true;
  
    return 0;
  

} @@ -203,6 +207,9 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo) size_t unmapped_len = 0; int ret;

•   if (!bo->is_mapped)
  

•           return;
  

Similarly, I think that what we should do is not to call panfrost_mmu_unmap when a BO is freed if we know it isn't mapped. And probably add a WARN here if it still gets called when the BO isn't mapped.

•   dev_dbg(pfdev->dev, "unmap: iova=%llx, len=%zx", iova, len);
  
    ret = pm_runtime_get_sync(pfdev->dev);
  

@@ -230,6 +237,7 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)

    pm_runtime_mark_last_busy(pfdev->dev);
    pm_runtime_put_autosuspend(pfdev->dev);

•   bo->is_mapped = false;
  

}

static void mmu_tlb_inv_context_s1(void *cookie)

2.20.1

Thanks for taking care of this!

Tomeu