Subject: Re: [PATCH] drm/tegra: remove no need NULL check before kfree
To:     Bernard Zhao <bernard@vivo.com>,
        Thierry Reding <thierry.reding@gmail.com>,
        David Airlie <airlied@linux.ie>,
        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
References: <20211116015513.27147-1-bernard@vivo.com>
From:   Mikko Perttunen <cyndis@kapsi.fi>
Message-ID: <053718d8-0436-97c2-694d-510415dfbc12@kapsi.fi>
Date:   Tue, 16 Nov 2021 12:01:05 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
MIME-Version: 1.0
In-Reply-To: <20211116015513.27147-1-bernard@vivo.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/16/21 3:55 AM, Bernard Zhao wrote:
> This change is to cleanup the code a bit.
> 
> Signed-off-by: Bernard Zhao <bernard@vivo.com>
> ---
>   drivers/gpu/drm/tegra/submit.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/submit.c b/drivers/gpu/drm/tegra/submit.c
> index 776f825df52f..c2fc9677742e 100644
> --- a/drivers/gpu/drm/tegra/submit.c
> +++ b/drivers/gpu/drm/tegra/submit.c
> @@ -608,12 +608,10 @@ int tegra_drm_ioctl_channel_submit(struct drm_device *drm, void *data,
>   	if (job_data && job_data->used_mappings) {
>   		for (i = 0; i < job_data->num_used_mappings; i++)
>   			tegra_drm_mapping_put(job_data->used_mappings[i].mapping);
> -
> -		kfree(job_data->used_mappings);
>   	}
>   
> -	if (job_data)
> -		kfree(job_data);
> +	kfree(job_data->used_mappings);

Now if job_data == NULL, we dereference a NULL pointer here.

In general I'm not a fan of relying on kfree() etc. being a no-op on 
NULL pointers, since doing so gets rid of visual hints on code paths 
indicating the "liveness" of pointer variables.

However I'll let Thierry/other maintainers decide according to their 
preferences.

Thanks,
Mikko

> +	kfree(job_data);
>   put_bo:
>   	gather_bo_put(&bo->base);
>   unlock:
>