[PATCH -next] drm/tegra: Use PTR_ERR_OR_ZERO in tegra_gem_create()

[PATCH -next] drm/tegra: Use PTR_ERR_OR_ZERO in tegra_gem_create()

[YueHaibing]

Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR

Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 drivers/gpu/drm/tegra/drm.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index e22352c..056f749 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -497,10 +497,7 @@ static int tegra_gem_create(struct drm_device *drm, void *data,
 
 	bo = tegra_bo_create_with_handle(file, drm, args->size, args->flags,
 					 &args->handle);
-	if (IS_ERR(bo))
-		return PTR_ERR(bo);
-
-	return 0;
+	return PTR_ERR_OR_ZERO(bo);
 }
 
 static int tegra_gem_mmap(struct drm_device *drm, void *data,



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH -next] drm/tegra: Use PTR_ERR_OR_ZERO in tegra_gem_create()

[Mikko Perttunen]

I'm not the maintainer, but in line with previous similar patches..

NAK: this makes the code harder to read.

Thanks,
Mikko

On 25/09/2018 10.35, YueHaibing wrote:
> Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR
> 
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
>   drivers/gpu/drm/tegra/drm.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index e22352c..056f749 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -497,10 +497,7 @@ static int tegra_gem_create(struct drm_device *drm, void *data,
>   
>   	bo = tegra_bo_create_with_handle(file, drm, args->size, args->flags,
>   					 &args->handle);
> -	if (IS_ERR(bo))
> -		return PTR_ERR(bo);
> -
> -	return 0;
> +	return PTR_ERR_OR_ZERO(bo);
>   }
>   
>   static int tegra_gem_mmap(struct drm_device *drm, void *data,
> 
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH -next] drm/tegra: Use PTR_ERR_OR_ZERO in tegra_gem_create()

[Julia Lawall]

On Tue, 25 Sep 2018, Mikko Perttunen wrote:

> I'm not the maintainer, but in line with previous similar patches..
>
> NAK: this makes the code harder to read.

If people don't like it, I wonder if it is a good thing for the function
to even exist?  Or at least the semantic patch that suggests this could be
removed.

julia

>
> Thanks,
> Mikko
>
> On 25/09/2018 10.35, YueHaibing wrote:
> > Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR
> >
> > Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> > ---
> >   drivers/gpu/drm/tegra/drm.c | 5 +----
> >   1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > index e22352c..056f749 100644
> > --- a/drivers/gpu/drm/tegra/drm.c
> > +++ b/drivers/gpu/drm/tegra/drm.c
> > @@ -497,10 +497,7 @@ static int tegra_gem_create(struct drm_device *drm,
> > void *data,
> >     	bo = tegra_bo_create_with_handle(file, drm, args->size, args->flags,
> >   					 &args->handle);
> > -	if (IS_ERR(bo))
> > -		return PTR_ERR(bo);
> > -
> > -	return 0;
> > +	return PTR_ERR_OR_ZERO(bo);
> >   }
> >     static int tegra_gem_mmap(struct drm_device *drm, void *data,
> >
> >
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH -next] drm/tegra: Use PTR_ERR_OR_ZERO in tegra_gem_create()

[Mikko Perttunen]

On 25/09/2018 16.37, Julia Lawall wrote:
> 
> 
> On Tue, 25 Sep 2018, Mikko Perttunen wrote:
> 
>> I'm not the maintainer, but in line with previous similar patches..
>>
>> NAK: this makes the code harder to read.
> 
> If people don't like it, I wonder if it is a good thing for the function
> to even exist?  Or at least the semantic patch that suggests this could be
> removed.

Good question. I think it may still have its place in some situations - 
e.g. if there's only one call, something like

   return PTR_ERR_OR_ZERO(do_something());

where this is the only potentially erroring thing in the function.

In this case (and the previous ones I referred to), it's been a function 
with a longer series of code like

   variable = function(...);
   if (IS_ERR(variable))
     return PTR_ERR(variable);

and if we just change the last one it looks out of place. Although 
honestly, I would just write the first example in long-form as well.

In the end it's a question of taste. With Tegra code we have gone for 
not using PTR_ERR_OR_ZERO.

Cheers,
Mikko

> 
> julia
> 
>>
>> Thanks,
>> Mikko
>>
>> On 25/09/2018 10.35, YueHaibing wrote:
>>> Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR
>>>
>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>>> ---
>>>    drivers/gpu/drm/tegra/drm.c | 5 +----
>>>    1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
>>> index e22352c..056f749 100644
>>> --- a/drivers/gpu/drm/tegra/drm.c
>>> +++ b/drivers/gpu/drm/tegra/drm.c
>>> @@ -497,10 +497,7 @@ static int tegra_gem_create(struct drm_device *drm,
>>> void *data,
>>>      	bo = tegra_bo_create_with_handle(file, drm, args->size, args->flags,
>>>    					 &args->handle);
>>> -	if (IS_ERR(bo))
>>> -		return PTR_ERR(bo);
>>> -
>>> -	return 0;
>>> +	return PTR_ERR_OR_ZERO(bo);
>>>    }
>>>      static int tegra_gem_mmap(struct drm_device *drm, void *data,
>>>
>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH -next] drm/tegra: Use PTR_ERR_OR_ZERO in tegra_gem_create()

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2156 bytes --]

On Tue, Sep 25, 2018 at 04:57:30PM +0900, Mikko Perttunen wrote:
> On 25/09/2018 16.37, Julia Lawall wrote:
> > 
> > 
> > On Tue, 25 Sep 2018, Mikko Perttunen wrote:
> > 
> > > I'm not the maintainer, but in line with previous similar patches..
> > > 
> > > NAK: this makes the code harder to read.
> > 
> > If people don't like it, I wonder if it is a good thing for the function
> > to even exist?  Or at least the semantic patch that suggests this could be
> > removed.
> 
> Good question. I think it may still have its place in some situations - e.g.
> if there's only one call, something like
> 
>   return PTR_ERR_OR_ZERO(do_something());
> 
> where this is the only potentially erroring thing in the function.
> 
> In this case (and the previous ones I referred to), it's been a function
> with a longer series of code like
> 
>   variable = function(...);
>   if (IS_ERR(variable))
>     return PTR_ERR(variable);
> 
> and if we just change the last one it looks out of place. Although honestly,
> I would just write the first example in long-form as well.
> 
> In the end it's a question of taste. With Tegra code we have gone for not
> using PTR_ERR_OR_ZERO.

This has come up recently for the Tegra PCI driver as well, and we were
wondering the same thing. I know that Bjorn (PCI maintainer) and I have
in the past agreed that PTR_ERR_OR_ZERO() is not something we like. For
mostly the same reasons that Mikko already cited. In addition I think
that PTR_ERR_OR_ZERO() makes it needlessly complicated to append code
to a function. Instead of just adding a:

	ptr = function(...);
	if (IS_ERR(ptr))
		return PTR_ERR(ptr);

You need to split up the PTR_ERR_OR_ZERO() first and then be careful
that you return the correct result, etc.

I'm sure there are people that prefer PTR_ERR_OR_ZERO() over the open-
coded alternative, so this is probably just something we're going to
have to live with. Perhaps a good compromise would be to keep the macro
around, but get rid of the semantic patch that suggests the change. At
least that way we would have to go over this over and over again.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel