Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <03a7a1be-a78a-3380-4b65-091bef0914b6@loongson.cn>
Date:   Wed, 11 Oct 2023 11:06:11 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH 1/2] drm/tegra: Return an error code if fails
To:     Thierry Reding <thierry.reding@gmail.com>
Cc:     Mikko Perttunen <mperttunen@nvidia.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
References: <20230626143331.640454-1-suijingfeng@loongson.cn>
 <ZSVQMPuRnXzC0lgf@orome.fritz.box> <ZSVuVcqdGfGtQIQj@orome.fritz.box>
Content-Language: en-US
From:   Sui Jingfeng <suijingfeng@loongson.cn>
In-Reply-To: <ZSVuVcqdGfGtQIQj@orome.fritz.box>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,


On 2023/10/10 23:31, Thierry Reding wrote:
> On Tue, Oct 10, 2023 at 03:22:56PM +0200, Thierry Reding wrote:
>> On Mon, Jun 26, 2023 at 10:33:30PM +0800, Sui Jingfeng wrote:
>>> Return -ENOMEM if tegra_bo_mmap() fails.
>>>
>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>> ---
>>>   drivers/gpu/drm/tegra/gem.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>> Sorry, this fell through the cracks. I think it'd be better if
>> tegra_bo_mmap() were to be improved to always return either an ERR_PTR()
>> encoded error code or a valid pointer. Throwing NULL into the mix isn't
>> useful because it typically means something like -ENOMEM anyway. Error
>> codes are more explicit, so since we're already using them for some
>> cases, might as well return them for all.
>>
>> Actually, looks like tegra_bo_mmap() never actually returns an ERR_PTR()
>> encoded error code. It's either obj->vaddr, the return value of vmap()
>> (which is either NULL or the address of the mapping), or the address
>> obtained from dma_buf_vmap_unlocked() (i.e. map.vaddr) or NULL on
>> failure. So I think it would equally make sense to keep your patch and
>> to remove the IS_ERR() check below it.
>>
>> I would slightly prefer the first option, but either is fine.
> How about the attached patch?


I also prefer the prefer the first option.
The attached patch is more better, because it solve the problem at lower level.

Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>

> Thierry