Re: [PATCH 1/2] drm/tegra: Return an error code if fails

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]

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.

Thierry

> diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
> index dea38892d6e6..0ce22935fbd3 100644
> --- a/drivers/gpu/drm/tegra/gem.c
> +++ b/drivers/gpu/drm/tegra/gem.c
> @@ -710,6 +710,8 @@ static int tegra_gem_prime_vmap(struct dma_buf *buf, struct iosys_map *map)
>  	void *vaddr;
>  
>  	vaddr = tegra_bo_mmap(&bo->base);
> +	if (!vaddr)
> +		return -ENOMEM;
>  	if (IS_ERR(vaddr))
>  		return PTR_ERR(vaddr);
>  
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]