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

Groups > linux.kernel > #1359360 > unrolled thread

[PATCH v2] staging/comedi/dt282x: avoid integer overflow warning

Started byArnd Bergmann <arnd@arndb.de>
First post2016-03-16 22:00 +0100
Last post2016-03-17 18:00 +0100
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning Arnd Bergmann <arnd@arndb.de> - 2016-03-16 22:00 +0100
    Re: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning Ian Abbott <abbotti@mev.co.uk> - 2016-03-17 11:30 +0100
    RE: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning Hartley Sweeten <HartleyS@visionengravers.com> - 2016-03-17 16:50 +0100
      Re: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning Arnd Bergmann <arnd@arndb.de> - 2016-03-17 17:10 +0100
        RE: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning Hartley Sweeten <HartleyS@visionengravers.com> - 2016-03-17 18:00 +0100

#1359360 — [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning

FromArnd Bergmann <arnd@arndb.de>
Date2016-03-16 22:00 +0100
Subject[PATCH v2] staging/comedi/dt282x: avoid integer overflow warning
Message-ID<rdqHT-5Ja-11@gated-at.bofh.it>
gcc-6 warns about passing negative signed integer into swab16()
in the dt282x driver:

drivers/staging/comedi/drivers/dt282x.c: In function 'dt282x_load_changain':
include/uapi/linux/swab.h:14:33: warning: integer overflow in expression [-Woverflow]
  (((__u16)(x) & (__u16)0xff00U) >> 8)))
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
include/uapi/linux/swab.h:107:2: note: in expansion of macro '___constant_swab16'
  ___constant_swab16(x) :   \
  ^~~~~~~~~~~~~~~~~~
include/uapi/linux/byteorder/big_endian.h:34:43: note: in expansion of macro '__swab16'
 #define __cpu_to_le16(x) ((__force __le16)__swab16((x)))
                                           ^~~~~~~~
include/linux/byteorder/generic.h:89:21: note: in expansion of macro '__cpu_to_le16'
 #define cpu_to_le16 __cpu_to_le16
                     ^~~~~~~~~~~~~
arch/arm/include/asm/io.h:250:6: note: in expansion of macro 'cpu_to_le16'
      cpu_to_le16(v),__io(p)); })
      ^~~~~~~~~~~
drivers/staging/comedi/drivers/dt282x.c:566:2: note: in expansion of macro 'outw'
  outw(DT2821_CHANCSR_LLE | DT2821_CHANCSR_NUMB(n),
  ^~~~

The warning makes sense, though the code is correct as far as I
can tell.

This disambiguates the operation by making the constant expressions
we pass here explicitly 'unsigned', which helps to avoid the warning.

As pointed out by Hartley Sweeten, scripts/checkpatch.pl notices that
the shifts here are rather unreadable, though the suggested BIT()
macro wouldn't work either. I'm changing it to a hexadecimal notation,
which hopefully improves readability. I'm leaving the DT2821_CHANCSR_PRESLA
alone because it seems wrong.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: also reformat to make checkpatch.pl happy

 drivers/staging/comedi/drivers/dt282x.c | 72 ++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/staging/comedi/drivers/dt282x.c b/drivers/staging/comedi/drivers/dt282x.c
index 40bf00984fa5..991e9f44c18c 100644
--- a/drivers/staging/comedi/drivers/dt282x.c
+++ b/drivers/staging/comedi/drivers/dt282x.c
@@ -69,48 +69,48 @@
  * Register map
  */
 #define DT2821_ADCSR_REG		0x00
-#define DT2821_ADCSR_ADERR		(1 << 15)
-#define DT2821_ADCSR_ADCLK		(1 << 9)
-#define DT2821_ADCSR_MUXBUSY		(1 << 8)
-#define DT2821_ADCSR_ADDONE		(1 << 7)
-#define DT2821_ADCSR_IADDONE		(1 << 6)
-#define DT2821_ADCSR_GS(x)		(((x) & 0x3) << 4)
-#define DT2821_ADCSR_CHAN(x)		(((x) & 0xf) << 0)
+#define DT2821_ADCSR_ADERR		0x8000u
+#define DT2821_ADCSR_ADCLK		0x0200u
+#define DT2821_ADCSR_MUXBUSY		0x0100u
+#define DT2821_ADCSR_ADDONE		0x0080u
+#define DT2821_ADCSR_IADDONE		0x0040u
+#define DT2821_ADCSR_GS(x)	       (0x0030u & ((x) << 4))
+#define DT2821_ADCSR_CHAN(x)	       (0x000fu & (x))
 #define DT2821_CHANCSR_REG		0x02
-#define DT2821_CHANCSR_LLE		(1 << 15)
-#define DT2821_CHANCSR_PRESLA(x)	(((x) & 0xf) >> 8)
-#define DT2821_CHANCSR_NUMB(x)		((((x) - 1) & 0xf) << 0)
+#define DT2821_CHANCSR_LLE		0x8000u
+#define DT2821_CHANCSR_PRESLA(x)	(((x) & 0xf) >> 8) /* always 0? */
+#define DT2821_CHANCSR_NUMB(x)	       (0x000fu & ((x) - 1))
 #define DT2821_ADDAT_REG		0x04
 #define DT2821_DACSR_REG		0x06
-#define DT2821_DACSR_DAERR		(1 << 15)
-#define DT2821_DACSR_YSEL(x)		((x) << 9)
-#define DT2821_DACSR_SSEL		(1 << 8)
-#define DT2821_DACSR_DACRDY		(1 << 7)
-#define DT2821_DACSR_IDARDY		(1 << 6)
-#define DT2821_DACSR_DACLK		(1 << 5)
-#define DT2821_DACSR_HBOE		(1 << 1)
-#define DT2821_DACSR_LBOE		(1 << 0)
+#define DT2821_DACSR_DAERR		0x8000u
+#define DT2821_DACSR_YSEL(x)	       (0x7e00u & (x) << 9)
+#define DT2821_DACSR_SSEL		0x0100u
+#define DT2821_DACSR_DACRDY		0x0080u
+#define DT2821_DACSR_IDARDY		0x0040u
+#define DT2821_DACSR_DACLK		0x0020u
+#define DT2821_DACSR_HBOE		0x0002u
+#define DT2821_DACSR_LBOE		0x0001u
 #define DT2821_DADAT_REG		0x08
 #define DT2821_DIODAT_REG		0x0a
 #define DT2821_SUPCSR_REG		0x0c
-#define DT2821_SUPCSR_DMAD		(1 << 15)
-#define DT2821_SUPCSR_ERRINTEN		(1 << 14)
-#define DT2821_SUPCSR_CLRDMADNE		(1 << 13)
-#define DT2821_SUPCSR_DDMA		(1 << 12)
-#define DT2821_SUPCSR_DS_PIO		(0 << 10)
-#define DT2821_SUPCSR_DS_AD_CLK		(1 << 10)
-#define DT2821_SUPCSR_DS_DA_CLK		(2 << 10)
-#define DT2821_SUPCSR_DS_AD_TRIG	(3 << 10)
-#define DT2821_SUPCSR_BUFFB		(1 << 9)
-#define DT2821_SUPCSR_SCDN		(1 << 8)
-#define DT2821_SUPCSR_DACON		(1 << 7)
-#define DT2821_SUPCSR_ADCINIT		(1 << 6)
-#define DT2821_SUPCSR_DACINIT		(1 << 5)
-#define DT2821_SUPCSR_PRLD		(1 << 4)
-#define DT2821_SUPCSR_STRIG		(1 << 3)
-#define DT2821_SUPCSR_XTRIG		(1 << 2)
-#define DT2821_SUPCSR_XCLK		(1 << 1)
-#define DT2821_SUPCSR_BDINIT		(1 << 0)
+#define DT2821_SUPCSR_DMAD		0x8000u
+#define DT2821_SUPCSR_ERRINTEN		0x4000u
+#define DT2821_SUPCSR_CLRDMADNE		0x2000u
+#define DT2821_SUPCSR_DDMA		0x1000u
+#define DT2821_SUPCSR_DS_PIO	       (0x0c00u & (0u << 10))
+#define DT2821_SUPCSR_DS_AD_CLK	       (0x0c00u & (1u << 10))
+#define DT2821_SUPCSR_DS_DA_CLK	       (0x0c00u & (2u << 10))
+#define DT2821_SUPCSR_DS_AD_TRIG       (0x0c00u & (3u << 10))
+#define DT2821_SUPCSR_BUFFB		0x0200u
+#define DT2821_SUPCSR_SCDN		0x0100u
+#define DT2821_SUPCSR_DACON		0x0080u
+#define DT2821_SUPCSR_ADCINIT		0x0040u
+#define DT2821_SUPCSR_DACINIT		0x0020u
+#define DT2821_SUPCSR_PRLD		0x0010u
+#define DT2821_SUPCSR_STRIG		0x0008u
+#define DT2821_SUPCSR_XTRIG		0x0004u
+#define DT2821_SUPCSR_XCLK		0x0002u
+#define DT2821_SUPCSR_BDINIT		0x0001u
 #define DT2821_TMRCTR_REG		0x0e
 
 static const struct comedi_lrange range_dt282x_ai_lo_bipolar = {
-- 
2.7.0

[toc] | [next] | [standalone]

#1359717

FromIan Abbott <abbotti@mev.co.uk>
Date2016-03-17 11:30 +0100
Message-ID<rdDlM-68F-27@gated-at.bofh.it>
In reply to#1359360
On 16/03/16 20:51, Arnd Bergmann wrote:
> gcc-6 warns about passing negative signed integer into swab16()
> in the dt282x driver:
>
> drivers/staging/comedi/drivers/dt282x.c: In function 'dt282x_load_changain':
> include/uapi/linux/swab.h:14:33: warning: integer overflow in expression [-Woverflow]
>    (((__u16)(x) & (__u16)0xff00U) >> 8)))
>    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
> include/uapi/linux/swab.h:107:2: note: in expansion of macro '___constant_swab16'
>    ___constant_swab16(x) :   \
>    ^~~~~~~~~~~~~~~~~~
> include/uapi/linux/byteorder/big_endian.h:34:43: note: in expansion of macro '__swab16'
>   #define __cpu_to_le16(x) ((__force __le16)__swab16((x)))
>                                             ^~~~~~~~
> include/linux/byteorder/generic.h:89:21: note: in expansion of macro '__cpu_to_le16'
>   #define cpu_to_le16 __cpu_to_le16
>                       ^~~~~~~~~~~~~
> arch/arm/include/asm/io.h:250:6: note: in expansion of macro 'cpu_to_le16'
>        cpu_to_le16(v),__io(p)); })
>        ^~~~~~~~~~~
> drivers/staging/comedi/drivers/dt282x.c:566:2: note: in expansion of macro 'outw'
>    outw(DT2821_CHANCSR_LLE | DT2821_CHANCSR_NUMB(n),
>    ^~~~
>
> The warning makes sense, though the code is correct as far as I
> can tell.
>
> This disambiguates the operation by making the constant expressions
> we pass here explicitly 'unsigned', which helps to avoid the warning.
>
> As pointed out by Hartley Sweeten, scripts/checkpatch.pl notices that
> the shifts here are rather unreadable, though the suggested BIT()
> macro wouldn't work either. I'm changing it to a hexadecimal notation,
> which hopefully improves readability. I'm leaving the DT2821_CHANCSR_PRESLA
> alone because it seems wrong.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: also reformat to make checkpatch.pl happy
>
>   drivers/staging/comedi/drivers/dt282x.c | 72 ++++++++++++++++-----------------
>   1 file changed, 36 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/dt282x.c b/drivers/staging/comedi/drivers/dt282x.c
[snip]
> -#define DT2821_CHANCSR_LLE		(1 << 15)
> -#define DT2821_CHANCSR_PRESLA(x)	(((x) & 0xf) >> 8)
> -#define DT2821_CHANCSR_NUMB(x)		((((x) - 1) & 0xf) << 0)
> +#define DT2821_CHANCSR_LLE		0x8000u
> +#define DT2821_CHANCSR_PRESLA(x)	(((x) & 0xf) >> 8) /* always 0? */

The patch is fine, thanks.

Just a note on that dodgy-looking '>>' in the DT2821_CHANCSR_PRESLA 
macro.... I seems to be a typo in 0f8e8c5ab67a ("staging: comedi: 
dt282x: tidy up the register map and bit defines").  It should be a 
left-shift.  Fortunately, it isn't used, but we ought to correct it 
sometime.

Reviewed-by: Ian Abbott <abbotti@mev.co.uk>

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-

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

#1359963

FromHartley Sweeten <HartleyS@visionengravers.com>
Date2016-03-17 16:50 +0100
Message-ID<rdIlr-MU-3@gated-at.bofh.it>
In reply to#1359360
On Wednesday, March 16, 2016 1:51 PM, Arnd Bergmann wrote:
>
> gcc-6 warns about passing negative signed integer into swab16()
> in the dt282x driver:
>
> drivers/staging/comedi/drivers/dt282x.c: In function 'dt282x_load_changain':
> include/uapi/linux/swab.h:14:33: warning: integer overflow in expression [-Woverflow]
>   (((__u16)(x) & (__u16)0xff00U) >> 8)))
>   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
> include/uapi/linux/swab.h:107:2: note: in expansion of macro '___constant_swab16'
>   ___constant_swab16(x) :   \
>   ^~~~~~~~~~~~~~~~~~
> include/uapi/linux/byteorder/big_endian.h:34:43: note: in expansion of macro '__swab16'
>  #define __cpu_to_le16(x) ((__force __le16)__swab16((x)))
>                                            ^~~~~~~~
> include/linux/byteorder/generic.h:89:21: note: in expansion of macro '__cpu_to_le16'
>  #define cpu_to_le16 __cpu_to_le16
>                      ^~~~~~~~~~~~~
> arch/arm/include/asm/io.h:250:6: note: in expansion of macro 'cpu_to_le16'
>       cpu_to_le16(v),__io(p)); })
>       ^~~~~~~~~~~
> drivers/staging/comedi/drivers/dt282x.c:566:2: note: in expansion of macro 'outw'
>   outw(DT2821_CHANCSR_LLE | DT2821_CHANCSR_NUMB(n),
>   ^~~~

Arnd,

Is this a gcc-6 specific issue? Seems line this warning should be showing
up in a lot of drivers.

> The warning makes sense, though the code is correct as far as I
> can tell.
>
> This disambiguates the operation by making the constant expressions
> we pass here explicitly 'unsigned', which helps to avoid the warning.
>
> As pointed out by Hartley Sweeten, scripts/checkpatch.pl notices that
> the shifts here are rather unreadable, though the suggested BIT()
> macro wouldn't work either. I'm changing it to a hexadecimal notation,
> which hopefully improves readability. I'm leaving the DT2821_CHANCSR_PRESLA
> alone because it seems wrong.

BIT() should work for the ones pointed out by checpatch.pl.

I would argue that the hexadecimal notation is still rather unreadable.
These ones make my head hurt...

-#define DT2821_ADCSR_GS(x)		(((x) & 0x3) << 4)
+#define DT2821_ADCSR_GS(x)	       (0x0030u & ((x) << 4))

-#define DT2821_DACSR_YSEL(x)		((x) << 9)
+#define DT2821_DACSR_YSEL(x)	       (0x7e00u & (x) << 9)

-#define DT2821_SUPCSR_DS_PIO		(0 << 10)
-#define DT2821_SUPCSR_DS_AD_CLK		(1 << 10)
-#define DT2821_SUPCSR_DS_DA_CLK		(2 << 10)
-#define DT2821_SUPCSR_DS_AD_TRIG	(3 << 10)
+#define DT2821_SUPCSR_DS_PIO	       (0x0c00u & (0u << 10))
+#define DT2821_SUPCSR_DS_AD_CLK	       (0x0c00u & (1u << 10))
+#define DT2821_SUPCSR_DS_DA_CLK	       (0x0c00u & (2u << 10))
+#define DT2821_SUPCSR_DS_AD_TRIG       (0x0c00u & (3u << 10))

Also, most of the comedi drivers use the BIT() macro. Are you planning on
changing all of them to use hexadecimal notation?

Regards,
Hartley

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

#1359985

FromArnd Bergmann <arnd@arndb.de>
Date2016-03-17 17:10 +0100
Message-ID<rdIEO-1aN-27@gated-at.bofh.it>
In reply to#1359963
On Thursday 17 March 2016 15:47:57 Hartley Sweeten wrote:
> On Wednesday, March 16, 2016 1:51 PM, Arnd Bergmann wrote:
> 
> Is this a gcc-6 specific issue? Seems line this warning should be showing
> up in a lot of drivers.

Yes, I did not see this before moving to gcc-6.0, but this is the only driver
I've encountered the warning for, in around 7000 randconfig builds.

> > The warning makes sense, though the code is correct as far as I
> > can tell.
> >
> > This disambiguates the operation by making the constant expressions
> > we pass here explicitly 'unsigned', which helps to avoid the warning.
> >
> > As pointed out by Hartley Sweeten, scripts/checkpatch.pl notices that
> > the shifts here are rather unreadable, though the suggested BIT()
> > macro wouldn't work either. I'm changing it to a hexadecimal notation,
> > which hopefully improves readability. I'm leaving the DT2821_CHANCSR_PRESLA
> > alone because it seems wrong.
> 
> BIT() should work for the ones pointed out by checpatch.pl.
> 
> I would argue that the hexadecimal notation is still rather unreadable.
> These ones make my head hurt...
> 
> -#define DT2821_ADCSR_GS(x)		(((x) & 0x3) << 4)
> +#define DT2821_ADCSR_GS(x)	       (0x0030u & ((x) << 4))
> 
> -#define DT2821_DACSR_YSEL(x)		((x) << 9)
> +#define DT2821_DACSR_YSEL(x)	       (0x7e00u & (x) << 9)
> 
> -#define DT2821_SUPCSR_DS_PIO		(0 << 10)
> -#define DT2821_SUPCSR_DS_AD_CLK		(1 << 10)
> -#define DT2821_SUPCSR_DS_DA_CLK		(2 << 10)
> -#define DT2821_SUPCSR_DS_AD_TRIG	(3 << 10)
> +#define DT2821_SUPCSR_DS_PIO	       (0x0c00u & (0u << 10))
> +#define DT2821_SUPCSR_DS_AD_CLK	       (0x0c00u & (1u << 10))
> +#define DT2821_SUPCSR_DS_DA_CLK	       (0x0c00u & (2u << 10))
> +#define DT2821_SUPCSR_DS_AD_TRIG       (0x0c00u & (3u << 10))

Feel free to come up with a different patch. I've put the patch on
my 'submitted' stack for now and will get back to it after 4.7-rc1 if
the warning remains.

> Also, most of the comedi drivers use the BIT() macro. Are you planning on
> changing all of them to use hexadecimal notation?

No.

	Arnd

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

#1360044

FromHartley Sweeten <HartleyS@visionengravers.com>
Date2016-03-17 18:00 +0100
Message-ID<rdJrd-1sp-17@gated-at.bofh.it>
In reply to#1359985
On Thursday, March 17, 2016 9:09 AM, Arnd Bergmann wrote:
> On Thursday 17 March 2016 15:47:57 Hartley Sweeten wrote:
>> On Wednesday, March 16, 2016 1:51 PM, Arnd Bergmann wrote:
>> 
>> Is this a gcc-6 specific issue? Seems line this warning should be showing
>> up in a lot of drivers.
>
> Yes, I did not see this before moving to gcc-6.0, but this is the only driver
> I've encountered the warning for, in around 7000 randconfig builds.

Weird...

> Feel free to come up with a different patch. I've put the patch on
> my 'submitted' stack for now and will get back to it after 4.7-rc1 if
> the warning remains.

I'll post an alternate patch shortly that is more consistent with the
other comedi drivers.

If you could test that it fixes the gcc-6 issue that would be great!

Thanks,
Hartley

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web