Re: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning
From: Arnd Bergmann
Date: Thu Mar 17 2016 - 12:08:53 EST
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