On 19.06.2019 17:34, Dan Carpenter wrote:

External E-Mail

Hi Codrin,

Linus recently corrected me on one of my patches and I said I would look through the kernel and fix similar bugs as well. The problem I realized is that I'm really stupid and I have forgot how to do math...

The patch 90f6e6803139: "ASoC: codecs: ad193x: Fix frame polarity for DSP_A format" from Feb 18, 2019, leads to the following static checker warning:

sound/soc/codecs/ad193x.c:244 ad193x_set_dai_fmt() warn: passing casted pointer '&dac_fmt' to 'change_bit()' 32 vs 64.

sound/soc/codecs/ad193x.c 194 static int ad193x_set_dai_fmt(struct snd_soc_dai *codec_dai, 195 unsigned int fmt) 196 { 197 struct ad193x_priv *ad193x = snd_soc_component_get_drvdata(codec_dai->component); 198 unsigned int adc_serfmt = 0; 199 unsigned int dac_serfmt = 0; 200 unsigned int adc_fmt = 0; 201 unsigned int dac_fmt = 0; ^^^^^^^^^^^^^^^^^^^^^^^^ This is a u32.

202
203          /* At present, the driver only support AUX ADC mode(SND_SOC_DAIFMT_I2S
204           * with TDM), ADC&DAC TDM mode(SND_SOC_DAIFMT_DSP_A) and DAC I2S mode
205           * (SND_SOC_DAIFMT_I2S)
206           */
207          switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
208          case SND_SOC_DAIFMT_I2S:
209                  adc_serfmt |= AD193X_ADC_SERFMT_TDM;
210                  dac_serfmt |= AD193X_DAC_SERFMT_STEREO;
211                  break;
212          case SND_SOC_DAIFMT_DSP_A:
213                  adc_serfmt |= AD193X_ADC_SERFMT_AUX;
214                  dac_serfmt |= AD193X_DAC_SERFMT_TDM;
215                  break;
216          default:
217                  if (ad193x_has_adc(ad193x))
218                          return -EINVAL;
219          }
220
221          switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
222          case SND_SOC_DAIFMT_NB_NF: /* normal bit clock + frame */
223                  break;
224          case SND_SOC_DAIFMT_NB_IF: /* normal bclk + invert frm */
225                  adc_fmt |= AD193X_ADC_LEFT_HIGH;
226                  dac_fmt |= AD193X_DAC_LEFT_HIGH;
227                  break;
228          case SND_SOC_DAIFMT_IB_NF: /* invert bclk + normal frm */
229                  adc_fmt |= AD193X_ADC_BCLK_INV;
230                  dac_fmt |= AD193X_DAC_BCLK_INV;
231                  break;
232          case SND_SOC_DAIFMT_IB_IF: /* invert bclk + frm */
233                  adc_fmt |= AD193X_ADC_LEFT_HIGH;
234                  adc_fmt |= AD193X_ADC_BCLK_INV;
235                  dac_fmt |= AD193X_DAC_LEFT_HIGH;
236                  dac_fmt |= AD193X_DAC_BCLK_INV;
237                  break;
238          default:
239                  return -EINVAL;
240          }
241
242          /* For DSP_*, LRCLK's polarity must be inverted */
243          if (fmt & SND_SOC_DAIFMT_DSP_A) {
244                  change_bit(ffs(AD193X_DAC_LEFT_HIGH) - 1,
245                             (unsigned long *)&dac_fmt);
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This change_bit() will work on little endian systems but on 64 bit big endian systems it will corrupt memory. I *think* the correct code looks like this, but again, I'm feeling dumb so I might be wrong:

	dac_fmt ^= 1 << (ffs(AD193X_DAC_LEFT_HIGH) - 1);

or just: dac_fmt ^= AD193X_DAC_LEFT_HIGH;

Thank you for reporting this. I will send a patch to fix it.

Best regards, Codrin