Message ID | Y6sn7XptKyk5cbrA@qemulion |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4e01:0:0:0:0:0 with SMTP id p1csp1504046wrt; Tue, 27 Dec 2022 09:43:52 -0800 (PST) X-Google-Smtp-Source: AMrXdXvwLXNl/PMLk6a4et2ZmQ7nS5RudFLiHK9NfERUJXzgoUca4PXrc1hWrv4KDeN1IUieBeef X-Received: by 2002:a17:902:eb84:b0:192:8e8b:59ab with SMTP id q4-20020a170902eb8400b001928e8b59abmr2454030plg.9.1672163032147; Tue, 27 Dec 2022 09:43:52 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1672163032; cv=none; d=google.com; s=arc-20160816; b=zwO5wIOb49HypRR2KHLeFGRaroqznC6zGOM58XU6bxqWvY/iviE1OVePzsmwQkQsAM 1Qxo9TTXHPNmQH//k8eoGYkTf3r81bQ1TtRmXRLSvDxoYMs07MQ1Pbk+0W8McYExXyRR 9TYOepEn8muHVOVoakda0oD9vK/n8H4XlSpO04xaY4C/p0W8RFgeJA6tkDMF3hukD5zi WeLGSYyl3HuttccatwJ3nl4RzHad8YLom8i91pi5LTnk31lHeqySalUjCjJ7k7jR/4OI x18Bm9ww5tDUlBYsn8TuWMM/DpGXxD07WDQGni7xuWe/KWhVFKktcH5qy/3tmuKhagXy 3lSg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-disposition:mime-version:message-id :subject:cc:to:from:date:dkim-signature; bh=mXDq2twm7AMJjSe3SF3E0znzKISr4JsiPRRtCic8muY=; b=CMpBNB7B3Q1AzzvZHr4oYeBi73zvxWn/jluKGlAHeFwjm9AkpWvketu/4rCat75nGd XFdrzBMrhrcZbRQ87RjnVgBqH+vDB/GCn138rjZos40TRuHsgRJYnf/ZvetNYjbjpcDb AZGF4mjVctr5oFkB+1P6rA8nkp+zA1EFY31gv8FneNn4O33wHP3r9eTLQOHxPkSHiFZw rL20hy4N+Hp2Kls/GpHWuTIJRgSmeUtV4V0NfSKuwYwKnleR/0vD8fIY7kQtZNDWY2+D I93WaF5C8pN4+/ny6mbi926sMkqRJl1UVlXzbtMWpVxskkLx8AWG+3ArWHqfD52Mc2Fk RttQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=PFvbFEkO; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id k3-20020a170902d58300b001927aa58c73si5574311plh.203.2022.12.27.09.43.40; Tue, 27 Dec 2022 09:43:52 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=PFvbFEkO; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229911AbiL0ROp (ORCPT <rfc822;eddaouddi.ayoub@gmail.com> + 99 others); Tue, 27 Dec 2022 12:14:45 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42808 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232007AbiL0ROh (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 27 Dec 2022 12:14:37 -0500 Received: from msg-2.mailo.com (msg-2.mailo.com [213.182.54.12]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C86A4BF7F; Tue, 27 Dec 2022 09:14:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=mailo.com; s=mailo; t=1672161267; bh=t7PT2uktgkerHfBW8tbhYinlf/iWYlEafzcWDES++jM=; h=X-EA-Auth:Date:From:To:Cc:Subject:Message-ID:MIME-Version: Content-Type; b=PFvbFEkOjToKt4pNFDu81BSQhHuNoyMGKRRB1qLS/ma35bUMEGFGb4WWuIzm+JJs9 /KBGjbdAWvKBrsl3hbI1VMkzh1m1q+B3NyPN3EnwTckyAp7FEHxf/2DsKzWa0yKogr xUhVjDhHGShJfc1i/8vvL0xMSqkEAQYpGRcPYHvE= Received: by b-6.in.mailobj.net [192.168.90.16] with ESMTP via ip-206.mailobj.net [213.182.55.206] Tue, 27 Dec 2022 18:14:27 +0100 (CET) X-EA-Auth: kTiepSjWpyi9Ji8iWRK5eWeMrKdUoRK+JaiTVgN5jm3nna2m7biYtAmN9vNNkbmNg67H6j5YGy8WqhY1z1C7Uqq/5buDi/8h Date: Tue, 27 Dec 2022 22:44:21 +0530 From: Deepak R Varma <drv@mailo.com> To: Thierry Reding <thierry.reding@gmail.com>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, Jonathan Hunter <jonathanh@nvidia.com>, dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Saurabh Singh Sengar <ssengar@microsoft.com>, Praveen Kumar <kumarpraveen@linux.microsoft.com>, Deepak R Varma <drv@mailo.com> Subject: [PATCH] drm/tegra: submit: No need for Null pointer check before kfree Message-ID: <Y6sn7XptKyk5cbrA@qemulion> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1753390023578717609?= X-GMAIL-MSGID: =?utf-8?q?1753390023578717609?= |
Series |
drm/tegra: submit: No need for Null pointer check before kfree
|
|
Commit Message
Deepak R Varma
Dec. 27, 2022, 5:14 p.m. UTC
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@mailo.com>
---
drivers/gpu/drm/tegra/submit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--
2.34.1
Comments
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@mailo.com> > --- > 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. Mikko
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@mailo.com> > > --- > > 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
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@mailo.com> >>> --- >>> 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. Mikko
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@mailo.com> > > > > --- > > > > 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. Hello Mikko, That really helps. Thank you for the detailed explanation. I do have an extended question though. In this context, when we are ready to release the memory, how is it useful to know if it is NULL or not this late in the flow when the scope is about to end? Thanks again! ./drv > > Mikko
On 12/28/22 15:34, Deepak R Varma 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@mailo.com> >>>>> --- >>>>> 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. > > Hello Mikko, > That really helps. Thank you for the detailed explanation. I do have an extended > question though. In this context, when we are ready to release the memory, how > is it useful to know if it is NULL or not this late in the flow when the scope > is about to end? In the current code it doesn't matter, but if someone went to change this code (for example to add another release step), and we just had 'kfree(job_data)', they would have to remember that kfree works with NULL pointers, and would have to go looking elsewhere in the code to see if it is in fact possible to assume that job_data cannot be NULL here, or not. If they forget about kfree working with NULL pointers, which wouldn't be that surprising since it is almost always only called with non-NULL pointers, they might instead introduce a bug. In this particular instance it's probably not that bad since immediately above we have another 'if' block that checks if job_data is NULL, which serves as a hint to the programmer; however, as a general principle it stands that having the NULL check here makes it obvious to any reading programmer that they any changes they make have to consider if the pointer is NULL or not. > > Thanks again! > ./drv > Thanks! Mikko > > > >> >> Mikko > >
On Wed, Dec 28, 2022 at 03:48:05PM +0200, Mikko Perttunen wrote: > On 12/28/22 15:34, Deepak R Varma wrote: > > On Wed, Dec 28, 2022 at 03:17:59PM +0200, Mikko Perttunen wrote: > > > On 12/28/22 15:08, Deepak R Varma wrote: > > > > > > 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. > > > > Hello Mikko, > > That really helps. Thank you for the detailed explanation. I do have an extended > > question though. In this context, when we are ready to release the memory, how > > is it useful to know if it is NULL or not this late in the flow when the scope > > is about to end? > > In the current code it doesn't matter, but if someone went to change this > code (for example to add another release step), and we just had > 'kfree(job_data)', they would have to remember that kfree works with NULL > pointers, and would have to go looking elsewhere in the code to see if it is > in fact possible to assume that job_data cannot be NULL here, or not. If > they forget about kfree working with NULL pointers, which wouldn't be that > surprising since it is almost always only called with non-NULL pointers, > they might instead introduce a bug. > > In this particular instance it's probably not that bad since immediately > above we have another 'if' block that checks if job_data is NULL, which > serves as a hint to the programmer; however, as a general principle it > stands that having the NULL check here makes it obvious to any reading > programmer that they any changes they make have to consider if the pointer > is NULL or not. Sounds good. Thanks again. Would like to see if other experts have any further guidance on this patch. Regards, ./drv > > > > > > Mikko > > > > >
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@mailo.com> > > > > --- > > > > 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
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@mailo.com> >>>>> --- >>>>> 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
On 12/30/22 12:01, Mikko Perttunen wrote: > 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@mailo.com> >>>>>> --- >>>>>> 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. Well, the refcount is dropped -- it's not necessarily freed immediately, if the job is in execution. Mikko > > (Yes, it is rather complicated..) > > Thanks, > Mikko
On Fri, Dec 30, 2022 at 12:01:23PM +0200, Mikko Perttunen wrote: > 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@mailo.com> > > > > > > --- > > > > > > 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..) Ok, then better to keep the check. Regards Stanislaw
On Fri, Dec 30, 2022 at 12:03:25PM +0200, Mikko Perttunen wrote: > On 12/30/22 12:01, Mikko Perttunen wrote: > > 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@mailo.com> > > > > > > > --- > > > > > > > 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. > > Well, the refcount is dropped -- it's not necessarily freed immediately, if > the job is in execution. Thanks Mikko. I Agree. Hence I think there is no change for the program at runtime. The proposed change looks safe to me. ./drv > > Mikko > > > > > (Yes, it is rather complicated..) > > > > Thanks, > > Mikko >
On Mon, Jan 02, 2023 at 11:50:36PM +0530, Deepak R Varma wrote: > On Fri, Dec 30, 2022 at 12:03:25PM +0200, Mikko Perttunen wrote: > > On 12/30/22 12:01, Mikko Perttunen wrote: > > > 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@mailo.com> > > > > > > > > --- > > > > > > > > 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. > > > > Well, the refcount is dropped -- it's not necessarily freed immediately, if > > the job is in execution. > > Thanks Mikko. I Agree. Hence I think there is no change for the program at > runtime. The proposed change looks safe to me. Hello, Requesting any further feedback/comment on this patch proposal. Thank you, ./drv > > ./drv > > > > > Mikko > > > > > > > > (Yes, it is rather complicated..) > > > > > > Thanks, > > > Mikko > >
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: