| Message ID | 20190529091836.22060-1-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panfrost: Make sure a BO is only unmapped when appropriate | expand |
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
On Fri, 31 May 2019 14:54:54 +0200 Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote: > 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. Okay, will add WARN_ON()s and add a check in the caller.
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; + 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; + 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)
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(+)