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

[Arnd Bergmann]

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