gcc-10 points out a few instances of suspicious integer arithmetic
leading to value truncation:
sound/isa/opti9xx/opti92x-ad1848.c: In function 'snd_opti9xx_configure':
sound/isa/opti9xx/opti92x-ad1848.c:322:43: error: overflow in conversion from 'int' to 'unsigned char' changes value from '(int)snd_opti9xx_read(chip, 3) & -256 | 240' to '240' [-Werror=overflow]
322 | (snd_opti9xx_read(chip, reg) & ~(mask)) | ((value) & (mask)))
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
sound/isa/opti9xx/opti92x-ad1848.c:351:3: note: in expansion of macro 'snd_opti9xx_write_mask'
351 | snd_opti9xx_write_mask(chip, OPTi9XX_MC_REG(3), 0xf0, 0xff);
| ^~~~~~~~~~~~~~~~~~~~~~
sound/isa/opti9xx/miro.c: In function 'snd_miro_configure':
sound/isa/opti9xx/miro.c:873:40: error: overflow in conversion from 'int' to 'unsigned char' changes value from '(int)snd_miro_read(chip, 3) & -256 | 240' to '240' [-Werror=overflow]
873 | (snd_miro_read(chip, reg) & ~(mask)) | ((value) & (mask)))
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
sound/isa/opti9xx/miro.c:1010:3: note: in expansion of macro 'snd_miro_write_mask'
1010 | snd_miro_write_mask(chip, OPTi9XX_MC_REG(3), 0xf0, 0xff);
| ^~~~~~~~~~~~~~~~~~~
These are all harmless here as only the low 8 bit are passed down
anyway. Change the macros to inline functions to make the code
more readable and also avoid the warning.
Strictly speaking those functions also need locking to make the
read/write pair atomic, but it seems unlikely that anyone would
still run into that issue.
Fixes: 1841f613fd2e ("[ALSA] Add snd-miro driver")
Signed-off-by: Arnd Bergmann <[email protected]>
---
sound/isa/opti9xx/miro.c | 9 ++++++---
sound/isa/opti9xx/opti92x-ad1848.c | 9 ++++++---
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/sound/isa/opti9xx/miro.c b/sound/isa/opti9xx/miro.c
index e764816a8f7a..b039429e6871 100644
--- a/sound/isa/opti9xx/miro.c
+++ b/sound/isa/opti9xx/miro.c
@@ -867,10 +867,13 @@ static void snd_miro_write(struct snd_miro *chip, unsigned char reg,
spin_unlock_irqrestore(&chip->lock, flags);
}
+static inline void snd_miro_write_mask(struct snd_miro *chip,
+ unsigned char reg, unsigned char value, unsigned char mask)
+{
+ unsigned char oldval = snd_miro_read(chip, reg);
-#define snd_miro_write_mask(chip, reg, value, mask) \
- snd_miro_write(chip, reg, \
- (snd_miro_read(chip, reg) & ~(mask)) | ((value) & (mask)))
+ snd_miro_write(chip, reg, (oldval & ~mask) | (value & mask));
+}
/*
* Proc Interface
diff --git a/sound/isa/opti9xx/opti92x-ad1848.c b/sound/isa/opti9xx/opti92x-ad1848.c
index d06b29693c85..0e6d20e49158 100644
--- a/sound/isa/opti9xx/opti92x-ad1848.c
+++ b/sound/isa/opti9xx/opti92x-ad1848.c
@@ -317,10 +317,13 @@ static void snd_opti9xx_write(struct snd_opti9xx *chip, unsigned char reg,
}
-#define snd_opti9xx_write_mask(chip, reg, value, mask) \
- snd_opti9xx_write(chip, reg, \
- (snd_opti9xx_read(chip, reg) & ~(mask)) | ((value) & (mask)))
+static inline void snd_opti9xx_write_mask(struct snd_opti9xx *chip,
+ unsigned char reg, unsigned char value, unsigned char mask)
+{
+ unsigned char oldval = snd_opti9xx_read(chip, reg);
+ snd_opti9xx_write(chip, reg, (oldval & ~mask) | (value & mask));
+}
static int snd_opti9xx_configure(struct snd_opti9xx *chip,
long port,
--
2.26.0
On Wed, 29 Apr 2020 21:02:03 +0200,
Arnd Bergmann wrote:
>
> gcc-10 points out a few instances of suspicious integer arithmetic
> leading to value truncation:
>
> sound/isa/opti9xx/opti92x-ad1848.c: In function 'snd_opti9xx_configure':
> sound/isa/opti9xx/opti92x-ad1848.c:322:43: error: overflow in conversion from 'int' to 'unsigned char' changes value from '(int)snd_opti9xx_read(chip, 3) & -256 | 240' to '240' [-Werror=overflow]
> 322 | (snd_opti9xx_read(chip, reg) & ~(mask)) | ((value) & (mask)))
> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
> sound/isa/opti9xx/opti92x-ad1848.c:351:3: note: in expansion of macro 'snd_opti9xx_write_mask'
> 351 | snd_opti9xx_write_mask(chip, OPTi9XX_MC_REG(3), 0xf0, 0xff);
> | ^~~~~~~~~~~~~~~~~~~~~~
> sound/isa/opti9xx/miro.c: In function 'snd_miro_configure':
> sound/isa/opti9xx/miro.c:873:40: error: overflow in conversion from 'int' to 'unsigned char' changes value from '(int)snd_miro_read(chip, 3) & -256 | 240' to '240' [-Werror=overflow]
> 873 | (snd_miro_read(chip, reg) & ~(mask)) | ((value) & (mask)))
> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
> sound/isa/opti9xx/miro.c:1010:3: note: in expansion of macro 'snd_miro_write_mask'
> 1010 | snd_miro_write_mask(chip, OPTi9XX_MC_REG(3), 0xf0, 0xff);
> | ^~~~~~~~~~~~~~~~~~~
>
> These are all harmless here as only the low 8 bit are passed down
> anyway. Change the macros to inline functions to make the code
> more readable and also avoid the warning.
>
> Strictly speaking those functions also need locking to make the
> read/write pair atomic, but it seems unlikely that anyone would
> still run into that issue.
>
> Fixes: 1841f613fd2e ("[ALSA] Add snd-miro driver")
> Signed-off-by: Arnd Bergmann <[email protected]>
Applied now, thanks.
Takashi
From: Arnd Bergmann
> Sent: 29 April 2020 20:02
> gcc-10 points out a few instances of suspicious integer arithmetic
> leading to value truncation:
>
> sound/isa/opti9xx/opti92x-ad1848.c: In function 'snd_opti9xx_configure':
> sound/isa/opti9xx/opti92x-ad1848.c:322:43: error: overflow in conversion from 'int' to 'unsigned char'
> changes value from '(int)snd_opti9xx_read(chip, 3) & -256 | 240' to '240' [-Werror=overflow]
> 322 | (snd_opti9xx_read(chip, reg) & ~(mask)) | ((value) & (mask)))
> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
> sound/isa/opti9xx/opti92x-ad1848.c:351:3: note: in expansion of macro 'snd_opti9xx_write_mask'
> 351 | snd_opti9xx_write_mask(chip, OPTi9XX_MC_REG(3), 0xf0, 0xff);
> | ^~~~~~~~~~~~~~~~~~~~~~
> sound/isa/opti9xx/miro.c: In function 'snd_miro_configure':
> sound/isa/opti9xx/miro.c:873:40: error: overflow in conversion from 'int' to 'unsigned char' changes
> value from '(int)snd_miro_read(chip, 3) & -256 | 240' to '240' [-Werror=overflow]
> 873 | (snd_miro_read(chip, reg) & ~(mask)) | ((value) & (mask)))
> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
> sound/isa/opti9xx/miro.c:1010:3: note: in expansion of macro 'snd_miro_write_mask'
> 1010 | snd_miro_write_mask(chip, OPTi9XX_MC_REG(3), 0xf0, 0xff);
> | ^~~~~~~~~~~~~~~~~~~
>
> These are all harmless here as only the low 8 bit are passed down
> anyway. Change the macros to inline functions to make the code
> more readable and also avoid the warning.
>
> Strictly speaking those functions also need locking to make the
> read/write pair atomic, but it seems unlikely that anyone would
> still run into that issue.
>
> Fixes: 1841f613fd2e ("[ALSA] Add snd-miro driver")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> sound/isa/opti9xx/miro.c | 9 ++++++---
> sound/isa/opti9xx/opti92x-ad1848.c | 9 ++++++---
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/sound/isa/opti9xx/miro.c b/sound/isa/opti9xx/miro.c
> index e764816a8f7a..b039429e6871 100644
> --- a/sound/isa/opti9xx/miro.c
> +++ b/sound/isa/opti9xx/miro.c
> @@ -867,10 +867,13 @@ static void snd_miro_write(struct snd_miro *chip, unsigned char reg,
> spin_unlock_irqrestore(&chip->lock, flags);
> }
>
> +static inline void snd_miro_write_mask(struct snd_miro *chip,
> + unsigned char reg, unsigned char value, unsigned char mask)
> +{
> + unsigned char oldval = snd_miro_read(chip, reg);
>
> -#define snd_miro_write_mask(chip, reg, value, mask) \
> - snd_miro_write(chip, reg, \
> - (snd_miro_read(chip, reg) & ~(mask)) | ((value) & (mask)))
> + snd_miro_write(chip, reg, (oldval & ~mask) | (value & mask));
> +}
Isn't that likely to add additional masking with 0xff at the call sites?
You will probably get better code if the arguments are 'unsigned int'.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Thu, 30 Apr 2020 08:12:07 +0200,
Takashi Iwai wrote:
>
> On Wed, 29 Apr 2020 21:02:03 +0200,
> Arnd Bergmann wrote:
> >
> > gcc-10 points out a few instances of suspicious integer arithmetic
> > leading to value truncation:
> >
> > sound/isa/opti9xx/opti92x-ad1848.c: In function 'snd_opti9xx_configure':
> > sound/isa/opti9xx/opti92x-ad1848.c:322:43: error: overflow in conversion from 'int' to 'unsigned char' changes value from '(int)snd_opti9xx_read(chip, 3) & -256 | 240' to '240' [-Werror=overflow]
> > 322 | (snd_opti9xx_read(chip, reg) & ~(mask)) | ((value) & (mask)))
> > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
> > sound/isa/opti9xx/opti92x-ad1848.c:351:3: note: in expansion of macro 'snd_opti9xx_write_mask'
> > 351 | snd_opti9xx_write_mask(chip, OPTi9XX_MC_REG(3), 0xf0, 0xff);
> > | ^~~~~~~~~~~~~~~~~~~~~~
> > sound/isa/opti9xx/miro.c: In function 'snd_miro_configure':
> > sound/isa/opti9xx/miro.c:873:40: error: overflow in conversion from 'int' to 'unsigned char' changes value from '(int)snd_miro_read(chip, 3) & -256 | 240' to '240' [-Werror=overflow]
> > 873 | (snd_miro_read(chip, reg) & ~(mask)) | ((value) & (mask)))
> > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
> > sound/isa/opti9xx/miro.c:1010:3: note: in expansion of macro 'snd_miro_write_mask'
> > 1010 | snd_miro_write_mask(chip, OPTi9XX_MC_REG(3), 0xf0, 0xff);
> > | ^~~~~~~~~~~~~~~~~~~
> >
> > These are all harmless here as only the low 8 bit are passed down
> > anyway. Change the macros to inline functions to make the code
> > more readable and also avoid the warning.
> >
> > Strictly speaking those functions also need locking to make the
> > read/write pair atomic, but it seems unlikely that anyone would
> > still run into that issue.
> >
> > Fixes: 1841f613fd2e ("[ALSA] Add snd-miro driver")
> > Signed-off-by: Arnd Bergmann <[email protected]>
>
> Applied now, thanks.
BTW, the lack of locking is no problem in this code.
Those lines are called only at the initialization in the probe
function, so can't race.
thanks,
Takashi
On Thu, 30 Apr 2020 10:15:02 +0200,
David Laight wrote:
>
> From: Arnd Bergmann
> > Sent: 29 April 2020 20:02
> > gcc-10 points out a few instances of suspicious integer arithmetic
> > leading to value truncation:
> >
> > sound/isa/opti9xx/opti92x-ad1848.c: In function 'snd_opti9xx_configure':
> > sound/isa/opti9xx/opti92x-ad1848.c:322:43: error: overflow in conversion from 'int' to 'unsigned char'
> > changes value from '(int)snd_opti9xx_read(chip, 3) & -256 | 240' to '240' [-Werror=overflow]
> > 322 | (snd_opti9xx_read(chip, reg) & ~(mask)) | ((value) & (mask)))
> > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
> > sound/isa/opti9xx/opti92x-ad1848.c:351:3: note: in expansion of macro 'snd_opti9xx_write_mask'
> > 351 | snd_opti9xx_write_mask(chip, OPTi9XX_MC_REG(3), 0xf0, 0xff);
> > | ^~~~~~~~~~~~~~~~~~~~~~
> > sound/isa/opti9xx/miro.c: In function 'snd_miro_configure':
> > sound/isa/opti9xx/miro.c:873:40: error: overflow in conversion from 'int' to 'unsigned char' changes
> > value from '(int)snd_miro_read(chip, 3) & -256 | 240' to '240' [-Werror=overflow]
> > 873 | (snd_miro_read(chip, reg) & ~(mask)) | ((value) & (mask)))
> > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
> > sound/isa/opti9xx/miro.c:1010:3: note: in expansion of macro 'snd_miro_write_mask'
> > 1010 | snd_miro_write_mask(chip, OPTi9XX_MC_REG(3), 0xf0, 0xff);
> > | ^~~~~~~~~~~~~~~~~~~
> >
> > These are all harmless here as only the low 8 bit are passed down
> > anyway. Change the macros to inline functions to make the code
> > more readable and also avoid the warning.
> >
> > Strictly speaking those functions also need locking to make the
> > read/write pair atomic, but it seems unlikely that anyone would
> > still run into that issue.
> >
> > Fixes: 1841f613fd2e ("[ALSA] Add snd-miro driver")
> > Signed-off-by: Arnd Bergmann <[email protected]>
> > ---
> > sound/isa/opti9xx/miro.c | 9 ++++++---
> > sound/isa/opti9xx/opti92x-ad1848.c | 9 ++++++---
> > 2 files changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/sound/isa/opti9xx/miro.c b/sound/isa/opti9xx/miro.c
> > index e764816a8f7a..b039429e6871 100644
> > --- a/sound/isa/opti9xx/miro.c
> > +++ b/sound/isa/opti9xx/miro.c
> > @@ -867,10 +867,13 @@ static void snd_miro_write(struct snd_miro *chip, unsigned char reg,
> > spin_unlock_irqrestore(&chip->lock, flags);
> > }
> >
> > +static inline void snd_miro_write_mask(struct snd_miro *chip,
> > + unsigned char reg, unsigned char value, unsigned char mask)
> > +{
> > + unsigned char oldval = snd_miro_read(chip, reg);
> >
> > -#define snd_miro_write_mask(chip, reg, value, mask) \
> > - snd_miro_write(chip, reg, \
> > - (snd_miro_read(chip, reg) & ~(mask)) | ((value) & (mask)))
> > + snd_miro_write(chip, reg, (oldval & ~mask) | (value & mask));
> > +}
>
> Isn't that likely to add additional masking with 0xff at the call sites?
> You will probably get better code if the arguments are 'unsigned int'.
I don't think such a micro optimization is needed.
All registers, values and masks in the driver are 8bit, so keeping all
unsigned char is rather an improvement of readability.
thanks,
Takashi
From: Takashi Iwai
> Sent: 30 April 2020 09:25
>
> On Thu, 30 Apr 2020 10:15:02 +0200,
> David Laight wrote:
> >
> > From: Arnd Bergmann
....
> > > +static inline void snd_miro_write_mask(struct snd_miro *chip,
> > > + unsigned char reg, unsigned char value, unsigned char mask)
> > > +{
> > > + unsigned char oldval = snd_miro_read(chip, reg);
> > >
> > > -#define snd_miro_write_mask(chip, reg, value, mask) \
> > > - snd_miro_write(chip, reg, \
> > > - (snd_miro_read(chip, reg) & ~(mask)) | ((value) & (mask)))
> > > + snd_miro_write(chip, reg, (oldval & ~mask) | (value & mask));
> > > +}
> >
> > Isn't that likely to add additional masking with 0xff at the call sites?
> > You will probably get better code if the arguments are 'unsigned int'.
>
> I don't think such a micro optimization is needed.
> All registers, values and masks in the driver are 8bit, so keeping all
> unsigned char is rather an improvement of readability.
And every time you do any arithmetic they get extended to int.
And if you pass them to a function (as char) the compiler
has to mask the result of any arithmetic back to 8 bits.
On x86 the compiler can use the 'as if' rule and do 8 bit arithmetic.
But only if it can determine that the high bits aren't actually used.
On almost every other architecture you are likely to get a lot
of masking operations.
Just because the domain of a variable of 0..255 doesn't mean
that 'unsigned char' is an appropriate type for a variable.
For x86-64 (and probably others) 'unsigned int' is usually best for
anything that cannot be negative.
In particular it saves the sign extension instruction that has to
be inserted when an 'int' variable is used as an array index.
FWIW I think that somewhere else the ~mask had to be replaced
with (mask ^ 0xff) do avoid another spurious compiler warning.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)