Messages in this thread | | | Subject | Re: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning | From | Ian Abbott <> | Date | Thu, 17 Mar 2016 10:21:50 +0000 |
| |
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/ )=-
| |