[PATCH] drm/drm_gem.c: remove surplus else after return clause

[Maxime Ripard]

Hi,

On Tue, Jun 20, 2023 at 06:18:31PM +0200, Thomas Zimmermann wrote:
> Am 20.06.23 um 18:06 schrieb Sui Jingfeng:
> > Hi,
> > 
> > On 2023/6/20 22:43, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > Am 20.06.23 um 06:08 schrieb Sui Jingfeng:
> > > > ping ?
> > > > 
> > > > On 2023/3/14 20:53, Sui Jingfeng wrote:
> > > > >   else is not generally useful after return
> > > 
> > > No indention please.
> > > 
> > OK, will be fixed at the next version.
> > > > > 
> > > > > Signed-off-by: Sui Jingfeng <15330273260 at 189.cn>
> > > > > ---
> > > > >   drivers/gpu/drm/drm_gem.c | 7 ++++---
> > > > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > > > index a6208e2c089b..364e3733af98 100644
> > > > > --- a/drivers/gpu/drm/drm_gem.c
> > > > > +++ b/drivers/gpu/drm/drm_gem.c
> > > > > @@ -1150,8 +1150,8 @@ int drm_gem_pin(struct drm_gem_object *obj)
> > > > >   {
> > > > >       if (obj->funcs->pin)
> > > > >           return obj->funcs->pin(obj);
> > > > > -    else
> > > > > -        return 0;
> > > > > +
> > > > > +    return 0;
> > > 
> > > This change is ok.
> > > 
> > > > >   }
> > > > >   void drm_gem_unpin(struct drm_gem_object *obj)
> > > > > @@ -1172,7 +1172,8 @@ int drm_gem_vmap(struct drm_gem_object
> > > > > *obj, struct iosys_map *map)
> > > > >       ret = obj->funcs->vmap(obj, map);
> > > > >       if (ret)
> > > > >           return ret;
> > > > > -    else if (iosys_map_is_null(map))
> > > > > +
> > > > > +    if (iosys_map_is_null(map))
> > > > >           return -ENOMEM;
> > > 
> > > This is not correct. Calling iosys_map_is_null() is part of handling
> > > the return value from vmap, so the else is fine.
> > > 
> > Are you serious ?
> > 
> > 
> > 1. Before apply this patch:
> > 
> > 
> > If the 'ret' is 0,  it stand for obj->funcs->vmap() is successful, then
> > if (iosys_map_is_null(map)) will be run.
> > 
> > If the 'ret' is NOT 0, then it return immediately.
> > 
> > 
> > 2. After apply this patch:
> > 
> > 
> > If the 'ret' is NOT 0, it stand for obj->funcs->vmap() failed, then it
> > return immediately.
> > 
> > If the 'ret' is 0, it stand for obj->funcs->vmap() is successful, then
> > the check if (iosys_map_is_null(map))
> > 
> > will be run!
> > 
> > 
> > I feel strange about the core here, I think the check ' if
> > (iosys_map_is_null(map))' is not needed,
> > 
> > the implement should responsible to handle all of possible errors.
> 
> The ->vmap() callback can succeed with ret=0, but we still have no memory.
> Then we return -ENOMEM. The call to _is_null(map) is part of the error
> handling for ->vmap(). That is a bit strange, but it as always worked like
> that. Keeping all error handling in the same if-else block make all this
> more obvious.

Reading that patch, it wasn't obvious to me at all and could have made
the same patch.

Could we add a comment maybe to make it more obvious?

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230626/79478e3f/attachment.sig>