| Message ID | 20240402134709.1706323-1-harshit.m.mogalapalli@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/panthor: Fix couple of NULL vs IS_ERR() bugs | expand |
On Tue, 2 Apr 2024 06:47:08 -0700 Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote: > 1. The devm_drm_dev_alloc() function returns error pointers. > Update the error handling to check for error pointers instead of NULL. > 2. Currently panthor_vm_get_heap_pool() returns both ERR_PTR() and > NULL(when create is false and if there is no poool attached to the > VM) > - Change the function to return error pointers, when pool is > NULL return -ENOENT > - Also handle the callers to check for IS_ERR() on failure. > > Fixes: 4bdca1150792 ("drm/panthor: Add the driver frontend block") > Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> > --- > This is spotted by smatch and the patch is only compile tested > > v1->v2: Fix the function panthor_vm_get_heap_pool() to only return error > pointers and handle the caller sites [Suggested by Boris Brezillon] > - Also merge these IS_ERR() vs NULL bugs into same patch > --- > drivers/gpu/drm/panthor/panthor_drv.c | 6 +++--- > drivers/gpu/drm/panthor/panthor_mmu.c | 2 ++ > drivers/gpu/drm/panthor/panthor_sched.c | 2 +- > 3 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > index 11b3ccd58f85..c8374cd4a30d 100644 > --- a/drivers/gpu/drm/panthor/panthor_drv.c > +++ b/drivers/gpu/drm/panthor/panthor_drv.c > @@ -1090,8 +1090,8 @@ static int panthor_ioctl_tiler_heap_destroy(struct drm_device *ddev, void *data, > return -EINVAL; > > pool = panthor_vm_get_heap_pool(vm, false); > - if (!pool) { > - ret = -EINVAL; > + if (IS_ERR(pool)) { > + ret = PTR_ERR(pool); > goto out_put_vm; > } > > @@ -1385,7 +1385,7 @@ static int panthor_probe(struct platform_device *pdev) > > ptdev = devm_drm_dev_alloc(&pdev->dev, &panthor_drm_driver, > struct panthor_device, base); > - if (!ptdev) > + if (IS_ERR(ptdev)) > return -ENOMEM; > Sorry, that one deserves a separate patch. > platform_set_drvdata(pdev, ptdev); > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c > index fdd35249169f..e1285cdb09ff 100644 > --- a/drivers/gpu/drm/panthor/panthor_mmu.c > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c > @@ -1893,6 +1893,8 @@ struct panthor_heap_pool *panthor_vm_get_heap_pool(struct panthor_vm *vm, bool c > vm->heaps.pool = panthor_heap_pool_get(pool); > } else { > pool = panthor_heap_pool_get(vm->heaps.pool); > + if (!pool) > + pool = ERR_PTR(-ENOENT); > } > mutex_unlock(&vm->heaps.lock); > > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c > index 5f7803b6fc48..617df2b980d0 100644 > --- a/drivers/gpu/drm/panthor/panthor_sched.c > +++ b/drivers/gpu/drm/panthor/panthor_sched.c > @@ -1343,7 +1343,7 @@ static int group_process_tiler_oom(struct panthor_group *group, u32 cs_id) > if (unlikely(csg_id < 0)) > return 0; > > - if (!heaps || frag_end > vt_end || vt_end >= vt_start) { > + if (IS_ERR(heaps) || frag_end > vt_end || vt_end >= vt_start) { > ret = -EINVAL; > } else { > /* We do the allocation without holding the scheduler lock to avoid
Hi Boris, On 02/04/24 19:32, Boris Brezillon wrote: > On Tue, 2 Apr 2024 06:47:08 -0700 > Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote: > >> 1. The devm_drm_dev_alloc() function returns error pointers. >> Update the error handling to check for error pointers instead of NULL. >> 2. Currently panthor_vm_get_heap_pool() returns both ERR_PTR() and >> NULL(when create is false and if there is no poool attached to the >> VM) >> - Change the function to return error pointers, when pool is >> NULL return -ENOENT >> - Also handle the callers to check for IS_ERR() on failure. >> >> Fixes: 4bdca1150792 ("drm/panthor: Add the driver frontend block") >> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> >> --- >> This is spotted by smatch and the patch is only compile tested >> >> v1->v2: Fix the function panthor_vm_get_heap_pool() to only return error >> pointers and handle the caller sites [Suggested by Boris Brezillon] >> - Also merge these IS_ERR() vs NULL bugs into same patch >> --- >> drivers/gpu/drm/panthor/panthor_drv.c | 6 +++--- >> drivers/gpu/drm/panthor/panthor_mmu.c | 2 ++ >> drivers/gpu/drm/panthor/panthor_sched.c | 2 +- >> 3 files changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c >> index 11b3ccd58f85..c8374cd4a30d 100644 >> --- a/drivers/gpu/drm/panthor/panthor_drv.c >> +++ b/drivers/gpu/drm/panthor/panthor_drv.c >> @@ -1090,8 +1090,8 @@ static int panthor_ioctl_tiler_heap_destroy(struct drm_device *ddev, void *data, >> return -EINVAL; >> >> pool = panthor_vm_get_heap_pool(vm, false); >> - if (!pool) { >> - ret = -EINVAL; >> + if (IS_ERR(pool)) { >> + ret = PTR_ERR(pool); >> goto out_put_vm; >> } >> >> @@ -1385,7 +1385,7 @@ static int panthor_probe(struct platform_device *pdev) >> >> ptdev = devm_drm_dev_alloc(&pdev->dev, &panthor_drm_driver, >> struct panthor_device, base); >> - if (!ptdev) >> + if (IS_ERR(ptdev)) >> return -ENOMEM; >> > > Sorry, that one deserves a separate patch. > Ah okay, I was confused about the same. So I will send a V3 removing that part and could you please use the independent patch that I sent before ? https://lore.kernel.org/all/20240402104041.1689951-1-harshit.m.mogalapalli@oracle.com/ Thanks, Harshit >> platform_set_drvdata(pdev, ptdev); >> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c >> index fdd35249169f..e1285cdb09ff 100644 >> --- a/drivers/gpu/drm/panthor/panthor_mmu.c >> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c >> @@ -1893,6 +1893,8 @@ struct panthor_heap_pool *panthor_vm_get_heap_pool(struct panthor_vm *vm, bool c >> vm->heaps.pool = panthor_heap_pool_get(pool); >> } else { >> pool = panthor_heap_pool_get(vm->heaps.pool); >> + if (!pool) >> + pool = ERR_PTR(-ENOENT); >> } >> mutex_unlock(&vm->heaps.lock); >> >> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c >> index 5f7803b6fc48..617df2b980d0 100644 >> --- a/drivers/gpu/drm/panthor/panthor_sched.c >> +++ b/drivers/gpu/drm/panthor/panthor_sched.c >> @@ -1343,7 +1343,7 @@ static int group_process_tiler_oom(struct panthor_group *group, u32 cs_id) >> if (unlikely(csg_id < 0)) >> return 0; >> >> - if (!heaps || frag_end > vt_end || vt_end >= vt_start) { >> + if (IS_ERR(heaps) || frag_end > vt_end || vt_end >= vt_start) { >> ret = -EINVAL; >> } else { >> /* We do the allocation without holding the scheduler lock to avoid > >
On Tue, 2 Apr 2024 19:35:41 +0530 Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote: > Hi Boris, > On 02/04/24 19:32, Boris Brezillon wrote: > > On Tue, 2 Apr 2024 06:47:08 -0700 > > Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote: > > > >> 1. The devm_drm_dev_alloc() function returns error pointers. > >> Update the error handling to check for error pointers instead of NULL. > >> 2. Currently panthor_vm_get_heap_pool() returns both ERR_PTR() and > >> NULL(when create is false and if there is no poool attached to the > >> VM) > >> - Change the function to return error pointers, when pool is > >> NULL return -ENOENT > >> - Also handle the callers to check for IS_ERR() on failure. > >> > >> Fixes: 4bdca1150792 ("drm/panthor: Add the driver frontend block") > >> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> > >> --- > >> This is spotted by smatch and the patch is only compile tested > >> > >> v1->v2: Fix the function panthor_vm_get_heap_pool() to only return error > >> pointers and handle the caller sites [Suggested by Boris Brezillon] > >> - Also merge these IS_ERR() vs NULL bugs into same patch > >> --- > >> drivers/gpu/drm/panthor/panthor_drv.c | 6 +++--- > >> drivers/gpu/drm/panthor/panthor_mmu.c | 2 ++ > >> drivers/gpu/drm/panthor/panthor_sched.c | 2 +- > >> 3 files changed, 6 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > >> index 11b3ccd58f85..c8374cd4a30d 100644 > >> --- a/drivers/gpu/drm/panthor/panthor_drv.c > >> +++ b/drivers/gpu/drm/panthor/panthor_drv.c > >> @@ -1090,8 +1090,8 @@ static int panthor_ioctl_tiler_heap_destroy(struct drm_device *ddev, void *data, > >> return -EINVAL; > >> > >> pool = panthor_vm_get_heap_pool(vm, false); > >> - if (!pool) { > >> - ret = -EINVAL; > >> + if (IS_ERR(pool)) { > >> + ret = PTR_ERR(pool); > >> goto out_put_vm; > >> } > >> > >> @@ -1385,7 +1385,7 @@ static int panthor_probe(struct platform_device *pdev) > >> > >> ptdev = devm_drm_dev_alloc(&pdev->dev, &panthor_drm_driver, > >> struct panthor_device, base); > >> - if (!ptdev) > >> + if (IS_ERR(ptdev)) > >> return -ENOMEM; > >> > > > > Sorry, that one deserves a separate patch. > > > > Ah okay, I was confused about the same. > So I will send a V3 removing that part and could you please use the > independent patch that I sent before ? > > https://lore.kernel.org/all/20240402104041.1689951-1-harshit.m.mogalapalli@oracle.com/ Yes, I already added my R-b on that one.
diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c index 11b3ccd58f85..c8374cd4a30d 100644 --- a/drivers/gpu/drm/panthor/panthor_drv.c +++ b/drivers/gpu/drm/panthor/panthor_drv.c @@ -1090,8 +1090,8 @@ static int panthor_ioctl_tiler_heap_destroy(struct drm_device *ddev, void *data, return -EINVAL; pool = panthor_vm_get_heap_pool(vm, false); - if (!pool) { - ret = -EINVAL; + if (IS_ERR(pool)) { + ret = PTR_ERR(pool); goto out_put_vm; } @@ -1385,7 +1385,7 @@ static int panthor_probe(struct platform_device *pdev) ptdev = devm_drm_dev_alloc(&pdev->dev, &panthor_drm_driver, struct panthor_device, base); - if (!ptdev) + if (IS_ERR(ptdev)) return -ENOMEM; platform_set_drvdata(pdev, ptdev); diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c index fdd35249169f..e1285cdb09ff 100644 --- a/drivers/gpu/drm/panthor/panthor_mmu.c +++ b/drivers/gpu/drm/panthor/panthor_mmu.c @@ -1893,6 +1893,8 @@ struct panthor_heap_pool *panthor_vm_get_heap_pool(struct panthor_vm *vm, bool c vm->heaps.pool = panthor_heap_pool_get(pool); } else { pool = panthor_heap_pool_get(vm->heaps.pool); + if (!pool) + pool = ERR_PTR(-ENOENT); } mutex_unlock(&vm->heaps.lock); diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c index 5f7803b6fc48..617df2b980d0 100644 --- a/drivers/gpu/drm/panthor/panthor_sched.c +++ b/drivers/gpu/drm/panthor/panthor_sched.c @@ -1343,7 +1343,7 @@ static int group_process_tiler_oom(struct panthor_group *group, u32 cs_id) if (unlikely(csg_id < 0)) return 0; - if (!heaps || frag_end > vt_end || vt_end >= vt_start) { + if (IS_ERR(heaps) || frag_end > vt_end || vt_end >= vt_start) { ret = -EINVAL; } else { /* We do the allocation without holding the scheduler lock to avoid
1. The devm_drm_dev_alloc() function returns error pointers. Update the error handling to check for error pointers instead of NULL. 2. Currently panthor_vm_get_heap_pool() returns both ERR_PTR() and NULL(when create is false and if there is no poool attached to the VM) - Change the function to return error pointers, when pool is NULL return -ENOENT - Also handle the callers to check for IS_ERR() on failure. Fixes: 4bdca1150792 ("drm/panthor: Add the driver frontend block") Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> --- This is spotted by smatch and the patch is only compile tested v1->v2: Fix the function panthor_vm_get_heap_pool() to only return error pointers and handle the caller sites [Suggested by Boris Brezillon] - Also merge these IS_ERR() vs NULL bugs into same patch --- drivers/gpu/drm/panthor/panthor_drv.c | 6 +++--- drivers/gpu/drm/panthor/panthor_mmu.c | 2 ++ drivers/gpu/drm/panthor/panthor_sched.c | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-)