Subject: Re: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning
To: Arnd Bergmann <arnd@arndb.de>,
        H Hartley Sweeten <hsweeten@visionengravers.com>
References: <1458161501-283680-1-git-send-email-arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
From: Ian Abbott <abbotti@mev.co.uk>
Message-ID: <56EA853E.10403@mev.co.uk>
Date: Thu, 17 Mar 2016 10:21:50 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Icedove/38.6.0
MIME-Version: 1.0
In-Reply-To: <1458161501-283680-1-git-send-email-arnd@arndb.de>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2827
Lines: 64

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/  )=-