lkml.org 
[lkml]   [2016]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning
From
Date
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/ )=-

\
 
 \ /
  Last update: 2016-03-17 12:01    [W:0.140 / U:0.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site