| Message ID | 20240402141412.1707949-1-harshit.m.mogalapalli@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/panthor: Fix couple of NULL vs IS_ERR() bugs | expand |
On Tue, 2 Apr 2024 07:14:11 -0700 Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote: > 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 ^ pool > 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") I would explain that the code was correct, but the documentation didn't match the function behavior, otherwise it feels a bit weird to have a Fixes tag here. > 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 > > v2->v3: pull out error checking for devm_drm_dev_alloc() failure. > --- > drivers/gpu/drm/panthor/panthor_drv.c | 4 ++-- > drivers/gpu/drm/panthor/panthor_mmu.c | 2 ++ > drivers/gpu/drm/panthor/panthor_sched.c | 2 +- > 3 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > index 11b3ccd58f85..050b905b0453 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; > } > > 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, Apr 02, 2024 at 04:38:38PM +0200, Boris Brezillon wrote: > On Tue, 2 Apr 2024 07:14:11 -0700 > Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote: > > > 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 > > ^ pool > > > 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") > > I would explain that the code was correct, but the documentation didn't > match the function behavior, otherwise it feels a bit weird to have a > Fixes tag here. The code wasn't correct, it returned a mix of error pointers and NULL. So it needs a Fixes tag. regards, dan carpenter
On Tue, 2 Apr 2024 17:44:18 +0300 Dan Carpenter <dan.carpenter@linaro.org> wrote: > On Tue, Apr 02, 2024 at 04:38:38PM +0200, Boris Brezillon wrote: > > On Tue, 2 Apr 2024 07:14:11 -0700 > > Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote: > > > > > 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 > > > > ^ pool > > > > > 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") > > > > I would explain that the code was correct, but the documentation didn't > > match the function behavior, otherwise it feels a bit weird to have a > > Fixes tag here. > > The code wasn't correct, it returned a mix of error pointers and NULL. AFAICT, this is allowed, otherwise why would we have IS_ERR_OR_NULL(). The fact smatch can't see through panthor_vm_get_heap_pool() and detect that the return value is different for create=false/true doesn't mean the code was wrong. I'm certainly not saying this is a good thing to have a function that encodes the error case with two different kind of return value, but I wouldn't qualify it as a bug either. What's incorrect though, is the fact the documentation doesn't match the code. > So it needs a Fixes tag. I didn't say we should drop the Fixes tag, but the bug being fixed here is a mismatch between the doc and the implementation, the code itself was correct, and the behavior is actually unchanged with this patch applied, it's just done in a less confusing way. Regards, Boris
On Tue, Apr 02, 2024 at 05:19:25PM +0200, Boris Brezillon wrote: > On Tue, 2 Apr 2024 17:44:18 +0300 > Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > On Tue, Apr 02, 2024 at 04:38:38PM +0200, Boris Brezillon wrote: > > > On Tue, 2 Apr 2024 07:14:11 -0700 > > > Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote: > > > > > > > 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 > > > > > > ^ pool > > > > > > > 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") > > > > > > I would explain that the code was correct, but the documentation didn't > > > match the function behavior, otherwise it feels a bit weird to have a > > > Fixes tag here. > > > > The code wasn't correct, it returned a mix of error pointers and NULL. > > AFAICT, this is allowed, otherwise why would we have IS_ERR_OR_NULL(). Yep. I have written a blog about this: https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/ > The fact smatch can't see through panthor_vm_get_heap_pool() and detect > that the return value is different for create=false/true doesn't mean > the code was wrong. I'm certainly not saying this is a good thing to > have a function that encodes the error case with two different kind of > return value, but I wouldn't qualify it as a bug either. What's > incorrect though, is the fact the documentation doesn't match the code. > > > So it needs a Fixes tag. > > I didn't say we should drop the Fixes tag, but the bug being fixed here > is a mismatch between the doc and the implementation, the code itself > was correct, and the behavior is actually unchanged with this patch > applied, it's just done in a less confusing way. Oh. Sorry, I haven't been following this thread closely and I misread the code as well. You're right that the code works. In this case, I would say actually that it does not need a Fixes tag because it's not a bug. It's just a cleanup. Sorry for the noise. regards, dan carpenter
On Tue, 2 Apr 2024 07:14:11 -0700 Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote: > 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> Queued to drm-misc-next with the following commit message: " drm/panthor: Don't return NULL from panthor_vm_get_heap_pool() The kernel doc says this function returns either a valid pointer or an ERR_PTR(), but in practice this function can return NULL if create=false. Fix the function to match the doc (return ERR_PTR(-ENOENT) instead of NULL) and adjust all call-sites accordingly. " Thanks, Boris > --- > 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 > > v2->v3: pull out error checking for devm_drm_dev_alloc() failure. > --- > drivers/gpu/drm/panthor/panthor_drv.c | 4 ++-- > drivers/gpu/drm/panthor/panthor_mmu.c | 2 ++ > drivers/gpu/drm/panthor/panthor_sched.c | 2 +- > 3 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c > index 11b3ccd58f85..050b905b0453 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; > } > > 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 03/04/24 12:47, Boris Brezillon wrote: > On Tue, 2 Apr 2024 07:14:11 -0700 > Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote: > >> 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> > > Queued to drm-misc-next with the following commit message: > > " > drm/panthor: Don't return NULL from panthor_vm_get_heap_pool() > > The kernel doc says this function returns either a valid pointer > or an ERR_PTR(), but in practice this function can return NULL if > create=false. Fix the function to match the doc (return > ERR_PTR(-ENOENT) instead of NULL) and adjust all call-sites > accordingly. > " > Thanks a lot for making the change! Regards, Harshit > Thanks, > > Boris > >> --- >> 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 >> >> v2->v3: pull out error checking for devm_drm_dev_alloc() failure. >> --- >> drivers/gpu/drm/panthor/panthor_drv.c | 4 ++-- >> drivers/gpu/drm/panthor/panthor_mmu.c | 2 ++ >> drivers/gpu/drm/panthor/panthor_sched.c | 2 +- >> 3 files changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c >> index 11b3ccd58f85..050b905b0453 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; >> } >> >> 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 >
diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c index 11b3ccd58f85..050b905b0453 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; } 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
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 v2->v3: pull out error checking for devm_drm_dev_alloc() failure. --- drivers/gpu/drm/panthor/panthor_drv.c | 4 ++-- drivers/gpu/drm/panthor/panthor_mmu.c | 2 ++ drivers/gpu/drm/panthor/panthor_sched.c | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-)