From: Hartley Sweeten <HartleyS@visionengravers.com>
To: Arnd Bergmann <arnd@arndb.de>
CC: Ian Abbott <abbotti@mev.co.uk>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning
Thread-Topic: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning
Thread-Index: AQHRf8Wrp8TTHLyDSEGX7TkInjYrEp9dxbXg
Date: Thu, 17 Mar 2016 15:47:57 +0000
Message-ID: <BN3PR0101MB1057257A9A9915F22A8DF0ABD08B0@BN3PR0101MB1057.prod.exchangelabs.com>
References: <1458161501-283680-1-git-send-email-arnd@arndb.de>
In-Reply-To: <1458161501-283680-1-git-send-email-arnd@arndb.de>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:23
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Mar 2016 15:47:57.9926
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: d698601f-af92-4269-8099-fd6f11636477
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN3PR0101MB1060
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2782
Lines: 67

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