linux-kernel - Re: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3053856.pfRJflhkJ8@wuerfel>
Date:	Thu, 17 Mar 2016 17:08:34 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Hartley Sweeten <HartleyS@...ionengravers.com>
Cc:	Ian Abbott <abbotti@....co.uk>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning

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