Re: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning
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 --- 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 -- -=( Ian Abbott @ MEV Ltd.E-mail: )=- -=( Web: http://www.mev.co.uk/ )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel