* Re: [PATCH 10/14] staging:iio:resolver:ad2s1210 fix negative IIO_ANGL_VEL read
From: Jonathan Cameron @ 2017-07-15 11:42 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel, Lars-Peter Clausen, Michael Hennerich,
Greg Kroah-Hartman, Linus Torvalds, Tejun Heo, Guenter Roeck,
linux-ide, linux-media, akpm, dri-devel, stable, Hartmut Knaack,
Peter Meerwald-Stadler, linux-iio, devel
In-Reply-To: <20170714093129.1366900-1-arnd@arndb.de>
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;
^ permalink raw reply
* Re: [PATCH] ata: sata_rcar: add gen[23] fallback compatibility strings
From: Tejun Heo @ 2017-07-15 11:09 UTC (permalink / raw)
To: Simon Horman
Cc: linux-ide, linux-renesas-soc, Magnus Damm, Rob Herring,
devicetree, Geert Uytterhoeven
In-Reply-To: <1499773460-8930-1-git-send-email-horms+renesas@verge.net.au>
On Tue, Jul 11, 2017 at 01:44:20PM +0200, Simon Horman wrote:
> Add fallback compatibility string for R-Car Gen 2 and 3.
>
> In the case of Renesas R-Car hardware we know that there are generations of
> SoCs, e.g. Gen 1 and 2. But beyond that its not clear what the relationship
> between IP blocks might be. For example, I believe that r8a7790 is older
> than r8a7791 but that doesn't imply that the latter is a descendant of the
> former or vice versa.
>
> We can, however, by examining the documentation and behaviour of the
> hardware at run-time observe that the current driver implementation appears
> to be compatible with the IP blocks on SoCs within a given generation.
>
> For the above reasons and convenience when enabling new SoCs a
> per-generation fallback compatibility string scheme being adopted for
> drivers for Renesas SoCs.
>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
Applied to libata/for-4.13-fixes.
Thanks.
--
tejun
^ permalink raw reply
* Re: Lots of new warnings with gcc-7.1.1
From: Tejun Heo @ 2017-07-15 11:03 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Linus Torvalds, Guenter Roeck, Jean Delvare,
Bartlomiej Zolnierkiewicz, Sathya Prakash, James E.J. Bottomley,
Greg Kroah-Hartman, the arch/x86 maintainers, xen-devel,
linux-block, Linux Media Mailing List, IDE-ML,
linux-fbdev@vger.kernel.org, Network Development
In-Reply-To: <CAK8P3a2itguODKUNtw8m-7RReUkyEqk8fHYRLa-ZjJYjwwhYdg@mail.gmail.com>
Hello,
On Wed, Jul 12, 2017 at 03:31:02PM +0200, Arnd Bergmann wrote:
> > We also have about a bazillion
> >
> > warning: ‘*’ in boolean context, suggest ‘&&’ instead
> >
> > warnings in drivers/ata/libata-core.c, all due to a single macro that
> > uses a pattern that gcc-7.1.1 doesn't like. The warning looks a bit
> > debatable, but I suspect the macro could easily be changed too.
> >
> > Tejun, would you hate just moving the "multiply by 1000" part _into_
> > that EZ() macro? Something like the attached (UNTESTED!) patch?
>
> Tejun applied an almost identical patch of mine a while ago, but it seems to
> have gotten lost in the meantime in some rebase:
Yeah, I was scratching my head remembering your patch. Sorry about
that. It should have been routed through for-4.12-fixes.
> https://patchwork.kernel.org/patch/9721397/
> https://patchwork.kernel.org/patch/9721399/
>
> I guess I should have resubmitted the second patch with the suggested
> improvement.
The new one looks good to me.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH, RESEND 02/14] ata: avoid gcc-7 warning in ata_timing_quantize
From: Tejun Heo @ 2017-07-15 10:56 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Damien Le Moal, Greg Kroah-Hartman, linux-kernel, dri-devel,
linux-ide, Geert Uytterhoeven, Hannes Reinecke, Adam Manzanares,
akpm, Mauro Carvalho Chehab, Linus Torvalds, Guenter Roeck,
linux-media
In-Reply-To: <20170714092540.1217397-3-arnd@arndb.de>
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Hello.
From: HUAT ONG @ 2017-07-15 6:57 UTC (permalink / raw)
Hello Dear,
I am in need of your services to assist in transferring funds. ongenghuat005@gmail.com
Yours faithfully.
Mr. Ong Eng.
^ permalink raw reply
* BUG Needs Broader Audience (Bisect Successfull, Bugzilla #196343) : CDROM-related bug introduced in 4.11 merge window makes kernel freeze instead of doing read retires.
From: Vecu Bosseur @ 2017-07-15 6:34 UTC (permalink / raw)
To: linux, linux-kernel, linux-scsi, linux-ide
Hi all,
I happened to stumble into some (Linux kernel) bug a few days ago and
hopefully managed to collect evidences of its cause, bissection for
instance.
I was really in need to share my report with the Community, but
pitifully failed to spot the right mailing lists involved. That's why
some of you might consider this message bothersome.
For the rest, the people who really care about this bug, please,
follow this pointer :
https://bugzilla.kernel.org/show_bug.cgi?id=196343
For eager people, offending commit is :
commit 82ed4db499b8598f16f8871261bff088d6b0597f
Author: Christoph Hellwig <hch@lst.de>
Date: Fri Jan 27 09:46:29 2017 +0100
block: split scsi_request out of struct request
And require all drivers that want to support BLOCK_PC to allocate it
as the first thing of their private data. To support this the legacy
IDE and BSG code is switched to set cmd_size on their queues to let
the block layer allocate the additional space.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jens Axboe <axboe@fb.com>
And, not as an option, you the specialists, please tell me which the
right mailing lists are to post such a bug report.
Thanks in advance for any kind of comment.
Yours Sincerely,
Valentin
^ permalink raw reply
* Re: [PATCH 05/14] isdn: isdnloop: suppress a gcc-7 warning
From: Kevin Easton @ 2017-07-15 4:20 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Joe Perches, Linux Kernel Mailing List, Karsten Keil,
David S. Miller, Greg Kroah-Hartman, Linus Torvalds, Tejun Heo,
Guenter Roeck, IDE-ML, Linux Media Mailing List, Andrew Morton,
dri-devel, Networking
In-Reply-To: <CAK8P3a2fYr11L-0sg-veKx5F5dGH5btQAhSZtFbNXxVtcHd8dg@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH 08/14] Input: adxl34x - fix gcc-7 -Wint-in-bool-context warning
From: Dmitry Torokhov @ 2017-07-14 21:40 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Linus Torvalds, Linux Kernel Mailing List, Michael Hennerich,
Greg Kroah-Hartman, Tejun Heo, Guenter Roeck, IDE-ML,
Linux Media Mailing List, Andrew Morton, DRI,
linux-input@vger.kernel.org
In-Reply-To: <CAK8P3a0nZsqWtFX84FTHmm=aVFevrU5ZATnaLVSA39PZeG=6UQ@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH, RESEND 03/14] drm/vmwgfx: avoid gcc-7 parentheses warning
From: Arnd Bergmann @ 2017-07-14 20:28 UTC (permalink / raw)
To: Linus Torvalds
Cc: Linux Kernel Mailing List, VMware Graphics, Sinclair Yeh,
Thomas Hellstrom, David Airlie, Greg Kroah-Hartman, Tejun Heo,
Guenter Roeck, IDE-ML, Linux Media Mailing List, Andrew Morton,
DRI, Brian Paul
In-Reply-To: <CA+55aFw+DTc_czppqfbqY+7kq6Uej=Nf1Wxf1HutRw4tRxC85Q@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH 08/14] Input: adxl34x - fix gcc-7 -Wint-in-bool-context warning
From: Arnd Bergmann @ 2017-07-14 20:17 UTC (permalink / raw)
To: Linus Torvalds
Cc: Linux Kernel Mailing List, Michael Hennerich, Dmitry Torokhov,
Greg Kroah-Hartman, Tejun Heo, Guenter Roeck, IDE-ML,
Linux Media Mailing List, Andrew Morton, DRI,
linux-input@vger.kernel.org
In-Reply-To: <CA+55aFw076kkn_NS1K+nSHDLoajhviHUsnCOmJOpz5YajpEtFw@mail.gmail.com>
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;
^ permalink raw reply
* Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
From: Arnd Bergmann @ 2017-07-14 19:32 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, Linux-Renesas, Linux Media Mailing List,
Greg Kroah-Hartman, Daeseok Youn, Linus Torvalds,
Linux Kernel Mailing List, dri-devel, adi-buildroot-devel,
Hans Verkuil, IDE-ML, Guenter Roeck, Niklas Söderlund,
Tejun Heo, Andrew Morton, Mauro Carvalho Chehab, Robert Jarzmik,
Linux ARM, Alan Cox
In-Reply-To: <20170714130955.zqe26g6zpixr3xj2@mwanda>
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH 08/14] Input: adxl34x - fix gcc-7 -Wint-in-bool-context warning
From: Linus Torvalds @ 2017-07-14 19:24 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Linux Kernel Mailing List, Michael Hennerich, Dmitry Torokhov,
Greg Kroah-Hartman, Tejun Heo, Guenter Roeck, IDE-ML,
Linux Media Mailing List, Andrew Morton, DRI,
linux-input@vger.kernel.org
In-Reply-To: <20170714092540.1217397-9-arnd@arndb.de>
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
^ permalink raw reply
* Re: [PATCH, RESEND 03/14] drm/vmwgfx: avoid gcc-7 parentheses warning
From: Linus Torvalds @ 2017-07-14 19:23 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Linux Kernel Mailing List, VMware Graphics, Sinclair Yeh,
Thomas Hellstrom, David Airlie, Greg Kroah-Hartman, Tejun Heo,
Guenter Roeck, IDE-ML, Linux Media Mailing List, Andrew Morton,
DRI, Brian Paul
In-Reply-To: <CA+55aFw9xE_+qnx1=6FbU8HJ23+Go1iS5bxDLOHefETgn5Wx6w@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH, RESEND 03/14] drm/vmwgfx: avoid gcc-7 parentheses warning
From: Linus Torvalds @ 2017-07-14 19:21 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Thomas Hellstrom, Greg Kroah-Hartman, Linux Kernel Mailing List,
DRI, IDE-ML, VMware Graphics, Tejun Heo, Brian Paul,
Andrew Morton, Guenter Roeck, Linux Media Mailing List
In-Reply-To: <20170714092540.1217397-4-arnd@arndb.de>
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH] ata: sata_rcar: add gen[23] fallback compatibility strings
From: Rob Herring @ 2017-07-14 16:11 UTC (permalink / raw)
To: Simon Horman
Cc: Tejun Heo, linux-ide-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA, Magnus Damm,
devicetree-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven
In-Reply-To: <1499773460-8930-1-git-send-email-horms+renesas-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
On Tue, Jul 11, 2017 at 01:44:20PM +0200, Simon Horman wrote:
> Add fallback compatibility string for R-Car Gen 2 and 3.
>
> In the case of Renesas R-Car hardware we know that there are generations of
> SoCs, e.g. Gen 1 and 2. But beyond that its not clear what the relationship
> between IP blocks might be. For example, I believe that r8a7790 is older
> than r8a7791 but that doesn't imply that the latter is a descendant of the
> former or vice versa.
>
> We can, however, by examining the documentation and behaviour of the
> hardware at run-time observe that the current driver implementation appears
> to be compatible with the IP blocks on SoCs within a given generation.
>
> For the above reasons and convenience when enabling new SoCs a
> per-generation fallback compatibility string scheme being adopted for
> drivers for Renesas SoCs.
>
> Signed-off-by: Simon Horman <horms+renesas-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
> ---
> Based on libata/for-next
> ---
> Documentation/devicetree/bindings/ata/sata_rcar.txt | 14 +++++++++++---
> drivers/ata/sata_rcar.c | 8 ++++++++
> 2 files changed, 19 insertions(+), 3 deletions(-)
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
From: Dan Carpenter @ 2017-07-14 13:09 UTC (permalink / raw)
To: Arnd Bergmann
Cc: devel, Linux-Renesas, Linux Media Mailing List,
Greg Kroah-Hartman, Daeseok Youn, Linus Torvalds,
Linux Kernel Mailing List, dri-devel, adi-buildroot-devel,
Hans Verkuil, IDE-ML, Guenter Roeck, Niklas Söderlund,
Tejun Heo, Andrew Morton, Mauro Carvalho Chehab, Robert Jarzmik,
Linux ARM, Alan Cox
In-Reply-To: <20170714125525.kjemhcn4poon6r3i@mwanda>
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
^ permalink raw reply
* Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
From: Dan Carpenter @ 2017-07-14 12:55 UTC (permalink / raw)
To: Arnd Bergmann
Cc: devel, Hans Verkuil, Alan Cox, Greg Kroah-Hartman, Daeseok Youn,
Linux Kernel Mailing List, dri-devel, adi-buildroot-devel,
Linux-Renesas, IDE-ML, Robert Jarzmik, Linux ARM,
Niklas Söderlund, Tejun Heo, Andrew Morton,
Mauro Carvalho Chehab, Linus Torvalds, Guenter Roeck,
Linux Media Mailing List
In-Reply-To: <CAK8P3a0f84OPcCK1r3P9inGYDJC2KaAO4mjE2vn+vCws-oo_bw@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
From: Dan Carpenter @ 2017-07-14 12:41 UTC (permalink / raw)
To: Arnd Bergmann
Cc: devel, linux-renesas-soc, linux-media, Greg Kroah-Hartman,
Daeseok Youn, Linus Torvalds, linux-kernel, dri-devel,
adi-buildroot-devel, Hans Verkuil, linux-ide, Guenter Roeck,
Niklas Söderlund, Tejun Heo, akpm, Mauro Carvalho Chehab,
Robert Jarzmik, linux-arm-kernel, Alan Cox
In-Reply-To: <20170714093938.1469319-1-arnd@arndb.de>
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH 07/14] proc/kcore: hide a harmless warning
From: Ard Biesheuvel @ 2017-07-14 12:28 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel@vger.kernel.org, Jiri Olsa, Greg Kroah-Hartman,
Linus Torvalds, Tejun Heo, Guenter Roeck, linux-ide, linux-media,
Andrew Morton, dri-devel, Kees Cook, Ingo Molnar, Laura Abbott,
Pratyush Anand
In-Reply-To: <20170714092540.1217397-8-arnd@arndb.de>
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
>
^ permalink raw reply
* Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
From: Arnd Bergmann @ 2017-07-14 12:27 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, Linux-Renesas, Linux Media Mailing List,
Greg Kroah-Hartman, Daeseok Youn, Linus Torvalds,
Linux Kernel Mailing List, dri-devel, adi-buildroot-devel,
Hans Verkuil, IDE-ML, Guenter Roeck, Niklas Söderlund,
Tejun Heo, Andrew Morton, Mauro Carvalho Chehab, Robert Jarzmik,
Linux ARM, Alan Cox
In-Reply-To: <20170714120512.ioe67nnloqivtbr7@mwanda>
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH 14/14] [media] fix warning on v4l2_subdev_call() result interpreted as bool
From: Dan Carpenter @ 2017-07-14 12:05 UTC (permalink / raw)
To: Arnd Bergmann
Cc: devel, linux-renesas-soc, linux-media, Greg Kroah-Hartman,
Daeseok Youn, Linus Torvalds, linux-kernel, dri-devel,
adi-buildroot-devel, Hans Verkuil, linux-ide, Guenter Roeck,
Niklas Söderlund, Tejun Heo, akpm, Mauro Carvalho Chehab,
Robert Jarzmik, linux-arm-kernel, Alan Cox
In-Reply-To: <20170714093938.1469319-1-arnd@arndb.de>
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
^ permalink raw reply
* Re: [PATCH 05/14] isdn: isdnloop: suppress a gcc-7 warning
From: Arnd Bergmann @ 2017-07-14 10:37 UTC (permalink / raw)
To: Joe Perches
Cc: Linux Kernel Mailing List, Karsten Keil, David S. Miller,
Greg Kroah-Hartman, Linus Torvalds, Tejun Heo, Guenter Roeck,
IDE-ML, Linux Media Mailing List, Andrew Morton, dri-devel,
Networking
In-Reply-To: <1500026936.4457.68.camel@perches.com>
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
^ permalink raw reply
* Re: [PATCH 00/14] gcc-7 warnings
From: Greg Kroah-Hartman @ 2017-07-14 10:29 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel, dri-devel, linux-ide, Tejun Heo, akpm,
Linus Torvalds, Guenter Roeck, linux-media
In-Reply-To: <20170714092540.1217397-1-arnd@arndb.de>
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH 13/14] iopoll: avoid -Wint-in-bool-context warning
From: Arnd Bergmann @ 2017-07-14 10:22 UTC (permalink / raw)
To: Joe Perches
Cc: Linux Kernel Mailing List, Mark Brown, Andrew Morton,
Greg Kroah-Hartman, Linus Torvalds, Tejun Heo, Guenter Roeck,
IDE-ML, Linux Media Mailing List, dri-devel, Masahiro Yamada,
Charles Keepax
In-Reply-To: <1500026123.4457.66.camel@perches.com>
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
^ permalink raw reply
* Re: [PATCH 05/14] isdn: isdnloop: suppress a gcc-7 warning
From: Joe Perches @ 2017-07-14 10:08 UTC (permalink / raw)
To: Arnd Bergmann, linux-kernel, Karsten Keil, David S. Miller
Cc: Greg Kroah-Hartman, Linus Torvalds, Tejun Heo, Guenter Roeck,
linux-ide, linux-media, akpm, dri-devel, netdev
In-Reply-To: <20170714092540.1217397-6-arnd@arndb.de>
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)))
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox