From: Arnd Bergmann <arnd@arndb.de>
To: Hartley Sweeten <HartleyS@visionengravers.com>
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
Date: Thu, 17 Mar 2016 17:08:34 +0100
Message-ID: <3053856.pfRJflhkJ8@wuerfel>
User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; )
In-Reply-To: <BN3PR0101MB1057257A9A9915F22A8DF0ABD08B0@BN3PR0101MB1057.prod.exchangelabs.com>
References: <1458161501-283680-1-git-send-email-arnd@arndb.de> <BN3PR0101MB1057257A9A9915F22A8DF0ABD08B0@BN3PR0101MB1057.prod.exchangelabs.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2078
Lines: 51

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