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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
--
-=( Ian Abbott @ MEV Ltd. E-mail: <[email protected]> )=-
-=( Web: http://www.mev.co.uk/ )=-
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
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
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