On 12/30/22 11:15, Stanislaw Gruszka wrote:
On Wed, Dec 28, 2022 at 03:17:59PM +0200, Mikko Perttunen wrote:
On 12/28/22 15:08, Deepak R Varma wrote:
On Wed, Dec 28, 2022 at 02:28:54PM +0200, Mikko Perttunen wrote:
On 12/27/22 19:14, Deepak R Varma wrote:
kfree() & vfree() internally perform NULL check on the pointer handed
to it and take no action if it indeed is NULL. Hence there is no need
for a pre-check of the memory pointer before handing it to
kfree()/vfree().
Issue reported by ifnullfree.cocci Coccinelle semantic patch script.
Signed-off-by: Deepak R Varma <drv@xxxxxxxxx>
---
drivers/gpu/drm/tegra/submit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/tegra/submit.c b/drivers/gpu/drm/tegra/submit.c
index 066f88564169..06f836db99d0 100644
--- a/drivers/gpu/drm/tegra/submit.c
+++ b/drivers/gpu/drm/tegra/submit.c
@@ -680,8 +680,8 @@ int tegra_drm_ioctl_channel_submit(struct drm_device *drm, void *data,
kfree(job_data->used_mappings);
}
- if (job_data)
- kfree(job_data);
+ kfree(job_data);
+
put_bo:
gather_bo_put(&bo->base);
unlock:
--
2.34.1
It continues to be the case that I think this transform is bad. Same applies
to the host1x patch.
Hello Mikko,
Thank you for responding to the patch proposal. Could you please explain why is
this bad?
Regards,
./drv
Mikko
Hi,
it gets rid of visual hints on code paths indicating the possible liveness
of pointer variables. I.e., after the change, whether the pointer can be
NULL or not is more difficult to reason about locally, instead requiring
more global reasoning which is mentally more taxing.
Since C's type system doesn't help with tracking these kinds of things, I
believe it is important to have these kinds of local contextual cues to help
the programmer.
I agree with your point of view. But regarding this particular patch,
at least on code base I can see, after free_job_data label job_done
can not be NULL. So patch seems to be ok, but maybe changelog need to
be different
Regards
Stanislaw
It can be NULL; see:
job->user_data = job_data;
job->release = release_job;
job->timeout = 10000;
/*
* job_data is now part of job reference counting, so don't release
* it from here.
*/
job_data = NULL;
If we go into free_job_data after this code (which happens if there is
no error, or if host1x_job_submit fails), job_data will be NULL.
The memory is instead released in the 'put_job' label; host1x_job_put
ends up calling release_job, which does the kfree.
(Yes, it is rather complicated..)
Thanks,
Mikko
