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

Groups > linux.kernel > #1357665 > unrolled thread

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

Started byArnd Bergmann <arnd@arndb.de>
First post2016-03-14 23:50 +0100
Last post2016-03-16 22:00 +0100
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] staging/comedi/dt282x: avoid integer overflow warning Arnd Bergmann <arnd@arndb.de> - 2016-03-14 23:50 +0100
    RE: [PATCH] staging/comedi/dt282x: avoid integer overflow warning Hartley Sweeten <HartleyS@visionengravers.com> - 2016-03-15 23:00 +0100
      Re: [PATCH] staging/comedi/dt282x: avoid integer overflow warning Arnd Bergmann <arnd@arndb.de> - 2016-03-15 23:00 +0100
        RE: [PATCH] staging/comedi/dt282x: avoid integer overflow warning Hartley Sweeten <HartleyS@visionengravers.com> - 2016-03-16 18:10 +0100
          Re: [PATCH] staging/comedi/dt282x: avoid integer overflow warning Arnd Bergmann <arnd@arndb.de> - 2016-03-16 22:00 +0100

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

FromArnd Bergmann <arnd@arndb.de>
Date2016-03-14 23:50 +0100
Subject[PATCH] staging/comedi/dt282x: avoid integer overflow warning
Message-ID<rcJtg-1DY-15@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.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/staging/comedi/drivers/dt282x.c | 62 ++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/drivers/staging/comedi/drivers/dt282x.c b/drivers/staging/comedi/drivers/dt282x.c
index 40bf00984fa5..d4d45c759c62 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_ADERR		(1u << 15)
+#define DT2821_ADCSR_ADCLK		(1u << 9)
+#define DT2821_ADCSR_MUXBUSY		(1u << 8)
+#define DT2821_ADCSR_ADDONE		(1u << 7)
+#define DT2821_ADCSR_IADDONE		(1u << 6)
 #define DT2821_ADCSR_GS(x)		(((x) & 0x3) << 4)
 #define DT2821_ADCSR_CHAN(x)		(((x) & 0xf) << 0)
 #define DT2821_CHANCSR_REG		0x02
-#define DT2821_CHANCSR_LLE		(1 << 15)
+#define DT2821_CHANCSR_LLE		(1u << 15)
 #define DT2821_CHANCSR_PRESLA(x)	(((x) & 0xf) >> 8)
 #define DT2821_CHANCSR_NUMB(x)		((((x) - 1) & 0xf) << 0)
 #define DT2821_ADDAT_REG		0x04
 #define DT2821_DACSR_REG		0x06
-#define DT2821_DACSR_DAERR		(1 << 15)
+#define DT2821_DACSR_DAERR		(1u << 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_SSEL		(1u << 8)
+#define DT2821_DACSR_DACRDY		(1u << 7)
+#define DT2821_DACSR_IDARDY		(1u << 6)
+#define DT2821_DACSR_DACLK		(1u << 5)
+#define DT2821_DACSR_HBOE		(1u << 1)
+#define DT2821_DACSR_LBOE		(1u << 0)
 #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		(1u << 15)
+#define DT2821_SUPCSR_ERRINTEN		(1u << 14)
+#define DT2821_SUPCSR_CLRDMADNE		(1u << 13)
+#define DT2821_SUPCSR_DDMA		(1u << 12)
+#define DT2821_SUPCSR_DS_PIO		(0u << 10)
+#define DT2821_SUPCSR_DS_AD_CLK		(1u << 10)
+#define DT2821_SUPCSR_DS_DA_CLK		(2u << 10)
+#define DT2821_SUPCSR_DS_AD_TRIG	(3u << 10)
+#define DT2821_SUPCSR_BUFFB		(1u << 9)
+#define DT2821_SUPCSR_SCDN		(1u << 8)
+#define DT2821_SUPCSR_DACON		(1u << 7)
+#define DT2821_SUPCSR_ADCINIT		(1u << 6)
+#define DT2821_SUPCSR_DACINIT		(1u << 5)
+#define DT2821_SUPCSR_PRLD		(1u << 4)
+#define DT2821_SUPCSR_STRIG		(1u << 3)
+#define DT2821_SUPCSR_XTRIG		(1u << 2)
+#define DT2821_SUPCSR_XCLK		(1u << 1)
+#define DT2821_SUPCSR_BDINIT		(1u << 0)
 #define DT2821_TMRCTR_REG		0x0e
 
 static const struct comedi_lrange range_dt282x_ai_lo_bipolar = {
-- 
2.7.0

[toc] | [next] | [standalone]

#1358281

FromHartley Sweeten <HartleyS@visionengravers.com>
Date2016-03-15 23:00 +0100
Message-ID<rd5aq-7XV-15@gated-at.bofh.it>
In reply to#1357665
On Monday, March 14, 2016 3:48 PM, Arnd Bergmann wrote:
> gcc-6 warns about passing negative signed integer into swab16()
> in the dt282x driver:

<snip>

> 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.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/staging/comedi/drivers/dt282x.c | 62 ++++++++++++++++-----------------
>  1 file changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/dt282x.c b/drivers/staging/comedi/drivers/dt282x.c
> index 40bf00984fa5..d4d45c759c62 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_ADERR		(1u << 15)

Changing all of these to use the BIT() macro should also avoid the warning.

Hartley

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

#1358284

FromArnd Bergmann <arnd@arndb.de>
Date2016-03-15 23:00 +0100
Message-ID<rd5aq-7XV-17@gated-at.bofh.it>
In reply to#1358281
On Tuesday 15 March 2016 21:35:40 Hartley Sweeten wrote:
> On Monday, March 14, 2016 3:48 PM, Arnd Bergmann wrote:
> > gcc-6 warns about passing negative signed integer into swab16()
> > in the dt282x driver:
> 
> <snip>
> 
> > 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.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  drivers/staging/comedi/drivers/dt282x.c | 62 ++++++++++++++++-----------------
> >  1 file changed, 31 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/drivers/dt282x.c b/drivers/staging/comedi/drivers/dt282x.c
> > index 40bf00984fa5..d4d45c759c62 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_ADERR		(1u << 15)
> 
> Changing all of these to use the BIT() macro should also avoid the warning.

Yes, but it won't work for the ones that have more than one bit:

#define DT2821_SUPCSR_DS_AD_TRIG       (3 << 10)

I considered using BIT() but decided against it for consistency.

	Arnd

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

#1359177

FromHartley Sweeten <HartleyS@visionengravers.com>
Date2016-03-16 18:10 +0100
Message-ID<rdn7k-3qk-17@gated-at.bofh.it>
In reply to#1358284
On Tuesday, March 15, 2016 2:50 PM, Arnd Bergmann wrote:
> On Tuesday 15 March 2016 21:35:40 Hartley Sweeten wrote:
>> On Monday, March 14, 2016 3:48 PM, Arnd Bergmann wrote:
>>> gcc-6 warns about passing negative signed integer into swab16()
>>> in the dt282x driver:
>> 
>> <snip>
>> 
>>> 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.
>>>
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>> ---
>>>  drivers/staging/comedi/drivers/dt282x.c | 62 ++++++++++++++++-----------------
>>>  1 file changed, 31 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/staging/comedi/drivers/dt282x.c b/drivers/staging/comedi/drivers/dt282x.c
>>> index 40bf00984fa5..d4d45c759c62 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_ADERR		(1u << 15)
>> 
>> Changing all of these to use the BIT() macro should also avoid the warning.
>
> Yes, but it won't work for the ones that have more than one bit:
>
> #define DT2821_SUPCSR_DS_AD_TRIG       (3 << 10)

Use a helper macro for those bits:

#define DT2821_SUPCSR_DS(x)		(((x) & 0x3) << 10)
#define DT2821_SUPCSR_DS_PIO		DT2821_SUPCSR_DS(0)
#define DT2821_SUPCSR_DS_AD_CLK		DT2821_SUPCSR_DS(1)
#define DT2821_SUPCSR_DS_DA_CLK		DT2821_SUPCSR_DS(2)
#define DT2821_SUPCSR_DS_AD_TRIG		DT2821_SUPCSR_DS(3)

> I considered using BIT() but decided against it for consistency.

Your change may fix the gcc-6 issue but it doesn't fix the 28 checkpatch.pl
issues:
CHECK: Prefer using the BIT macro

Regards,
Hartley

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

#1359361

FromArnd Bergmann <arnd@arndb.de>
Date2016-03-16 22:00 +0100
Message-ID<rdqHT-5Ja-13@gated-at.bofh.it>
In reply to#1359177
On Wednesday 16 March 2016 17:04:15 Hartley Sweeten wrote:
> > #define DT2821_SUPCSR_DS_AD_TRIG       (3 << 10)
> 
> Use a helper macro for those bits:
> 
> #define DT2821_SUPCSR_DS(x)             (((x) & 0x3) << 10)
> #define DT2821_SUPCSR_DS_PIO            DT2821_SUPCSR_DS(0)
> #define DT2821_SUPCSR_DS_AD_CLK         DT2821_SUPCSR_DS(1)
> #define DT2821_SUPCSR_DS_DA_CLK         DT2821_SUPCSR_DS(2)
> #define DT2821_SUPCSR_DS_AD_TRIG                DT2821_SUPCSR_DS(3)
> 
> > I considered using BIT() but decided against it for consistency.
> 
> Your change may fix the gcc-6 issue but it doesn't fix the 28 checkpatch.pl
> issues:
> CHECK: Prefer using the BIT macro

I sent a new version now, and found a better solution that avoids
using BIT().

	Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web