| Message ID | 5560AB9D.1010807@maciej.szmigiero.name (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e712bfca1ac1f63f622f87c2f33b57608f2a4d19 |
| Headers | show |
On Sat, May 23, 2015 at 06:32:29PM +0200, Maciej S. Szmigiero wrote: > snd_soc_pcm_stream.formats is a bitmask of SNDRV_PCM_FMTBIT_*, > not of SNDRV_PCM_FORMAT_* (which are sequential integers), > however some of ASoC CODEC drivers use these values instead. > > Found out by sparse on 0-day kernel tester. > > Signed-off-by: Maciej Szmigiero <mail@maciej.szmigiero.name> > Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com> Thanks, Charles
On Sat, May 23, 2015 at 06:32:29PM +0200, Maciej S. Szmigiero wrote: > snd_soc_pcm_stream.formats is a bitmask of SNDRV_PCM_FMTBIT_*, > not of SNDRV_PCM_FORMAT_* (which are sequential integers), > however some of ASoC CODEC drivers use these values instead. Applied, htanks.
At Sat, 23 May 2015 18:32:29 +0200, Maciej S. Szmigiero wrote: > > snd_soc_pcm_stream.formats is a bitmask of SNDRV_PCM_FMTBIT_*, > not of SNDRV_PCM_FORMAT_* (which are sequential integers), > however some of ASoC CODEC drivers use these values instead. > > Found out by sparse on 0-day kernel tester. > > Signed-off-by: Maciej Szmigiero <mail@maciej.szmigiero.name> Wow, that made me wonder how these drivers could actually work. BTW, how did you detect it? Any static analyzer like sparse or smatch? sparse didn't detect it at the last time I tried, IIRC... thanks, Takashi > > diff --git a/sound/soc/codecs/88pm860x-codec.c b/sound/soc/codecs/88pm860x-codec.c > index ee31fa7..38b3dad 100644 > --- a/sound/soc/codecs/88pm860x-codec.c > +++ b/sound/soc/codecs/88pm860x-codec.c > @@ -1186,16 +1186,16 @@ static struct snd_soc_dai_driver pm860x_dai[] = { > .channels_min = 2, > .channels_max = 2, > .rates = PM860X_RATES, > - .formats = SNDRV_PCM_FORMAT_S16_LE | \ > - SNDRV_PCM_FORMAT_S18_3LE, > + .formats = SNDRV_PCM_FMTBIT_S16_LE | \ > + SNDRV_PCM_FMTBIT_S18_3LE, > }, > .capture = { > .stream_name = "PCM Capture", > .channels_min = 2, > .channels_max = 2, > .rates = PM860X_RATES, > - .formats = SNDRV_PCM_FORMAT_S16_LE | \ > - SNDRV_PCM_FORMAT_S18_3LE, > + .formats = SNDRV_PCM_FMTBIT_S16_LE | \ > + SNDRV_PCM_FMTBIT_S18_3LE, > }, > .ops = &pm860x_pcm_dai_ops, > }, { > @@ -1207,16 +1207,16 @@ static struct snd_soc_dai_driver pm860x_dai[] = { > .channels_min = 2, > .channels_max = 2, > .rates = SNDRV_PCM_RATE_8000_48000, > - .formats = SNDRV_PCM_FORMAT_S16_LE | \ > - SNDRV_PCM_FORMAT_S18_3LE, > + .formats = SNDRV_PCM_FMTBIT_S16_LE | \ > + SNDRV_PCM_FMTBIT_S18_3LE, > }, > .capture = { > .stream_name = "I2S Capture", > .channels_min = 2, > .channels_max = 2, > .rates = SNDRV_PCM_RATE_8000_48000, > - .formats = SNDRV_PCM_FORMAT_S16_LE | \ > - SNDRV_PCM_FORMAT_S18_3LE, > + .formats = SNDRV_PCM_FMTBIT_S16_LE | \ > + SNDRV_PCM_FMTBIT_S18_3LE, > }, > .ops = &pm860x_i2s_dai_ops, > }, > diff --git a/sound/soc/codecs/stac9766.c b/sound/soc/codecs/stac9766.c > index 2341e8e..ed4cca7 100644 > --- a/sound/soc/codecs/stac9766.c > +++ b/sound/soc/codecs/stac9766.c > @@ -320,7 +320,7 @@ static struct snd_soc_dai_driver stac9766_dai[] = { > .channels_max = 2, > .rates = SNDRV_PCM_RATE_32000 | \ > SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000, > - .formats = SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE, > + .formats = SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE, > }, > /* alsa ops */ > .ops = &stac9766_dai_ops_digital, > diff --git a/sound/soc/codecs/wm8900.c b/sound/soc/codecs/wm8900.c > index e7d2ecd..493faf5 100644 > --- a/sound/soc/codecs/wm8900.c > +++ b/sound/soc/codecs/wm8900.c > @@ -998,8 +998,8 @@ static int wm8900_digital_mute(struct snd_soc_dai *codec_dai, int mute) > SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000) > > #define WM8900_PCM_FORMATS \ > - (SNDRV_PCM_FORMAT_S16_LE | SNDRV_PCM_FORMAT_S20_3LE | \ > - SNDRV_PCM_FORMAT_S24_LE) > + (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \ > + SNDRV_PCM_FMTBIT_S24_LE) > > static const struct snd_soc_dai_ops wm8900_dai_ops = { > .hw_params = wm8900_hw_params, > diff --git a/sound/soc/codecs/wm9713.c b/sound/soc/codecs/wm9713.c > index 9d18a0e..89cd2d6 100644 > --- a/sound/soc/codecs/wm9713.c > +++ b/sound/soc/codecs/wm9713.c > @@ -1054,8 +1054,8 @@ static int ac97_aux_prepare(struct snd_pcm_substream *substream, > SNDRV_PCM_RATE_48000) > > #define WM9713_PCM_FORMATS \ > - (SNDRV_PCM_FORMAT_S16_LE | SNDRV_PCM_FORMAT_S20_3LE | \ > - SNDRV_PCM_FORMAT_S24_LE) > + (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \ > + SNDRV_PCM_FMTBIT_S24_LE) > > static const struct snd_soc_dai_ops wm9713_dai_ops_hifi = { > .prepare = ac97_hifi_prepare, >
On Tue, May 26, 2015 at 07:29:57AM +0200, Takashi Iwai wrote: > Maciej S. Szmigiero wrote: > > Found out by sparse on 0-day kernel tester. > Wow, that made me wonder how these drivers could actually work. AFAICT the bits were being set by being ored in from the sequential values. > BTW, how did you detect it? Any static analyzer like sparse or > smatch? sparse didn't detect it at the last time I tried, IIRC... Commit log says sparse.
Hello Takashi, W dniu 26.05.2015 07:29, Takashi Iwai pisze: > At Sat, 23 May 2015 18:32:29 +0200, > Maciej S. Szmigiero wrote: >> >> snd_soc_pcm_stream.formats is a bitmask of SNDRV_PCM_FMTBIT_*, >> not of SNDRV_PCM_FORMAT_* (which are sequential integers), >> however some of ASoC CODEC drivers use these values instead. >> >> Found out by sparse on 0-day kernel tester. >> >> Signed-off-by: Maciej Szmigiero <mail@maciej.szmigiero.name> > > Wow, that made me wonder how these drivers could actually work. Maybe, by coincidence, the wrong defines contained enough bits set to actually select some common, working format with their controllers? > BTW, how did you detect it? Any static analyzer like sparse or > smatch? sparse didn't detect it at the last time I tried, IIRC... I've received an e-mail from "kbuild test robot" at "0-DAY kernel test infrastructure" that automated testing there using sparse found this issue on wm9713 and stac9766 CODECs. The exact warning was: >> sound/soc/codecs/stac9766.c:324:28: sparse: incorrect type in initializer (different base types) sound/soc/codecs/stac9766.c:324:28: expected unsigned long long [unsigned] [usertype] formats sound/soc/codecs/stac9766.c:324:28: got restricted snd_pcm_format_t [usertype] <noident> What is important the warning doesn't show unless a check build is made with CF=-D__CHECK_ENDIAN__ . Upon checking I've found the same issue also in two other CODECs, which aren't normally being built on x86_64 (target architecture for above automated build) even when SND_SOC_ALL_CODECS is selected. > thanks, > > Takashi Best regards, Maciej Szmigiero
At Tue, 26 May 2015 16:03:17 +0200, Maciej S. Szmigiero wrote: > > Hello Takashi, > > W dniu 26.05.2015 07:29, Takashi Iwai pisze: > > At Sat, 23 May 2015 18:32:29 +0200, > > Maciej S. Szmigiero wrote: > >> > >> snd_soc_pcm_stream.formats is a bitmask of SNDRV_PCM_FMTBIT_*, > >> not of SNDRV_PCM_FORMAT_* (which are sequential integers), > >> however some of ASoC CODEC drivers use these values instead. > >> > >> Found out by sparse on 0-day kernel tester. > >> > >> Signed-off-by: Maciej Szmigiero <mail@maciej.szmigiero.name> > > > > Wow, that made me wonder how these drivers could actually work. > > Maybe, by coincidence, the wrong defines contained enough bits > set to actually select some common, working format with their > controllers? Well, FORMAT_S16_LE = 2, and FORMAT_S18_3LE = 40. So bits 1, 3 and 5 are set, which corresponds to U8, S16_BE and U16_BE. Hmm. > > BTW, how did you detect it? Any static analyzer like sparse or > > smatch? sparse didn't detect it at the last time I tried, IIRC... > > I've received an e-mail from "kbuild test robot" at > "0-DAY kernel test infrastructure" that automated testing there > using sparse found this issue on wm9713 and stac9766 CODECs. > > The exact warning was: > >> sound/soc/codecs/stac9766.c:324:28: sparse: incorrect type in initializer (different base types) > sound/soc/codecs/stac9766.c:324:28: expected unsigned long long [unsigned] [usertype] formats > sound/soc/codecs/stac9766.c:324:28: got restricted snd_pcm_format_t [usertype] <noident> > > What is important the warning doesn't show unless a check build > is made with CF=-D__CHECK_ENDIAN__ . Ah, thanks, that was the missing piece. > Upon checking I've found the same issue also in two other CODECs, > which aren't normally being built on x86_64 (target architecture > for above automated build) even when SND_SOC_ALL_CODECS is selected. Oh it'd be great if you submit fixes :) thanks, Takashi
W dniu 26.05.2015 16:09, Takashi Iwai pisze: > At Tue, 26 May 2015 16:03:17 +0200, > Maciej S. Szmigiero wrote: >> >> Hello Takashi, >> >> W dniu 26.05.2015 07:29, Takashi Iwai pisze: >>> At Sat, 23 May 2015 18:32:29 +0200, >>> Maciej S. Szmigiero wrote: >>>> >>>> snd_soc_pcm_stream.formats is a bitmask of SNDRV_PCM_FMTBIT_*, >>>> not of SNDRV_PCM_FORMAT_* (which are sequential integers), >>>> however some of ASoC CODEC drivers use these values instead. >>>> >>>> Found out by sparse on 0-day kernel tester. >>>> >>>> Signed-off-by: Maciej Szmigiero <mail@maciej.szmigiero.name> >>> >>> Wow, that made me wonder how these drivers could actually work. >> >> Maybe, by coincidence, the wrong defines contained enough bits >> set to actually select some common, working format with their >> controllers? > > Well, FORMAT_S16_LE = 2, and FORMAT_S18_3LE = 40. So bits 1, 3 and 5 > are set, which corresponds to U8, S16_BE and U16_BE. Hmm. > >>> BTW, how did you detect it? Any static analyzer like sparse or >>> smatch? sparse didn't detect it at the last time I tried, IIRC... >> >> I've received an e-mail from "kbuild test robot" at >> "0-DAY kernel test infrastructure" that automated testing there >> using sparse found this issue on wm9713 and stac9766 CODECs. >> >> The exact warning was: >>>> sound/soc/codecs/stac9766.c:324:28: sparse: incorrect type in initializer (different base types) >> sound/soc/codecs/stac9766.c:324:28: expected unsigned long long [unsigned] [usertype] formats >> sound/soc/codecs/stac9766.c:324:28: got restricted snd_pcm_format_t [usertype] <noident> >> >> What is important the warning doesn't show unless a check build >> is made with CF=-D__CHECK_ENDIAN__ . > > Ah, thanks, that was the missing piece. > >> Upon checking I've found the same issue also in two other CODECs, >> which aren't normally being built on x86_64 (target architecture >> for above automated build) even when SND_SOC_ALL_CODECS is selected. > > Oh it'd be great if you submit fixes :) The fixes for these two remaining CODECs were included in the patch (I've meant that these two weren't in the automated checking report I've received). Upon looking again at it the "0-DAY kernel test infrastructure" have to be using some limited config, since all of the CODECs with this issue are being built/checked in allyesconfig (it selects the required dependences for them even on x86_64). > thanks, > > Takashi > Thanks, Maciej Szmigiero
diff --git a/sound/soc/codecs/88pm860x-codec.c b/sound/soc/codecs/88pm860x-codec.c index ee31fa7..38b3dad 100644 --- a/sound/soc/codecs/88pm860x-codec.c +++ b/sound/soc/codecs/88pm860x-codec.c @@ -1186,16 +1186,16 @@ static struct snd_soc_dai_driver pm860x_dai[] = { .channels_min = 2, .channels_max = 2, .rates = PM860X_RATES, - .formats = SNDRV_PCM_FORMAT_S16_LE | \ - SNDRV_PCM_FORMAT_S18_3LE, + .formats = SNDRV_PCM_FMTBIT_S16_LE | \ + SNDRV_PCM_FMTBIT_S18_3LE, }, .capture = { .stream_name = "PCM Capture", .channels_min = 2, .channels_max = 2, .rates = PM860X_RATES, - .formats = SNDRV_PCM_FORMAT_S16_LE | \ - SNDRV_PCM_FORMAT_S18_3LE, + .formats = SNDRV_PCM_FMTBIT_S16_LE | \ + SNDRV_PCM_FMTBIT_S18_3LE, }, .ops = &pm860x_pcm_dai_ops, }, { @@ -1207,16 +1207,16 @@ static struct snd_soc_dai_driver pm860x_dai[] = { .channels_min = 2, .channels_max = 2, .rates = SNDRV_PCM_RATE_8000_48000, - .formats = SNDRV_PCM_FORMAT_S16_LE | \ - SNDRV_PCM_FORMAT_S18_3LE, + .formats = SNDRV_PCM_FMTBIT_S16_LE | \ + SNDRV_PCM_FMTBIT_S18_3LE, }, .capture = { .stream_name = "I2S Capture", .channels_min = 2, .channels_max = 2, .rates = SNDRV_PCM_RATE_8000_48000, - .formats = SNDRV_PCM_FORMAT_S16_LE | \ - SNDRV_PCM_FORMAT_S18_3LE, + .formats = SNDRV_PCM_FMTBIT_S16_LE | \ + SNDRV_PCM_FMTBIT_S18_3LE, }, .ops = &pm860x_i2s_dai_ops, }, diff --git a/sound/soc/codecs/stac9766.c b/sound/soc/codecs/stac9766.c index 2341e8e..ed4cca7 100644 --- a/sound/soc/codecs/stac9766.c +++ b/sound/soc/codecs/stac9766.c @@ -320,7 +320,7 @@ static struct snd_soc_dai_driver stac9766_dai[] = { .channels_max = 2, .rates = SNDRV_PCM_RATE_32000 | \ SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000, - .formats = SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE, + .formats = SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_BE, }, /* alsa ops */ .ops = &stac9766_dai_ops_digital, diff --git a/sound/soc/codecs/wm8900.c b/sound/soc/codecs/wm8900.c index e7d2ecd..493faf5 100644 --- a/sound/soc/codecs/wm8900.c +++ b/sound/soc/codecs/wm8900.c @@ -998,8 +998,8 @@ static int wm8900_digital_mute(struct snd_soc_dai *codec_dai, int mute) SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000) #define WM8900_PCM_FORMATS \ - (SNDRV_PCM_FORMAT_S16_LE | SNDRV_PCM_FORMAT_S20_3LE | \ - SNDRV_PCM_FORMAT_S24_LE) + (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \ + SNDRV_PCM_FMTBIT_S24_LE) static const struct snd_soc_dai_ops wm8900_dai_ops = { .hw_params = wm8900_hw_params, diff --git a/sound/soc/codecs/wm9713.c b/sound/soc/codecs/wm9713.c index 9d18a0e..89cd2d6 100644 --- a/sound/soc/codecs/wm9713.c +++ b/sound/soc/codecs/wm9713.c @@ -1054,8 +1054,8 @@ static int ac97_aux_prepare(struct snd_pcm_substream *substream, SNDRV_PCM_RATE_48000) #define WM9713_PCM_FORMATS \ - (SNDRV_PCM_FORMAT_S16_LE | SNDRV_PCM_FORMAT_S20_3LE | \ - SNDRV_PCM_FORMAT_S24_LE) + (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \ + SNDRV_PCM_FMTBIT_S24_LE) static const struct snd_soc_dai_ops wm9713_dai_ops_hifi = { .prepare = ac97_hifi_prepare,
snd_soc_pcm_stream.formats is a bitmask of SNDRV_PCM_FMTBIT_*, not of SNDRV_PCM_FORMAT_* (which are sequential integers), however some of ASoC CODEC drivers use these values instead. Found out by sparse on 0-day kernel tester. Signed-off-by: Maciej Szmigiero <mail@maciej.szmigiero.name>