[PATCH 00/14] gcc-7 warnings

[PATCH 00/14] gcc-7 warnings

[Arnd Bergmann]

This series should shut up all warnings introduced by gcc-6 or gcc-7 on
today's linux-next, as observed in "allmodconfig" builds on x86,
arm and arm64.

I have sent some of these before, but some others are new, as I had
at some point disabled the -Wint-in-bool-context warning in my
randconfig testing and did not notice the other warnings.

I have another series to address all -Wformat-overflow warnings,
and one more patch to turn off the -Wformat-truncation warnings
unless we build with "make W=1". I'll send that separately.

Most of these are consist of trivial refactoring of the code to
shut up false-positive warnings, the one exception being
"staging:iio:resolver:ad2s1210 fix negative IIO_ANGL_VEL read",
which fixes a regression against linux-3.1 that has gone
unnoticed since then. Still, review from subsystem maintainers
would be appreciated.

I would suggest that Andrew Morton can pick these up into linux-mm
so we can make sure they all make it into the release. Alternatively
Linus might feel like picking them all up himself.

While I did not mark the harmless ones for stable backports,
Greg may also want to pick them up once they go upstream, to
help build-test the stable kernels with gcc-7.

      Arnd

Arnd Bergmann (14):
  [SUBMITTED 20170511] ide: avoid warning for timings calculation
  [SUBMITTED 20170511] ata: avoid gcc-7 warning in ata_timing_quantize
  [SUBMITTED 20170314] drm/vmwgfx: avoid gcc-7 parentheses warning
  x86: math-emu: avoid -Wint-in-bool-context warning
  isdn: isdnloop: suppress a gcc-7 warning
  acpi: thermal: fix gcc-6/ccache warning
  proc/kcore: hide a harmless warning
  Input: adxl34x - fix gcc-7 -Wint-in-bool-context warning
  SFI: fix tautological-compare warning
  staging:iio:resolver:ad2s1210 fix negative IIO_ANGL_VEL read
  IB/uverbs: fix gcc-7 type warning
  drm/nouveau/clk: fix gcc-7 -Wint-in-bool-context warning
  iopoll: avoid -Wint-in-bool-context warning
  [media] fix warning on v4l2_subdev_call() result interpreted as bool

 arch/x86/math-emu/fpu_emu.h                          |  2 +-
 drivers/acpi/processor_thermal.c                     |  6 ++++--
 drivers/ata/libata-core.c                            | 20 ++++++++++----------
 drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c      |  6 +++---
 drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c              |  2 +-
 drivers/ide/ide-timings.c                            | 18 +++++++++---------
 drivers/infiniband/core/uverbs.h                     | 14 ++++++++------
 drivers/input/misc/adxl34x.c                         |  2 +-
 drivers/isdn/isdnloop/isdnloop.c                     |  2 +-
 drivers/media/pci/cx18/cx18-ioctl.c                  |  6 ++++--
 drivers/media/pci/saa7146/mxb.c                      |  5 +++--
 drivers/media/platform/atmel/atmel-isc.c             |  4 ++--
 drivers/media/platform/atmel/atmel-isi.c             |  4 ++--
 drivers/media/platform/blackfin/bfin_capture.c       |  4 ++--
 drivers/media/platform/omap3isp/ispccdc.c            |  5 +++--
 drivers/media/platform/pxa_camera.c                  |  3 ++-
 drivers/media/platform/rcar-vin/rcar-core.c          |  2 +-
 drivers/media/platform/rcar-vin/rcar-dma.c           |  4 +++-
 drivers/media/platform/soc_camera/soc_camera.c       |  4 ++--
 drivers/media/platform/stm32/stm32-dcmi.c            |  4 ++--
 drivers/media/platform/ti-vpe/cal.c                  |  6 ++++--
 drivers/sfi/sfi_core.c                               |  9 ++++++---
 drivers/staging/iio/resolver/ad2s1210.c              |  2 +-
 .../staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 13 +++++++------
 fs/proc/kcore.c                                      | 10 ++++++----
 include/linux/iopoll.h                               |  6 ++++--
 include/linux/regmap.h                               |  2 +-
 27 files changed, 93 insertions(+), 72 deletions(-)

-- 
2.9.0

[PATCH, RESEND 01/14] ide: avoid warning for timings calculation

[Arnd Bergmann]

gcc-7 warns about the result of a constant multiplication used as
a boolean:

drivers/ide/ide-timings.c: In function 'ide_timing_quantize':
drivers/ide/ide-timings.c:112:24: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
  q->setup   = EZ(t->setup   * 1000,  T);

This slightly rearranges the macro to simplify the code and avoid
the warning at the same time.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/ide/ide-timings.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/ide/ide-timings.c b/drivers/ide/ide-timings.c
index 0e05f75934c9..1858e3ce3993 100644
--- a/drivers/ide/ide-timings.c
+++ b/drivers/ide/ide-timings.c
@@ -104,19 +104,19 @@ u16 ide_pio_cycle_time(ide_drive_t *drive, u8 pio)
 EXPORT_SYMBOL_GPL(ide_pio_cycle_time);
 
 #define ENOUGH(v, unit)		(((v) - 1) / (unit) + 1)
-#define EZ(v, unit)		((v) ? ENOUGH(v, unit) : 0)
+#define EZ(v, unit)		((v) ? ENOUGH((v) * 1000, unit) : 0)
 
 static void ide_timing_quantize(struct ide_timing *t, struct ide_timing *q,
 				int T, int UT)
 {
-	q->setup   = EZ(t->setup   * 1000,  T);
-	q->act8b   = EZ(t->act8b   * 1000,  T);
-	q->rec8b   = EZ(t->rec8b   * 1000,  T);
-	q->cyc8b   = EZ(t->cyc8b   * 1000,  T);
-	q->active  = EZ(t->active  * 1000,  T);
-	q->recover = EZ(t->recover * 1000,  T);
-	q->cycle   = EZ(t->cycle   * 1000,  T);
-	q->udma    = EZ(t->udma    * 1000, UT);
+	q->setup   = EZ(t->setup,   T);
+	q->act8b   = EZ(t->act8b,   T);
+	q->rec8b   = EZ(t->rec8b,   T);
+	q->cyc8b   = EZ(t->cyc8b,   T);
+	q->active  = EZ(t->active,  T);
+	q->recover = EZ(t->recover, T);
+	q->cycle   = EZ(t->cycle,   T);
+	q->udma    = EZ(t->udma,    UT);
 }
 
 void ide_timing_merge(struct ide_timing *a, struct ide_timing *b,
-- 
2.9.0

[PATCH, RESEND 02/14] ata: avoid gcc-7 warning in ata_timing_quantize

[Arnd Bergmann]

gcc-7 warns about the result of a constant multiplication used as
a boolean:

drivers/ata/libata-core.c: In function 'ata_timing_quantize':
drivers/ata/libata-core.c:3164:30: warning: '*' in boolean context, suggest '&&' instead [-Wint-in-bool-context]

This slightly rearranges the macro to simplify the code and avoid
the warning at the same time.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/ata/libata-core.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index fa7dd4394c02..450059dd06ba 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3231,19 +3231,19 @@ static const struct ata_timing ata_timing[] = {
 };
 
 #define ENOUGH(v, unit)		(((v)-1)/(unit)+1)
-#define EZ(v, unit)		((v)?ENOUGH(v, unit):0)
+#define EZ(v, unit)		((v)?ENOUGH(((v) * 1000), unit):0)
 
 static void ata_timing_quantize(const struct ata_timing *t, struct ata_timing *q, int T, int UT)
 {
-	q->setup	= EZ(t->setup      * 1000,  T);
-	q->act8b	= EZ(t->act8b      * 1000,  T);
-	q->rec8b	= EZ(t->rec8b      * 1000,  T);
-	q->cyc8b	= EZ(t->cyc8b      * 1000,  T);
-	q->active	= EZ(t->active     * 1000,  T);
-	q->recover	= EZ(t->recover    * 1000,  T);
-	q->dmack_hold	= EZ(t->dmack_hold * 1000,  T);
-	q->cycle	= EZ(t->cycle      * 1000,  T);
-	q->udma		= EZ(t->udma       * 1000, UT);
+	q->setup	= EZ(t->setup,       T);
+	q->act8b	= EZ(t->act8b,       T);
+	q->rec8b	= EZ(t->rec8b,       T);
+	q->cyc8b	= EZ(t->cyc8b,       T);
+	q->active	= EZ(t->active,      T);
+	q->recover	= EZ(t->recover,     T);
+	q->dmack_hold	= EZ(t->dmack_hold,  T);
+	q->cycle	= EZ(t->cycle,       T);
+	q->udma		= EZ(t->udma,       UT);
 }
 
 void ata_timing_merge(const struct ata_timing *a, const struct ata_timing *b,
-- 
2.9.0

Re: [PATCH, RESEND 02/14] ata: avoid gcc-7 warning in ata_timing_quantize

[Tejun Heo]

On Fri, Jul 14, 2017 at 11:25:14AM +0200, Arnd Bergmann wrote:
> gcc-7 warns about the result of a constant multiplication used as
> a boolean:
> 
> drivers/ata/libata-core.c: In function 'ata_timing_quantize':
> drivers/ata/libata-core.c:3164:30: warning: '*' in boolean context, suggest '&&' instead [-Wint-in-bool-context]
> 
> This slightly rearranges the macro to simplify the code and avoid
> the warning at the same time.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

If the whole series will be routed together,

 Acked-by: Tejun Heo <tj@kernel.org>

If not, please let me know.  I'll push it through
libata/for-4.13-fixes.

Thanks!

-- 
tejun

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

[Arnd Bergmann]

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

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

[Jani Nikula]

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

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

[Linus Torvalds]

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

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

[Linus Torvalds]

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

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

[Arnd Bergmann]

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

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

[Sinclair Yeh]

On Fri, Jul 14, 2017 at 10:28:29PM +0200, Arnd Bergmann wrote:
> 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.

Correct.  I'll fix this.

> >
> > 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.

I've done some digging and noticed that my -fixes pull request
didn't get picked up last December.  It's most likely because I
initially made an address typo in the original request, and then
followed it up with a direct email with the correct address.

Sinclair

[PATCH 04/14] x86: math-emu: avoid -Wint-in-bool-context warning

[Arnd Bergmann]

The setsign() macro gets called with an integer argument in a
few places, leading to a harmless warning in gcc-7:

arch/x86/math-emu/reg_add_sub.c: In function 'FPU_add':
arch/x86/math-emu/reg_add_sub.c:80:48: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]

This turns the integer into a boolean expression by comparing it
to zero.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/x86/math-emu/fpu_emu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/math-emu/fpu_emu.h b/arch/x86/math-emu/fpu_emu.h
index afbc4d805d66..c9c320dccca1 100644
--- a/arch/x86/math-emu/fpu_emu.h
+++ b/arch/x86/math-emu/fpu_emu.h
@@ -157,7 +157,7 @@ extern u_char const data_sizes_16[32];
 
 #define signbyte(a) (((u_char *)(a))[9])
 #define getsign(a) (signbyte(a) & 0x80)
-#define setsign(a,b) { if (b) signbyte(a) |= 0x80; else signbyte(a) &= 0x7f; }
+#define setsign(a,b) { if ((b) != 0) signbyte(a) |= 0x80; else signbyte(a) &= 0x7f; }
 #define copysign(a,b) { if (getsign(a)) signbyte(b) |= 0x80; \
                         else signbyte(b) &= 0x7f; }
 #define changesign(a) { signbyte(a) ^= 0x80; }
-- 
2.9.0

[PATCH 05/14] isdn: isdnloop: suppress a gcc-7 warning

[Arnd Bergmann]

We test whether a bit is set in a mask here, which is correct
but gcc warns about it as it thinks it might be confusing:

drivers/isdn/isdnloop/isdnloop.c:412:37: error: ?: using integer constants in boolean context, the expression will always evaluate to 'true' [-Werror=int-in-bool-context]

This replaces the negation of an integer with an equivalent
comparison to zero, which gets rid of the warning.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/isdn/isdnloop/isdnloop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
index 6ffd13466b8c..5792928b944d 100644
--- a/drivers/isdn/isdnloop/isdnloop.c
+++ b/drivers/isdn/isdnloop/isdnloop.c
@@ -409,7 +409,7 @@ isdnloop_sendbuf(int channel, struct sk_buff *skb, isdnloop_card *card)
 		return -EINVAL;
 	}
 	if (len) {
-		if (!(card->flags & (channel) ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE))
+		if ((card->flags & (channel) ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE) == 0)
 			return 0;
 		if (card->sndcount[channel] > ISDNLOOP_MAX_SQUEUE)
 			return 0;
-- 
2.9.0

Re: [PATCH 05/14] isdn: isdnloop: suppress a gcc-7 warning

[Joe Perches]

On Fri, 2017-07-14 at 11:25 +0200, Arnd Bergmann wrote:
> We test whether a bit is set in a mask here, which is correct
> but gcc warns about it as it thinks it might be confusing:
> 
> drivers/isdn/isdnloop/isdnloop.c:412:37: error: ?: using integer constants in boolean context, the expression will always evaluate to 'true' [-Werror=int-in-bool-context]
> 
> This replaces the negation of an integer with an equivalent
> comparison to zero, which gets rid of the warning.
[]
> diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
[]
> @@ -409,7 +409,7 @@ isdnloop_sendbuf(int channel, struct sk_buff *skb, isdnloop_card *card)
>  		return -EINVAL;
>  	}
>  	if (len) {
> -		if (!(card->flags & (channel) ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE))
> +		if ((card->flags & (channel) ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE) == 0)
>  			return 0;
>  		if (card->sndcount[channel] > ISDNLOOP_MAX_SQUEUE)
>  			return 0;

The if as written can not be zero.

drivers/isdn/isdnloop/isdnloop.h:#define ISDNLOOP_FLAGS_B1ACTIVE 1      /* B-Channel-1 is open           */
drivers/isdn/isdnloop/isdnloop.h:#define ISDNLOOP_FLAGS_B2ACTIVE 2      /* B-Channel-2 is open           */

Perhaps this is a logic defect and should be:

		if (!(card->flags & ((channel) ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE)))

Re: [PATCH 05/14] isdn: isdnloop: suppress a gcc-7 warning

[Arnd Bergmann]

On Fri, Jul 14, 2017 at 12:08 PM, Joe Perches <joe@perches.com> wrote:
> On Fri, 2017-07-14 at 11:25 +0200, Arnd Bergmann wrote:
>> We test whether a bit is set in a mask here, which is correct
>> but gcc warns about it as it thinks it might be confusing:
>>
>> drivers/isdn/isdnloop/isdnloop.c:412:37: error: ?: using integer constants in boolean context, the expression will always evaluate to 'true' [-Werror=int-in-bool-context]
>>
>> This replaces the negation of an integer with an equivalent
>> comparison to zero, which gets rid of the warning.
> []
>> diff --git a/drivers/isdn/isdnloop/isdnloop.c b/drivers/isdn/isdnloop/isdnloop.c
> []
>> @@ -409,7 +409,7 @@ isdnloop_sendbuf(int channel, struct sk_buff *skb, isdnloop_card *card)
>>               return -EINVAL;
>>       }
>>       if (len) {
>> -             if (!(card->flags & (channel) ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE))
>> +             if ((card->flags & (channel) ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE) == 0)
>>                       return 0;
>>               if (card->sndcount[channel] > ISDNLOOP_MAX_SQUEUE)
>>                       return 0;
>
> The if as written can not be zero.
>
> drivers/isdn/isdnloop/isdnloop.h:#define ISDNLOOP_FLAGS_B1ACTIVE 1      /* B-Channel-1 is open           */
> drivers/isdn/isdnloop/isdnloop.h:#define ISDNLOOP_FLAGS_B2ACTIVE 2      /* B-Channel-2 is open           */
>
> Perhaps this is a logic defect and should be:
>
>                 if (!(card->flags & ((channel) ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE)))

Yes, good catch. I had thought about it for a bit whether that would be
the answer, but come to the wrong conclusion on my own.

Note that the version you suggested will still have the warning, so I think
it needs to be

                 if (card->flags &
                    ((channel) ? ISDNLOOP_FLAGS_B2ACTIVE :
ISDNLOOP_FLAGS_B1ACTIVE)
                     == 0)

or something like that, probably having a temporary flag variable would be best:

          int flag =  channel ? ISDNLOOP_FLAGS_B2ACTIVE :
ISDNLOOP_FLAGS_B1ACTIVE;
          if ((card->flags & flag) == 0)
                       return 0;

         Arnd

Re: [PATCH 05/14] isdn: isdnloop: suppress a gcc-7 warning

[Kevin Easton]

On Fri, Jul 14, 2017 at 12:37:05PM +0200, Arnd Bergmann wrote:
> On Fri, Jul 14, 2017 at 12:08 PM, Joe Perches <joe@perches.com> wrote:
> > On Fri, 2017-07-14 at 11:25 +0200, Arnd Bergmann wrote:
> >> We test whether a bit is set in a mask here, which is correct
> >> but gcc warns about it as it thinks it might be confusing:
> >>
> >> drivers/isdn/isdnloop/isdnloop.c:412:37: error: ?: using integer constants in boolean context, the expression will always evaluate to 'true' [-Werror=int-in-bool-context]

...

> > Perhaps this is a logic defect and should be:
> >
> >                 if (!(card->flags & ((channel) ? ISDNLOOP_FLAGS_B2ACTIVE : ISDNLOOP_FLAGS_B1ACTIVE)))
> 
> Yes, good catch. I had thought about it for a bit whether that would be
> the answer, but come to the wrong conclusion on my own.
> 
> Note that the version you suggested will still have the warning, so I think
> it needs to be

It shouldn't - the warning is for using an integer *constant* in boolean
context, but the result of & isn't a constant and should be fine.

!(flags & mask) is a very common idiom.

    - Kevin 

[PATCH 06/14] acpi: thermal: fix gcc-6/ccache warning

[Arnd Bergmann]

In some configurations, topology_physical_package_id() is trivially
defined as '-1' for any input, resulting a comparison that is
always true:

drivers/acpi/processor_thermal.c: In function ‘cpufreq_set_cur_state’:
drivers/acpi/processor_thermal.c:137:36: error: self-comparison always evaluates to true [-Werror=tautological-compare]

By introducing a temporary variable, we can tell gcc that this is
intentional.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/acpi/processor_thermal.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/processor_thermal.c b/drivers/acpi/processor_thermal.c
index 59c3a5d1e600..411f3a7f4a7c 100644
--- a/drivers/acpi/processor_thermal.c
+++ b/drivers/acpi/processor_thermal.c
@@ -122,20 +122,22 @@ static int cpufreq_get_cur_state(unsigned int cpu)
 static int cpufreq_set_cur_state(unsigned int cpu, int state)
 {
 	int i;
+	int id;
 
 	if (!cpu_has_cpufreq(cpu))
 		return 0;
 
 	reduction_pctg(cpu) = state;
 
+	id = topology_physical_package_id(cpu);
+
 	/*
 	 * Update all the CPUs in the same package because they all
 	 * contribute to the temperature and often share the same
 	 * frequency.
 	 */
 	for_each_online_cpu(i) {
-		if (topology_physical_package_id(i) ==
-		    topology_physical_package_id(cpu))
+		if (topology_physical_package_id(i) == id)
 			cpufreq_update_policy(i);
 	}
 	return 0;
-- 
2.9.0

[PATCH 07/14] proc/kcore: hide a harmless warning

[Arnd Bergmann]

gcc warns when MODULES_VADDR/END is defined to the same value as
VMALLOC_START/VMALLOC_END, e.g. on x86-32:

fs/proc/kcore.c: In function ‘add_modules_range’:
fs/proc/kcore.c:622:161: error: self-comparison always evaluates to false [-Werror=tautological-compare]
  if (/*MODULES_VADDR != VMALLOC_START && */MODULES_END != VMALLOC_END) {

The code is correct as it is required for most other configurations.
The best workaround I found for shutting up that warning is to make
it a little more complex by adding a temporary variable. The compiler
will still optimize away the code as it finds the two to be identical,
but it no longer warns because it doesn't condider the comparison
"tautological" any more.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/proc/kcore.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 45629f4b5402..c503ad657c46 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -620,12 +620,14 @@ static void __init proc_kcore_text_init(void)
 /*
  * MODULES_VADDR has no intersection with VMALLOC_ADDR.
  */
-struct kcore_list kcore_modules;
+static struct kcore_list kcore_modules;
 static void __init add_modules_range(void)
 {
-	if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) {
-		kclist_add(&kcore_modules, (void *)MODULES_VADDR,
-			MODULES_END - MODULES_VADDR, KCORE_VMALLOC);
+	void *start = (void *)MODULES_VADDR;
+	size_t len = MODULES_END - MODULES_VADDR;
+
+	if (start != (void *)VMALLOC_START && len != VMALLOC_END - VMALLOC_START) {
+		kclist_add(&kcore_modules, start, len, KCORE_VMALLOC);
 	}
 }
 #else
-- 
2.9.0

Re: [PATCH 07/14] proc/kcore: hide a harmless warning

[Ard Biesheuvel]

On 14 July 2017 at 10:25, Arnd Bergmann <arnd@arndb.de> wrote:
> gcc warns when MODULES_VADDR/END is defined to the same value as
> VMALLOC_START/VMALLOC_END, e.g. on x86-32:
>
> fs/proc/kcore.c: In function ‘add_modules_range’:
> fs/proc/kcore.c:622:161: error: self-comparison always evaluates to false [-Werror=tautological-compare]
>   if (/*MODULES_VADDR != VMALLOC_START && */MODULES_END != VMALLOC_END) {
>

Does it occur for subtraction as well? Or only for comparison?

> The code is correct as it is required for most other configurations.
> The best workaround I found for shutting up that warning is to make
> it a little more complex by adding a temporary variable. The compiler
> will still optimize away the code as it finds the two to be identical,
> but it no longer warns because it doesn't condider the comparison
> "tautological" any more.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  fs/proc/kcore.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
> index 45629f4b5402..c503ad657c46 100644
> --- a/fs/proc/kcore.c
> +++ b/fs/proc/kcore.c
> @@ -620,12 +620,14 @@ static void __init proc_kcore_text_init(void)
>  /*
>   * MODULES_VADDR has no intersection with VMALLOC_ADDR.
>   */
> -struct kcore_list kcore_modules;
> +static struct kcore_list kcore_modules;
>  static void __init add_modules_range(void)
>  {
> -       if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) {
> -               kclist_add(&kcore_modules, (void *)MODULES_VADDR,
> -                       MODULES_END - MODULES_VADDR, KCORE_VMALLOC);
> +       void *start = (void *)MODULES_VADDR;
> +       size_t len = MODULES_END - MODULES_VADDR;
> +
> +       if (start != (void *)VMALLOC_START && len != VMALLOC_END - VMALLOC_START) {
> +               kclist_add(&kcore_modules, start, len, KCORE_VMALLOC);
>         }
>  }
>  #else
> --
> 2.9.0
>

Re: [PATCH 07/14] proc/kcore: hide a harmless warning

[Arnd Bergmann]

On Fri, Jul 14, 2017 at 2:28 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 14 July 2017 at 10:25, Arnd Bergmann <arnd@arndb.de> wrote:
>> gcc warns when MODULES_VADDR/END is defined to the same value as
>> VMALLOC_START/VMALLOC_END, e.g. on x86-32:
>>
>> fs/proc/kcore.c: In function ‘add_modules_range’:
>> fs/proc/kcore.c:622:161: error: self-comparison always evaluates to false [-Werror=tautological-compare]
>>   if (/*MODULES_VADDR != VMALLOC_START && */MODULES_END != VMALLOC_END) {
>>
>
> Does it occur for subtraction as well? Or only for comparison?

This replacement patch would also address the warning:

diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
index 45629f4b5402..35824e986c2c 100644
--- a/fs/proc/kcore.c
+++ b/fs/proc/kcore.c
@@ -623,7 +623,7 @@ static void __init proc_kcore_text_init(void)
 struct kcore_list kcore_modules;
 static void __init add_modules_range(void)
 {
-       if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) {
+       if (MODULES_VADDR - VMALLOC_START && MODULES_END - VMALLOC_END) {
                kclist_add(&kcore_modules, (void *)MODULES_VADDR,
                        MODULES_END - MODULES_VADDR, KCORE_VMALLOC);
        }

I have also verified that four of the 14 patches are not needed when building
without ccache, this is one of them:

 acpi: thermal: fix gcc-6/ccache warning
 proc/kcore: hide a harmless warning
 SFI: fix tautological-compare warning
 [media] fix warning on v4l2_subdev_call() result interpreted as bool

Not sure what to do with those, we could either ignore them all and
not care about ccache, or we try to address them all in some way.

        Arnd

Re: [PATCH 07/14] proc/kcore: hide a harmless warning

[Ard Biesheuvel]

On 18 July 2017 at 20:53, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Jul 14, 2017 at 2:28 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 14 July 2017 at 10:25, Arnd Bergmann <arnd@arndb.de> wrote:
>>> gcc warns when MODULES_VADDR/END is defined to the same value as
>>> VMALLOC_START/VMALLOC_END, e.g. on x86-32:
>>>
>>> fs/proc/kcore.c: In function ‘add_modules_range’:
>>> fs/proc/kcore.c:622:161: error: self-comparison always evaluates to false [-Werror=tautological-compare]
>>>   if (/*MODULES_VADDR != VMALLOC_START && */MODULES_END != VMALLOC_END) {
>>>
>>
>> Does it occur for subtraction as well? Or only for comparison?
>
> This replacement patch would also address the warning:
>
> diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
> index 45629f4b5402..35824e986c2c 100644
> --- a/fs/proc/kcore.c
> +++ b/fs/proc/kcore.c
> @@ -623,7 +623,7 @@ static void __init proc_kcore_text_init(void)
>  struct kcore_list kcore_modules;
>  static void __init add_modules_range(void)
>  {
> -       if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) {
> +       if (MODULES_VADDR - VMALLOC_START && MODULES_END - VMALLOC_END) {
>                 kclist_add(&kcore_modules, (void *)MODULES_VADDR,
>                         MODULES_END - MODULES_VADDR, KCORE_VMALLOC);
>         }
>
> I have also verified that four of the 14 patches are not needed when building
> without ccache, this is one of them:
>
>  acpi: thermal: fix gcc-6/ccache warning
>  proc/kcore: hide a harmless warning
>  SFI: fix tautological-compare warning
>  [media] fix warning on v4l2_subdev_call() result interpreted as bool
>
> Not sure what to do with those, we could either ignore them all and
> not care about ccache, or we try to address them all in some way.
>

Any idea why ccache makes a difference here? It is not obvious (not to
me at least)

Re: [PATCH 07/14] proc/kcore: hide a harmless warning

[Arnd Bergmann]

On Tue, Jul 18, 2017 at 9:55 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 18 July 2017 at 20:53, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Jul 14, 2017 at 2:28 PM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> On 14 July 2017 at 10:25, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> gcc warns when MODULES_VADDR/END is defined to the same value as
>>>> VMALLOC_START/VMALLOC_END, e.g. on x86-32:
>>>>
>>>> fs/proc/kcore.c: In function ‘add_modules_range’:
>>>> fs/proc/kcore.c:622:161: error: self-comparison always evaluates to false [-Werror=tautological-compare]
>>>>   if (/*MODULES_VADDR != VMALLOC_START && */MODULES_END != VMALLOC_END) {
>>>>
>>>
>>> Does it occur for subtraction as well? Or only for comparison?
>>
>> This replacement patch would also address the warning:
>>
>> diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
>> index 45629f4b5402..35824e986c2c 100644
>> --- a/fs/proc/kcore.c
>> +++ b/fs/proc/kcore.c
>> @@ -623,7 +623,7 @@ static void __init proc_kcore_text_init(void)
>>  struct kcore_list kcore_modules;
>>  static void __init add_modules_range(void)
>>  {
>> -       if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) {
>> +       if (MODULES_VADDR - VMALLOC_START && MODULES_END - VMALLOC_END) {
>>                 kclist_add(&kcore_modules, (void *)MODULES_VADDR,
>>                         MODULES_END - MODULES_VADDR, KCORE_VMALLOC);
>>         }
>>
>> I have also verified that four of the 14 patches are not needed when building
>> without ccache, this is one of them:
>>
>>  acpi: thermal: fix gcc-6/ccache warning
>>  proc/kcore: hide a harmless warning
>>  SFI: fix tautological-compare warning
>>  [media] fix warning on v4l2_subdev_call() result interpreted as bool
>>
>> Not sure what to do with those, we could either ignore them all and
>> not care about ccache, or we try to address them all in some way.
>>
>
> Any idea why ccache makes a difference here? It is not obvious (not to
> me at least)

When ccache is used, the compilation stage is apparently always done on
the preprocessed source file. So instead of parsing (with the integrated
preprocessor)

          if (MODULES_VADDR != VMALLOC_START ...)

the compiler sees

          if (((unsigned long)high_memory + (8 * 1024 * 1024))  !=
              ((unsigned long)high_memory + (8 * 1024 * 1024))  ...)

and it correctly considers the first expression something that one
would write in source code, while -Wtautological-compare
is intended to warn about the second version being always true,
which makes the 'if()' pointless.

       Arnd

Re: [PATCH 07/14] proc/kcore: hide a harmless warning

[Ard Biesheuvel]

On 18 July 2017 at 21:01, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Jul 18, 2017 at 9:55 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 18 July 2017 at 20:53, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Fri, Jul 14, 2017 at 2:28 PM, Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org> wrote:
>>>> On 14 July 2017 at 10:25, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>> gcc warns when MODULES_VADDR/END is defined to the same value as
>>>>> VMALLOC_START/VMALLOC_END, e.g. on x86-32:
>>>>>
>>>>> fs/proc/kcore.c: In function ‘add_modules_range’:
>>>>> fs/proc/kcore.c:622:161: error: self-comparison always evaluates to false [-Werror=tautological-compare]
>>>>>   if (/*MODULES_VADDR != VMALLOC_START && */MODULES_END != VMALLOC_END) {
>>>>>
>>>>
>>>> Does it occur for subtraction as well? Or only for comparison?
>>>
>>> This replacement patch would also address the warning:
>>>
>>> diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c
>>> index 45629f4b5402..35824e986c2c 100644
>>> --- a/fs/proc/kcore.c
>>> +++ b/fs/proc/kcore.c
>>> @@ -623,7 +623,7 @@ static void __init proc_kcore_text_init(void)
>>>  struct kcore_list kcore_modules;
>>>  static void __init add_modules_range(void)
>>>  {
>>> -       if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) {
>>> +       if (MODULES_VADDR - VMALLOC_START && MODULES_END - VMALLOC_END) {
>>>                 kclist_add(&kcore_modules, (void *)MODULES_VADDR,
>>>                         MODULES_END - MODULES_VADDR, KCORE_VMALLOC);
>>>         }
>>>
>>> I have also verified that four of the 14 patches are not needed when building
>>> without ccache, this is one of them:
>>>
>>>  acpi: thermal: fix gcc-6/ccache warning
>>>  proc/kcore: hide a harmless warning
>>>  SFI: fix tautological-compare warning
>>>  [media] fix warning on v4l2_subdev_call() result interpreted as bool
>>>
>>> Not sure what to do with those, we could either ignore them all and
>>> not care about ccache, or we try to address them all in some way.
>>>
>>
>> Any idea why ccache makes a difference here? It is not obvious (not to
>> me at least)
>
> When ccache is used, the compilation stage is apparently always done on
> the preprocessed source file. So instead of parsing (with the integrated
> preprocessor)
>
>           if (MODULES_VADDR != VMALLOC_START ...)
>
> the compiler sees
>
>           if (((unsigned long)high_memory + (8 * 1024 * 1024))  !=
>               ((unsigned long)high_memory + (8 * 1024 * 1024))  ...)
>
> and it correctly considers the first expression something that one
> would write in source code, while -Wtautological-compare
> is intended to warn about the second version being always true,
> which makes the 'if()' pointless.
>

Ah, now it makes sense. I was a bit surprised that
-Wtautological-compare complains about symbolic constants that resolve
to the same expression, but apparently it doesn't.

I see how ccache needs to preprocess first: that is how it notices
changes, by hashing the preprocessed input and comparing it to the
stored hash. I'd still expect it to go back to letting the compiler
preprocess for the actual build, but apparently it doesn't.

A quick google search didn't produce anything useful, but I'd expect
other ccache users to run into the same issue.

Re: [PATCH 07/14] proc/kcore: hide a harmless warning

[Arnd Bergmann]

On Tue, Jul 18, 2017 at 10:07 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 18 July 2017 at 21:01, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tue, Jul 18, 2017 at 9:55 PM, Ard Biesheuvel
>
> Ah, now it makes sense. I was a bit surprised that
> -Wtautological-compare complains about symbolic constants that resolve
> to the same expression, but apparently it doesn't.
>
> I see how ccache needs to preprocess first: that is how it notices
> changes, by hashing the preprocessed input and comparing it to the
> stored hash. I'd still expect it to go back to letting the compiler
> preprocess for the actual build, but apparently it doesn't.

When I tried to figure this out, I saw that ccache has two modes, "direct"
and "preprocessed". It usually tries to use direct mode unless something
prevents that.

In "direct" mode, it hashes the source file and the included headers
instead of the preprocessed source file, however it still calls the compiler
for the preprocessed source file, I guess since it has to preprocess the
file the first time it is seen so it can record which headers are included.

> A quick google search didn't produce anything useful, but I'd expect
> other ccache users to run into the same issue.

I suspect gcc-7 is still too new for most people to have noticed this.
The kernel is a very large codebase, and we only got a handful
of -Wtautological-compare warnings at all, most of them happen
wtihout ccache, too.

Among the four patches, three are for -Wtautological-compare, and one
 is for -Wint-in-bool-context:

         if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))

v4l2_subdev_call() in this case is a function-like macro that may return
-ENODEV if its first argument is NULL. The other -Wint-in-bool-context
I found all happen with or without ccache, most commonly there is
an constant integer expression passed into a macro and then checked
like

#define macro(arg) \
       do { \
            if (arg) \
               do_something(arg);  \
       } while (0)

         Arnd

[PATCH 08/14] Input: adxl34x - fix gcc-7 -Wint-in-bool-context warning

[Arnd Bergmann]

FIFO_MODE is an macro expression with a '<<' operator, which
gcc points out could be misread as a '<':

drivers/input/misc/adxl34x.c: In function 'adxl34x_probe':
drivers/input/misc/adxl34x.c:799:36: error: '<<' in boolean context, did you mean '<' ? [-Werror=int-in-bool-context]

This converts the test to an explicit comparison with zero,
making it clearer to gcc and the reader what is intended.

Fixes: e27c729219ad ("Input: add driver for ADXL345/346 Digital Accelerometers")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/input/misc/adxl34x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/misc/adxl34x.c b/drivers/input/misc/adxl34x.c
index 2b2d02f408bb..e0caaa0de454 100644
--- a/drivers/input/misc/adxl34x.c
+++ b/drivers/input/misc/adxl34x.c
@@ -796,7 +796,7 @@ struct adxl34x *adxl34x_probe(struct device *dev, int irq,
 
 	if (pdata->watermark) {
 		ac->int_mask |= WATERMARK;
-		if (!FIFO_MODE(pdata->fifo_mode))
+		if (FIFO_MODE(pdata->fifo_mode) == 0)
 			ac->pdata.fifo_mode |= FIFO_STREAM;
 	} else {
 		ac->int_mask |= DATA_READY;
-- 
2.9.0

Re: [PATCH 08/14] Input: adxl34x - fix gcc-7 -Wint-in-bool-context warning

[Linus Torvalds]

On Fri, Jul 14, 2017 at 2:25 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> FIFO_MODE is an macro expression with a '<<' operator, which
> gcc points out could be misread as a '<':

Yeah, no, NAK again.

We don't make the code look worse just because gcc is being a f*cking
moron about things.

This warning is clearly pure garbage.

                 Linus

Re: [PATCH 08/14] Input: adxl34x - fix gcc-7 -Wint-in-bool-context warning

[Arnd Bergmann]

On Fri, Jul 14, 2017 at 9:24 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jul 14, 2017 at 2:25 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> FIFO_MODE is an macro expression with a '<<' operator, which
>> gcc points out could be misread as a '<':
>
> Yeah, no, NAK again.
>
> We don't make the code look worse just because gcc is being a f*cking
> moron about things.
>
> This warning is clearly pure garbage.
>

I looked at this one again and found a better approach, matching the
check that is done a few lines later. Unless you find something wrong
with that one, I'd resubmit it with the fixup below.

      Arnd

--- a/drivers/input/misc/adxl34x.c
+++ b/drivers/input/misc/adxl34x.c
@@ -789,21 +789,21 @@ struct adxl34x *adxl34x_probe(struct device *dev, int irq,
                __set_bit(pdata->ev_code_ff, input_dev->keybit);
        }

        if (pdata->ev_code_act_inactivity)
                __set_bit(pdata->ev_code_act_inactivity, input_dev->keybit);

        ac->int_mask |= ACTIVITY | INACTIVITY;

        if (pdata->watermark) {
                ac->int_mask |= WATERMARK;
-               if (FIFO_MODE(pdata->fifo_mode) == 0)
+               if (FIFO_MODE(pdata->fifo_mode) == FIFO_BYPASS)
                        ac->pdata.fifo_mode |= FIFO_STREAM;
        } else {
                ac->int_mask |= DATA_READY;
        }

        if (pdata->tap_axis_control & (TAP_X_EN | TAP_Y_EN | TAP_Z_EN))
                ac->int_mask |= SINGLE_TAP | DOUBLE_TAP;

        if (FIFO_MODE(pdata->fifo_mode) == FIFO_BYPASS)
                ac->fifo_delay = false;

Re: [PATCH 08/14] Input: adxl34x - fix gcc-7 -Wint-in-bool-context warning

[Dmitry Torokhov]

On Fri, Jul 14, 2017 at 10:17:10PM +0200, Arnd Bergmann wrote:
> On Fri, Jul 14, 2017 at 9:24 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > On Fri, Jul 14, 2017 at 2:25 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> FIFO_MODE is an macro expression with a '<<' operator, which
> >> gcc points out could be misread as a '<':
> >
> > Yeah, no, NAK again.
> >
> > We don't make the code look worse just because gcc is being a f*cking
> > moron about things.
> >
> > This warning is clearly pure garbage.
> >
> 
> I looked at this one again and found a better approach, matching the
> check that is done a few lines later. Unless you find something wrong
> with that one, I'd resubmit it with the fixup below.
> 
>       Arnd
> 
> --- a/drivers/input/misc/adxl34x.c
> +++ b/drivers/input/misc/adxl34x.c
> @@ -789,21 +789,21 @@ struct adxl34x *adxl34x_probe(struct device *dev, int irq,
>                 __set_bit(pdata->ev_code_ff, input_dev->keybit);
>         }
> 
>         if (pdata->ev_code_act_inactivity)
>                 __set_bit(pdata->ev_code_act_inactivity, input_dev->keybit);
> 
>         ac->int_mask |= ACTIVITY | INACTIVITY;
> 
>         if (pdata->watermark) {
>                 ac->int_mask |= WATERMARK;
> -               if (FIFO_MODE(pdata->fifo_mode) == 0)
> +               if (FIFO_MODE(pdata->fifo_mode) == FIFO_BYPASS)

This is better, not because of GCC, but it makes sense logically; 0 is
not a special value here.

Still, I am not sure that GCC is being that helpful here. Checking
result of shift for 0/non 0 with "!" is very common pattern.

Thanks.

-- 
Dmitry

[PATCH 09/14] SFI: fix tautological-compare warning

[Arnd Bergmann]

With ccache in combination with gcc-6, we get a harmless warning for the sfi subsystem,
as ccache only sees the preprocessed source:

drivers/sfi/sfi_core.c: In function ‘sfi_map_table’:
drivers/sfi/sfi_core.c:175:53: error: self-comparison always evaluates to true [-Werror=tautological-compare]

Using an inline function to do the comparison tells the compiler what is
going on even for preprocessed files, and avoids the warning.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/sfi/sfi_core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/sfi/sfi_core.c b/drivers/sfi/sfi_core.c
index 296db7a69c27..a8f2313a2613 100644
--- a/drivers/sfi/sfi_core.c
+++ b/drivers/sfi/sfi_core.c
@@ -71,9 +71,12 @@
 
 #include "sfi_core.h"
 
-#define ON_SAME_PAGE(addr1, addr2) \
-	(((unsigned long)(addr1) & PAGE_MASK) == \
-	((unsigned long)(addr2) & PAGE_MASK))
+static inline bool on_same_page(unsigned long addr1, unsigned long addr2)
+{
+	return (addr1 & PAGE_MASK) == (addr2 & PAGE_MASK);
+}
+
+#define ON_SAME_PAGE(addr1, addr2) on_same_page((unsigned long)addr1, (unsigned long)addr2)
 #define TABLE_ON_PAGE(page, table, size) (ON_SAME_PAGE(page, table) && \
 				ON_SAME_PAGE(page, table + size))
 
-- 
2.9.0

[PATCH 10/14] staging:iio:resolver:ad2s1210 fix negative IIO_ANGL_VEL read

[Arnd Bergmann]

gcc-7 points out an older regression:

drivers/staging/iio/resolver/ad2s1210.c: In function 'ad2s1210_read_raw':
drivers/staging/iio/resolver/ad2s1210.c:515:42: error: '<<' in boolean context, did you mean '<' ? [-Werror=int-in-bool-context]

The original code had 'unsigned short' here, but incorrectly got
converted to 'bool'. This reverts the regression and uses a normal
type instead.

Fixes: 29148543c521 ("staging:iio:resolver:ad2s1210 minimal chan spec conversion.")
Cc: stable@vger.kernel.org
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/staging/iio/resolver/ad2s1210.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index a6a8393d6664..3e00df74b18c 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -472,7 +472,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
 			     long m)
 {
 	struct ad2s1210_state *st = iio_priv(indio_dev);
-	bool negative;
+	u16 negative;
 	int ret = 0;
 	u16 pos;
 	s16 vel;
-- 
2.9.0

Re: [PATCH 10/14] staging:iio:resolver:ad2s1210 fix negative IIO_ANGL_VEL read

[Jonathan Cameron]

On Fri, 14 Jul 2017 11:31:03 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> gcc-7 points out an older regression:
> 
> drivers/staging/iio/resolver/ad2s1210.c: In function 'ad2s1210_read_raw':
> drivers/staging/iio/resolver/ad2s1210.c:515:42: error: '<<' in boolean context, did you mean '<' ? [-Werror=int-in-bool-context]
> 
> The original code had 'unsigned short' here, but incorrectly got
> converted to 'bool'. This reverts the regression and uses a normal
> type instead.
> 
> Fixes: 29148543c521 ("staging:iio:resolver:ad2s1210 minimal chan spec conversion.")
> Cc: stable@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Thanks Arnd,

Applied to the fixes-togreg branch of iio.git.

Jonathan
> ---
>  drivers/staging/iio/resolver/ad2s1210.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index a6a8393d6664..3e00df74b18c 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -472,7 +472,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
>  			     long m)
>  {
>  	struct ad2s1210_state *st = iio_priv(indio_dev);
> -	bool negative;
> +	u16 negative;
>  	int ret = 0;
>  	u16 pos;
>  	s16 vel;

[PATCH 11/14] IB/uverbs: fix gcc-7 type warning

[Arnd Bergmann]

When using ccache, we get a harmless warning about the fact that
we use the result of a multiplication as a condition:

drivers/infiniband/core/uverbs_main.c: In function 'ib_uverbs_write':
drivers/infiniband/core/uverbs_main.c:787:40: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
drivers/infiniband/core/uverbs_main.c:787:117: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
drivers/infiniband/core/uverbs_main.c:790:50: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
drivers/infiniband/core/uverbs_main.c:790:151: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]

This changes the macro to explicitly check the number for a positive
length, which avoids the warning.

Fixes: a96e4e2ffe43 ("IB/uverbs: New macro to set pointers to NULL if length is 0 in INIT_UDATA()")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/infiniband/core/uverbs.h | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 64d494a64daf..364d7de05721 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -55,12 +55,14 @@
 		(udata)->outlen = (olen);				\
 	} while (0)
 
-#define INIT_UDATA_BUF_OR_NULL(udata, ibuf, obuf, ilen, olen)			\
-	do {									\
-		(udata)->inbuf  = (ilen) ? (const void __user *) (ibuf) : NULL;	\
-		(udata)->outbuf = (olen) ? (void __user *) (obuf) : NULL;	\
-		(udata)->inlen  = (ilen);					\
-		(udata)->outlen = (olen);					\
+#define INIT_UDATA_BUF_OR_NULL(udata, ibuf, obuf, ilen, olen)		\
+	do {								\
+		(udata)->inbuf  = (ilen) > 0 ?				\
+				  (const void __user *) (ibuf) : NULL;	\
+		(udata)->outbuf = (olen) > 0 ?				\
+				  (void __user *) (obuf) : NULL;	\
+		(udata)->inlen  = (ilen);				\
+		(udata)->outlen = (olen);				\
 	} while (0)
 
 /*
-- 
2.9.0

Re: [PATCH 11/14] IB/uverbs: fix gcc-7 type warning

[Leon Romanovsky]

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

On Fri, Jul 14, 2017 at 11:31:04AM +0200, Arnd Bergmann wrote:
> When using ccache, we get a harmless warning about the fact that
> we use the result of a multiplication as a condition:
>
> drivers/infiniband/core/uverbs_main.c: In function 'ib_uverbs_write':
> drivers/infiniband/core/uverbs_main.c:787:40: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
> drivers/infiniband/core/uverbs_main.c:787:117: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
> drivers/infiniband/core/uverbs_main.c:790:50: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
> drivers/infiniband/core/uverbs_main.c:790:151: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
>
> This changes the macro to explicitly check the number for a positive
> length, which avoids the warning.
>
> Fixes: a96e4e2ffe43 ("IB/uverbs: New macro to set pointers to NULL if length is 0 in INIT_UDATA()")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/infiniband/core/uverbs.h | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>

Thanks,
Reviewed-by: Leon Romanovsky <leonro@mellanox.com>

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

[PATCH 12/14] drm/nouveau/clk: fix gcc-7 -Wint-in-bool-context warning

[Arnd Bergmann]

gcc thinks that interpreting a multiplication result as a bool
is confusing:

drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c: In function 'read_pll':
drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c:133:8: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]

In this instance, I think using multiplication is more intuitive
than '&&', so I'm adding a comparison to zero instead to shut up
the warning. To further improve readability, I also make the
error case indented and leave the normal case as the final 'return'
statement.

Fixes: 7632b30e4b8b ("drm/nouveau/clk: namespace + nvidia gpu names (no binary change)")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c
index 96e0941c8edd..04b4f4ccf186 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c
@@ -130,10 +130,10 @@ read_pll(struct gt215_clk *clk, int idx, u32 pll)
 		sclk = read_clk(clk, 0x10 + idx, false);
 	}
 
-	if (M * P)
-		return sclk * N / (M * P);
+	if (M * P == 0)
+		return 0;
 
-	return 0;
+	return sclk * N / (M * P);
 }
 
 static int
-- 
2.9.0

[PATCH 13/14] iopoll: avoid -Wint-in-bool-context warning

[Arnd Bergmann]

When we pass the result of a multiplication as the timeout, we
can get a warning:

drivers/mmc/host/bcm2835.c:596:149: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
drivers/mfd/arizona-core.c:247:195: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]

This is easy to avoid by comparing the timeout to zero instead,
making it a boolean expression.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/iopoll.h | 6 ++++--
 include/linux/regmap.h | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
index d29e1e21bf3f..7a17ba02253b 100644
--- a/include/linux/iopoll.h
+++ b/include/linux/iopoll.h
@@ -48,7 +48,8 @@
 		(val) = op(addr); \
 		if (cond) \
 			break; \
-		if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
+		if ((timeout_us) > 0 && \
+		    ktime_compare(ktime_get(), timeout) > 0) { \
 			(val) = op(addr); \
 			break; \
 		} \
@@ -82,7 +83,8 @@
 		(val) = op(addr); \
 		if (cond) \
 			break; \
-		if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
+		if ((timeout_us) > 0 && \
+		    ktime_compare(ktime_get(), timeout) > 0) { \
 			(val) = op(addr); \
 			break; \
 		} \
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 1474ab0a3922..0889dbf37161 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -129,7 +129,7 @@ struct reg_sequence {
 			break; \
 		if (cond) \
 			break; \
-		if ((timeout_us) && \
+		if ((timeout_us) > 0 && \
 		    ktime_compare(ktime_get(), __timeout) > 0) { \
 			__ret = regmap_read((map), (addr), &(val)); \
 			break; \
-- 
2.9.0

Re: [PATCH 13/14] iopoll: avoid -Wint-in-bool-context warning

[Joe Perches]

On Fri, 2017-07-14 at 11:31 +0200, Arnd Bergmann wrote:
> When we pass the result of a multiplication as the timeout, we
> can get a warning:
> 
> drivers/mmc/host/bcm2835.c:596:149: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
> drivers/mfd/arizona-core.c:247:195: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
> 
> This is easy to avoid by comparing the timeout to zero instead,
> making it a boolean expression.

Perhaps this is better as != 0 if the multiply is signed.

> diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
[]
> @@ -48,7 +48,8 @@
>  		(val) = op(addr); \
>  		if (cond) \
>  			break; \
> -		if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
> +		if ((timeout_us) > 0 && \
> +		    ktime_compare(ktime_get(), timeout) > 0) { \
>  			(val) = op(addr); \
>  			break; \
>  		} \

etc...

Re: [PATCH 13/14] iopoll: avoid -Wint-in-bool-context warning

[Arnd Bergmann]

On Fri, Jul 14, 2017 at 11:55 AM, Joe Perches <joe@perches.com> wrote:
> On Fri, 2017-07-14 at 11:31 +0200, Arnd Bergmann wrote:
>> When we pass the result of a multiplication as the timeout, we
>> can get a warning:
>>
>> drivers/mmc/host/bcm2835.c:596:149: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
>> drivers/mfd/arizona-core.c:247:195: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
>>
>> This is easy to avoid by comparing the timeout to zero instead,
>> making it a boolean expression.
>
> Perhaps this is better as != 0 if the multiply is signed.

I thought about that, but decided that as a negative timeout_us already
gives us rather random behavior (ktime_add_us() takes an unsigned
argument), the '>' comparison gives a more well-defined result by
ignoring the timeout.

         Arnd

[PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool

[Arnd Bergmann]

v4l2_subdev_call is a macro returning whatever the callback return
type is, usually 'int'. With gcc-7 and ccache, this can lead to
many wanings like:

media/platform/pxa_camera.c: In function 'pxa_mbus_build_fmts_xlate':
media/platform/pxa_camera.c:766:27: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]
  while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
media/atomisp/pci/atomisp2/atomisp_cmd.c: In function 'atomisp_s_ae_window':
media/atomisp/pci/atomisp2/atomisp_cmd.c:6414:52: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]
  if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,

The best workaround I could come up with is to change all the
callers that use the return code from v4l2_subdev_call() in an
'if' or 'while' condition.

In case of simple 'if' checks, adding a temporary variable is
usually ok, and sometimes this can be used to propagate or
print an error code, so I do that.

For the 'while' loops, I ended up adding an otherwise useless
comparison with zero, which unfortunately makes the code a little
uglied.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/pci/cx18/cx18-ioctl.c                      |  6 ++++--
 drivers/media/pci/saa7146/mxb.c                          |  5 +++--
 drivers/media/platform/atmel/atmel-isc.c                 |  4 ++--
 drivers/media/platform/atmel/atmel-isi.c                 |  4 ++--
 drivers/media/platform/blackfin/bfin_capture.c           |  4 ++--
 drivers/media/platform/omap3isp/ispccdc.c                |  5 +++--
 drivers/media/platform/pxa_camera.c                      |  3 ++-
 drivers/media/platform/rcar-vin/rcar-core.c              |  2 +-
 drivers/media/platform/rcar-vin/rcar-dma.c               |  4 +++-
 drivers/media/platform/soc_camera/soc_camera.c           |  4 ++--
 drivers/media/platform/stm32/stm32-dcmi.c                |  4 ++--
 drivers/media/platform/ti-vpe/cal.c                      |  6 ++++--
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 13 +++++++------
 13 files changed, 37 insertions(+), 27 deletions(-)

diff --git a/drivers/media/pci/cx18/cx18-ioctl.c b/drivers/media/pci/cx18/cx18-ioctl.c
index 80b902b12a78..1803f28fc501 100644
--- a/drivers/media/pci/cx18/cx18-ioctl.c
+++ b/drivers/media/pci/cx18/cx18-ioctl.c
@@ -188,6 +188,7 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
 {
 	struct cx18 *cx = fh2id(fh)->cx;
 	struct v4l2_sliced_vbi_format *vbifmt = &fmt->fmt.sliced;
+	int ret;
 
 	/* sane, V4L2 spec compliant, defaults */
 	vbifmt->reserved[0] = 0;
@@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
 	 * digitizer/slicer.  Note, cx18_av_vbi() wipes the passed in
 	 * fmt->fmt.sliced under valid calling conditions
 	 */
-	if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
-		return -EINVAL;
+	ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
+	if (ret)
+		return ret;
 
 	vbifmt->service_set = cx18_get_service_set(vbifmt);
 	return 0;
diff --git a/drivers/media/pci/saa7146/mxb.c b/drivers/media/pci/saa7146/mxb.c
index 504d78807639..d2d843c38579 100644
--- a/drivers/media/pci/saa7146/mxb.c
+++ b/drivers/media/pci/saa7146/mxb.c
@@ -525,8 +525,9 @@ static int vidioc_s_input(struct file *file, void *fh, unsigned int input)
 		return err;
 
 	/* switch video in saa7111a */
-	if (saa7111a_call(mxb, video, s_routing, i, SAA7111_FMT_CCIR, 0))
-		pr_err("VIDIOC_S_INPUT: could not address saa7111a\n");
+	err = saa7111a_call(mxb, video, s_routing, i, SAA7111_FMT_CCIR, 0);
+	if (err)
+		pr_err("VIDIOC_S_INPUT: could not address saa7111a: %d\n", err);
 
 	mxb->cur_audinput = video_audio_connect[input];
 	/* switch the audio-source only if necessary */
diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index d6534252cdcd..704b34a0cc00 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1475,8 +1475,8 @@ static int isc_formats_init(struct isc_device *isc)
 		fmt++;
 	}
 
-	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
-	       NULL, &mbus_code)) {
+	while (v4l2_subdev_call(subdev, pad, enum_mbus_code,
+	       NULL, &mbus_code) == 0) {
 		mbus_code.index++;
 		fmt = find_format_by_code(mbus_code.code, &i);
 		if (!fmt)
diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
index 891fa2505efa..30b7e6f298ed 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -1013,8 +1013,8 @@ static int isi_formats_init(struct atmel_isi *isi)
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 	};
 
-	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
-				 NULL, &mbus_code)) {
+	while (v4l2_subdev_call(subdev, pad, enum_mbus_code,
+				NULL, &mbus_code) == 0) {
 		for (i = 0; i < ARRAY_SIZE(isi_formats); i++) {
 			if (isi_formats[i].mbus_code != mbus_code.code)
 				continue;
diff --git a/drivers/media/platform/blackfin/bfin_capture.c b/drivers/media/platform/blackfin/bfin_capture.c
index 1c5166df46f5..864c98f21a0e 100644
--- a/drivers/media/platform/blackfin/bfin_capture.c
+++ b/drivers/media/platform/blackfin/bfin_capture.c
@@ -157,8 +157,8 @@ static int bcap_init_sensor_formats(struct bcap_device *bcap_dev)
 	unsigned int num_formats = 0;
 	int i, j;
 
-	while (!v4l2_subdev_call(bcap_dev->sd, pad,
-				enum_mbus_code, NULL, &code)) {
+	while (v4l2_subdev_call(bcap_dev->sd, pad,
+				enum_mbus_code, NULL, &code) == 0) {
 		num_formats++;
 		code.index++;
 	}
diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 7207558d722c..a94157461f58 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1132,6 +1132,7 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	unsigned int sph;
 	u32 syn_mode;
 	u32 ccdc_pattern;
+	int ret;
 
 	ccdc->bt656 = false;
 	ccdc->fields = 0;
@@ -1140,7 +1141,6 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
 	if (ccdc->input == CCDC_INPUT_PARALLEL) {
 		struct v4l2_mbus_config cfg;
-		int ret;
 
 		ret = v4l2_subdev_call(sensor, video, g_mbus_config, &cfg);
 		if (!ret)
@@ -1158,7 +1158,8 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	 */
 	fmt_src.pad = pad->index;
 	fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-	if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) {
+	ret = v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src);
+	if (!ret) {
 		fmt_info = omap3isp_video_format_info(fmt_src.format.code);
 		depth_in = fmt_info->width;
 	}
diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
index 399095170b6e..5236c7b171ea 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -763,7 +763,8 @@ static struct soc_camera_format_xlate *pxa_mbus_build_fmts_xlate(
 	};
 	struct soc_camera_format_xlate *user_formats;
 
-	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
+	while (v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code) ==
+               0) {
 		raw_fmts++;
 		code.index++;
 	}
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 77dff047c41c..a41f4a3d9b69 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -54,7 +54,7 @@ static bool rvin_mbus_supported(struct rvin_graph_entity *entity)
 
 	code.index = 0;
 	code.pad = entity->source_pad;
-	while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code)) {
+	while (v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code) == 0) {
 		code.index++;
 		switch (code.code) {
 		case MEDIA_BUS_FMT_YUYV8_1X16:
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index b136844499f6..ee16e9886041 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -143,6 +143,7 @@ static int rvin_setup(struct rvin_dev *vin)
 	u32 vnmc, dmr, dmr2, interrupts;
 	v4l2_std_id std;
 	bool progressive = false, output_is_yuv = false, input_is_yuv = false;
+	int ret;
 
 	switch (vin->format.field) {
 	case V4L2_FIELD_TOP:
@@ -155,7 +156,8 @@ static int rvin_setup(struct rvin_dev *vin)
 		/* Default to TB */
 		vnmc = VNMC_IM_FULL;
 		/* Use BT if video standard can be read and is 60 Hz format */
-		if (!v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) {
+		ret = v4l2_subdev_call(vin_to_source(vin), video, g_std, &std);
+		if (ret) {
 			if (std & V4L2_STD_525_60)
 				vnmc = VNMC_IM_FULL | VNMC_FOC;
 		}
diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 45a0429d75bb..3ef648fc2db6 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -454,7 +454,7 @@ static int soc_camera_init_user_formats(struct soc_camera_device *icd)
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 	};
 
-	while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code)) {
+	while (v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code) == 0) {
 		raw_fmts++;
 		code.index++;
 	}
@@ -1202,7 +1202,7 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
 		goto evidstart;
 
 	/* Try to improve our guess of a reasonable window format */
-	if (!v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt)) {
+	if (v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt) == 0) {
 		icd->user_width		= mf->width;
 		icd->user_height	= mf->height;
 		icd->colorspace		= mf->colorspace;
diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
index 83d32a5d0f40..96084dfd5d11 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -1034,8 +1034,8 @@ static int dcmi_formats_init(struct stm32_dcmi *dcmi)
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 	};
 
-	while (!v4l2_subdev_call(subdev, pad, enum_mbus_code,
-				 NULL, &mbus_code)) {
+	while (v4l2_subdev_call(subdev, pad, enum_mbus_code,
+				 NULL, &mbus_code) == 0) {
 		for (i = 0; i < ARRAY_SIZE(dcmi_formats); i++) {
 			if (dcmi_formats[i].mbus_code != mbus_code.code)
 				continue;
diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 177faa36bc16..df0216a6367c 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -1348,9 +1348,11 @@ static void cal_stop_streaming(struct vb2_queue *vq)
 	struct cal_dmaqueue *dma_q = &ctx->vidq;
 	struct cal_buffer *buf, *tmp;
 	unsigned long flags;
+	int ret;
 
-	if (v4l2_subdev_call(ctx->sensor, video, s_stream, 0))
-		ctx_err(ctx, "stream off failed in subdev\n");
+	ret = v4l2_subdev_call(ctx->sensor, video, s_stream, 0);
+	if (ret)
+		ctx_err(ctx, "stream off failed in subdev: %d\n", ret);
 
 	csi2_ppi_disable(ctx);
 	disable_irqs(ctx);
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
index 97093baf28ac..fe56a037f065 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
@@ -6405,19 +6405,20 @@ int atomisp_s_ae_window(struct atomisp_sub_device *asd,
 	struct atomisp_device *isp = asd->isp;
 	/* Coverity CID 298071 - initialzize struct */
 	struct v4l2_subdev_selection sel = { 0 };
+	int ret;
 
 	sel.r.left = arg->x_left;
 	sel.r.top = arg->y_top;
 	sel.r.width = arg->x_right - arg->x_left + 1;
 	sel.r.height = arg->y_bottom - arg->y_top + 1;
 
-	if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
-			     pad, set_selection, NULL, &sel)) {
-		dev_err(isp->dev, "failed to call sensor set_selection.\n");
-		return -EINVAL;
-	}
+	ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
+			       pad, set_selection, NULL, &sel);
+	if (ret)
+		dev_err(isp->dev, "failed to call sensor set_selection: %d\n",
+			ret);
 
-	return 0;
+	return ret;
 }
 
 int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
-- 
2.9.0

Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool

[Dan Carpenter]

Changing:

- if (!frob()) {
+ if (frob() == 0) {

is a totally pointless change.  They're both bad, because they're doing
success testing instead of failure testing, but probably the second one
is slightly worse.

This warning seems dumb.  I can't imagine it has even a 10% success rate
at finding real bugs.  Just disable it.

Changing the code to propagate error codes, is the right thing of course
so long as it doesn't introduce bugs.

regards,
dan carpenter

Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool

[Arnd Bergmann]

On Fri, Jul 14, 2017 at 2:05 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Changing:
>
> - if (!frob()) {
> + if (frob() == 0) {
>
> is a totally pointless change.  They're both bad, because they're doing
> success testing instead of failure testing, but probably the second one
> is slightly worse.
>
> This warning seems dumb.  I can't imagine it has even a 10% success rate
> at finding real bugs.  Just disable it.
>
> Changing the code to propagate error codes, is the right thing of course
> so long as it doesn't introduce bugs.

It found a two of bugs that I fixed earlier:

f0e8faa7a5e8 ("ARM: ux500: fix prcmu_is_cpu_in_wfi() calculation")
af15769ffab1 ("scsi: mvsas: fix command_active typo")

plus three patches from this series:

1. staging:iio:resolver:ad2s1210 fix negative IIO_ANGL_VEL read
2. isdn: isdnloop: suppress a gcc-7 warning (my patch is wrong,
   as Joe pointed out there is a real bug)
3. drm/vmwgfx: avoid gcc-7 parentheses (here, Linus had a better
   analysis of the problem, so we should consider that a bug as well)

I would estimate around 25% success rate here, which isn't that
bad for a new warning.

I agree that most of the false positives are really dumb though.

       Arnd

Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool

[Dan Carpenter]

Ah...  I see why it's complaining about these ones in particular...

I don't agree with it as a static analysis dev, and I don't like the
changes too much.  But since it's only generating a hand full of
warnings then I don't care.

regards,
dan carpenter

Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool

[Dan Carpenter]

On Fri, Jul 14, 2017 at 03:55:26PM +0300, Dan Carpenter wrote:
> I don't agree with it as a static analysis dev...

What I mean is if it's a macro that returns -ENODEV or a function that
returns -ENODEV, they should both be treated the same.  The other
warnings this check prints are quite clever.

regards,
dan carpenter

Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool

[Arnd Bergmann]

On Fri, Jul 14, 2017 at 3:09 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Fri, Jul 14, 2017 at 03:55:26PM +0300, Dan Carpenter wrote:
>> I don't agree with it as a static analysis dev...
>
> What I mean is if it's a macro that returns -ENODEV or a function that
> returns -ENODEV, they should both be treated the same.  The other
> warnings this check prints are quite clever.

I think this is what gcc tries to do, and it should work normally, but it
fails when using ccache. I know I had cases like that, not entirely sure
if this is one of them. Maybe it just means I should give up on using
ccache in preprocessor mode.

       Arnd

Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool

[Dan Carpenter]

On Fri, Jul 14, 2017 at 11:36:56AM +0200, Arnd Bergmann wrote:
> @@ -1158,7 +1158,8 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
>  	 */
>  	fmt_src.pad = pad->index;
>  	fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> -	if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) {
> +	ret = v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src);
> +	if (!ret) {
>  		fmt_info = omap3isp_video_format_info(fmt_src.format.code);
>  		depth_in = fmt_info->width;
>  	}

Is the original code buggy?

media/platform/omap3isp/ispccdc.c
  1156          /* Compute the lane shifter shift value and enable the bridge when the
  1157           * input format is a non-BT.656 YUV variant.
  1158           */
  1159          fmt_src.pad = pad->index;
  1160          fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
  1161          if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) {
  1162                  fmt_info = omap3isp_video_format_info(fmt_src.format.code);
  1163                  depth_in = fmt_info->width;
  1164          }

If v4l2_subdev_call() then depth_in is zero.

  1165  
  1166          fmt_info = omap3isp_video_format_info(format->code);
  1167          depth_out = fmt_info->width;
  1168          shift = depth_in - depth_out;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

How do we know that this subtraction doesn't set "shift" to a very high
positive?

  1169  
  1170          if (ccdc->bt656)
  1171                  bridge = ISPCTRL_PAR_BRIDGE_DISABLE;
  1172          else if (fmt_info->code == MEDIA_BUS_FMT_YUYV8_2X8)
  1173                  bridge = ISPCTRL_PAR_BRIDGE_LENDIAN;
  1174          else if (fmt_info->code == MEDIA_BUS_FMT_UYVY8_2X8)
  1175                  bridge = ISPCTRL_PAR_BRIDGE_BENDIAN;
  1176          else
  1177                  bridge = ISPCTRL_PAR_BRIDGE_DISABLE;
  1178  
  1179          omap3isp_configure_bridge(isp, ccdc->input, parcfg, shift, bridge);

regards,
dan carpenter

Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool

[Hans Verkuil]

On 14/07/17 11:36, Arnd Bergmann wrote:
> v4l2_subdev_call is a macro returning whatever the callback return
> type is, usually 'int'. With gcc-7 and ccache, this can lead to
> many wanings like:
> 
> media/platform/pxa_camera.c: In function 'pxa_mbus_build_fmts_xlate':
> media/platform/pxa_camera.c:766:27: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]
>   while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
> media/atomisp/pci/atomisp2/atomisp_cmd.c: In function 'atomisp_s_ae_window':
> media/atomisp/pci/atomisp2/atomisp_cmd.c:6414:52: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context]
>   if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> 
> The best workaround I could come up with is to change all the
> callers that use the return code from v4l2_subdev_call() in an
> 'if' or 'while' condition.
> 
> In case of simple 'if' checks, adding a temporary variable is
> usually ok, and sometimes this can be used to propagate or
> print an error code, so I do that.
> 
> For the 'while' loops, I ended up adding an otherwise useless
> comparison with zero, which unfortunately makes the code a little
> uglied.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/media/pci/cx18/cx18-ioctl.c                      |  6 ++++--
>  drivers/media/pci/saa7146/mxb.c                          |  5 +++--
>  drivers/media/platform/atmel/atmel-isc.c                 |  4 ++--
>  drivers/media/platform/atmel/atmel-isi.c                 |  4 ++--
>  drivers/media/platform/blackfin/bfin_capture.c           |  4 ++--
>  drivers/media/platform/omap3isp/ispccdc.c                |  5 +++--
>  drivers/media/platform/pxa_camera.c                      |  3 ++-
>  drivers/media/platform/rcar-vin/rcar-core.c              |  2 +-
>  drivers/media/platform/rcar-vin/rcar-dma.c               |  4 +++-
>  drivers/media/platform/soc_camera/soc_camera.c           |  4 ++--
>  drivers/media/platform/stm32/stm32-dcmi.c                |  4 ++--
>  drivers/media/platform/ti-vpe/cal.c                      |  6 ++++--
>  drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 13 +++++++------
>  13 files changed, 37 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/pci/cx18/cx18-ioctl.c b/drivers/media/pci/cx18/cx18-ioctl.c
> index 80b902b12a78..1803f28fc501 100644
> --- a/drivers/media/pci/cx18/cx18-ioctl.c
> +++ b/drivers/media/pci/cx18/cx18-ioctl.c
> @@ -188,6 +188,7 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>  {
>  	struct cx18 *cx = fh2id(fh)->cx;
>  	struct v4l2_sliced_vbi_format *vbifmt = &fmt->fmt.sliced;
> +	int ret;
>  
>  	/* sane, V4L2 spec compliant, defaults */
>  	vbifmt->reserved[0] = 0;
> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>  	 * digitizer/slicer.  Note, cx18_av_vbi() wipes the passed in
>  	 * fmt->fmt.sliced under valid calling conditions
>  	 */
> -	if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
> -		return -EINVAL;
> +	ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
> +	if (ret)
> +		return ret;

Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
break something.

>  
>  	vbifmt->service_set = cx18_get_service_set(vbifmt);
>  	return 0;

<snip>

> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> index 97093baf28ac..fe56a037f065 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> @@ -6405,19 +6405,20 @@ int atomisp_s_ae_window(struct atomisp_sub_device *asd,
>  	struct atomisp_device *isp = asd->isp;
>  	/* Coverity CID 298071 - initialzize struct */
>  	struct v4l2_subdev_selection sel = { 0 };
> +	int ret;
>  
>  	sel.r.left = arg->x_left;
>  	sel.r.top = arg->y_top;
>  	sel.r.width = arg->x_right - arg->x_left + 1;
>  	sel.r.height = arg->y_bottom - arg->y_top + 1;
>  
> -	if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> -			     pad, set_selection, NULL, &sel)) {
> -		dev_err(isp->dev, "failed to call sensor set_selection.\n");
> -		return -EINVAL;
> -	}
> +	ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> +			       pad, set_selection, NULL, &sel);
> +	if (ret)
> +		dev_err(isp->dev, "failed to call sensor set_selection: %d\n",
> +			ret);

Same here: just keep the 'return -EINVAL'.

>  
> -	return 0;
> +	return ret;
>  }
>  
>  int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
> 

This is all very hackish, though. I'm not terribly keen on this patch. It's not
clear to me *why* these warnings appear in your setup.

Regards,

	Hans

Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool

[Arnd Bergmann]

On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 14/07/17 11:36, Arnd Bergmann wrote:
>> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>>        * digitizer/slicer.  Note, cx18_av_vbi() wipes the passed in
>>        * fmt->fmt.sliced under valid calling conditions
>>        */
>> -     if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
>> -             return -EINVAL;
>> +     ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
>> +     if (ret)
>> +             return ret;
>
> Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
> break something.

I think Dan was recommending the opposite here, if I understood you
both correctly:
he said we should propagate the error code unless we know it's wrong, while you
want to keep the current behavior to avoid introducing changes ;-)

I guess in either case, looking at the callers more carefully would be
a good idea.

>> -     return 0;
>> +     return ret;
>>  }
>>
>>  int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
>>
>
> This is all very hackish, though. I'm not terribly keen on this patch. It's not
> clear to me *why* these warnings appear in your setup.

it's possible that this only happened with 'ccache', which first preprocesses
the source and the passes it with v4l2_subdev_call expanded into the
compiler. This means the line looks like

        if ((!(cx->sd_av) ? -ENODEV :
            (((cx->sd_av)->ops->vbi && (cx->sd_av)->ops->vbi->g_sliced_fmt) ?
               (cx->sd_av)->ops->vbi->g_sliced_fmt(cx->sd_av)),
&fmt->fmt.sliced) :
               -ENOIOCTLCMD))

The compiler now complains about the sub-expression that it sees for
cx->sd_av==NULL:

   if (-ENODEV)

which it considers nonsense because it is always true and the value gets
ignored.

Let me try again without ccache for now and see what warnings remain.
We can find a solution for those first, and then decide how to deal with
ccache.

        Arnd

Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool

[Dan Carpenter]

On Mon, Jul 17, 2017 at 04:26:23PM +0200, Arnd Bergmann wrote:
> On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On 14/07/17 11:36, Arnd Bergmann wrote:
> >> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
> >>        * digitizer/slicer.  Note, cx18_av_vbi() wipes the passed in
> >>        * fmt->fmt.sliced under valid calling conditions
> >>        */
> >> -     if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
> >> -             return -EINVAL;
> >> +     ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
> >> +     if (ret)
> >> +             return ret;
> >
> > Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
> > break something.
> 
> I think Dan was recommending the opposite here, if I understood you
> both correctly:
> he said we should propagate the error code unless we know it's wrong, while you
> want to keep the current behavior to avoid introducing changes ;-)
> 

I don't know the subsystem rules at all, so don't listen to me.

regards,
dan carpenter

Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool

[Hans Verkuil]

On 17/07/17 16:26, Arnd Bergmann wrote:
> On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 14/07/17 11:36, Arnd Bergmann wrote:
>>> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>>>        * digitizer/slicer.  Note, cx18_av_vbi() wipes the passed in
>>>        * fmt->fmt.sliced under valid calling conditions
>>>        */
>>> -     if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
>>> -             return -EINVAL;
>>> +     ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
>>> +     if (ret)
>>> +             return ret;
>>
>> Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
>> break something.
> 
> I think Dan was recommending the opposite here, if I understood you
> both correctly:
> he said we should propagate the error code unless we know it's wrong, while you
> want to keep the current behavior to avoid introducing changes ;-)
> 
> I guess in either case, looking at the callers more carefully would be
> a good idea.

The subtle problem here is that v4l2_subdev_call will return -ENOIOCTLCMD if
ops->vbi->g_sliced_fmt == NULL, which typically is not returned to userspace
but either ignored or replaced by another error. It indicates that the
sub device doesn't implement this operation, and it depends on the context
and the operation whether or not that is to be considered an error.

I have no clue what is expected here, without digging deep in the code.

Better to keep it as-is. It really isn't important to waste time on this.

> 
>>> -     return 0;
>>> +     return ret;
>>>  }
>>>
>>>  int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
>>>
>>
>> This is all very hackish, though. I'm not terribly keen on this patch. It's not
>> clear to me *why* these warnings appear in your setup.
> 
> it's possible that this only happened with 'ccache', which first preprocesses
> the source and the passes it with v4l2_subdev_call expanded into the
> compiler. This means the line looks like
> 
>         if ((!(cx->sd_av) ? -ENODEV :
>             (((cx->sd_av)->ops->vbi && (cx->sd_av)->ops->vbi->g_sliced_fmt) ?
>                (cx->sd_av)->ops->vbi->g_sliced_fmt(cx->sd_av)),
> &fmt->fmt.sliced) :
>                -ENOIOCTLCMD))
> 
> The compiler now complains about the sub-expression that it sees for
> cx->sd_av==NULL:
> 
>    if (-ENODEV)
> 
> which it considers nonsense because it is always true and the value gets
> ignored.
> 
> Let me try again without ccache for now and see what warnings remain.
> We can find a solution for those first, and then decide how to deal with
> ccache.

Sounds good.

I'm OK with applying this if there is no other way to prevent these warnings.

Regards,

	Hans

> 
>         Arnd
> 

Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool

[Hans Verkuil]

On 17/07/17 16:26, Arnd Bergmann wrote:
> On Mon, Jul 17, 2017 at 3:45 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 14/07/17 11:36, Arnd Bergmann wrote:
>>> @@ -201,8 +202,9 @@ static int cx18_g_fmt_sliced_vbi_cap(struct file *file, void *fh,
>>>        * digitizer/slicer.  Note, cx18_av_vbi() wipes the passed in
>>>        * fmt->fmt.sliced under valid calling conditions
>>>        */
>>> -     if (v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced))
>>> -             return -EINVAL;
>>> +     ret = v4l2_subdev_call(cx->sd_av, vbi, g_sliced_fmt, &fmt->fmt.sliced);
>>> +     if (ret)
>>> +             return ret;
>>
>> Please keep the -EINVAL here. I can't be 100% certain that returning 'ret' wouldn't
>> break something.
> 
> I think Dan was recommending the opposite here, if I understood you
> both correctly:
> he said we should propagate the error code unless we know it's wrong, while you
> want to keep the current behavior to avoid introducing changes ;-)
> 
> I guess in either case, looking at the callers more carefully would be
> a good idea.

The subtle problem here is that v4l2_subdev_call will return -ENOIOCTLCMD if
ops->vbi->g_sliced_fmt == NULL, which typically is not returned to userspace
but either ignored or replaced by another error. It indicates that the
sub device doesn't implement this operation, and it depends on the context
and the operation whether or not that is to be considered an error.

I have no clue what is expected here, without digging deep in the code.

Better to keep it as-is. It really isn't important to waste time on this.

> 
>>> -     return 0;
>>> +     return ret;
>>>  }
>>>
>>>  int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
>>>
>>
>> This is all very hackish, though. I'm not terribly keen on this patch. It's not
>> clear to me *why* these warnings appear in your setup.
> 
> it's possible that this only happened with 'ccache', which first preprocesses
> the source and the passes it with v4l2_subdev_call expanded into the
> compiler. This means the line looks like
> 
>         if ((!(cx->sd_av) ? -ENODEV :
>             (((cx->sd_av)->ops->vbi && (cx->sd_av)->ops->vbi->g_sliced_fmt) ?
>                (cx->sd_av)->ops->vbi->g_sliced_fmt(cx->sd_av)),
> &fmt->fmt.sliced) :
>                -ENOIOCTLCMD))
> 
> The compiler now complains about the sub-expression that it sees for
> cx->sd_av==NULL:
> 
>    if (-ENODEV)
> 
> which it considers nonsense because it is always true and the value gets
> ignored.
> 
> Let me try again without ccache for now and see what warnings remain.
> We can find a solution for those first, and then decide how to deal with
> ccache.

Sounds good.

I'm OK with applying this if there is no other way to prevent these warnings.

Regards,

	Hans

> 
>         Arnd
> 

Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool

[Arnd Bergmann]

On Mon, Jul 17, 2017 at 4:35 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 17/07/17 16:26, Arnd Bergmann wrote:

>> Let me try again without ccache for now and see what warnings remain.
>> We can find a solution for those first, and then decide how to deal with
>> ccache.
>
> Sounds good.
>
> I'm OK with applying this if there is no other way to prevent these warnings.

Small update: I noticed that having ccache being the default compiler
even with CCACHE_DISABLE=1 causes a lot of these warnings. Completely
taking ccache out of the picture however seems to have eliminated the
warnings about v4l2_subdev_call() and other silly warnings, but not
the interesting ones in the -Wint-in-bool-context category.

       Arnd

Re: [PATCH 00/14] gcc-7 warnings

[Greg Kroah-Hartman]

On Fri, Jul 14, 2017 at 11:25:12AM +0200, Arnd Bergmann wrote:
> This series should shut up all warnings introduced by gcc-6 or gcc-7 on
> today's linux-next, as observed in "allmodconfig" builds on x86,
> arm and arm64.
> 
> I have sent some of these before, but some others are new, as I had
> at some point disabled the -Wint-in-bool-context warning in my
> randconfig testing and did not notice the other warnings.
> 
> I have another series to address all -Wformat-overflow warnings,
> and one more patch to turn off the -Wformat-truncation warnings
> unless we build with "make W=1". I'll send that separately.
> 
> Most of these are consist of trivial refactoring of the code to
> shut up false-positive warnings, the one exception being
> "staging:iio:resolver:ad2s1210 fix negative IIO_ANGL_VEL read",
> which fixes a regression against linux-3.1 that has gone
> unnoticed since then. Still, review from subsystem maintainers
> would be appreciated.
> 
> I would suggest that Andrew Morton can pick these up into linux-mm
> so we can make sure they all make it into the release. Alternatively
> Linus might feel like picking them all up himself.
> 
> While I did not mark the harmless ones for stable backports,
> Greg may also want to pick them up once they go upstream, to
> help build-test the stable kernels with gcc-7.

Thanks for these, I'll keep an eye out for them to get into the stable
trees, so I can eventually update my test-build box to gcc-7.

thanks,

greg k-h