Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1687183 > unrolled thread

[PATCH, RESEND 03/14] drm/vmwgfx: avoid gcc-7 parentheses warning

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH, RESEND 03/14] drm/vmwgfx: avoid gcc-7 parentheses warning Arnd Bergmann <arnd@arndb.de> - 2017-07-14 11:30 +0200
    Re: [PATCH, RESEND 03/14] drm/vmwgfx: avoid gcc-7 parentheses warning Jani Nikula <jani.nikula@linux.intel.com> - 2017-07-14 12:10 +0200
    Re: [PATCH, RESEND 03/14] drm/vmwgfx: avoid gcc-7 parentheses warning Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-14 21:30 +0200
      Re: [PATCH, RESEND 03/14] drm/vmwgfx: avoid gcc-7 parentheses warning Arnd Bergmann <arnd@arndb.de> - 2017-07-14 22:30 +0200
    Re: [PATCH, RESEND 03/14] drm/vmwgfx: avoid gcc-7 parentheses warning Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-14 21:30 +0200

#1687183 — [PATCH, RESEND 03/14] drm/vmwgfx: avoid gcc-7 parentheses warning

gcc-7 warns about slightly suspicious code in vmw_cmd_invalid:

drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c: In function 'vmw_cmd_invalid':
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:522:23: error: the omitted middle operand in ?: will always be 'true', suggest explicit middle operand [-Werror=parentheses]

The problem is that it is mixing boolean and integer values here.
I assume that the code actually works correctly, so making it use
a literal '1' instead of the implied 'true' makes it more readable
and avoids the warning.

The code has been in this file since the start, but it could
make sense to backport this patch to stable to make it build cleanly
with gcc-7.

Fixes: fb1d9738ca05 ("drm/vmwgfx: Add DRM driver for VMware Virtual GPU")
Reviewed-by: Sinclair Yeh <syeh@vmware.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
Originally submitted on Nov 16, but for some reason it never appeared
upstream. The patch is still needed as of v4.11-rc2
---
 drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index c7b53d987f06..3f343e55972a 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -519,7 +519,7 @@ static int vmw_cmd_invalid(struct vmw_private *dev_priv,
 			   struct vmw_sw_context *sw_context,
 			   SVGA3dCmdHeader *header)
 {
-	return capable(CAP_SYS_ADMIN) ? : -EINVAL;
+	return capable(CAP_SYS_ADMIN) ? 1 : -EINVAL;
 }
 
 static int vmw_cmd_ok(struct vmw_private *dev_priv,
-- 
2.9.0

[toc] | [next] | [standalone]

#1687229

On Fri, 14 Jul 2017, Arnd Bergmann <arnd@arndb.de> wrote:
> gcc-7 warns about slightly suspicious code in vmw_cmd_invalid:
>
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c: In function 'vmw_cmd_invalid':
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:522:23: error: the omitted middle operand in ?: will always be 'true', suggest explicit middle operand [-Werror=parentheses]
>
> The problem is that it is mixing boolean and integer values here.
> I assume that the code actually works correctly, so making it use
> a literal '1' instead of the implied 'true' makes it more readable
> and avoids the warning.
>
> The code has been in this file since the start, but it could
> make sense to backport this patch to stable to make it build cleanly
> with gcc-7.
>
> Fixes: fb1d9738ca05 ("drm/vmwgfx: Add DRM driver for VMware Virtual GPU")
> Reviewed-by: Sinclair Yeh <syeh@vmware.com>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> Originally submitted on Nov 16, but for some reason it never appeared
> upstream. The patch is still needed as of v4.11-rc2

See also the thread starting at
http://lkml.kernel.org/r/CA+55aFwZV-QirBTY8y4y+h796V2CzpWdL=tWB27rJ1u3rojXYQ@mail.gmail.com

BR,
Jani.


> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> index c7b53d987f06..3f343e55972a 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> @@ -519,7 +519,7 @@ static int vmw_cmd_invalid(struct vmw_private *dev_priv,
>  			   struct vmw_sw_context *sw_context,
>  			   SVGA3dCmdHeader *header)
>  {
> -	return capable(CAP_SYS_ADMIN) ? : -EINVAL;
> +	return capable(CAP_SYS_ADMIN) ? 1 : -EINVAL;
>  }
>  
>  static int vmw_cmd_ok(struct vmw_private *dev_priv,

-- 
Jani Nikula, Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1687616

On Fri, Jul 14, 2017 at 12:21 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> NAK. This takes unintentionally insane code and turns it intentionally
> insane. Any non-zero return is considered an error.
>
> The right fix is almost certainly to just return -EINVAL unconditionally.

Btw, this is why I hate compiler warning fix patch series. Even when
they don't actually break the code (and sometimes they do that too),
they can actually end up making the code worse.

The *intent* of that code was to return zero for the CAP_SYS_ADMIN.
But the code has never done that in its lifetime and nobody ever
noticed, so clearly the code shouldn't even have tried.

                    Linus

[toc] | [prev] | [next] | [standalone]

#1687651

On Fri, Jul 14, 2017 at 9:23 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jul 14, 2017 at 12:21 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> NAK. This takes unintentionally insane code and turns it intentionally
>> insane. Any non-zero return is considered an error.
>>
>> The right fix is almost certainly to just return -EINVAL unconditionally.
>
> Btw, this is why I hate compiler warning fix patch series. Even when
> they don't actually break the code (and sometimes they do that too),
> they can actually end up making the code worse.

I generally agree, and this is also why I held up sending patches for the
-Wformat warnings until you brought those up. I also frequently send
patches for recently introduced warnings, which tend to have a better
chance of getting reviewed by the person that just introduced the code,
to catch this kind of mistake in my patches.

I also regularly run into cases where I send a correct patch and find
that another broken patch has been applied the following day ;-)

> The *intent* of that code was to return zero for the CAP_SYS_ADMIN.
> But the code has never done that in its lifetime and nobody ever
> noticed, so clearly the code shouldn't even have tried.

Makes sense, yes. In this case, the review process has failed as
well, as one of the maintainers even gave an Ack on the wrong patch,
and then the patch got dropped without any feedback.

        Arnd

[toc] | [prev] | [next] | [standalone]

#1687620

On Fri, Jul 14, 2017 at 2:25 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> -       return capable(CAP_SYS_ADMIN) ? : -EINVAL;
> +       return capable(CAP_SYS_ADMIN) ? 1 : -EINVAL;

NAK. This takes unintentionally insane code and turns it intentionally
insane. Any non-zero return is considered an error.

The right fix is almost certainly to just return -EINVAL unconditionally.

                  Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web